-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix VM and volume metrics listing regressions #12284
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
Fix VM and volume metrics listing regressions #12284
Conversation
|
@blueorangutan package |
|
@winterhazel 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12284 +/- ##
============================================
+ Coverage 16.22% 16.23% +0.01%
- Complexity 13358 13379 +21
============================================
Files 5657 5657
Lines 498692 498865 +173
Branches 60530 60545 +15
============================================
+ Hits 80932 81011 +79
- Misses 408738 408820 +82
- Partials 9022 9034 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16080 |
kiranchavala
left a comment
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.
DaanHoogland
left a comment
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, one remark/question though; two bits of code are repeated with a similar statement in between, Can we create a method for it?
|
@kiranchavala thanks for testing :] @DaanHoogland yup, I will do some refactoring (and some extra tests to be sure) before marking it as ready. |
…gnored when passing multiple IDs
|
@blueorangutan package |
|
@winterhazel 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]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16090 |
|
@DaanHoogland I extracted the code that builds the parameters to a method. Tests are also ok. I noticed that there was another regression in which admins were not able to list the metrics of VMs/volumes belonging to projects via the ExampleAlso, passing the |
|
@blueorangutan package |
|
@winterhazel 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]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16091 |
|
@blueorangutan package |
|
@winterhazel 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]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16092 |
we hit the github API limit @winterhazel , don’t worry about this. |
plugins/metrics/src/main/java/org/apache/cloudstack/metrics/MetricsServiceImpl.java
Show resolved
Hide resolved
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16103 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15033)
|
* 4.22: Update templateConfig.sh to not break with directorys with space on t… (#10898) Fix VM and volume metrics listing regressions (#12284) packaging: use latest cmk release link directly (#11429) api:rename RegisterCmd.java => RegisterUserKeyCmd.java (#12259) Prioritize copying templates from other secondary storages instead of downloading them (#10363) Show time correctly in the backup schedule UI (#12012) kvm: use preallocation option for fat disk resize (#11986) Python exception processing static routes fixed (#11967) KVM memballooning requires free page reporting and autodeflate (#11932) api: create/register/upload template with empty template tag (#12234)


Description
This PR fixes a regression in commit c8d44d9 that prevents non-admin accounts from listing the metrics of VMs and volumes that belong to a project (see #12237).
Previously, the following example condition was used in the
listVirtualMachinesMetricsAPI in order to find VMs that could have their metrics listed by a user:In 4.20.2 and 4.22.0, the condition also restricts the result to VMs that belong to the calling account; hence, VMs that belong to a project are not returned:
With this patch, the ID of projects that the user has access to is also included in the condition:
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
I manually verified that, using both the
idandidsparameters: