feat(vm.delete): release resource set and IP-pool addresses (#460)

Fixes vatesfr/xo-web#1657, fixes vatesfr/xo-web#1748

package.json

@@ -49,6 +49,7 @@
     "cron": "^1.0.9",
     "d3-time-format": "^2.0.0",
     "debug": "^2.1.3",
+    "decorator-synchronized": "^0.2.2",
     "escape-string-regexp": "^1.0.3",
     "event-to-promise": "^0.7.0",
     "exec-promise": "^0.6.1",

src/api/vm.coffee

@@ -7,6 +7,7 @@ concat = require 'lodash/concat'
 endsWith = require 'lodash/endsWith'
 escapeStringRegexp = require 'escape-string-regexp'
 eventToPromise = require 'event-to-promise'
+merge = require 'lodash/merge'
 sortBy = require 'lodash/sortBy'
 startsWith = require 'lodash/startsWith'
 {coroutine: $coroutine} = require 'bluebird'
@@ -294,7 +295,7 @@ exports.create = create
 
 #---------------------------------------------------------------------
 
-delete_ = ({vm, delete_disks: deleteDisks}) ->
+delete_ = $coroutine ({vm, delete_disks: deleteDisks}) ->
   cpus = vm.CPUs.number
   memory = vm.memory.size
 
@@ -316,10 +317,28 @@ delete_ = ({vm, delete_disks: deleteDisks}) ->
       return
     )
 
-    pCatch.call(@releaseLimitsInResourceSet(
-      @computeVmResourcesUsage(vm),
-      resourceSet
-    ), noop)
+    yield Promise.all(map(vm.VIFs, (vifId) =>
+      vif = xapi.getObject(vifId)
+      return pCatch.call(
+        this.allocIpAddresses(
+          vifId,
+          null,
+          concat(vif.ipv4_allowed, vif.ipv6_allowed)
+        ),
+        noop
+      )
+    ))
+
+    resourceSetUsage = @computeVmResourcesUsage(vm)
+    ipPoolsUsage = yield @computeVmIpPoolsUsage(vm)
+
+    pCatch.call(
+      @releaseLimitsInResourceSet(
+        merge(resourceSetUsage, ipPoolsUsage),
+        resourceSet
+      ),
+      noop
+    )
 
   return xapi.deleteVm(vm._xapiId, deleteDisks)
 

src/xo-mixins/ip-pools.js

@@ -1,13 +1,16 @@
 import concat from 'lodash/concat'
+import countBy from 'lodash/countBy'
 import diff from 'lodash/difference'
 import findIndex from 'lodash/findIndex'
 import flatten from 'lodash/flatten'
 import highland from 'highland'
 import includes from 'lodash/includes'
+import isObject from 'lodash/isObject'
 import keys from 'lodash/keys'
 import mapValues from 'lodash/mapValues'
 import pick from 'lodash/pick'
 import remove from 'lodash/remove'
+import synchronized from 'decorator-synchronized'
 import { noSuchObject } from 'xo-common/api-errors'
 import { fromCallback } from 'promise-toolbox'
 
@@ -37,6 +40,11 @@ const normalize = ({
   resourceSets
 })
 
+const _isAddressInIpPool = (address, network, ipPool) => (
+  ipPool.addresses && (address in ipPool.addresses) &&
+  includes(ipPool.networks, isObject(network) ? network.id : network)
+)
+
 // ===================================================================
 
 // Note: an address cannot be in two different pools sharing a
@@ -87,7 +95,14 @@ export default class IpPools {
     throw noSuchObject(id, 'ipPool')
   }
 
-  async getAllIpPools (userId = undefined) {
+  _getAllIpPools (filter) {
+    return streamToArray(this._store.createValueStream(), {
+      filter,
+      mapper: normalize
+    })
+  }
+
+  async getAllIpPools (userId) {
     let filter
     if (userId != null) {
       const user = await this._xo.getUser(userId)
@@ -98,10 +113,7 @@ export default class IpPools {
       }
     }
 
-    return streamToArray(this._store.createValueStream(), {
-      filter,
-      mapper: normalize
-    })
+    return this._getAllIpPools(filter)
   }
 
   getIpPool (id) {
@@ -110,6 +122,30 @@ export default class IpPools {
     })
   }
 
+  async _getAddressIpPool (address, network) {
+    const ipPools = await this._getAllIpPools(ipPool => _isAddressInIpPool(address, network, ipPool))
+
+    return ipPools && ipPools[0]
+  }
+
+  // Returns a map that indicates how many IPs from each IP pool the VM uses
+  // e.g.: { 'ipPool:abc': 3, 'ipPool:xyz': 7 }
+  async computeVmIpPoolsUsage (vm) {
+    const vifs = vm.VIFs
+    const ipPools = []
+    for (const vifId of vifs) {
+      const { allowedIpv4Addresses, allowedIpv6Addresses, $network } = this._xo.getObject(vifId)
+
+      for (const address of concat(allowedIpv4Addresses, allowedIpv6Addresses)) {
+        const ipPool = await this._getAddressIpPool(address, $network)
+        ipPool && ipPools.push(ipPool.id)
+      }
+    }
+
+    return countBy(ipPools, ({ id }) => `ipPool:${id}`)
+  }
+
+  @synchronized
   allocIpAddresses (vifId, addAddresses, removeAddresses) {
     const updatedIpPools = {}
     const limits = {}
@@ -193,7 +229,13 @@ export default class IpPools {
 
     const { getXapi } = this._xo
     return Promise.all(mapToArray(mapVifAddresses, (addresses, vifId) => {
-      const vif = this._xo.getObject(vifId)
+      let vif
+      try {
+        // The IP may not have been correctly deallocated from the IP pool when the VIF was deleted
+        vif = this._xo.getObject(vifId)
+      } catch (error) {
+        return
+      }
       const { allowedIpv4Addresses, allowedIpv6Addresses } = vif
       remove(allowedIpv4Addresses, address => includes(addresses, address))
       remove(allowedIpv6Addresses, address => includes(addresses, address))

src/xo-mixins/resource-sets.js

@@ -2,6 +2,7 @@ import every from 'lodash/every'
 import keyBy from 'lodash/keyBy'
 import remove from 'lodash/remove'
 import some from 'lodash/some'
+import synchronized from 'decorator-synchronized'
 import {
   noSuchObject,
   unauthorized
@@ -272,6 +273,7 @@ export default class {
     await this._save(set)
   }
 
+  @synchronized
   async allocateLimitsInResourceSet (limits, setId) {
     const set = await this.getResourceSet(setId)
     forEach(limits, (quantity, id) => {
@@ -287,6 +289,7 @@ export default class {
     await this._save(set)
   }
 
+  @synchronized
   async releaseLimitsInResourceSet (limits, setId) {
     const set = await this.getResourceSet(setId)
     forEach(limits, (quantity, id) => {