fix(xo-server/self): always de-allocate resources in case of failure (#4861)

See support#2240

CHANGELOG.unreleased.md

@@ -16,6 +16,7 @@
 > Users must be able to say: “I had this issue, happy to know it's fixed”
 
 - [XOSAN] Fix the installer (PR [#4839](https://github.com/vatesfr/xen-orchestra/pull/4839))
+- [Self] When a Self Service related operation fails, always revert the quotas to what they were before the operation (PR [#4861](https://github.com/vatesfr/xen-orchestra/pull/4861))
 
 ### Released packages
 

packages/xo-server/src/api/disk.js

@@ -1,6 +1,7 @@
-import createLogger from '@xen-orchestra/log'
-import pump from 'pump'
 import convertVmdkToVhdStream from 'xo-vmdk-to-vhd'
+import createLogger from '@xen-orchestra/log'
+import defer from 'golike-defer'
+import pump from 'pump'
 import { format, JsonRpcError } from 'json-rpc-peer'
 import { noSuchObject } from 'xo-common/api-errors'
 import { peekFooterFromVhdStream } from 'vhd-lib'
@@ -11,7 +12,10 @@ const log = createLogger('xo:disk')
 
 // ===================================================================
 
-export async function create({ name, size, sr, vm, bootable, position, mode }) {
+export const create = defer(async function(
+  $defer,
+  { name, size, sr, vm, bootable, position, mode }
+) {
   const attach = vm !== undefined
 
   do {
@@ -22,6 +26,9 @@ export async function create({ name, size, sr, vm, bootable, position, mode }) {
           sr.id,
         ])
         await this.allocateLimitsInResourceSet({ disk: size }, resourceSet)
+        $defer.onFailure(() =>
+          this.releaseLimitsInResourceSet({ disk: size }, resourceSet)
+        )
 
         break
       } catch (error) {
@@ -42,6 +49,7 @@ export async function create({ name, size, sr, vm, bootable, position, mode }) {
     size,
     sr: sr._xapiId,
   })
+  $defer.onFailure(() => xapi.deleteVdi(vdi.$id))
 
   if (attach) {
     await xapi.createVbd({
@@ -54,7 +62,7 @@ export async function create({ name, size, sr, vm, bootable, position, mode }) {
   }
 
   return vdi.$id
-}
+})
 
 create.description = 'create a new disk on a SR'
 

packages/xo-server/src/api/vdi.js

@@ -1,5 +1,6 @@
 // FIXME: rename to disk.*
 
+import defer from 'golike-defer'
 import { invalidParameters } from 'xo-common/api-errors'
 import { reduce } from 'lodash'
 
@@ -15,11 +16,11 @@ export async function delete_({ vdi }) {
     undefined
   )
 
-  if (resourceSet !== undefined) {
-    await this.allocateLimitsInResourceSet({ disk: -vdi.size }, resourceSet)
-  }
-
   await this.getXapi(vdi).deleteVdi(vdi._xapiId)
+
+  if (resourceSet !== undefined) {
+    await this.releaseLimitsInResourceSet({ disk: vdi.size }, resourceSet)
+  }
 }
 
 delete_.params = {
@@ -35,7 +36,7 @@ export { delete_ as delete }
 // -------------------------------------------------------------------
 
 // FIXME: human readable strings should be handled.
-export async function set(params) {
+export const set = defer(async function($defer, params) {
   const { vdi } = params
   const xapi = this.getXapi(vdi)
   const ref = vdi._xapiRef
@@ -67,6 +68,12 @@ export async function set(params) {
         { disk: size - vdi.size },
         resourceSetId
       )
+      $defer.onFailure(() =>
+        this.releaseLimitsInResourceSet(
+          { disk: size - vdi.size },
+          resourceSetId
+        )
+      )
     } else {
       await this.checkPermissions(this.user.id, [[vdi.$SR, 'operate']])
     }
@@ -89,7 +96,7 @@ export async function set(params) {
       await xapi.call(`VDI.set_${field}`, ref, `${params[param]}`)
     }
   }
-}
+})
 
 set.params = {
   // Identifier of the VDI to update.

packages/xo-server/src/api/vm.js

@@ -45,7 +45,7 @@ const extract = (obj, prop) => {
 }
 
 // TODO: Implement ACLs
-export async function create(params) {
+export const create = defer(async function($defer, params) {
   const { user } = this
   const resourceSet = extract(params, 'resourceSet')
   const template = extract(params, 'template')
@@ -143,14 +143,17 @@ export async function create(params) {
   if (resourceSet) {
     await this.checkResourceSetConstraints(resourceSet, user.id, objectIds)
     checkLimits = async limits2 => {
-      await this.allocateLimitsInResourceSet(
-        assignWith({}, limits, limits2, (l1 = 0, l2) => l1 + l2),
-        resourceSet
+      const _limits = assignWith({}, limits, limits2, (l1 = 0, l2) => l1 + l2)
+      await this.allocateLimitsInResourceSet(_limits, resourceSet)
+      $defer.onFailure(() =>
+        this.releaseLimitsInResourceSet(_limits, resourceSet)
       )
     }
   }
 
   const xapiVm = await xapi.createVm(template._xapiId, params, checkLimits)
+  $defer.onFailure(() => xapi.deleteVm(xapiVm.$id, true, true))
+
   const vm = xapi.xo.addObject(xapiVm)
 
   if (resourceSet) {
@@ -179,7 +182,7 @@ export async function create(params) {
   }
 
   return vm.id
-}
+})
 
 create.params = {
   affinityHost: { type: 'string', optional: true },
@@ -338,14 +341,17 @@ create.resolve = {
 
 // -------------------------------------------------------------------
 
-async function delete_({
-  delete_disks, // eslint-disable-line camelcase
-  force,
-  forceDeleteDefaultTemplate,
-  vm,
+const delete_ = defer(async function(
+  $defer,
+  {
+    delete_disks, // eslint-disable-line camelcase
+    force,
+    forceDeleteDefaultTemplate,
+    vm,
 
-  deleteDisks = delete_disks,
-}) {
+    deleteDisks = delete_disks,
+  }
+) {
   const xapi = this.getXapi(vm)
 
   this.getAllAcls().then(acls => {
@@ -375,11 +381,15 @@ async function delete_({
   )
 
   // Update resource sets
+  let resourceSet
   if (
     vm.type === 'VM' && // only regular VMs
-    xapi.xo.getData(vm._xapiId, 'resourceSet') != null
+    (resourceSet = xapi.xo.getData(vm._xapiId, 'resourceSet')) != null
   ) {
-    this.setVmResourceSet(vm._xapiId, null)::ignoreErrors()
+    await this.setVmResourceSet(vm._xapiId, null)::ignoreErrors()
+    $defer.onFailure(() =>
+      this.setVmResourceSet(vm._xapiId, resourceSet)::ignoreErrors()
+    )
   }
 
   return xapi.deleteVm(
@@ -388,7 +398,7 @@ async function delete_({
     force,
     forceDeleteDefaultTemplate
   )
-}
+})
 
 delete_.params = {
   id: { type: 'string' },
@@ -531,7 +541,7 @@ migrate.resolve = {
 
 // -------------------------------------------------------------------
 
-export async function set(params) {
+export const set = defer(async function($defer, params) {
   const VM = extract(params, 'VM')
   const xapi = this.getXapi(VM)
   const vmId = VM._xapiId
@@ -555,7 +565,11 @@ export async function set(params) {
 
     if (resourceSet) {
       try {
-        return await this.allocateLimitsInResourceSet(limits, resourceSet)
+        await this.allocateLimitsInResourceSet(limits, resourceSet)
+        $defer.onFailure(() =>
+          this.releaseLimitsInResourceSet(limits, resourceSet)
+        )
+        return
       } catch (error) {
         // if the resource set no longer exist, behave as if the VM is free
         if (!noSuchObject.is(error)) {
@@ -568,7 +582,7 @@ export async function set(params) {
       throw unauthorized()
     }
   })
-}
+})
 
 set.params = {
   // Identifier of the VM to update.

packages/xo-server/src/xo-mixins/resource-sets.js

@@ -1,4 +1,5 @@
 import asyncMap from '@xen-orchestra/async-map'
+import deferrable from 'golike-defer'
 import synchronized from 'decorator-synchronized'
 import {
   every,
@@ -354,7 +355,8 @@ export default class {
     await Promise.all(mapToArray(sets, set => this._save(set)))
   }
 
-  async setVmResourceSet(vmId, resourceSetId) {
+  @deferrable
+  async setVmResourceSet($defer, vmId, resourceSetId) {
     const xapi = this._xo.getXapi(vmId)
     const previousResourceSetId = xapi.xo.getData(vmId, 'resourceSet')
 
@@ -371,6 +373,9 @@ export default class {
 
     if (resourceSetId != null) {
       await this.allocateLimitsInResourceSet(resourcesUsage, resourceSetId)
+      $defer.onFailure(() =>
+        this.releaseLimitsInResourceSet(resourcesUsage, resourceSetId)
+      )
     }
 
     if (
@@ -381,6 +386,9 @@ export default class {
         resourcesUsage,
         previousResourceSetId
       )
+      $defer.onFailure(() =>
+        this.allocateLimitsInResourceSet(resourcesUsage, previousResourceSetId)
+      )
     }
 
     await xapi.xo.setData(
@@ -388,6 +396,13 @@ export default class {
       'resourceSet',
       resourceSetId === undefined ? null : resourceSetId
     )
+    $defer.onFailure(() =>
+      xapi.xo.setData(
+        vmId,
+        'resourceSet',
+        previousResourceSetId === undefined ? null : previousResourceSetId
+      )
+    )
 
     if (previousResourceSetId !== undefined) {
       await this._xo.removeAclsForObject(vmId)