Copilot commented on code in PR #14672:
URL: https://github.com/apache/cloudstack/pull/14672#discussion_r4545331111
##########
plugins/integrations/veeam-control-service/src/main/java/org/apache/cloudstack/veeam/adapter/ServerAdapter.java:
##########
@@ -2092,14 +2107,18 @@ public List<Checkpoint>
listCheckpointsByInstanceUuid(final String uuid) {
throw new InvalidParameterValueException("VM with ID " + uuid + "
not found");
}
accountService.checkAccess(CallContext.current().getCallingAccount(),
null, false, vo);
- Map<String, String> details =
vmInstanceDetailsDao.listDetailsKeyPairs(vo.getId());
- Checkpoint checkpoint = UserVmVOToCheckpointConverter.toCheckpoint(
- details.get(VmDetailConstants.ACTIVE_CHECKPOINT_ID),
- details.get(VmDetailConstants.ACTIVE_CHECKPOINT_CREATE_TIME));
- if (checkpoint == null) {
- return Collections.emptyList();
+ List<Checkpoint> checkpoints = new ArrayList<>();
+ for (CheckpointResponse response :
kvmBackupExportService.listVmCheckpoints(vo.getId())) {
+ VmCheckpointVO row =
vmCheckpointDao.findByVmIdAndCheckpointId(vo.getId(), response.getId());
+ BackupVO backup = row != null && row.getBackupId() != null ?
backupDao.findByIdIncludingRemoved(row.getBackupId()) : null;
Review Comment:
Listing checkpoints now performs one checkpoint lookup and one backup lookup
for every result, after the service has already loaded all checkpoint rows.
With uncapped retention, a VM with N checkpoints requires up to 2N+1 database
queries for each list request. Return checkpoint metadata in a joined/batched
query (including removed backup records) rather than re-fetching each
checkpoint and backup inside this loop.
##########
server/src/main/java/org/apache/cloudstack/backup/KVMBackupExportServiceImpl.java:
##########
@@ -197,9 +198,12 @@ public Backup createBackup(StartBackupCmd cmd) {
throw new CloudRuntimeException("Instance must be running or
stopped to start Backup");
}
- Backup existingBackup = backupDao.findByVmId(vmId);
- if (existingBackup != null && existingBackup.getStatus() ==
Backup.Status.BackingUp) {
- throw new CloudRuntimeException("Backup already in progress for
Instance: " + vm.getUuid());
+ // An export backup keeps its row only until it is finalized or fails;
libvirt runs one backup job per
+ // domain, so a second one would fail on the host with a less helpful
error.
+ for (Backup existingBackup : backupDao.listByVmId(null, vmId)) {
+ if (IN_PROGRESS_STATUSES.contains(existingBackup.getStatus())) {
+ throw new CloudRuntimeException(String.format("Backup %s is
already in progress for Instance %s", existingBackup.getUuid(), vm.getUuid()));
Review Comment:
Two concurrent requests for the same VM can both pass this check before
either persists its `Queued` backup, so both are accepted and one can fail
later at libvirt. The VM wait job is submitted only after the host backup
starts, so it does not protect this check. Make the check and insert atomic
with a per-VM lock shared across management servers.
--
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]
