Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14310 +/- ##
============================================
+ Coverage 18.00% 18.04% +0.04%
- Complexity 16219 16273 +54
============================================
Files 5936 5938 +2
Lines 535716 535890 +174
Branches 65596 65615 +19
============================================
+ Hits 96459 96706 +247
+ Misses 428268 428182 -86
- Partials 10989 11002 +13
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:
|
ec6e3ef to
5dbc2ef
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Stateful billing chronology and in-memory event deduplication warrant final human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes backup usage accounting so size changes are billed proportionally while reducing redundant metric events.
Changes:
- Splits active usage rows when backup sizes change and merges duplicates.
- Prevents duplicate rows when offerings are reassigned.
- Publishes metrics only when values change and reports current offerings consistently.
| File | Description |
|---|---|
usage/src/test/java/com/cloud/usage/UsageManagerImplTest.java |
Tests assignment and metric handling. |
usage/src/main/java/com/cloud/usage/UsageManagerImpl.java |
Prevents duplicate active usage and forwards event timestamps. |
server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java |
Tests metric deduplication and zero-size reporting. |
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java |
Caches published metrics and reports current offerings. |
engine/schema/src/test/java/com/cloud/usage/dao/UsageBackupDaoImplTest.java |
Tests usage-row splitting and duplicate consolidation. |
engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDaoImpl.java |
Implements timestamped metric history and active-row merging. |
engine/schema/src/main/java/com/cloud/usage/dao/UsageBackupDao.java |
Extends the backup usage DAO contract. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Pair<Long, Long> key = new Pair<>(vm.getId(), offeringId); | ||
| Pair<Long, Long> metric = new Pair<>(backupSize, protectedSize); | ||
| published.put(key, metric); | ||
| if (metric.equals(lastPublished.get(key))) { |
There was a problem hiding this comment.
what happens with two management servers if one sends a new size and then goes down? if the size goes back after that, the other one still remembers the old value and skips it, so billing stays on the wrong size
There was a problem hiding this comment.
Thanks @Damans227 . That's a big issue with the current implementation.
I'll think about an alternative.
There was a problem hiding this comment.
Added a new BackupUsageMetrics table in DB to track the last updated value.
Also added a lock so that two management servers don't race while syncing the backup metrics.
Please take a look @Damans227
A BACKUP.USAGE.METRIC event overwrote the size of the usage_backup row in place, so the parser billed the last reported size for the whole period. A size change now closes the active row at the event date and opens a new one. Unchanged sizes are ignored since the metric can repeat an unchanged value, closed rows are no longer updated, and duplicate active rows for the same VM and offering are merged. Fixes apache#13070.
Removing a backup offering while keeping the backups leaves its usage row active. Re-assigning the same offering added another active row, so the same backups were billed twice. Skip the new row when one is active.
The backup sync published a BACKUP.USAGE.METRIC event for every VM on every run (every 5 minutes by default) even when nothing changed, which made up most of the usage_event rows. Keep the last value published per VM and offering in the new backup_usage_metric table and skip unchanged ones. The event and the stored value are committed together. Every management server runs the backup sync, so each zone is now synced under a per-zone lock. The usage metrics are then always compared against the last published values, and two servers no longer add or remove the same out-of-band backups at the same time. The VM's current offering is now always reported. It was skipped when the VM still had backups from an earlier offering, so deleting its last backup never brought its usage down to zero.
5dbc2ef to
48169b4
Compare
|
@blueorangutan package |
Publish the usage event for an offering whose backups are all gone and delete its row in backup_usage_metric in one transaction, as is already done when publishing the metric, so a stale stored value cannot outlive the usage.
6df3ef2 to
8086034
Compare
| txn.commit(); | ||
| } catch (final Exception e) { | ||
| txn.rollback(); | ||
| logger.error("Error updating backup metrics: " + e.getMessage(), e); |
There was a problem hiding this comment.
what happens if this fails once? the size is already saved as sent, so it never gets sent again and billing stays on the old size until it changes
Description
This PR fixes how VM backup usage is recorded and billed:
BACKUP.USAGE.METRICevent overwrote the size of the activeusage_backuprow in place, soBackupUsageParserapplied the last size to the entire aggregation window. A size change now closes the active row at the event date and opens a new row with the new size, so each size is billed only for the time it was in effect. An unchanged size is a no-op, rows that are already removed are no longer updated, and duplicate active rows for the same VM and offering are merged into one. A metric for a VM and offering without active usage is still ignored, now with a warning.BACKUP.USAGE.METRICpublished on every backup sync. The backup sync published the metric for every VM on every run (backup.framework.sync.interval, 5 minutes by default) whether or not it changed. The last value published per VM and offering is now kept in a newbackup_usage_metrictable, and the metric is only published when the current value differs from it. The event and the stored value are committed in the same transaction, so the stored value is always what the usage server last received, whichever management server sent it.backup.sync.<zoneId>), asBucketUsageTaskdoes for bucket usage. This keeps the usage metric comparison consistent across management servers, and also stops two servers from adding or removing the same out-of-band backups (Veeam, Networker) at the same time, which created duplicate backup rows and double resource count updates.Behaviour changes:
cloud.backup_usage_metric(upgrade from 4.22.1.0 to 4.22.2.0). A row is removed when the offering usage is removed (BACKUP.OFFERING.BACKUPS.DEL).usage_backupgains a row per size change instead of one row per assignment, and billing records are split accordingly.BACKUP.USAGE.METRICis published once per actual change instead of once per VM per sync, so far fewer rows incloud.usage_eventandcloud_usage.usage_event. Existing rows are left in place. Management server restarts do not republish.usage_backuprows created by the re-assign bug.Fixes: #13070
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
How Has This Been Tested?
Tested on 4.22 with NAS backup on an NFS repository. The usage job ran with a 15 minute aggregation range so each window could be checked.
Before the fix:
usage_backuprows from an earlier remove and re-assign of the offering, so every window had two billing records.BACKUP.USAGE.METRIC(73,669 events with 64 distinct values for 2 VMs).Usage after the fix (VM backup usage records per 15 minute window):
Before the fix, each of these windows would have been billed at a single size for the full 15 minutes (twice, while the duplicate row existed).
Also:
BACKUP.USAGE.METRICevents were published for 16 hours overnight while sizes did not change. Before, one was published per VM every 5 minutes.How did you try to break this feature and the system with this change?