feat(xo-server): make the RPU/RPR migrate back phase reliable and optional (#10295)

Fixes #10260.
Introduced by #7242.
Takes over #10274.

CHANGELOG.unreleased.md

@@ -14,6 +14,7 @@
 - [XO6/Vm] Add the VM name to VM related actions that open a modal (PR [#10310](https://github.com/vatesfr/xen-orchestra/pull/10310))
 - [XO6] Allow changing which PIF a host uses for its management interface, without deleting and recreating the network config (PR [#10110](https://github.com/vatesfr/xen-orchestra/pull/10110))
 - [XO6/SR] Add dedicated Storage Repository page hosts sidepanel (PR [#10140](https://github.com/vatesfr/xen-orchestra/pull/10140))
+- [Rolling pool update/reboot] A pool can now skip the phase which brings the VMs back to the host they were running on, which halves the migrations of the run (PR [#10295](https://github.com/vatesfr/xen-orchestra/pull/10295))
 
 ### Bug fixes
 
@@ -24,6 +25,7 @@
 - [Backup-archive] No longer create a `cache.json.gz` file on immutable/S3 remote during cleanup, which could not be deleted afterwards and stayed billed forever (PR [#10243](https://github.com/vatesfr/xen-orchestra/pull/10243))
 - [Servers] Fix endless connection attempts to a pool which is already connected through another server entry (PR [#10355](https://github.com/vatesfr/xen-orchestra/pull/10355))
 - [Servers] fix a mishandling in the grace period before marking a pool disconnected, this will keep the ui in sync AND not redownload all the xapi object for a transient issue (PR [#10355](https://github.com/vatesfr/xen-orchestra/pull/10355))
+- [Rolling pool update/reboot] VMs are brought back to the host they were running on more reliably, and a VM that cannot be moved back no longer fails the whole operation (PR [#10295](https://github.com/vatesfr/xen-orchestra/pull/10295))
 
 ### Packages to release
 
@@ -45,6 +47,6 @@
 - @xen-orchestra/backups patch
 - @xen-orchestra/web minor
 - @xen-orchestra/web-core minor
-- xo-server patch
+- xo-server minor
 
 <!--packages-end-->

packages/xo-server/docs/rolling-pool-update-reboot.md

@@ -46,7 +46,15 @@ Old traces are garbage-collected on mtime (`rpu.tracesRetention`, 31 days by def
 
    A step that shows up here but has no matching `"type":"end"` line was still running when the run died. The interrupted task in the XO tasks view shows the same thing per subtask.
 
-4. For a `failure`, the root `end` event has the error. `journalctl` is not reliable for this: the UI polls `listMissingPatches` during the run and produces the exact same `updater plugin is busy` stack traces as a real failure.
+4. A run can end in `success` and still have left VMs on another host. List them:
+
+   ```bash
+   grep '"name":"strandedVms"' <trace>.ndjson
+   ```
+
+   Each entry names the VM, the host it should be running on, and why its last migration was rejected, usually `HOST_NOT_ENOUGH_FREE_MEMORY`. Nothing is broken and the VMs are running; move them back once the pool has room.
+
+5. For a `failure`, the root `end` event has the error. `journalctl` is not reliable for this: the UI polls `listMissingPatches` during the run and produces the exact same `updater plugin is busy` stack traces as a real failure.
 
 ### Failure signatures seen in the field
 
@@ -91,10 +99,18 @@ task.start({ name: 'Rolling pool reboot', poolId: string, poolName: string })
 |  |  ├─ task.start({ name: `Migrating VM ${vmId} back to host ${hostId}`, hostId: string, hostName: string, vmId: string, vmName: string })
 │  │  │  └─ task.end
 │  │  └─ task.end
+|  ├─ task.start({ name: 'Retry migrating VMs back', total: number })
+|  |  ├─ task.start({ name: `Migrating VM ${vmId} back to host ${hostId}`, hostId: string, hostName: string, vmId: string, vmName: string })
+│  │  │  └─ task.end
+│  │  └─ task.end
 │  └─ task.end
 └─ task.end
 ```
 
+`Migrate VMs back` is best effort. The per-host pass brings each VM back to the host it was running on before the reboot. A VM rejected because that host is still full is retried in a `Retry migrating VMs back` pass, which repeats as long as a pass moves at least one VM, so there are zero to n of them, each carrying the number of VMs it tried in its `total`. Whatever is left after that does not fail the run: those VMs are running, only not where they started. They are listed on the `strandedVms` property of `Migrate VMs back`, one entry per VM with `vmId`, `vmName`, `hostId`, `hostName` and the XAPI `code`/`message` of the last rejection, and logged as `could not migrate all the VMs back to their host`.
+
+A pool can opt out of that phase entirely by setting `xo:rpuMigrateVmsBack` to `false` in its `other_config`, with `xo-cli pool.set id=<pool> rpuMigrateVmsBack=false` or with `xe pool-param-set uuid=<pool> other-config:xo:rpuMigrateVmsBack=false`. The run then ends after the reboots and leaves the VMs where the successive `host.evacuate` calls put them, which halves the migrations of the run. `Migrate VMs back` is replaced by a `Skip migrating VMs back` task, so a trace still tells a disabled phase apart from a run which died before reaching it. Without a load balancer to rebalance the pool afterwards, the placement stays scrambled and the host rebooted last stays empty.
+
 ### Rolling pool update
 
 ```
@@ -131,6 +147,10 @@ task.start({ name: 'Rolling pool update', poolId: string, poolName: string })
 │  |  |  ├─ task.start({ name: `Migrating VM ${vmId} back to host ${hostId}`, hostId: string, hostName: string, vmId: string, vmName: string })
 │  │  │  │  └─ task.end
 │  │  │  └─ task.end
+│  |  ├─ task.start({ name: 'Retry migrating VMs back', total: number })
+│  |  |  ├─ task.start({ name: `Migrating VM ${vmId} back to host ${hostId}`, hostId: string, hostName: string, vmId: string, vmName: string })
+│  │  │  │  └─ task.end
+│  │  │  └─ task.end
 │  │  └─ task.end
 │  └─ task.end
 └─ task.end

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

@@ -33,6 +33,7 @@ export async function set({
   backupNetwork,
   migrationCompression,
   migrationNetwork,
+  rpuMigrateVmsBack,
   suspendSr,
   crashDumpSr,
 }) {
@@ -45,6 +46,7 @@ export async function set({
     migrationCompression !== undefined && pool.set_migration_compression(migrationCompression),
     migrationNetwork !== undefined && pool.update_other_config('xo:migrationNetwork', migrationNetwork),
     backupNetwork !== undefined && pool.update_other_config('xo:backupNetwork', backupNetwork),
+    rpuMigrateVmsBack !== undefined && pool.update_other_config('xo:rpuMigrateVmsBack', String(rpuMigrateVmsBack)),
     suspendSr !== undefined && pool.$call('set_suspend_image_SR', suspendSr === null ? Ref.EMPTY : suspendSr._xapiRef),
     crashDumpSr !== undefined &&
       pool.$call('set_crash_dump_SR', crashDumpSr === null ? Ref.EMPTY : crashDumpSr._xapiRef),
@@ -80,6 +82,13 @@ set.params = {
     type: ['string', 'null'],
     optional: true,
   },
+
+  // whether a rolling pool update or reboot brings the VMs back to the host
+  // they were running on, defaults to true
+  rpuMigrateVmsBack: {
+    type: 'boolean',
+    optional: true,
+  },
   suspendSr: {
     type: ['string', 'null'],
     optional: true,

packages/xo-server/src/xapi/mixins/_pool.test.mjs

@@ -0,0 +1,276 @@
+import assert from 'assert/strict'
+import test from 'node:test'
+import { Task } from '@xen-orchestra/mixins/Tasks.mjs'
+
+import poolMethods from './pool.mjs'
+
+const { describe, it } = test
+
+const HOST_CAPACITY = 100
+
+// Fake XAPI modelling only what the migrate back phase depends on: where the
+// VMs run, and how much memory is left on each host.
+class FakeXapi {
+  // vmMemoriesByHost: memory of the VMs initially running on each host
+  //
+  // memoryTakenByHostAfterReboots: memory each host loses to something else
+  // once the whole pool has rebooted, ie during the migrate back phase
+  //
+  // vmsDestroyedAfterReboots: VMs which disappear once the whole pool has
+  // rebooted, as if an operator had destroyed them during the run
+  constructor(vmMemoriesByHost, memoryTakenByHostAfterReboots = [], vmsDestroyedAfterReboots = []) {
+    this.hosts = []
+    this.vms = []
+    vmMemoriesByHost.forEach((memories, i) => {
+      const letter = String.fromCharCode(65 + i)
+      const host = {
+        $type: 'host',
+        $ref: `OpaqueRef:host-${letter}`,
+        $id: `host-${letter}`,
+        uuid: `host-${letter}`,
+        name_label: `host ${letter}`,
+        metrics: `OpaqueRef:metrics-${letter}`,
+        $metrics: { live: true },
+        enabled: true,
+        other_config: { agent_start_time: '0' },
+        $call: async method => {
+          if (method === 'get_vms_which_prevent_evacuation') {
+            return {}
+          }
+          if (method !== 'assert_can_evacuate') {
+            throw new Error(`unexpected host.$call ${method}`)
+          }
+        },
+      }
+      this.hosts.push(host)
+      memories.forEach((memory, j) => {
+        const id = `${letter.toLowerCase()}${j + 1}`
+        this.vms.push({
+          $type: 'VM',
+          $ref: `OpaqueRef:vm-${id}`,
+          $id: `vm-${id}`,
+          uuid: `vm-${id}`,
+          name_label: `vm ${id}`,
+          power_state: 'Running',
+          is_control_domain: false,
+          memory,
+          $resident_on: host,
+        })
+      })
+    })
+
+    this.homeOf = new Map(this.vms.map(vm => [vm.uuid, vm.$resident_on.uuid]))
+    this.objects = { all: [...this.hosts, ...this.vms] }
+    this.pool = {
+      uuid: 'pool-1',
+      master: this.hosts[0].$ref,
+      ha_enabled: false,
+      other_config: {},
+      update_other_config: async () => {},
+    }
+    this._restartHostTimeout = 60e3
+    this._vmShutdownTimeout = 60e3
+
+    this._memoryTakenByHost = memoryTakenByHostAfterReboots
+    this._vmsDestroyedAfterReboots = vmsDestroyedAfterReboots
+    this._nReboots = 0
+
+    // migrations back, the evacuations go through clearHost
+    this.nMigrations = 0
+  }
+
+  _find(key) {
+    return this.getObject(key)
+  }
+
+  _residentVms(host) {
+    return this.vms.filter(vm => vm.$resident_on === host)
+  }
+
+  _free(host) {
+    const taken = this._nReboots < this.hosts.length ? 0 : (this._memoryTakenByHost[this.hosts.indexOf(host)] ?? 0)
+    return HOST_CAPACITY - this._residentVms(host).reduce((sum, vm) => sum + vm.memory, 0) - taken
+  }
+
+  // VMs which are not running on the host they started the run on
+  strayedVms() {
+    return this.vms.filter(vm => vm.$resident_on.uuid !== this.homeOf.get(vm.uuid)).map(vm => vm.uuid)
+  }
+
+  // same contract as xen-api: only throws when no default value is passed
+  getObject(key, defaultValue) {
+    const object = [...this.hosts, ...this.vms].find(_ => _.uuid === key || _.$ref === key)
+    if (object === undefined && arguments.length < 2) {
+      throw new Error(`no such object ${key}`)
+    }
+    return object ?? defaultValue
+  }
+
+  async getField(type, ref, field) {
+    assert.equal(type, 'host')
+    assert.equal(field, 'resident_VMs')
+    return this._residentVms(this._find(ref)).map(vm => vm.$ref)
+  }
+
+  async barrier() {}
+
+  async _waitObjectState() {}
+
+  async call(method) {
+    if (method !== 'host.get_servertime') {
+      throw new Error(`unexpected call ${method}`)
+    }
+    return '0'
+  }
+
+  async callAsync(method) {
+    if (method !== 'host.reboot') {
+      throw new Error(`unexpected callAsync ${method}`)
+    }
+    this._nReboots++
+    if (this._nReboots === this.hosts.length) {
+      this.vms = this.vms.filter(vm => !this._vmsDestroyedAfterReboots.includes(vm.uuid))
+    }
+  }
+
+  // host.evacuate. XAPI does its own placement, this is an approximation: pack
+  // each VM onto the emptiest host with enough memory
+  async clearHost(host) {
+    for (const vm of this._residentVms(host)) {
+      const target = this.hosts
+        .filter(_ => _ !== host && this._free(_) >= vm.memory)
+        .sort((a, b) => this._free(b) - this._free(a))[0]
+      if (target === undefined) {
+        throw Object.assign(new Error('CANNOT_EVACUATE_HOST'), { code: 'CANNOT_EVACUATE_HOST' })
+      }
+      vm.$resident_on = target
+    }
+  }
+
+  async migrateVm(vmId, xapi, hostId) {
+    this.nMigrations++
+    const vm = this._find(vmId)
+    const target = this._find(hostId)
+    if (this._free(target) < vm.memory) {
+      throw Object.assign(new Error(`HOST_NOT_ENOUGH_FREE_MEMORY ${vm.uuid} -> ${target.uuid}`), {
+        code: 'HOST_NOT_ENOUGH_FREE_MEMORY',
+        params: [target.$ref, vm.$ref],
+      })
+    }
+    vm.$resident_on = target
+  }
+}
+
+// the mixin reaches its own methods through `this`
+Object.setPrototypeOf(FakeXapi.prototype, poolMethods)
+
+const rollingPoolReboot = async xapi => {
+  const events = []
+  const parentTask = new Task({
+    properties: { name: 'rolling pool reboot', progress: 0 },
+    onProgress: event => events.push(event),
+  })
+
+  let error
+  await parentTask.run(async () => {
+    try {
+      await poolMethods.rollingPoolReboot.call(xapi, parentTask)
+    } catch (err) {
+      error = err
+    }
+  })
+
+  const strandedVms = events.findLast(_ => _.type === 'property' && _.name === 'strandedVms')?.value ?? []
+  const taskNames = events.filter(_ => _.type === 'start').map(_ => _.properties.name)
+  return { error, strandedVms, taskNames }
+}
+
+describe('rollingPoolReboot', function () {
+  it('brings every VM back to the host it was running on', async function () {
+    const xapi = new FakeXapi([
+      [30, 30],
+      [30, 30],
+      [30, 30],
+      [30, 30],
+    ])
+
+    const { error } = await rollingPoolReboot(xapi)
+
+    assert.equal(error, undefined)
+    assert.deepEqual(xapi.strayedVms(), [])
+  })
+
+  it('retries the migrations rejected for lack of memory', async function () {
+    // host B is only freed by the migrations of the hosts handled after it, so
+    // its second VM is rejected on the first pass whatever the order of hosts
+    const xapi = new FakeXapi([[30], [40, 40], [10], [10]])
+
+    const { error } = await rollingPoolReboot(xapi)
+
+    assert.equal(error, undefined)
+    assert.deepEqual(xapi.strayedVms(), [])
+  })
+
+  it('reports the VMs it could not bring back instead of failing the whole run', async function () {
+    // 45 units of host B are taken while the pool reboots: its own VMs no
+    // longer fit, and no retry can ever change that
+    const xapi = new FakeXapi(
+      [
+        [30, 30],
+        [30, 30],
+        [30, 30],
+        [30, 30],
+      ],
+      [0, 45]
+    )
+
+    const { error, strandedVms } = await rollingPoolReboot(xapi)
+
+    assert.equal(error, undefined)
+    assert.deepEqual(strandedVms.map(_ => _.vmId).sort(), ['vm-a2', 'vm-b1', 'vm-b2'])
+    for (const strandedVm of strandedVms) {
+      assert.equal(strandedVm.code, 'HOST_NOT_ENOUGH_FREE_MEMORY')
+      assert.equal(strandedVm.hostId, xapi.homeOf.get(strandedVm.vmId))
+    }
+  })
+
+  it('skips the VMs destroyed while the pool was rebooting', async function () {
+    const xapi = new FakeXapi(
+      [
+        [30, 30],
+        [30, 30],
+        [30, 30],
+        [30, 30],
+      ],
+      [],
+      ['vm-a2']
+    )
+
+    const { error, strandedVms } = await rollingPoolReboot(xapi)
+
+    assert.equal(error, undefined)
+    assert.deepEqual(strandedVms, [])
+    assert.deepEqual(xapi.strayedVms(), [])
+  })
+
+  it('does not migrate the VMs back when the pool opted out', async function () {
+    const xapi = new FakeXapi([
+      [30, 30],
+      [30, 30],
+      [30, 30],
+      [30, 30],
+    ])
+    xapi.pool.other_config['xo:rpuMigrateVmsBack'] = 'false'
+
+    const { error, taskNames } = await rollingPoolReboot(xapi)
+
+    assert.equal(error, undefined)
+    assert.equal(xapi.nMigrations, 0)
+    assert.notDeepEqual(xapi.strayedVms(), [])
+
+    // the skipped phase leaves a trace, otherwise it cannot be told apart from
+    // a run which died before reaching it
+    assert.ok(taskNames.includes('Skip migrating VMs back'))
+    assert.ok(!taskNames.includes('Migrate VMs back'))
+  })
+})

packages/xo-server/src/xapi/mixins/pool.mjs

@@ -53,6 +53,10 @@ const methods = {
     parentTask,
     { beforeEvacuateVms, beforeRebootHost, ignoreHost, shutdownPinnedVms = false } = {}
   ) {
+    // migrating the VMs back to their host doubles the migrations of the run: a
+    // pool can opt out of that phase by setting this key to `false`
+    const migrateVmsBack = this.pool.other_config['xo:rpuMigrateVmsBack'] !== 'false'
+
     if (this.pool.ha_enabled) {
       const haSrs = this.pool.$ha_statefiles.map(vdi => vdi.SR)
       const haConfig = this.pool.ha_configuration
@@ -136,8 +140,9 @@ const methods = {
       }
     })
 
-    // Steps in the RPR : Evacuate hosts, reboot hosts, migrate VMs back, and potentially updateHosts (beforeEvacuateVms and beforeRebootHost)
-    const nSteps = 3 + Number(beforeEvacuateVms !== undefined) + Number(beforeRebootHost !== undefined)
+    // Steps in the RPR : Evacuate hosts, reboot hosts, and potentially migrate VMs back and updateHosts (beforeEvacuateVms and beforeRebootHost)
+    const nSteps =
+      2 + Number(migrateVmsBack) + Number(beforeEvacuateVms !== undefined) + Number(beforeRebootHost !== undefined)
 
     const progressStep = 100 / nSteps
     const progressStepPerHost = progressStep / hosts.length
@@ -332,32 +337,78 @@ const methods = {
       }
     })
 
-    // Start with the last host since it's the emptiest one after the rolling
-    // update
-    ;[hosts[0], hosts[hosts.length - 1]] = [hosts[hosts.length - 1], hosts[0]]
+    if (migrateVmsBack) {
+      // Handle the hosts in the reverse order of their reboot: the last rebooted
+      // one is the emptiest, and serving it frees memory on the ones still to come
+      hosts.reverse()
+
+      await this._migrateVmsBack(hosts, vmRefsByHost, ignoreHost, () => {
+        rprProgress += progressStepPerHost
+        setProgress(parentTask, rprProgress)
+      })
+    } else {
+      await Task.run({ properties: { name: `Skip migrating VMs back` } }, () => {
+        log.info('migrating the VMs back is disabled on this pool', { pool: this.pool.uuid })
+      })
+    }
+
+    // in case task progress has not been incremented properly
+    setProgress(parentTask, 100)
+  },
+
+  // Bring every VM back to the host it was running on before the reboot.
+  //
+  // onHostDone is called once per host, whether its VMs moved or not, so that
+  // the caller can advance the progress of the whole rolling operation.
+  async _migrateVmsBack(hosts, vmRefsByHost, ignoreHost, onHostDone) {
+    // returns a report entry instead of throwing: a rejected migration is
+    // queued for the retry passes below, it aborts nothing
+    const migrateVmBack = async (vmRef, host) => {
+      const hostId = host.uuid
+      const hostName = host.name_label
+
+      // the VM may have been destroyed since its host was evacuated: there is
+      // nothing left to migrate back, and it must not abort the whole phase
+      const vm = this.getObject(vmRef, undefined)
+      if (vm === undefined) {
+        log.warn('a VM to migrate back no longer exists', { pool: this.pool.uuid, vmRef })
+        return
+      }
+
+      const { uuid: vmId, name_label: vmName } = vm
+      try {
+        await Task.run(
+          { properties: { name: `Migrating VM ${vmId} back to host ${hostId}`, hostId, hostName, vmId, vmName } },
+          () => this.migrateVm(vmId, this, hostId)
+        )
+      } catch (error) {
+        return { code: error.code, host, hostId, hostName, message: error.message, vmId, vmName, vmRef }
+      }
+    }
 
     const migrationsSubtask = new Task({ properties: { name: `Migrate VMs back`, progress: 0 } })
     await migrationsSubtask.run(async () => {
       let done = 0
-      let error
+      let pendingVms = []
+
+      // the retry passes are the last step of this task, hence the extra unit
+      const hostDone = () => {
+        onHostDone()
+        setProgress(migrationsSubtask, (100 * ++done) / (hosts.length + 1))
+      }
+
       for (const host of hosts) {
         const hostId = host.uuid
         const hostName = host.name_label
         if (ignoreHost && ignoreHost(host)) {
-          done++
-          setProgress(migrationsSubtask, (100 * done) / hosts.length)
-          rprProgress += progressStepPerHost
-          setProgress(parentTask, rprProgress)
+          hostDone()
           continue
         }
 
         const vmRefs = vmRefsByHost[hostId]
 
         if (vmRefs === undefined) {
-          done++
-          setProgress(migrationsSubtask, (100 * done) / hosts.length)
-          rprProgress += progressStepPerHost
-          setProgress(parentTask, rprProgress)
+          hostDone()
           continue
         }
         const oneHostMigrationsTask = new Task({
@@ -376,37 +427,50 @@ const methods = {
               continue
             }
 
-            try {
-              const { uuid: vmId, name_label: vmName } = this.getObject(vmRef)
-              await Task.run(
-                {
-                  properties: { name: `Migrating VM ${vmId} back to host ${hostId}`, hostId, hostName, vmId, vmName },
-                },
-                async () => {
-                  await this.migrateVm(vmId, this, hostId)
-                }
-              )
-            } catch (err) {
-              if (error === undefined) {
-                error = err
-              }
+            const pendingVm = await migrateVmBack(vmRef, host)
+            if (pendingVm !== undefined) {
+              pendingVms.push(pendingVm)
             }
             done++
             setProgress(oneHostMigrationsTask, (100 * done) / vmRefs.length)
           }
         })
-        done++
-        setProgress(migrationsSubtask, (100 * done) / hosts.length)
-        rprProgress += progressStepPerHost
-        setProgress(parentTask, rprProgress)
+        hostDone()
       }
-      // making the migration task fail if any of the migrations failed
-      if (error !== undefined) {
-        throw error
+
+      // a VM may have been rejected only because its host was still hosting the
+      // VMs of a host handled later: retry as long as a pass moves at least one
+      while (pendingVms.length > 0) {
+        const failedVms = await Task.run(
+          { properties: { name: `Retry migrating VMs back`, total: pendingVms.length } },
+          async () => {
+            const failedVms = []
+            for (const { host, vmRef } of pendingVms) {
+              const failedVm = await migrateVmBack(vmRef, host)
+              if (failedVm !== undefined) {
+                failedVms.push(failedVm)
+              }
+            }
+            return failedVms
+          }
+        )
+
+        if (failedVms.length === pendingVms.length) {
+          break
+        }
+        pendingVms = failedVms
+      }
+      setProgress(migrationsSubtask, 100)
+
+      // a VM left on another host is not worth failing the whole run: it is
+      // running, only not where it started, and an operator has to move it back
+      if (pendingVms.length > 0) {
+        // host and vmRef have no place in a task property
+        const strandedVms = pendingVms.map(({ host, vmRef, ...strandedVm }) => strandedVm)
+        migrationsSubtask.set('strandedVms', strandedVms)
+        log.warn('could not migrate all the VMs back to their host', { pool: this.pool.uuid, strandedVms })
       }
     })
-    // in case task progress has not been incremented properly
-    setProgress(parentTask, 100)
   },
 }