fix(backups): clean snapshot correctly on failure

The previous implementation only considered version upgrades and did not take into account the installation of missing patches.

See zammad#21487
Introduced by 85ec261

@xen-orchestra/backups/_runners/_vmRunners/_AbstractXapi.mjs

@@ -2,6 +2,7 @@ import assert from 'node:assert'
 import groupBy from 'lodash/groupBy.js'
 import ignoreErrors from 'promise-toolbox/ignoreErrors'
 import { asyncMap } from '@xen-orchestra/async-map'
+import { createLogger } from '@xen-orchestra/log'
 import { decorateMethodsWith } from '@vates/decorate-with'
 import { defer } from 'golike-defer'
 import { formatDateTime } from '@xen-orchestra/xapi'
@@ -10,6 +11,8 @@ import { getOldEntries } from '../../_getOldEntries.mjs'
 import { Task } from '../../Task.mjs'
 import { Abstract } from './_Abstract.mjs'
 
+const { info, warn } = createLogger('xo:backups:AbstractXapi')
+
 export const AbstractXapi = class AbstractXapiVmBackupRunner extends Abstract {
   constructor({
     config,
@@ -171,6 +174,20 @@ export const AbstractXapi = class AbstractXapiVmBackupRunner extends Abstract {
     }
   }
 
+  // this will delete current snapshot in case of failure
+  // to ensure any retry will start with a clean state, especially in the case of rolling snapshots
+  #removeCurrentSnapshotOnFailure() {
+    if (this._mustDoSnapshot() && this._exportedVm !== undefined) {
+      info('will delete snapshot on failure', { vm: this._vm, snapshot: this._exportedVm })
+      assert.notStrictEqual(
+        this._vm.$ref,
+        this._exportedVm.$ref,
+        'there should have a snapshot, but vm and snapshot have the same ref'
+      )
+      return this._xapi.VM_destroy(this._exportedVm.$ref)
+    }
+  }
+
   async _fetchJobSnapshots() {
     const jobId = this._jobId
     const vmRef = this._vm.$ref
@@ -271,11 +288,17 @@ export const AbstractXapi = class AbstractXapiVmBackupRunner extends Abstract {
           await this._exportedVm.update_blocked_operations({ pool_migrate, migrate_send })
         }
       }
+    } catch (error) {
+      try {
+        await this.#removeCurrentSnapshotOnFailure()
+      } catch (removeSnapshotError) {
+        warn('fail removing current snapshot', { error: removeSnapshotError })
+      }
+      throw error
     } finally {
       if (startAfter) {
         ignoreErrors.call(vm.$callAsync('start', false, false))
       }
-
       await this._fetchJobSnapshots()
       await this._removeUnusedSnapshots()
     }

CHANGELOG.unreleased.md

@@ -22,6 +22,7 @@
 - [Plugins/audit] Don't log `tag.getAllConfigured` calls
 - [Remotes] Correctly clear error when the remote is tested with success
 - [Import/VMWare] Fix importing last snapshot (PR [#7370](https://github.com/vatesfr/xen-orchestra/pull/7370))
+- [Host/Reboot] Fix false positive warning when restarting an host after updates (PR [#7366](https://github.com/vatesfr/xen-orchestra/pull/7366))
 
 ### Packages to release
 

packages/xo-server/src/api/host.mjs

@@ -1,3 +1,4 @@
+import semver from 'semver'
 import { createLogger } from '@xen-orchestra/log'
 import assert from 'assert'
 import { format } from 'json-rpc-peer'
@@ -136,13 +137,38 @@ export async function restart({
     const pool = this.getObject(host.$poolId, 'pool')
     const master = this.getObject(pool.master, 'host')
     const hostRebootRequired = host.rebootRequired
-    if (hostRebootRequired && host.id !== master.id && host.version === master.version) {
-      throw incorrectState({
-        actual: hostRebootRequired,
-        expected: false,
-        object: master.id,
-        property: 'rebootRequired',
-      })
+
+    // we are currently in an host upgrade process
+    if (hostRebootRequired && host.id !== master.id) {
+      // this error is not ideal but it means that the pool master must be fully upgraded/rebooted before the current host can be rebooted.
+      //
+      // there is a single error for the 3 cases because the client must handle them the same way
+      const throwError = () =>
+        incorrectState({
+          actual: hostRebootRequired,
+          expected: false,
+          object: master.id,
+          property: 'rebootRequired',
+        })
+
+      if (semver.lt(master.version, host.version)) {
+        log.error(`master version (${master.version}) is older than the host version (${host.version})`, {
+          masterId: master.id,
+          hostId: host.id,
+        })
+        throwError()
+      }
+
+      if (semver.eq(master.version, host.version)) {
+        if ((await this.getXapi(host).listMissingPatches(master._xapiId)).length > 0) {
+          log.error('master has missing patches', { masterId: master.id })
+          throwError()
+        }
+        if (master.rebootRequired) {
+          log.error('master needs to reboot')
+          throwError()
+        }
+      }
     }
   }