fix(Backup NG): remove all unnecessary snapshots (#3439)

Even those from other schedules.

Fixes #3132

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
 

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 = <T>(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}`)
     }
   }