-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api: improve listVM API handling and response speed #8985
api: improve listVM API handling and response speed #8985
Conversation
@blueorangutan package |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm, no idea about performance so needs testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks ok
my only concern is the backward compatibility
we may need a highlighted section to list breaking changes
@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). |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9435 |
@blueorangutan test |
@rohityadavcloud a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
[SF] Trillian test result (tid-10044)
|
- Changes behaviour of details param handling for: - listVirtualMachines API: when the detail param is not provided, it uses `all` details except `stats` - listVirtualMachinesMetrics API: when the detail param is not provided, it uses `all` details including `stats` - Remove ConfigKey vm.stats.increment.metrics.in.memory which was renamed to `vm.stats.increment.metrics` in apache#5984 - Changes default value of VM stats accumulation setting `vm.stats.increment.metrics` to false until a better solution emerges. Since apache#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. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
dcdea09
to
085038e
Compare
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. |
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9452 |
@blueorangutan test |
@rohityadavcloud a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
[SF] Trillian test result (tid-10065)
|
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@blueorangutan package |
@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. |
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 9461 |
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
@blueorangutan package
all
details excluding/includingstats
which is controllable by a new global settinglist.vm.default.details.stats
all
details includingstats
list.vm.default.details.stats
to truevm.stats.increment.metrics
in Persistence of VM stats #5984 and also remove unused/unnecessary global settings via upgrade pathvm.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.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:
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Tested a workaround in a 4.18.2.0 env KVM based, with only 12 VMs:
vm.stats.increment.metrics
disabled:vm.stats.increment.metrics
enabled: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?