From 7c6e423d242ac06ec954511df4e6394465ea1fcd Mon Sep 17 00:00:00 2001 From: Julien Fontanet Date: Fri, 21 Sep 2018 17:37:49 +0200 Subject: [PATCH] fix(Backup NG): remove all unnecessary snapshots (#3439) Even those from other schedules. Fixes #3132 --- CHANGELOG.md | 1 + .../src/xo-mixins/backups-ng/index.js | 65 +++++++++++-------- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef784c884..845ed11d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ - [Backup NG logs] Fix log's value not being updated in the copy and report button [#3273](https://github.com/vatesfr/xen-orchestra/issues/3273) (PR [#3360](https://github.com/vatesfr/xen-orchestra/pull/3360)) - [Backup NG] Fix issue when *Delete first* was enabled for some of the remotes [#3424](https://github.com/vatesfr/xen-orchestra/issues/3424) (PR [#3427](https://github.com/vatesfr/xen-orchestra/pull/3427)) - [VM/host consoles] Work around a XenServer/XCP-ng issue which lead to some consoles not working [#3432](https://github.com/vatesfr/xen-orchestra/issues/3432) (PR [#3435](https://github.com/vatesfr/xen-orchestra/pull/3435)) +- [Backup NG] Remove extraneous snapshots in case of multiple schedules [#3132](https://github.com/vatesfr/xen-orchestra/issues/3132) (PR [#3439](https://github.com/vatesfr/xen-orchestra/pull/3439)) ### Released packages diff --git a/packages/xo-server/src/xo-mixins/backups-ng/index.js b/packages/xo-server/src/xo-mixins/backups-ng/index.js index 27cf67d7f..50acf33c4 100644 --- a/packages/xo-server/src/xo-mixins/backups-ng/index.js +++ b/packages/xo-server/src/xo-mixins/backups-ng/index.js @@ -11,7 +11,9 @@ import { AssertionError } from 'assert' import { basename, dirname } from 'path' import { countBy, + flatMap, forEach, + groupBy, isEmpty, last, mapValues, @@ -117,14 +119,11 @@ const compareReplicatedVmDatetime = (a: Vm, b: Vm): number => const compareTimestamp = (a: Metadata, b: Metadata): number => a.timestamp - b.timestamp -// returns all entries but the last (retention - 1)-th -// -// the “-1” is because this code is usually run with entries computed before the -// new entry is created +// returns all entries but the last retention-th const getOldEntries = (retention: number, entries?: T[]): T[] => entries === undefined ? [] - : --retention > 0 + : retention > 0 ? entries.slice(0, -retention) : entries @@ -777,7 +776,7 @@ export default class BackupNg { ) } - const { id: jobId, settings } = job + const { id: jobId, mode, settings } = job const { id: scheduleId } = schedule let exportRetention: number = getSetting(settings, 'exportRetention', [ @@ -888,18 +887,29 @@ export default class BackupNg { }) ) - $defer(() => - asyncMap( + snapshot = await xapi.barrier(snapshot.$ref) + + let baseSnapshot = mode === 'delta' ? last(snapshots) : undefined + snapshots.push(snapshot) + + // snapshots to delete due to the snapshot retention settings + const snapshotsToDelete = flatMap( + groupBy(snapshots, _ => _.other_config['xo:backup:schedule']), + (snapshots, scheduleId) => getOldEntries( - snapshotRetention, - snapshots.filter( - _ => _.other_config['xo:backup:schedule'] === scheduleId - ) - ), - _ => xapi.deleteVm(_) - ) + getSetting(settings, 'snapshotRetention', [scheduleId]), + snapshots + ) ) + // delete unused snapshots + await asyncMap(snapshotsToDelete, vm => { + // snapshot and baseSnapshot should not be deleted right now + if (vm !== snapshot && vm !== baseSnapshot) { + return xapi.deleteVm(vm) + } + }) + snapshot = ((await wrapTask( { logger, @@ -922,10 +932,10 @@ export default class BackupNg { const metadataFilename = `${vmDir}/${basename}.json` - if (job.mode === 'full') { + if (mode === 'full') { // TODO: do not create the snapshot if there are no snapshotRetention and // the VM is not running - if (snapshotRetention === 0) { + if (snapshotsToDelete.includes(snapshot)) { $defer.call(xapi, 'deleteVm', snapshot) } @@ -991,7 +1001,7 @@ export default class BackupNg { )::ignoreErrors() const oldBackups: MetadataFull[] = (getOldEntries( - exportRetention, + exportRetention - 1, await this._listVmBackups( handler, vm, @@ -1038,7 +1048,7 @@ export default class BackupNg { const { $id: srId, xapi } = sr const oldVms = getOldEntries( - copyRetention, + copyRetention - 1, listReplicatedVms(xapi, scheduleId, srId, vmUuid) ) @@ -1085,11 +1095,13 @@ export default class BackupNg { ], noop // errors are handled in logs ) - } else if (job.mode === 'delta') { - if (snapshotRetention === 0) { - // only keep the snapshot in case of success + } else if (mode === 'delta') { + if (snapshotsToDelete.includes(snapshot)) { $defer.onFailure.call(xapi, 'deleteVm', snapshot) } + if (snapshotsToDelete.includes(baseSnapshot)) { + $defer.onSuccess.call(xapi, 'deleteVm', baseSnapshot) + } // JFT: TODO: remove when enough time has passed (~2018-09) // @@ -1114,9 +1126,8 @@ export default class BackupNg { ) ) - let baseSnapshot, fullVdisRequired + let fullVdisRequired await (async () => { - baseSnapshot = (last(snapshots): Vm | void) if (baseSnapshot === undefined) { return } @@ -1276,7 +1287,7 @@ export default class BackupNg { const fork = forkExport() const oldBackups: MetadataDelta[] = (getOldEntries( - exportRetention, + exportRetention - 1, await this._listVmBackups( handler, vm, @@ -1380,7 +1391,7 @@ export default class BackupNg { const { $id: srId, xapi } = sr const oldVms = getOldEntries( - copyRetention, + copyRetention - 1, listReplicatedVms(xapi, scheduleId, srId, vmUuid) ) @@ -1426,7 +1437,7 @@ export default class BackupNg { noop // errors are handled in logs ) } else { - throw new Error(`no exporter for backup mode ${job.mode}`) + throw new Error(`no exporter for backup mode ${mode}`) } }