-
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
base: 4.20
Are you sure you want to change the base?
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. |
|
|
||
| // If no ID was provided, then the listing will skip project resources (null); otherwise, project resources should | ||
| // be listed as well (any long allows this) | ||
| Long id = isIdProvided ? 1L : null; |
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.
is there side-effect when id is set to 1L ?
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.
The value itself is not used, com.cloud.user.AccountManagerImpl#buildACLSearchParameters just checks whether the ID is null in order to decide whether project resources should be skipped or not, so no.
cloudstack/server/src/main/java/com/cloud/user/AccountManagerImpl.java
Lines 3376 to 3378 in eb93f01
| if (id == null) { | |
| domainIdRecursiveListProject.third(Project.ListProjectResourcesCriteria.SkipProjectResources); | |
| } |
|
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 |


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: