api: improve listVM API handling and response speed

[rohityadavcloud]

• Changes behaviour of details param handling for:
  • listVirtualMachines API: when the details param is not provided, it returns all details excluding/including stats which is controllable by a new global setting list.vm.default.details.stats
  • listVirtualMachinesMetrics API: when the details param is not provided, it uses all details including stats
• Users who are affected by the stats related change, can have backward compatibility at the higher-cost of listVirtualMachines API response time by setting list.vm.default.details.stats to true
• Remove ConfigKey vm.stats.increment.metrics.in.memory which was renamed to vm.stats.increment.metrics in Persistence of VM stats #5984 and also remove unused/unnecessary global settings via upgrade path
• Changes default value of VM stats accumulation setting vm.stats.increment.metrics to false until a better solution emerges. Since Persistence of VM stats #5984, this is true and during the execution of listVM APIs the stats are clubbed/calculated which can immensely slow down list VM API calls.
• Fix UI that uses listVirtualMachinesMetrics to not call stats detail when in list view without metrics selected.

These changes see 2-4x gains in the listVirtualMachines APIs call and in the UI. For environment where code changes are not possible, disabling vm.stats.increment.metrics global setting saw 2-4x speed gain in the list API calls.

Known issues that led to this:

• VM listing slowness can cause side-effects with integrations such as When deploying node to Kubernetes it times out #8975
• Side-effects seen in UI and CAPC (while listing VMs) in users/prod environments

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Tested a workaround in a 4.18.2.0 env KVM based, with only 12 VMs:

1. List VM API call without details and with details but global setting vm.stats.increment.metrics disabled:

2. List VM API call without detail and with detail and vm.stats.increment.metrics enabled:

• We see ~3-4x gains by simply disabling the global setting
• We see 15x gains when stats details isn't used but the global setting is enabled
• We see 4x gains when stats detail isn't used and global setting is disabled

With this PR, I propose to change the earlier behaviour for listVirtualMachines API to exclude stats when details aren't provided. This can immensely speed up several integrations, who call listVMs APIs that not necessarily need stats in it.
While, the listVirtualMachinesMetrics API was amended to retain old behaviour so when details aren't passed it could
return all details including stats as expected. In the release notes likely we can mention about this, so folks who don't want accumulated stats can disable it but for new env we don't want to encourage this by default (as we already have the nice vm history metrics feature/tab).

How did you try to break this feature and the system with this change?

[rohityadavcloud]

@blueorangutan package

[blueorangutan]

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[DaanHoogland]

clgtm, no idea about performance so needs testing

[weizhouapache]

code looks ok

my only concern is the backward compatibility
we may need a highlighted section to list breaking changes

[rohityadavcloud]

@weizhouapache this is breaking largely wrt 4.17 but not much as earlier versions; the only issue is for listVirtualMachines API which IMO shouldn't return vmstats (by looping through accumulating stats while the API is made). But I agree it could be an edge case where this break somebody's integration who aren't using the listVirtualMachinesMetrics instead (which doesn't break the compatibility).

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9435

[rohityadavcloud]

@blueorangutan test

[blueorangutan]

@rohityadavcloud a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 18.18182% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 14.96%. Comparing base (e409c6d) to head (a2cf0be).
Report is 8 commits behind head on 4.19.

Files	Patch %	Lines
...a/org/apache/cloudstack/api/ListVMsMetricsCmd.java	0.00%	6 Missing ⚠️
...che/cloudstack/api/command/user/vm/ListVMsCmd.java	0.00%	2 Missing ⚠️
...ava/com/cloud/api/query/dao/UserVmJoinDaoImpl.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19    #8985      +/-   ##
============================================
- Coverage     14.96%   14.96%   -0.01%     
+ Complexity    10995    10994       -1     
============================================
  Files          5373     5373              
  Lines        468989   469002      +13     
  Branches      61009    57150    -3859     
============================================
  Hits          70191    70191              
- Misses       391019   391029      +10     
- Partials       7779     7782       +3     

Flag	Coverage Δ
uitests	4.31% <ø> (-0.01%)	⬇️
unittests	15.67% <18.18%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JoaoJandre]

Hello @rohityadavcloud , @weizhouapache

Looking briefly at this PR I noticed that it has a big intersection with #8782, especially the changes on the listVirtualMachines and listVirtualMachineMetrics APIs.

When #8782 proposed to change the default behavior of them to not list the stats, both @weizhouapache and @rohityadavcloud were firmly against changing the default behavior (see #8782 (comment) and #8782 (comment)). So now, only a month later, we get this PR: what is the big difference on these two PRs? And if there is, why not just propose an adjustment to #8782?

In any case, this PR not only breaks compatibility on the APIs, but also changes the default value of another configuration; and all this is going into 4.18! Are we going to introduce breaking changes on a maintenance version? I would really like to understand what made you change your opinion on compatibility so fast.

[sureshanaparti]

clgtm

[blueorangutan]

[SF] Trillian test result (tid-10044)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 43454 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8985-t10044-kvm-centos7.zip
Smoke tests completed. 109 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_04_nonsecured_to_secured_vm_migration	Error	166.37	test_vm_life_cycle.py
test_08_migrate_vm	Error	46.21	test_vm_life_cycle.py

[rohityadavcloud]

Thanks @weizhouapache @JoaoJandre I've addressed the feedback and comments.

Joao's remarks are valid, however, my comments at the time were largely around metrics vs non-metrics APIs - how they should be used treated. At the time I couldn't co-relate what made the API slow only to later find out through community and customer issues who are facing degraded performance.

Generally speaking, we shouldn't break API compatibility which can break potential integrations (as also on this PR Wei has objected). I feel in this case whatever we do we break what was introduced by #5984 in 4.17. The root cause that I could determine is, we accumulate VM stats for each VM in list API calls which isn't a good idea. That said, I think we need a stop-gap until a better solution emerges on how we should process and return stats are returned by the list API (I think we shouldn't reduce/accumulate stats during a list API call).

The list VM API response method is used not only for listVMs API but also for several other VM related lifecycle APIs (such as migrate, stop/start etc) which all return the same vm response class/object. So, I've tried to have a different approach with the PR, and introduce a new global setting that can allow users who want backward compatibility but largely benefit users who don't want it (as default). I've updated the description to reflect this.

[rohityadavcloud]

@blueorangutan package

[blueorangutan]

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9452

[rohityadavcloud]

@blueorangutan test

[blueorangutan]

@rohityadavcloud a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-10065)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45766 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8985-t10065-kvm-centos7.zip
Smoke tests completed. 128 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_events_resource	Error	417.66	test_events_resource.py
test_01_redundant_vpc_site2site_vpn	Failure	409.80	test_vpc_vpn.py
test_01_vpc_site2site_vpn	Failure	294.07	test_vpc_vpn.py

[rohityadavcloud]

@blueorangutan package

[blueorangutan]

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9461

[GutoVeronezi]

At the time I couldn't co-relate what made the API slow only to later find out through community and customer issues who are facing degraded performance.

That is the main point of the PR #8782 and I explained it in #8737 (comment), which is a discussion you participated, @rohityadavcloud.

It is not the first time that I have seen some going against a PR and sometime later the same guys or group proposing the same thing. @JoaoJandre had already proposed the same type of solution on #8782 and received several comments against it; all lacking technical reason to be against it. And now we are facing a PR from those who were against João's PR doing the same thing. This is a duplicate of PR #8782; I am -1 on this PR and PR #8782 should be prioritized.

[rohityadavcloud]

My bad I didn't look at prior work #8782 or remember what others have said on each and every other PRs. To be fair, I don't remember what I wrote on #8782 last month before I started this PR.

I don't see anybody against PR #8782, and I see @weizhouapache has shared the same technical concerns around backward compatibility on both the PRs. On #8782 I had suggested to introduce a global setting that can allow admins to maintain backward compatibility if they really want that. This PR incorporates all those suggestions, and some other changes - so this PR isn't exactly a duplicate of the other one.

I don't care which PR gets accepted as long as it address the issue in way that's acceptable to the wider stakeholders. I'm okay to drop this one, but PR #8782 needs to address the outstanding concerns.

[shwstppr]

Code LGTM

@blueorangutan package

[rohityadavcloud]

As I don't care much for the PR anymore and #8782 has addressed most if not all my concerns, I'm closing this PR. I may raise a new PR for changes that aren't in #8782 and we probably may need a doc PR in release notes.