jmsperu commented on code in PR #12847:
URL: https://github.com/apache/cloudstack/pull/12847#discussion_r4333661701
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -167,26 +210,188 @@ public boolean takeBackup(final VirtualMachine vm) {
command.setVolumePaths(volumePaths);
}
- BackupAnswer answer = null;
+ // Concurrent backups are bounded per host across all management
servers: the BackingUp row of this
+ // backup is the reservation of one of the host's slots, created only
while the host has a free slot.
+ // The wait happens here, on the management server, never on the
agent's worker threads.
+ final BackupVO backupVO = parallel
+ ? reserveHostBackupSlot(vm, host.getId(), backupPath,
NASBackupParallelMaxPerHost.valueIn(zoneId),
NASBackupParallelQueueTimeout.valueIn(zoneId))
+ : createBackupObject(vm, backupPath);
+ boolean succeeded = false;
try {
- answer = (BackupAnswer) agentManager.send(host.getId(), command);
- } catch (AgentUnavailableException e) {
- throw new CloudRuntimeException("Unable to contact backend control
plane to initiate backup");
- } catch (OperationTimedoutException e) {
- throw new CloudRuntimeException("Operation to initiate backup
timed out, please try again");
- }
-
- if (answer != null && answer.getResult()) {
+ final BackupAnswer answer;
+ try {
+ answer = (BackupAnswer) agentManager.send(host.getId(),
command);
+ } catch (AgentUnavailableException e) {
+ throw new CloudRuntimeException("Unable to contact backend
control plane to initiate backup");
+ } catch (OperationTimedoutException e) {
+ throw new CloudRuntimeException("Operation to initiate backup
timed out, please try again");
+ }
+ if (answer == null || !answer.getResult()) {
+ return false;
+ }
+ succeeded = true;
backupVO.setDate(new Date());
backupVO.setSize(answer.getSize());
backupVO.setStatus(Backup.Status.BackedUp);
backupVO.setBackedUpVolumes(BackupManagerImpl.createVolumeInfoFromVolumes(volumeDao.findByInstance(vm.getId())));
return backupDao.update(backupVO.getId(), backupVO);
- } else {
- backupVO.setStatus(Backup.Status.Failed);
- backupDao.remove(backupVO.getId());
+ } finally {
+ if (!succeeded) {
+ // Releases the slot: a failed or interrupted backup must
never stay BackingUp.
Review Comment:
Good catch, you are right: the management server gave up after the global
`wait` (30 min) while the host keeps the script running up to the agent's
`cmds.timeout` (2 h), and the `finally` then removed a backup that was still
being written. Fixed in 8c441c586f:
- New zone setting `backup.nas.backup.timeout` (default 7200, same as the
agent's `cmds.timeout`) is set as the TakeBackupCommand's wait, so the
management server now waits as long as the host may run the backup.
`commands.timeout` for TakeBackupCommand still wins if set.
- If the wait runs out anyway, the outcome is unknown, so the row stays
`BackingUp` and keeps its host slot, on every management server since the slot
is the row. The job fails with "may still be running on the host" instead of
removing anything. Only a failure the host actually reported, or a host that
never got the command, marks the row Failed and removes it as before.
- Slots cannot leak: a `BackingUp` row twice the wait old (at least wait + 5
min) no longer counts towards the cap, which covers a management server or
agent crash, and the periodic backup sync marks it `Error`. It stays in the
list so it can be seen and deleted, because its files on the NAS may be
incomplete.
New tests cover the timed out case (row kept, slot still taken when a second
management server asks), host reported failure, unreachable host, success, and
the abandoned row being marked Error and kept. NASBackupProviderTest 15/15,
checkstyle clean.
Ready for your two management server test whenever you have time.
--
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]
