Various fix on VM migration

CHANGELOG.unreleased.md

@@ -12,6 +12,9 @@
 > Users must be able to say: “I had this issue, happy to know it's fixed”
 
 - [ISO SR] During ISO migration, the destination SRs were not ISO SRs [#7392](https://github.com/vatesfr/xen-orchestra/issues/7392) (PR [#7431](https://github.com/vatesfr/xen-orchestra/pull/7431))
+- [VM/Migration] Fix VDIs that were not migrated to the destination SR (PR [#7360](https://github.com/vatesfr/xen-orchestra/pull/7360))
+- [Home/VM] VMs migration from the home view will no longer execute a [Migration with Storage Motion](https://github.com/vatesfr/xen-orchestra/blob/master/docs/manage_infrastructure.md#vm-migration-with-storage-motion-vmmigrate_send) unless it is necessary [Forum#8279](https://xcp-ng.org/forum/topic/8279/getting-errors-when-migrating-4-out-5-vmguest/)(PR [#7360](https://github.com/vatesfr/xen-orchestra/pull/7360))
+- [VM/Migration] SR is no longer required if you select a migration network (PR [#7360](https://github.com/vatesfr/xen-orchestra/pull/7360))
 
 ### Packages to release
 

packages/xo-server/src/xapi/index.mjs

@@ -495,17 +495,37 @@ export default class Xapi extends XapiBase {
       bypassAssert = false,
     }
   ) {
+    const srRef = sr !== undefined ? hostXapi.getObject(sr).$ref : undefined
     const getDefaultSrRef = once(() => {
-      if (sr !== undefined) {
-        return hostXapi.getObject(sr).$ref
-      }
       const defaultSr = host.$pool.$default_SR
       if (defaultSr === undefined) {
         throw new Error(`This operation requires a default SR to be set on the pool ${host.$pool.name_label}`)
       }
       return defaultSr.$ref
     })
 
+    // VDIs/SRs mapping
+    // For VDI:
+    // - If a map of VDI -> SR was explicitly passed: use it
+    // - Else if SR was explicitly passed: use it
+    // - Else if VDI SR is reachable from the destination host: use it
+    // - Else: use the migration main SR or the pool's default SR (error if none of them is defined)
+    function getMigrationSrRef(vdi) {
+      if (mapVdisSrs[vdi.$id] !== undefined) {
+        return hostXapi.getObject(mapVdisSrs[vdi.$id]).$ref
+      }
+
+      if (srRef !== undefined) {
+        return srRef
+      }
+
+      if (isSrConnected(vdi.$SR)) {
+        return vdi.$SR.$ref
+      }
+
+      return getDefaultSrRef()
+    }
+
     const hostPbds = new Set(host.PBDs)
     const connectedSrs = new Map()
     const isSrConnected = sr => {
@@ -518,10 +538,6 @@ export default class Xapi extends XapiBase {
     }
 
     // VDIs/SRs mapping
-    // For VDI:
-    // - If SR was explicitly passed: use it
-    // - Else if VDI SR is reachable from the destination host: use it
-    // - Else: use the migration main SR or the pool's default SR (error if none of them is defined)
     // For VDI-snapshot:
     // - If VDI-snapshot is an orphan snapshot: same logic as a VDI
     // - Else: don't add it to the map (VDI -> SR). It will be managed by the XAPI (snapshot will be migrated to the same SR as its parent active VDI)
@@ -534,12 +550,7 @@ export default class Xapi extends XapiBase {
         if (vdi.$snapshot_of !== undefined) {
           continue
         }
-        vdis[vdi.$ref] =
-          mapVdisSrs[vdi.$id] !== undefined
-            ? hostXapi.getObject(mapVdisSrs[vdi.$id]).$ref
-            : isSrConnected(vdi.$SR)
-              ? vdi.$SR.$ref
-              : getDefaultSrRef()
+        vdis[vdi.$ref] = getMigrationSrRef(vdi)
       }
     }
 

packages/xo-web/src/common/intl/messages.js

@@ -2062,6 +2062,7 @@ const messages = {
   vmsWithDuplicatedMacAddressesMessage:
     '{nVms, number} VM{nVms, plural, one {} other {s}} contain{nVms, plural, one {s} other {}} duplicate MAC addresses or {nVms, plural, one {has} other {have}} the same MAC addresses as other running VMs. Do you want to continue?',
   ignoreVdi: 'Ignore this VDI',
+  selectDestinationSr: 'Select a destination SR',
 
   // ----- Servers -----
   enableServerErrorTitle: 'Enable server',

packages/xo-web/src/common/xo/choose-sr-for-each-vdis-modal/index.js

@@ -59,22 +59,29 @@ export default class ChooseSrForEachVdisModal extends Component {
       ignorableVdis = false,
       mainSrPredicate = isSrWritable,
       placeholder,
+      required,
       srPredicate = mainSrPredicate,
       value: { mainSr, mapVdisSrs },
       vdis,
     } = props
 
     return (
       <div>
-        <SelectSr
-          onChange={this._onChangeMainSr}
-          placeholder={placeholder !== undefined ? placeholder : _('chooseSrForEachVdisModalMainSr')}
-          predicate={mainSrPredicate}
-          required
-          value={mainSr}
-        />
+        <SingleLineRow>
+          <Col size={6}>{_('selectDestinationSr')}</Col>
+          <Col size={6}>
+            <SelectSr
+              onChange={this._onChangeMainSr}
+              placeholder={placeholder !== undefined ? placeholder : _('chooseSrForEachVdisModalMainSr')}
+              predicate={mainSrPredicate}
+              required={required}
+              value={mainSr}
+            />
+          </Col>
+        </SingleLineRow>
+        {!required && <i>{_('optionalEntry')}</i>}
         <br />
-        {!isEmpty(vdis) && mainSr != null && (
+        {!isEmpty(vdis) && (
           <Collapsible buttonText={_('chooseSrForEachVdisModalSelectSr')} collapsible size='small'>
             <br />
             <Container>

packages/xo-web/src/common/xo/index.js

@@ -1689,17 +1689,16 @@ export const migrateVm = async (vm, host) => {
     return
   }
 
-  const { migrationNetwork, sr, targetHost } = params
+  const { sr, srRequired, targetHost } = params
 
   if (!targetHost) {
     return error(_('migrateVmNoTargetHost'), _('migrateVmNoTargetHostMessage'))
   }
 
-  // Workaround to prevent VM's VDIs from unexpectedly migrating to the default SR
-  // if migration network is defined, the SR is required.
-  if (migrationNetwork !== undefined && sr === undefined) {
+  if (srRequired && sr === undefined) {
     return error(_('migrateVmNoSr'), _('migrateVmNoSrMessage'))
   }
+  delete params.srRequired
 
   try {
     await _call('vm.migrate', { vm: vm.id, ...params })
@@ -1734,6 +1733,11 @@ export const migrateVms = vms =>
       return error(_('migrateVmNoTargetHost'), _('migrateVmNoTargetHostMessage'))
     }
 
+    if (params.srRequired && params.sr === undefined) {
+      return error(_('migrateVmNoTargetHost'), _('migrateVmNoTargetHostMessage'))
+    }
+    delete params.srRequired
+
     const { mapVmsMapVdisSrs, mapVmsMapVifsNetworks, migrationNetwork, sr, targetHost, vms } = params
     Promise.all(
       map(vms, ({ id }) =>

packages/xo-web/src/common/xo/migrate-vm-modal/index.js

@@ -122,6 +122,7 @@ export default class MigrateVmModalBody extends BaseComponent {
       migrationNetwork: this.state.migrationNetworkId,
       sr: resolveId(this.state.targetSrs.mainSr),
       targetHost: this.state.host && this.state.host.id,
+      srRequired: !this.state.doNotMigrateVdis,
     }
   }
 
@@ -222,14 +223,14 @@ export default class MigrateVmModalBody extends BaseComponent {
             </Col>
           </SingleLineRow>
         </div>
-        {host && (!doNotMigrateVdis || migrationNetworkId != null) && (
+        {host && (
           <div className={styles.groupBlock}>
             <SingleLineRow>
               <Col size={12}>
                 <ChooseSrForEachVdisModal
                   mainSrPredicate={this._getSrPredicate()}
                   onChange={this.linkState('targetSrs')}
-                  required
+                  required={!doNotMigrateVdis}
                   value={targetSrs}
                   vdis={vdis}
                 />

packages/xo-web/src/common/xo/migrate-vms-modal/index.js

@@ -101,33 +101,40 @@ export default class MigrateVmsModalBody extends BaseComponent {
     )
   }
 
-  componentDidMount() {
-    this._selectHost(this.props.host)
-  }
-
   get value() {
     const { host } = this.state
     const vms = filter(this.props.vms, vm => vm.$container !== host.id)
     if (!host || isEmpty(vms)) {
       return { vms }
     }
     const { networks, pifs, vbdsByVm, vifsByVm } = this.props
-    const { doNotMigrateVdi, doNotMigrateVmVdis, migrationNetworkId, networkId, smartVifMapping, srId } = this.state
+    const {
+      doNotMigrateVdi,
+      doNotMigrateVmVdis,
+      migrationNetworkId,
+      noVdisMigration,
+      networkId,
+      smartVifMapping,
+      srId,
+    } = this.state
 
     // Map VM --> ( Map VDI --> SR )
-    // 2021-02-16: Fill the map (VDI -> SR) with *all* the VDIs to avoid unexpectedly migrating them to the wrong SRs:
-    // - Intra-pool: a VDI will only be migrated to the selected SR if the VDI was on a local SR.
+    // - Intra-pool: a VDI will only be migrated to the selected SR if the VDI was on a local SR or if an SR was explicitly selected.
     // - Inter-pool: all VDIs will be migrated to the selected SR.
     const mapVmsMapVdisSrs = {}
     forEach(vbdsByVm, (vbds, vm) => {
       const mapVdisSrs = {}
       forEach(vbds, vbd => {
         const vdi = vbd.VDI
         if (!vbd.is_cd_drive && vdi) {
-          mapVdisSrs[vdi] = doNotMigrateVmVdis[vm] || doNotMigrateVdi[vdi] ? this._getObject(vdi).$SR : srId
+          if (!doNotMigrateVmVdis[vm] && !doNotMigrateVdi[vdi]) {
+            mapVdisSrs[vdi] = srId
+          }
         }
       })
-      mapVmsMapVdisSrs[vm] = mapVdisSrs
+      if (!isEmpty(mapVdisSrs)) {
+        mapVmsMapVdisSrs[vm] = mapVdisSrs
+      }
     })
 
     const defaultNetwork =
@@ -160,6 +167,7 @@ export default class MigrateVmsModalBody extends BaseComponent {
       mapVmsMapVifsNetworks,
       migrationNetwork: migrationNetworkId,
       sr: srId,
+      srRequired: !noVdisMigration,
       targetHost: host.id,
       vms,
     }
@@ -212,7 +220,7 @@ export default class MigrateVmsModalBody extends BaseComponent {
       networkId: defaultMigrationNetworkId,
       noVdisMigration,
       smartVifMapping: true,
-      srId: defaultSrConnectedToHost ? defaultSrId : undefined,
+      srId: !noVdisMigration && defaultSrConnectedToHost ? defaultSrId : undefined,
     })
   }
 
@@ -254,27 +262,11 @@ export default class MigrateVmsModalBody extends BaseComponent {
             </Col>
           </SingleLineRow>
         </div>
-        {host !== undefined && (
-          <div style={LINE_STYLE}>
-            <SingleLineRow>
-              <Col size={6}>{_('migrateVmSelectMigrationNetwork')}</Col>
-              <Col size={6}>
-                <SelectNetwork
-                  onChange={this._selectMigrationNetwork}
-                  predicate={this._getMigrationNetworkPredicate()}
-                  required={!intraPool}
-                  value={migrationNetworkId}
-                />
-              </Col>
-            </SingleLineRow>
-            {intraPool && <i>{_('optionalEntry')}</i>}
-          </div>
-        )}
-        {host && (!noVdisMigration || migrationNetworkId != null) && (
+        {host !== undefined && [
           <div key='sr' style={LINE_STYLE}>
             <SingleLineRow>
               <Col size={6}>
-                {!intraPool ? _('migrateVmsSelectSr') : _('migrateVmsSelectSrIntraPool')}{' '}
+                {_('selectDestinationSr')}{' '}
                 {(defaultSrId === undefined || !defaultSrConnectedToHost) && (
                   <Tooltip
                     content={
@@ -292,11 +284,31 @@ export default class MigrateVmsModalBody extends BaseComponent {
                 )}
               </Col>
               <Col size={6}>
-                <SelectSr onChange={this._selectSr} predicate={this._getSrPredicate()} required value={srId} />
+                <SelectSr
+                  onChange={this._selectSr}
+                  predicate={this._getSrPredicate()}
+                  required={!noVdisMigration}
+                  value={srId}
+                />
               </Col>
             </SingleLineRow>
-          </div>
-        )}
+            {noVdisMigration && <i>{_('optionalEntry')}</i>}
+          </div>,
+          <div style={LINE_STYLE} key='network'>
+            <SingleLineRow>
+              <Col size={6}>{_('migrateVmSelectMigrationNetwork')}</Col>
+              <Col size={6}>
+                <SelectNetwork
+                  onChange={this._selectMigrationNetwork}
+                  predicate={this._getMigrationNetworkPredicate()}
+                  required={!intraPool}
+                  value={migrationNetworkId}
+                />
+              </Col>
+            </SingleLineRow>
+            {intraPool && <i>{_('optionalEntry')}</i>}
+          </div>,
+        ]}
         {host && !intraPool && (
           <div key='network' style={LINE_STYLE}>
             <SingleLineRow>