Alpha162 commented on issue #13399: URL: https://github.com/apache/cloudstack/issues/13399#issuecomment-5296693357
````markdown **Follow-up — bisected this to a specific PR, and I need to correct one thing I asked above.** ### Introduced by [#11531](https://github.com/apache/cloudstack/pull/11531) — *"Track volume usage data at a vm granularity as well"* (fixes #10985), merged **2025-11-12**, milestone **4.22.1**. `schema-42200to42210.sql` lines 22-27 add `vm_id` to `cloud.usage_event`, `cloud_usage.usage_event` and `cloud_usage.usage_volume` via `IDEMPOTENT_ADD_COLUMN`. No index change accompanies it, while `createVolumeHelperEvent()` was changed in the same PR to write two rows sharing `(volume_id, created)`. **Version bound, verified against the tags:** `4.22.0.0` performs a single `persist()` and its `UsageVolumeVO` has no `vm_id` at all. **4.22.0.0 is unaffected; the defect enters in 4.22.1.0.** The author's own review comment on `UsageManagerImpl.java:1486` states the requirement precisely: > "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." That is exactly the collision case, because **both inserts use `event.getCreateDate()`**. This also refines the trigger I gave above: a volume **attached later** is fine, because the attach path produces a distinct timestamp. It's specifically root disks created *during instance deployment*, where both rows necessarily share `created`. In practice, every VM deployment. ### Correcting my open question Above I asked whether `VolumeUsageParser` aggregates both rows, and whether widening the unique key would cause double-counting. I can answer that now — it doesn't. #11531 changed the parser's map key to include `vmId`: ```java - String key = volId + "-" + doId + "-" + size; + String key = volId + "-" + doId + "-" + vmId + "-" + size; ``` and `createUsageRecord()` now passes `vmId` into `UsageVO`. The two rows are designed to produce distinct records — cumulative and per-VM — exactly as the PR description sets out. So the missing DDL is simply `UNIQUE KEY (volume_id, created, vm_id)`, and shipping it is aligned with the feature's intent rather than a workaround. (One nuance for whoever writes it: MySQL treats NULLs in a unique index as distinct, so that key permits multiple `vm_id IS NULL` rows for the same pair. Probably fine, but worth a deliberate decision.) ### The part I think matters most **#13112 was reported on 4.21.0.0 with `usage_type = 13`.** That predates #11531 by a release, so the volume double-insert cannot have caused it — same symptom, different trigger. Which means the durable defects are the other two: `GenericDaoBase.persist()` leaving the caller's transaction unbalanced on the exception path, and the unbounded backwards-only rewind in `parse()`. #11531 didn't create the fragility, it just added a very reliable new way to hit it. So I'd rank the fixes: 1. **`finally` in `GenericDaoBase.persist()`** to release the nesting level. Version-independent, and the reason an isolated constraint violation escalates into permanent aggregation failure instead of being the recoverable log-and-continue that `createHelperRecord()` clearly intends. 2. **Bound the rewind in `parse()`** so a permanently-unprocessable event can't pin aggregation indefinitely. 3. **Ship the missing DDL** for `usage_volume`. Fixing only (3) closes this particular trigger while leaving the mechanism intact — which would be consistent with #13112 having been closed as fixed and the symptom returning here. ```` -- 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]
