diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index fb2a6423e1..1c3cb5814e 100644 --- a/CHANGELOG.unreleased.md +++ b/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 diff --git a/packages/xo-server/docs/rolling-pool-update-reboot.md b/packages/xo-server/docs/rolling-pool-update-reboot.md index c4ca3165b6..020b38ff7a 100644 --- a/packages/xo-server/docs/rolling-pool-update-reboot.md +++ b/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"' .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= rpuMigrateVmsBack=false` or with `xe pool-param-set uuid= 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 diff --git a/packages/xo-server/src/api/pool.mjs b/packages/xo-server/src/api/pool.mjs index fa8fbd3529..80215ad052 100644 --- a/packages/xo-server/src/api/pool.mjs +++ b/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, diff --git a/packages/xo-server/src/xapi/mixins/_pool.test.mjs b/packages/xo-server/src/xapi/mixins/_pool.test.mjs new file mode 100644 index 0000000000..82a1f0cb5b --- /dev/null +++ b/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')) + }) +}) diff --git a/packages/xo-server/src/xapi/mixins/pool.mjs b/packages/xo-server/src/xapi/mixins/pool.mjs index 30957b5438..98dbe81d55 100644 --- a/packages/xo-server/src/xapi/mixins/pool.mjs +++ b/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) }, }