-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Track volume usage data at a vm granularity as well #11531
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: main
Are you sure you want to change the base?
Conversation
@blueorangutan package |
@abh1sar 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11531 +/- ##
============================================
- Coverage 17.50% 17.50% -0.01%
Complexity 15425 15425
============================================
Files 5894 5894
Lines 526877 526937 +60
Branches 64337 64341 +4
============================================
+ Hits 92232 92233 +1
- Misses 424269 424327 +58
- Partials 10376 10377 +1
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:
|
public UsageVolumeDaoImpl() { | ||
} | ||
|
||
@Override |
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.
This kind of update method is not required since #5785 added auto-increment primary keys. We can directly use the GenericDao.update()
if (volumesVOs.size() > 0) { | ||
//This is a safeguard to avoid double counting of volumes. | ||
s_logger.error("Found duplicate usage entry for volume: " + volId + " assigned to account: " + event.getAccountId() + "; marking as deleted..."); | ||
if (event.getVmId() != 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.
VOLUME_CREATE event will contain the vmId also when it is created during Instance deployment. Need to add a vm specific entry also in that case.
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14760 |
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.
looks good, except for the upgrade path (and needs testing)
engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade41930to41940.java
Show resolved
Hide resolved
clgtm |
code looks good in isolation @abh1sar , but I see some bears on the road: what if someone upgrades from 20.1 to 21 and then on to 22; How do we guarantee they get the DB updates? Sorry, I overlooked this when I asked you to rebase earlier. |
@blueorangutan package |
@abh1sar 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 15238 |
@blueorangutan package |
@abh1sar 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 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15241 |
@blueorangutan test keepEnv |
@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
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
Co-authored-by: Vishesh <[email protected]>
@blueorangutan package |
@abh1sar 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 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15249 |
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
@blueorangutan package |
@abh1sar 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 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15257 |
[SF] Trillian test result (tid-14492)
|
Description
This PR fixes #10985
Some users have asked to show the virtualmachineid also in volume usage, to track it per instance.
There will be two kinds of records now.
If the volume was attached to multiple instances in the same usage window, we will have separate entries for each VM.
Schema changes
I have also done some code refactoring in createVolumeHelperEvent() to make it easier to follow.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
listUsageRecords for a volume in a 2 minute (0.033333 hours) window.
The volume was created, then attached to VM-00a42c28-74ae-4а0c-b38a-4e3b88f11241. It was then detached and attached to another VM QA-d6df4657-2fb4-4e26-b7de-21743c5463cd. So we have 3 records.
How Has This Been Tested?
How did you try to break this feature and the system with this change?