usage: widen usage_volume unique key to include vm_id - #13909
Conversation
createVolumeHelperEvent() writes two usage_volume rows for a single VOLUME.CREATE when the event carries vm_id, both using the same created timestamp. The unique key was left at (volume_id, created) when vm_id was added in 4.22.1, so the second insert always fails with a duplicate key error. Uses the existing idempotent helpers per review guidance on apache#13399. Refs apache#13399
Copy of cloud.IDEMPOTENT_ADD_UNIQUE_KEY with the schema changed, so cloud_usage DDL can go through a cloud_usage procedure like the rest of the usage schema changes. Refs apache#13399
Copy of cloud.IDEMPOTENT_DROP_UNIQUE_KEY with the schema changed, so cloud_usage DDL can go through a cloud_usage procedure like the rest of the usage schema changes. Refs apache#13399
Per review on apache#13399, cloud_usage DDL should use cloud_usage-schema procedures rather than reaching across to the cloud schema. Refs apache#13399
|
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)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@winterhazel @DaanHoogland |
@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. |
|
@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: On the red checks? |
|
@blueorangutan package |
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18905 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
abh1sar
left a comment
abh1sar
left a comment
There was a problem hiding this comment.
Thanks for the PR @Alpha162.
One review comment.
cc @DaanHoogland @vladimirpetrov this will need a backport to 4.22.2
cloud_usage.IDEMPOTENT_ADD_UNIQUE_INDEX is already defined in procedures/cloud.idempotent_add_unique_index.sql (the cloud.* filename notwithstanding) and is loaded on every upgrade by DatabaseUpgradeChecker.executeProcedureScripts(), so a new procedure is unnecessary. Raised by @abh1sar in review.
cloud_usage.IDEMPOTENT_DROP_INDEX is already defined in procedures/usage.idempotent_drop_index.sql. DROP INDEX <name> ON <table> is equivalent to ALTER TABLE <table> DROP KEY <name>, so the existing procedure covers this case. Raised by @abh1sar in review.
Use the pre-existing cloud_usage procedures instead of the two added in this PR. schema-41600to41610.sql:72 created this same key with IDEMPOTENT_ADD_UNIQUE_INDEX: CALL `cloud_usage`.`IDEMPOTENT_ADD_UNIQUE_INDEX`( 'cloud_usage.usage_volume', 'id', '(volume_id ASC, created ASC)'); so widening it the same way keeps the table's schema history consistent. Note the two procedures take their arguments in opposite orders: IDEMPOTENT_DROP_INDEX is (index_name, table_name), while IDEMPOTENT_ADD_UNIQUE_INDEX is (table_name, index_name, definition).
winterhazel
left a comment
winterhazel
left a comment
There was a problem hiding this comment.
@Alpha162 the code looks good. This should already fix the Usage reprocessing. I will do some manual testing tomorrow.
On a related note, the Quota plugin probably needs to be updated to consider that a single volume can have more than two usage records for the same period now; otherwise, it will assign the incorrect monetary value. I will properly investigate this and, if required, open a PR addressing it tomorrow as well.
|
[SF] Trillian test result (tid-16778)
|
5 participants
Description
createVolumeHelperEvent()writes twousage_volumerows for a singleVOLUME.CREATEwhen the event carriesvm_id, one cumulative and one per-VM, as intended by #11531. Both useevent.getCreateDate(), but the unique key is still(volume_id, created), so the second insert can never succeed.#11531 added
vm_idtocloud_usage.usage_volumeviaIDEMPOTENT_ADD_COLUMNwithout widening the key. This adds the missing DDL.Uses the existing
cloud_usage.IDEMPOTENT_DROP_INDEX/IDEMPOTENT_ADD_UNIQUE_INDEXprocedures, which are loaded on every upgrade by
DatabaseUpgradeChecker.executeProcedureScripts(). Both carryCONTINUE HANDLERs (1091and1061), so a re-run is a no-op. This mirrorsschema-41600to41610.sql:72, which created this same key withIDEMPOTENT_ADD_UNIQUE_INDEX.Partially addresses #13399. The remaining two defects from that investigation are tracked in #13905 and #13906.
Scope: this prevents the problem, it does not remediate a deployment that already has it
The DDL runs both on fresh installs and on upgrades from 4.22.1.0, so the key is widened
either way. On a deployment that has already hit this, however, widening the key only stops
further collisions; it does not undo the existing damage. The
cloud_usage.usage_eventrowsstuck at
processed = 0stay stuck, the duplicatecloud_usagerows remain, and because therewind in #13906 is unbounded the aggregation window stays pinned to the oldest of those
events. Operators in that position still need to force the stuck events processed and then
dedupe
cloud_usageseparately.Note for anyone doing that: update the flag, never
DELETEfromcloud_usage.usage_event.getMostRecentEventId()isORDER BY id DESC LIMIT 1over the whole table and returns 0 onan empty one, which triggers
COPY_ALL_EVENTSand re-copies every event withprocessed = 0.Note on the widened key and NULLs
vm_idis nullable, andVOLUME.CREATEdeliberately writes one row with it NULL (cumulative)and one with it set (per-VM). MySQL permits multiple NULLs in a UNIQUE index, so
(volume_id, created, vm_id)no longer constrains the cumulative row the way(volume_id, created)did. That is intentional and unavoidable here; keeping it strict wouldrequire a generated column or
NOT NULL DEFAULT 0, both of which change the meaning of thecolumn and need a data migration. The per-VM rows remain constrained, and duplicate cumulative
rows only arise through the re-aggregation in #13906, which seems the right place to address it.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
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.1 today, so it's no longer available to me.
What I can attest to:
UNIQUE KEY id (volume_id, created)was confirmed viaSHOW CREATE TABLEon a live 4.22.1.0 install, alongside thevm_idcolumn added by Track volume usage data at a vm granularity as well #11531.One point worth noting: the key is named
idbut covers(volume_id, created).That name is not incidental;
schema-41600to41610.sql:72sets it explicitly viaIDEMPOTENT_ADD_UNIQUE_INDEX('cloud_usage.usage_volume', 'id', '(volume_id ASC, created ASC)'),so any install that has passed through 4.16.1.0 will have it. The drop targets that
same name.
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), running against a table
whose key name is not
id(the drop no-ops and the add creates a second key instead ofreplacing the first; unlikely, since
schema-41600to41610.sql:72sets that name explicitly),and existing data violating the widened key (not possible; it is less restrictive than the key
it replaces).
Branch targeting
Targeted at
mainper @winterhazel's guidance: a separate backport PR to4.22afterthis merges, rather than a cherry-pick, to avoid merge-forward issues. Per @abh1sar the
backport lands in 4.22.2.