From e1b051324dc74f7790b18a576bd50617c0a4b90a Mon Sep 17 00:00:00 2001 From: "Rajaa.BARHTAOUI" Date: Fri, 31 Jan 2020 11:14:18 +0100 Subject: [PATCH] feat(xo-web/migration): dont force migration by default (#4364) Fixes #2136 --- CHANGELOG.unreleased.md | 2 + packages/xo-common/src/api-errors.js | 8 ++++ packages/xo-server/src/api/vm.js | 21 ++++++--- packages/xo-server/src/xapi/index.js | 10 +++-- packages/xo-web/src/common/intl/messages.js | 3 ++ packages/xo-web/src/common/xo/index.js | 50 +++++++++++++++------ 6 files changed, 70 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index d43ddbe30..aa3b63aa1 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -15,6 +15,7 @@ - [Tag] Adding a tag: ability to select from existing tags [#2810](https://github.com/vatesfr/xen-orchestra/issues/2810) (PR [#4530](https://github.com/vatesfr/xen-orchestra/pull/4530)) - [Smart backup] Ability to manually add custom tags [#2810](https://github.com/vatesfr/xen-orchestra/issues/2810) (PR [#4648](https://github.com/vatesfr/xen-orchestra/pull/4648)) - [Proxy] Ability to backup VMs via registered proxy [#4254](https://github.com/vatesfr/xen-orchestra/issues/4254) (PR [#4495](https://github.com/vatesfr/xen-orchestra/pull/4495)) +- [VM/Migrate] Ask user before forcing migration [#2136](https://github.com/vatesfr/xen-orchestra/issues/2136) (PR [#4364](https://github.com/vatesfr/xen-orchestra/pull/4364)) ### Bug fixes @@ -29,5 +30,6 @@ > > Rule of thumb: add packages on top. +- xo-common v0.3.0 - xo-server v5.55.0 - xo-web v5.55.0 diff --git a/packages/xo-common/src/api-errors.js b/packages/xo-common/src/api-errors.js index 15039714c..1c2437495 100644 --- a/packages/xo-common/src/api-errors.js +++ b/packages/xo-common/src/api-errors.js @@ -172,3 +172,11 @@ export const patchPrecheckFailed = create(20, ({ errorType, patch }) => ({ }, message: `patch precheck failed: ${errorType}`, })) + +export const operationFailed = create(21, ({ objectId, code }) => ({ + data: { + objectId, + code, + }, + message: 'operation failed', +})) diff --git a/packages/xo-server/src/api/vm.js b/packages/xo-server/src/api/vm.js index 3fd80fb76..fd3b7dc6c 100644 --- a/packages/xo-server/src/api/vm.js +++ b/packages/xo-server/src/api/vm.js @@ -6,6 +6,7 @@ import { forbiddenOperation, invalidParameters, noSuchObject, + operationFailed, unauthorized, } from 'xo-common/api-errors' @@ -453,6 +454,7 @@ export async function migrate({ mapVdisSrs, mapVifsNetworks, migrationNetwork, + force, }) { let mapVdisSrsXapi, mapVifsNetworksXapi const permissions = [] @@ -480,24 +482,29 @@ export async function migrate({ await this.checkPermissions(this.user.id, permissions) - await this.getXapi(vm).migrateVm( - vm._xapiId, - this.getXapi(host), - host._xapiId, - { + await this.getXapi(vm) + .migrateVm(vm._xapiId, this.getXapi(host), host._xapiId, { sr: sr && this.getObject(sr, 'SR')._xapiId, migrationNetworkId: migrationNetwork != null ? migrationNetwork._xapiId : undefined, mapVifsNetworks: mapVifsNetworksXapi, mapVdisSrs: mapVdisSrsXapi, - } - ) + force, + }) + .catch(error => { + if (error?.code !== undefined) { + throw operationFailed({ objectId: vm.id, code: error.code }) + } + throw error + }) } migrate.params = { // Identifier of the VM to migrate. vm: { type: 'string' }, + force: { type: 'boolean', optional: true }, + // Identifier of the host to migrate to. targetHost: { type: 'string' }, diff --git a/packages/xo-server/src/xapi/index.js b/packages/xo-server/src/xapi/index.js index b34fbf2e8..ff9e33ab6 100644 --- a/packages/xo-server/src/xapi/index.js +++ b/packages/xo-server/src/xapi/index.js @@ -1139,6 +1139,7 @@ export default class Xapi extends XapiBase { sr, mapVdisSrs, mapVifsNetworks, + force = false, } ) { // VDIs/SRs mapping @@ -1186,7 +1187,7 @@ export default class Xapi extends XapiBase { vdis, vifsMap, { - force: 'true', + force: force ? 'true' : 'false', } // FIXME: missing param `vgu_map`, it does not cause issues ATM but it // might need to be changed one day. @@ -1440,7 +1441,7 @@ export default class Xapi extends XapiBase { vmId, hostXapi, hostId, - { sr, migrationNetworkId, mapVifsNetworks, mapVdisSrs } = {} + { force = false, mapVdisSrs, mapVifsNetworks, migrationNetworkId, sr } = {} ) { const vm = this.getObject(vmId) const host = hostXapi.getObject(hostId) @@ -1460,11 +1461,12 @@ export default class Xapi extends XapiBase { sr, mapVdisSrs, mapVifsNetworks, + force, }) } else { try { await this.callAsync('VM.pool_migrate', vm.$ref, host.$ref, { - force: 'true', + force: force ? 'true' : 'false', }) } catch (error) { if (error.code !== 'VM_REQUIRES_SR') { @@ -1472,7 +1474,7 @@ export default class Xapi extends XapiBase { } // Retry using motion storage. - await this._migrateVmWithStorageMotion(vm, hostXapi, host, {}) + await this._migrateVmWithStorageMotion(vm, hostXapi, host, { force }) } } } diff --git a/packages/xo-web/src/common/intl/messages.js b/packages/xo-web/src/common/intl/messages.js index ec5e4edcd..9c34c822e 100644 --- a/packages/xo-web/src/common/intl/messages.js +++ b/packages/xo-web/src/common/intl/messages.js @@ -1621,6 +1621,9 @@ const messages = { deleteVmBlockedModalTitle: 'Blocked operation', deleteVmBlockedModalMessage: 'Removing the VM is a blocked operation. Would you like to remove it anyway?', + forceVmMigrateModalTitle: 'Force migration', + forceVmMigrateModalMessage: + 'The VM is incompatible with the CPU features of the destination host. Would you like to force it anyway?', migrateVmModalTitle: 'Migrate VM', migrateVmSelectHost: 'Select a destination host:', migrateVmSelectMigrationNetwork: 'Select a migration network:', diff --git a/packages/xo-web/src/common/xo/index.js b/packages/xo-web/src/common/xo/index.js index 428726576..d45141453 100644 --- a/packages/xo-web/src/common/xo/index.js +++ b/packages/xo-web/src/common/xo/index.js @@ -1,7 +1,6 @@ import asap from 'asap' import cookies from 'cookies-js' import fpSortBy from 'lodash/fp/sortBy' -import Icon from 'icon' import pFinally from 'promise-toolbox/finally' import React from 'react' import reflect from 'promise-toolbox/reflect' @@ -1285,19 +1284,44 @@ export const deleteSnapshots = vms => ) import MigrateVmModalBody from './migrate-vm-modal' // eslint-disable-line import/first -export const migrateVm = (vm, host) => - confirm({ - title: _('migrateVmModalTitle'), - body: , - }).then(params => { - if (!params.targetHost) { - return error( - _('migrateVmNoTargetHost'), - _('migrateVmNoTargetHostMessage') - ) +export const migrateVm = async (vm, host) => { + let params + try { + params = await confirm({ + title: _('migrateVmModalTitle'), + body: , + }) + } catch (error) { + return + } + + if (!params.targetHost) { + return error(_('migrateVmNoTargetHost'), _('migrateVmNoTargetHostMessage')) + } + + try { + await _call('vm.migrate', { vm: vm.id, ...params }) + } catch (error) { + // https://developer-docs.citrix.com/projects/citrix-hypervisor-management-api/en/latest/api-ref-autogen-errors/#vmincompatiblewiththishost + if ( + error != null && + error.data !== undefined && + error.data.code === 'VM_INCOMPATIBLE_WITH_THIS_HOST' + ) { + // Retry with force. + try { + await confirm({ + body: _('forceVmMigrateModalMessage'), + title: _('forceVmMigrateModalTitle'), + }) + } catch (error) { + return + } + return _call('vm.migrate', { vm: vm.id, force: true, ...params }) } - return _call('vm.migrate', { vm: vm.id, ...params }) - }, noop) + throw error + } +} import MigrateVmsModalBody from './migrate-vms-modal' // eslint-disable-line import/first export const migrateVms = vms =>