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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
