From 5f1c1278e39a95a6cde4d5aa5b85fb6cf313b6eb Mon Sep 17 00:00:00 2001 From: Mathieu <70369997+MathieuRA@users.noreply.github.com> Date: Tue, 23 Mar 2021 17:28:18 +0100 Subject: [PATCH] fix(xapi/VM_destroy): handle is_default_template (#5644) --- @xen-orchestra/xapi/package.json | 3 +- @xen-orchestra/xapi/src/index.js | 2 + @xen-orchestra/xapi/src/isDefaultTemplate.js | 1 + @xen-orchestra/xapi/src/vm.js | 12 ++- CHANGELOG.unreleased.md | 2 + packages/xo-common/src/api-errors.js | 3 +- packages/xo-server/src/xapi-object-to-xo.js | 7 +- packages/xo-server/src/xapi/index.js | 81 ++++---------------- packages/xo-server/src/xapi/utils.js | 18 ----- packages/xo-web/src/common/xo/index.js | 9 ++- 10 files changed, 44 insertions(+), 94 deletions(-) create mode 100644 @xen-orchestra/xapi/src/isDefaultTemplate.js diff --git a/@xen-orchestra/xapi/package.json b/@xen-orchestra/xapi/package.json index f73e6bded..929b24d21 100644 --- a/@xen-orchestra/xapi/package.json +++ b/@xen-orchestra/xapi/package.json @@ -21,7 +21,8 @@ "@babel/plugin-proposal-decorators": "^7.3.0", "@babel/preset-env": "^7.3.1", "cross-env": "^7.0.2", - "rimraf": "^3.0.0" + "rimraf": "^3.0.0", + "xo-common": "^0.6.0" }, "peerDependencies": { "xen-api": "^0.31.0" diff --git a/@xen-orchestra/xapi/src/index.js b/@xen-orchestra/xapi/src/index.js index 3454d74dc..9745d7347 100644 --- a/@xen-orchestra/xapi/src/index.js +++ b/@xen-orchestra/xapi/src/index.js @@ -3,6 +3,8 @@ const defer = require('promise-toolbox/defer') const { utcFormat, utcParse } = require('d3-time-format') const { Xapi: Base } = require('xen-api') +exports.isDefaultTemplate = require('./isDefaultTemplate') + // VDI formats. (Raw is not available for delta vdi.) exports.VDI_FORMAT_RAW = 'raw' exports.VDI_FORMAT_VHD = 'vhd' diff --git a/@xen-orchestra/xapi/src/isDefaultTemplate.js b/@xen-orchestra/xapi/src/isDefaultTemplate.js new file mode 100644 index 000000000..837f3b97c --- /dev/null +++ b/@xen-orchestra/xapi/src/isDefaultTemplate.js @@ -0,0 +1 @@ +module.exports = vmTpl => vmTpl.is_default_template || vmTpl.other_config.default_template === 'true' diff --git a/@xen-orchestra/xapi/src/vm.js b/@xen-orchestra/xapi/src/vm.js index 5e18d912f..e838b5d81 100644 --- a/@xen-orchestra/xapi/src/vm.js +++ b/@xen-orchestra/xapi/src/vm.js @@ -7,9 +7,11 @@ const pCatch = require('promise-toolbox/catch') const pRetry = require('promise-toolbox/retry') const { asyncMap } = require('@xen-orchestra/async-map') const { createLogger } = require('@xen-orchestra/log') +const { incorrectState } = require('xo-common/api-errors') const { Ref } = require('xen-api') const extractOpaqueRef = require('./_extractOpaqueRef') +const isDefaultTemplate = require('./isDefaultTemplate') const isVmRunning = require('./_isVmRunning') const { warn } = createLogger('xo:xapi:vm') @@ -275,8 +277,13 @@ module.exports = class Vm { throw new Error('destroy is blocked') } - if (!forceDeleteDefaultTemplate && vm.other_config.default_template === 'true') { - throw new Error('VM is default template') + if (!forceDeleteDefaultTemplate && isDefaultTemplate(vm)) { + throw incorrectState({ + actual: true, + expected: false, + object: vm.$id, + property: 'isDefaultTemplate', + }) } // It is necessary for suspended VMs to be shut down @@ -286,6 +293,7 @@ module.exports = class Vm { } await Promise.all([ + vm.set_is_default_template(false), vm.set_is_a_template(false), vm.update_blocked_operations('destroy', null), vm.update_other_config('default_template', null), diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index 2920a9a1e..289d43ee1 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -13,6 +13,7 @@ - [Proxy] _Force upgrade_ no longer fails on broken proxy - [Proxy] _Redeploy_ now works when the bound VM is missing +- [VM template] Fix confirmation modal doesn't appear on deleting a default template (PR [#5644](https://github.com/vatesfr/xen-orchestra/pull/5644)) ### Packages to release @@ -33,5 +34,6 @@ - xo-server-transport-email minor - @xen-orchestra/fs minor +- @xen-orchestra/xapi minor - xo-server patch - xo-web patch diff --git a/packages/xo-common/src/api-errors.js b/packages/xo-common/src/api-errors.js index f94d01ac2..d04404886 100644 --- a/packages/xo-common/src/api-errors.js +++ b/packages/xo-common/src/api-errors.js @@ -190,11 +190,12 @@ export const notEnoughResources = create(24, data => ({ message: 'not enough resources in resource set', })) -export const incorrectState = create(25, ({ actual, expected, object }) => ({ +export const incorrectState = create(25, ({ actual, expected, object, property }) => ({ data: { actual, expected, object, + property, }, message: 'incorrect state', })) diff --git a/packages/xo-server/src/xapi-object-to-xo.js b/packages/xo-server/src/xapi-object-to-xo.js index 169ed8529..803e7c567 100644 --- a/packages/xo-server/src/xapi-object-to-xo.js +++ b/packages/xo-server/src/xapi-object-to-xo.js @@ -1,3 +1,5 @@ +import { isDefaultTemplate } from '@xen-orchestra/xapi' + import * as sensitiveValues from './sensitive-values' import ensureArray from './_ensureArray' import { extractIpFromVmNetworks } from './_extractIpFromVmNetworks' @@ -445,13 +447,14 @@ const TRANSFORMS = { vm.snapshot_time = toTimestamp(obj.snapshot_time) vm.$snapshot_of = link(obj, 'snapshot_of') } else if (obj.is_a_template) { + const defaultTemplate = isDefaultTemplate(obj) vm.type += '-template' - - if (obj.other_config.default_template === 'true') { + if (defaultTemplate) { vm.id = obj.$ref // use refs for templates as they } vm.CPUs.number = +obj.VCPUs_at_startup + vm.isDefaultTemplate = defaultTemplate vm.template_info = { arch: otherConfig['install-arch'], disks: (function () { diff --git a/packages/xo-server/src/xapi/index.js b/packages/xo-server/src/xapi/index.js index 86cee332f..41520a874 100644 --- a/packages/xo-server/src/xapi/index.js +++ b/packages/xo-server/src/xapi/index.js @@ -35,7 +35,6 @@ import { asInteger, extractOpaqueRef, filterUndefineds, - getVmDisks, canSrHaveNewVdiOfSize, isVmHvm, isVmRunning, @@ -404,7 +403,7 @@ export default class Xapi extends XapiBase { return await this.call('VM.copy', snapshot ? snapshot.$ref : vm.$ref, nameLabel, sr ? sr.$ref : '') } finally { if (snapshot) { - await this._deleteVm(snapshot) + await this.VM_destroy(snapshot.$ref) } } } @@ -564,67 +563,13 @@ export default class Xapi extends XapiBase { ) } - async _deleteVm(vmOrRef, deleteDisks = true, force = false, forceDeleteDefaultTemplate = false) { - const $ref = typeof vmOrRef === 'string' ? vmOrRef : vmOrRef.$ref + /** + * @deprecated Use VM_destroy instead + */ + async deleteVm(vmOrId, deleteDisks = true, force = false, forceDeleteDefaultTemplate = false) { + const $ref = typeof vmOrId === 'string' ? this.getObject(vmOrId).$ref : vmOrId.$ref - // ensure the vm record is up-to-date - const vm = await this.barrier($ref) - - log.debug(`Deleting VM ${vm.name_label}`) - - if (!force && 'destroy' in vm.blocked_operations) { - throw forbiddenOperation('destroy', vm.blocked_operations.destroy.reason) - } - - if (!forceDeleteDefaultTemplate && vm.other_config.default_template === 'true') { - throw forbiddenOperation('destroy', 'VM is default template') - } - - // It is necessary for suspended VMs to be shut down - // to be able to delete their VDIs. - if (vm.power_state !== 'Halted') { - await this.callAsync('VM.hard_shutdown', $ref) - } - - await Promise.all([ - vm.set_is_a_template(false), - vm.update_blocked_operations('destroy', null), - vm.update_other_config('default_template', null), - ]) - - // must be done before destroying the VM - const disks = getVmDisks(vm) - - // this cannot be done in parallel, otherwise disks and snapshots will be - // destroyed even if this fails - await this.callAsync('VM.destroy', $ref) - - return Promise.all([ - asyncMapSettled(vm.$snapshots, snapshot => this._deleteVm(snapshot))::ignoreErrors(), - - vm.power_state === 'Suspended' && - Ref.isNotEmpty(vm.suspend_VDI) && - this._deleteVdi(vm.suspend_VDI)::ignoreErrors(), - - deleteDisks && - asyncMapSettled(disks, async vdi => { - // Dont destroy if attached to other (non control domain) VMs - if (vdi.$VBDs.some(vbd => vbd !== undefined && vbd.VM !== $ref && !vbd.$VM.is_control_domain)) { - return - } - - return pRetry(() => vdi.$callAsync('destroy'), { - // work around a race condition in XCP-ng/XenServer where the disk is not fully unmounted yet. - delay: 5e3, - retries: 5, - when: { code: 'VDI_IN_USE' }, - }) - })::ignoreErrors(), - ]) - } - - async deleteVm(vmId, deleteDisks, force, forceDeleteDefaultTemplate) { - return /* await */ this._deleteVm(this.getObject(vmId), deleteDisks, force, forceDeleteDefaultTemplate) + return this.VM_destroy($ref, { deleteDisks, force, forceDeleteDefaultTemplate }) } getVmConsole(vmId) { @@ -660,7 +605,7 @@ export default class Xapi extends XapiBase { }) if (useSnapshot) { - const destroySnapshot = () => this.deleteVm(exportedVm)::ignoreErrors() + const destroySnapshot = () => this.VM_destroy(exportedVm.$ref)::ignoreErrors() promise.then(_ => _.task.finally(destroySnapshot), destroySnapshot) } @@ -696,7 +641,7 @@ export default class Xapi extends XapiBase { } vm = await this._snapshotVm($cancelToken, vm, snapshotNameLabel) - $defer.onFailure(() => this._deleteVm(vm)) + $defer.onFailure(() => this.VM_destroy(vm.$ref)) } const baseVm = baseVmId && this.getObject(baseVmId) @@ -886,7 +831,7 @@ export default class Xapi extends XapiBase { { suspend_VDI: suspendVdi?.$ref } ) ) - $defer.onFailure(() => this._deleteVm(vm)) + $defer.onFailure(() => this.VM_destroy(vm.$ref)) // 2. Delete all VBDs which may have been created by the import. await asyncMapSettled(vm.$VBDs, vbd => this._deleteVbd(vbd))::ignoreErrors() @@ -997,7 +942,7 @@ export default class Xapi extends XapiBase { ]) if (deleteBase && baseVm) { - this._deleteVm(baseVm)::ignoreErrors() + this.VM_destroy(baseVm.$ref)::ignoreErrors() } await Promise.all([ @@ -1246,7 +1191,7 @@ export default class Xapi extends XapiBase { VCPUs_max: nCpus, }) ) - $defer.onFailure(() => this._deleteVm(vm)) + $defer.onFailure(() => this.VM_destroy(vm.$ref)) // Disable start and change the VM name label during import. await Promise.all([ vm.update_blocked_operations('start', 'OVA import in progress...'), @@ -1389,7 +1334,7 @@ export default class Xapi extends XapiBase { vm.snapshots.map(async ref => { const nameLabel = await this.getField('VM', ref, 'name_label') if (nameLabel.startsWith(snapshotNameLabelPrefix)) { - return this._deleteVm(ref) + return this.VM_destroy(ref) } }) ) diff --git a/packages/xo-server/src/xapi/utils.js b/packages/xo-server/src/xapi/utils.js index 04e3d5459..ea26afb6d 100644 --- a/packages/xo-server/src/xapi/utils.js +++ b/packages/xo-server/src/xapi/utils.js @@ -58,24 +58,6 @@ export const extractOpaqueRef = str => { // ------------------------------------------------------------------- -export const getVmDisks = vm => { - const disks = { __proto__: null } - forEach(vm.$VBDs, vbd => { - let vdi - if ( - // Do not remove CDs and Floppies. - vbd.type === 'Disk' && - // Ignore VBD without VDI. - (vdi = vbd.$VDI) - ) { - disks[vdi.$id] = vdi - } - }) - return disks -} - -// ------------------------------------------------------------------- - const parseDateTimeHelper = utcParse('%Y%m%dT%H:%M:%SZ') export function parseDateTime(str, defaultValue) { diff --git a/packages/xo-web/src/common/xo/index.js b/packages/xo-web/src/common/xo/index.js index 21695d4a7..54fd25a6a 100644 --- a/packages/xo-web/src/common/xo/index.js +++ b/packages/xo-web/src/common/xo/index.js @@ -10,7 +10,7 @@ import { get as getDefined } from '@xen-orchestra/defined' import { pFinally, reflect, tap, tapCatch } from 'promise-toolbox' import { SelectHost } from 'select-objects' import { filter, forEach, get, includes, isEmpty, isEqual, map, once, size, sortBy, throttle } from 'lodash' -import { forbiddenOperation, noHostsAvailable, vmIsTemplate } from 'xo-common/api-errors' +import { forbiddenOperation, incorrectState, noHostsAvailable } from 'xo-common/api-errors' import _ from '../intl' import fetch, { post } from '../fetch' @@ -1141,7 +1141,12 @@ export const deleteTemplates = templates => await Promise.all( map(resolveIds(templates), id => _call('vm.delete', { id }).catch(reason => { - if (vmIsTemplate.is(reason)) { + if ( + incorrectState.is(reason, { + expected: false, + property: 'isDefaultTemplate', + }) + ) { defaultTemplates.push(id) } else { nErrors++