Copilot commented on code in PR #13048:
URL: https://github.com/apache/cloudstack/pull/13048#discussion_r3115394821
##########
engine/schema/src/main/resources/META-INF/db/schema-42200to42210-cleanup.sql:
##########
@@ -18,3 +18,9 @@
--;
-- Schema upgrade cleanup from 4.22.0.0 to 4.22.1.0
--;
+
+-- Entries remaining on `cloud`.`resource_reservation` during the upgrade
process are stale, so delete them.
+-- This script was added to normalize volume/primary storage reservations that
got stuck due to a bug on VM deployment,
+-- but it is more interesting to introduce a smarter logic to clean these
stale reservations in the future without the need
+-- for upgrades (for instance, by having a heartbeat_time column for the
reservations and automatically cleaning old entries).
+DELETE FROM `cloud`.`resource_reservation`;
Review Comment:
The upgrade cleanup deletes *all* rows from `cloud.resource_reservation`,
even though the described bug affects stale `volume`/`primary_storage`
reservations. This broad delete could also wipe unrelated in-flight
reservations (e.g., public IP/template/etc.) if an operator mistakenly runs the
upgrade while other management servers are still operating. Consider
restricting the DELETE to the affected resource types (and optionally to rows
older than a conservative threshold) to reduce blast radius while still fixing
the reported issue.
```suggestion
-- Entries remaining on `cloud`.`resource_reservation` during the upgrade
process for the affected resource types are stale, so delete them.
-- This script was added to normalize volume/primary storage reservations
that got stuck due to a bug on VM deployment,
-- so scope the cleanup to those reservation types to avoid removing
unrelated in-flight reservations.
-- It would still be more interesting to introduce a smarter logic to clean
these stale reservations in the future without the need
-- for upgrades (for instance, by having a heartbeat_time column for the
reservations and automatically cleaning old entries).
DELETE FROM `cloud`.`resource_reservation`
WHERE `resource_type` IN ('volume', 'primary_storage');
```
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -4347,7 +4344,7 @@ private UserVm getUncheckedUserVmResource(DataCenter
zone, String hostName, Stri
List<CheckedReservation> checkedReservations = new ArrayList<>();
try {
- checkedReservations = reserveStorageResourcesForVm(owner,
diskOfferingId, diskSize, dataDiskInfoList, rootDiskOfferingId, offering,
volumesSize);
+ reserveStorageResourcesForVm(checkedReservations, owner,
diskOfferingId, diskSize, dataDiskInfoList, rootDiskOfferingId, offering,
volumesSize);
Review Comment:
This change fixes a subtle leak path where partially-created
`CheckedReservation`s could be lost if an exception is thrown mid-reservation.
There are existing `UserVmManagerImplTest` tests that use
`MockedConstruction<CheckedReservation>`; adding a focused unit test that
forces `CheckedReservation` construction to throw after the first/second
reservation and then asserts that previously-created reservations are still
closed (i.e., `close()` invoked) would help prevent regressions.
--
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]
