You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "Alpha162 (via GitHub)" <gi...@apache.org> on 2026/08/18 10:09:06 UTC

[PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Alpha162 opened a new pull request, #13909:
URL: https://github.com/apache/cloudstack/pull/13909

   ### Description
   
   `createVolumeHelperEvent()` writes two `usage_volume` rows for a single `VOLUME.CREATE` when the event carries `vm_id`, one cumulative and one per-VM, as intended by #11531. Both use `event.getCreateDate()`, but the unique key is still `(volume_id, created)`, so the second insert can never succeed.
   
   #11531 added `vm_id` to `cloud_usage.usage_volume` via `IDEMPOTENT_ADD_COLUMN` without widening the key. This adds the missing DDL.
   
   Uses the existing `` `cloud`.`IDEMPOTENT_DROP_UNIQUE_KEY` `` / `` `IDEMPOTENT_ADD_UNIQUE_KEY` `` helpers per @DaanHoogland's guidance on idempotency β€” both carry `CONTINUE HANDLER`s (`1091, 1025` and `1061` respectively), so a re-run is a no-op and no new procedures are needed. Same pattern as the existing `cloud.oauth_provider` call in this file.
   
   Partially addresses #13399. The remaining two defects from that investigation are tracked in #13905 and #13906.
   
   ### Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [X] 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
   - [ ] Test (unit or integration test code)
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Feature/Enhancement Scale
   
   - [ ] Major
   - [ ] Minor
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [X] Major
   - [ ] Minor
   - [ ] Trivial
   
   ### How Has This Been Tested?
   
   Not tested end-to-end, and I'd rather say so plainly: I'm an operator rather than a dev, and the affected environment is being rebuilt to 4.22.0.0 today, so it's no longer available to me.
   
   What I can attest to:
   
   - The narrow `UNIQUE KEY id (volume_id, created)` was confirmed via `SHOW CREATE TABLE` on a live 4.22.1.0 install, alongside the `vm_id` column added by #11531.
   - Widening a unique key is strictly less restrictive, so no existing row can violate the new constraint β€” the DDL cannot fail on data.
   - Both helpers are idempotent, so a re-run or resumed upgrade is safe.
   
   One caveat worth review: the key is *named* `id` but covers `(volume_id, created)`, because `create-schema-premium.sql` declares it unnamed and MySQL auto-named it after the first column. `IDEMPOTENT_DROP_UNIQUE_KEY` therefore depends on that generated name. On an install where it differs, the drop would no-op and the add would create a second key rather than replacing the first.
   
   Would appreciate someone running this against a real upgrade before merge.
   
   #### How did you try to break this feature and the system with this change?
   
   Reasoned through the failure modes rather than executing them, given no test environment: re-running the migration (safe, the handlers swallow 1061 and 1091/1025), running against a table whose key name differs (drop no-ops, add creates a second key, flagged above), and existing data violating the widened key (not possible, it is less restrictive than the key it replaces).
   
   ### A note on branch targeting
   
   `CONTRIBUTING.md` says bug fixes should target a release branch and be merged forward, which would mean `4.22` here. I've targeted `main` because `4.22` has no 4.22.1 β†’ 4.22.2 upgrade path yet, its newest is `Upgrade42200to42210.java`, whereas `main` already has `Upgrade42210to42300.java`. Creating a 4.22.2 path felt like a release-management decision rather than mine to make.
   
   Happy to redo this against `4.22` with a new `schema-42210to42220.sql` and upgrade class if that's preferred, or for it to be cherry-picked, just say which.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel merged PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5328796856

   @blueorangutan package


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "Alpha162 (via GitHub)" <gi...@apache.org>.
Alpha162 commented on code in PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#discussion_r3806452760


##########
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:
##########
@@ -646,3 +646,7 @@ CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backup_schedule', 'isolated', 'TINYI
 
 UPDATE `cloud`.`configuration` SET `value`=CONCAT(`value`, ', backupValidationCommandTimeout, backupValidationScreenshotWait, backupValidationBootTimeout')
 WHERE `name`='user.vm.readonly.details' AND `value` IS NOT NULL;
+-- Widen the unique key on cloud_usage.usage_volume to include vm_id, so the two volume
+-- usage records introduced in 4.22.1 (cumulative and per-VM) can coexist. See #13399.
+CALL `cloud_usage`.`IDEMPOTENT_DROP_UNIQUE_KEY`('cloud_usage.usage_volume', 'id');

Review Comment:
   Thanks @abh1sar, you're right, cloud_usage.IDEMPOTENT_ADD_UNIQUE_INDEX is defined over in procedures/cloud.idempotent_add_unique_index.sql, which the cloud. filename disguises, and schema 41600to41610.sql:72 created this exact key with it so widening it the same way keeps the table's history consistent.
   
   Both new procedures are dropped and the change is now simply just the two CALLs. Noted on the 4.22.2 backport, thanks πŸ‘.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "winterhazel (via GitHub)" <gi...@apache.org>.
winterhazel commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5328068076

   > @winterhazel @DaanHoogland shall we add this to 4.23.0 milestone ?
   
   @weizhouapache yes.
   
   This patch also applies to 4.22. However, to avoid having to cherry-pick it from 4.22 and resulting in merge-forward issues, I think it is better to open a separate backport PR after this one is merged.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5328827633

   @weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with  no SystemVM templates. I'll keep you posted as I make progress.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5444904572

   @blueorangutan sweep


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5338627708

   <b>[SF] Trillian test result (tid-16778)</b>
   Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
   Total time taken: 55984 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13909-t16778-kvm-ol8.zip
   Smoke tests completed. 156 look OK, 0 have errors, 0 did not run
   Only failed and skipped tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5329609293

   @blueorangutan test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5347119623

   Awesome work, congrats on your first merged pull request!
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5326717851

   Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
   Here are some useful points:
   - In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
   - Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
   - Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
   - Please follow [ASF Code of Conduct](https://github.com/apache/.github/blob/main/.github/CODE_OF_CONDUCT.md) for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
   - Be sure to read the [CloudStack Coding Conventions](https://cwiki.apache.org/confluence/display/CLOUDSTACK/Coding+conventions).
   Apache CloudStack is a community-driven project and together we are making it better πŸš€.
   In case of doubts contact the developers at:
   Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
   Slack: https://apachecloudstack.slack.com/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "abh1sar (via GitHub)" <gi...@apache.org>.
abh1sar commented on code in PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#discussion_r3805981126


##########
engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql:
##########
@@ -646,3 +646,7 @@ CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.backup_schedule', 'isolated', 'TINYI
 
 UPDATE `cloud`.`configuration` SET `value`=CONCAT(`value`, ', backupValidationCommandTimeout, backupValidationScreenshotWait, backupValidationBootTimeout')
 WHERE `name`='user.vm.readonly.details' AND `value` IS NOT NULL;
+-- Widen the unique key on cloud_usage.usage_volume to include vm_id, so the two volume
+-- usage records introduced in 4.22.1 (cumulative and per-VM) can coexist. See #13399.
+CALL `cloud_usage`.`IDEMPOTENT_DROP_UNIQUE_KEY`('cloud_usage.usage_volume', 'id');

Review Comment:
   These procedures are already present and used in code (no need to add new ones)
   
   See schema-41600to41610.sql for example:
   ```
   CALL `cloud_usage`.`IDEMPOTENT_DROP_INDEX`('i_usage_snapshot_on_primary','cloud_usage.usage_snapshot_on_primary');
   CALL `cloud_usage`.`IDEMPOTENT_ADD_UNIQUE_INDEX`('cloud_usage.usage_snapshot_on_primary', 'i_usage_snapshot_on_primary', '(account_id ASC, volume_id ASC, vm_id ASC, created ASC)');
   
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5355553141

   > I'll include schema-42210to42220.sql, its cleanup companion, the Upgrade42210to42220 class, and the .next("4.22.1.0", ...) entry in the backport PR.
   
   @Alpha162 please go ahead, you may refer to #12111 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5327759921

   @winterhazel @DaanHoogland 
   shall we add this to 4.23.0 milestone ?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5329631448

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "Alpha162 (via GitHub)" <gi...@apache.org>.
Alpha162 commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5328510529

   @winterhazel @weizhouapache thank you both, that settles the branch question I raised in the description, a separate backport after merge sounds cleaner than a cherry-pick.
   
   Worth flagging for whenever that happens: `4.22`'s newest upgrade path is `Upgrade42200to42210.java`, so there's no 4.22.1 β†’ 4.22.2 schema file for the two CALLs to live in yet.
   
   On the red checks? `build` failed after 14s on `Java setup process failed due to: Could not find satisfied version for SemVer '17'`, and the Sonar job on `Resource not accessible by integration` posting a comment, which looks like the standard fork-PR token restriction. Neither appears related to the change.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5329536305

   Packaging result [SF]: βœ”οΈ el8 βœ”οΈ el9 βœ”οΈ el10 βœ”οΈ debian βœ”οΈ suse15. SL-JID 18905


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "Alpha162 (via GitHub)" <gi...@apache.org>.
Alpha162 commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5355426435

   Correcting my own question: the 4.22 branch is already at 4.22.2.0-SNAPSHOT, so 4.22.2.0 is declared. The upgrade path hasn't been created because nothing has needed a schema change on 4.22 since 4.22.1.0 was cut. So unless you'd rather it were structured differently, I'll include schema-42210to42220.sql, its cleanup companion, the Upgrade42210to42220 class, and the .next("4.22.1.0", ...) entry in the backport PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5327669866

   ## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13909?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 3.41%. Comparing base ([`121d931`](https://app.codecov.io/gh/apache/cloudstack/commit/121d93164cbcbc273271905b5db72eaff895c690?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3a2b69b`](https://app.codecov.io/gh/apache/cloudstack/commit/3a2b69bd0acb20e05a60bdea786d91e712960f5f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 1 commits behind head on main.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@          Coverage Diff           @@
   ##            main   #13909   +/-   ##
   ======================================
     Coverage   3.41%    3.41%           
   ======================================
     Files        487      487           
     Lines      41867    41867           
     Branches    7912     7912           
   ======================================
     Hits        1429     1429           
     Misses     40238    40238           
     Partials     200      200           
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13909/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ” | |
   |---|---|---|
   | [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13909/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.41% <ΓΈ> (ΓΈ)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/13909?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "Alpha162 (via GitHub)" <gi...@apache.org>.
Alpha162 commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5339902261

   @winterhazel WE restored a pre-rebuild backup of that 4.22.1.0 database to check this.
   
   Every VOLUME.CREATE on that cluster carried a vm_id, so each should have written two usage_volume rows:
   
   type            events  with_vm
   VOLUME.CREATE      34      34
   
   What actually persisted:
   
   count(*)   count(vm_id)
   33         0
   
   All 34 per-VM inserts were lost to the duplicate key, a 100% failure rate, not an intermittent one. (The 34th cumulative row is absent because that event never processed.)
   
   Which I think answers your question: nothing on that cluster was ever double-counted, because the second row never existed. Once this merges those rows begin landing, so the Quota exposure looks like one this fix introduces rather than one already in the field, supporting your instinct that the Quota change should go in alongside it.
   
   One scope caveat: there were no VOLUME.ATTACH events on this cluster at-all. It was a lightly-used internal environment being prepped ahead of customer workloads, and every volume carried a vm_id at creation, so I can't tell you anything about that path.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] usage: widen usage_volume unique key to include vm_id [cloudstack]

Posted by "Alpha162 (via GitHub)" <gi...@apache.org>.
Alpha162 commented on PR #13909:
URL: https://github.com/apache/cloudstack/pull/13909#issuecomment-5355249924

   Thanks @winterhazel @abh1sar. 
   
   On the 4.22 backport, it looks like we need a 4.22.2 upgrade path that doesn't exist yet. The chain in DatabaseUpgradeChecker on 4.22 ends at .next("4.22.0.0", new Upgrade42200to42210()), and that class reports 4.22.1.0 as its upgraded version, so there's nowhere for the two CALLs to live that a cluster already on 4.22.1.0 would re-run. Adding them to schema-42200to42210.sql wouldn't reach the affected population, since that file has already run on any 4.22.1.0 install.
   
   So the backport needs schema-42210to42220.sql plus its cleanup companion? An Upgrade42210to42220 class, and a .next("4.22.1.0", ...) entry. 
   
   Happy to attempt it, but declaring 4.22.2.0 as a version looked like an RM call rather than mine.
   
   @DaanHoogland @vladimirpetrov, would you prefer to cut that path yourselves and I add the CALLs to it, or should I include it in the backport PR?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org