changes after first review

vatesfr · Nov 27, 2024 · 00948e5 · 00948e5
1 parent 81feb6c
commit 00948e5
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 44 deletions.
diff --git a/packages/xo-server/src/api/vm.mjs b/packages/xo-server/src/api/vm.mjs
@@ -148,16 +148,11 @@ export const create = defer(async function ($defer, params) {
   params.vifs =
     vifs &&
     map(vifs, vif => {
-      if (vif.remove) {
-        return vif
-      }
-
       const network = this.getObject(vif.network)
 
       objectIds.push(network.id)
 
       return {
-        device: vif.device,
         mac: vif.mac,
         network: network._xapiId,
         ipv4_allowed: vif.allowedIpv4Addresses,
@@ -185,7 +180,7 @@ export const create = defer(async function ($defer, params) {
     params.tags = paramsTags !== undefined ? paramsTags.concat(resourceSetTags) : resourceSetTags
   }
 
-  const xapiVm = await xapi.createVm(template._xapiId, params, checkLimits, user.id)
+  const xapiVm = await xapi.createVm(template._xapiId, params, checkLimits, user.id, { destroyAllVifs: true })
   $defer.onFailure(() => xapi.VM_destroy(xapiVm.$ref, { deleteDisks: true, force: true }))
 
   const vm = xapi.xo.addObject(xapiVm)
@@ -320,7 +315,7 @@ create.params = {
       type: 'object',
       properties: {
         // UUID of the network to create the interface in.
-        network: { type: 'string', optional: true },
+        network: { type: 'string' },
 
         mac: {
           optional: true, // Auto-generated per default.
@@ -338,8 +333,6 @@ create.params = {
           type: 'array',
           items: { type: 'string' },
         },
-        device: { type: 'string', optional: true },
-        remove: { type: 'boolean', optional: true },
       },
     },
   },

diff --git a/packages/xo-server/src/xapi/mixins/vm.mjs b/packages/xo-server/src/xapi/mixins/vm.mjs
@@ -47,7 +47,8 @@ const methods = {
       ...props
     } = {},
     checkLimits,
-    creatorId
+    creatorId,
+    { destroyAllVifs = false } = {}
   ) {
     const installMethod = (() => {
       if (installRepository == null) {
@@ -188,10 +189,16 @@ const methods = {
       )
     }
 
+    if (destroyAllVifs) {
+      // Destroys the VIFs cloned from the template.
+      await Promise.all(vm.$VIFs.map(vif => this._deleteVif(vif)))
+    }
+
+    // Creates the VIFs specified by the user.
     if (vifs) {
       const devices = await this.call('VM.get_allowed_VIF_devices', vm.$ref)
       const _vifsToCreate = []
-      const _vifsToRemove = []
+      const _vifsToDestroy = []
       const vmVifByDevice = keyBy(vm.$VIFs, 'device')
 
       vifs.forEach(vif => {
@@ -202,20 +209,20 @@ const methods = {
         }
 
         const vmVif = vmVifByDevice[vif.device]
-        if (vif.remove) {
+        if (vif.destroy) {
           if (vmVif !== undefined) {
-            _vifsToRemove.push(vmVif)
+            _vifsToDestroy.push(vmVif)
           }
           return
         }
 
         if (vmVif !== undefined) {
-          _vifsToRemove.push(vmVif)
+          _vifsToDestroy.push(vmVif)
         }
         _vifsToCreate.push(vif)
       })
 
-      await Promise.all(_vifsToRemove.map(vif => this._deleteVif(vif)))
+      await Promise.all(_vifsToDestroy.map(vif => this._deleteVif(vif)))
       await Promise.all(
         _vifsToCreate.map(vif =>
           this.VIF_create(

diff --git a/packages/xo-server/src/xo-mixins/rest-api.mjs b/packages/xo-server/src/xo-mixins/rest-api.mjs
@@ -771,12 +771,12 @@ export default class RestApi {
               items: {
                 type: 'object',
                 properties: {
-                  remove: { type: 'boolean', optional: true },
-                  network: { type: 'string', optional: true },
+                  destroy: { type: 'boolean', optional: true },
                   device: { type: 'string', optional: true },
-                  mac: { type: 'string', optional: true },
                   ipv4_allowed: { type: 'array', items: { type: 'string' }, optional: true },
                   ipv6_allowed: { type: 'array', items: { type: 'string' }, optional: true },
+                  mac: { type: 'string', optional: true },
+                  network: { type: 'string', optional: true },
                 },
               },
             },

diff --git a/packages/xo-web/src/xo-app/new-vm/index.js b/packages/xo-web/src/xo-app/new-vm/index.js
@@ -38,11 +38,9 @@ import {
   isEmpty,
   isEqual,
   join,
-  keyBy,
   map,
   size,
   slice,
-  sortBy,
   sum,
   sumBy,
 } from 'lodash'
@@ -229,9 +227,6 @@ const isVdiPresent = vdi => !vdi.missing
     keys => keys
   )
   const getHosts = createGetObjectsOfType('host')
-  const getTemplateVifs = createGetObjectsOfType('VIF').filter(
-    createSelector(getTemplate, template => vif => vif.$VM === template?.uuid)
-  )
   return (state, props) => ({
     isAdmin: getIsAdmin(state, props),
     isPoolAdmin: getIsPoolAdmin(state, props),
@@ -246,7 +241,6 @@ const isVdiPresent = vdi => !vdi.missing
     srs: getSrs(state, props),
     template: getTemplate(state, props, props.pool === undefined),
     templates: getTemplates(state, props),
-    templateVifs: getTemplateVifs(state, props),
     userSshKeys: getUserSshKeys(state, props),
     hosts: getHosts(state, props),
   })
@@ -456,8 +450,7 @@ export default class NewVm extends BaseComponent {
 
     // Split allowed IPs into IPv4 and IPv6
     const { VIFs } = state
-    const templateVifs = Object.values(this.props.templateVifs)
-    const _VIFs = map(VIFs, (vif, index) => {
+    const _VIFs = map(VIFs, vif => {
       const _vif = { ...vif }
       if (_vif.mac?.trim() === '') {
         delete _vif.mac
@@ -475,28 +468,12 @@ export default class NewVm extends BaseComponent {
           _vif.allowedIpv6Addresses.push(ip)
         }
       })
-
-      if (index + 1 <= templateVifs.length) {
-        _vif.device = String(index)
-      }
       return _vif
     })
 
     const resourceSet = this._getResourceSet()
     const { template } = this.props
 
-    // In case user deletes some VIF created by the template
-    // We need to mark them with `remove:true`
-    // so that xo-server deletes them properly
-    if (_VIFs.length < templateVifs.length) {
-      const _vifByDevice = keyBy(_VIFs, 'device')
-      templateVifs.forEach(templateVif => {
-        if (_vifByDevice[templateVif.device] === undefined) {
-          _VIFs.push({ remove: true, device: templateVif.device })
-        }
-      })
-    }
-
     // Either use `memory` OR `memory*` params
     let { memory, memoryStaticMax, memoryDynamicMin, memoryDynamicMax } = state
     if ((memoryStaticMax != null || memoryDynamicMin != null) && memoryDynamicMax == null) {
@@ -564,7 +541,7 @@ export default class NewVm extends BaseComponent {
     const storeState = store.getState()
     const isInResourceSet = this._getIsInResourceSet()
     const { state } = this.state
-    const { pool, templateVifs } = this.props
+    const { pool } = this.props
     const resourceSet = this._getResolvedResourceSet()
 
     const existingDisks = {}
@@ -588,7 +565,8 @@ export default class NewVm extends BaseComponent {
     const defaultNetworkIds = this._getDefaultNetworkIds(template)
     forEach(
       // iterate template VIFs in device order
-      sortBy(templateVifs, 'device'),
+      template.VIFs.map(id => getObject(storeState, id, resourceSet)).sort((a, b) => a.device - b.device),
+
       vif => {
         VIFs.push({
           network: pool || isInResourceSet(vif.$network) ? vif.$network : defaultNetworkIds[0],