fix(xapi/VM_destroy): handle is_default_template (#5644)

@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"

@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'

@xen-orchestra/xapi/src/isDefaultTemplate.js

@@ -0,0 +1 @@
+module.exports = vmTpl => vmTpl.is_default_template || vmTpl.other_config.default_template === 'true'

@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),

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

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',
 }))

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 () {

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)
           }
         })
       )

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) {

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++