XenAPI: XCP2.1+ Swallow VDI_NOT_IN_MAP Exception

Changes within XenAPI have enforced a more strict policy when checking
assert_can_migrate.  In particular when checking the source_vdi:dest_sr
mapping it insists that the SR actually exist.  This is not a problem for
local disks, however this assertation is called extremely early in the
live migration process (check_can_migrate_source) which is called from
conductor, which makes a problem for attached volumes.

This early in the process the host has just barely been chosen and no SR
information has been configured yet for these volumes or their initiators.
Additionally we cannot prepare this SR any earlier as BDM information is
not set up until the pre_live_migration method. With the options to either
skip this assertion completely or swallow the exception, I have chosen to
swallow the exception.  My reasons for this are two-fold:

1. --block-migration can be called without regard for whether an iSCSI
volume is attached, and we still want to ensure that VIF, CPU and other
factors are checked, and not just skip all checks entirely.
2. Currently the Assert only exists within the --block-migration code
base but this needs to change. A future commit will remove this logic
to ensure that the commit runs without this flag. Once that is done we
want to be able to continue to use this Exception swallow logic rather
than continuing to skip the assert for all XCP2.1.0+ even without volumes.

This decision should help us handle less work in a future commit and does not
seem to align with the goals of that commit, where it does align properly here.
This commit still changes very little of the current codebase and puts us in
a good position to refactor the way this is handled at a later date, while
adding a TODO note to correct VM.assert_can_migrate only running during a
block migration.

Additionally there seems to be some confusion that the mapping data that is
generated during this initial trip through _call_live_migrate_command is needed
to continue along the code, however this data appears to be purely used to send
the mapping information through the assertation call, and is then discarded.
The only data returned from these methods is the original dest_data which
is carried into the live_migration method. The _call_live_migration method is
called again during the live_migration() method, and during this time it does
need that mapping to send along to XenAPI for the actual migration, but not
yet. Because this codebase is so confusing, I am providing a little bit of
context on the movement of these variables with some psuedocode:

---CONDUCTOR.TASKS.LIVE_MIGRATE---
LiveMigrationTask.Execute()
	self._find_destination() <-----
	Unrelated Work
	compute.live_migration(self, host, instance, destination,
			       block_migrate, migration, migrate_data)

LiveMigrationTask._find_destination()
	Scheduler Things.  Gets a Dest ref.
	_check_compatible_with_source_hyp
	_call_livem_checks_on_host(host) <-----
		_check_can_live_migrate_destination()
	returns Host Node Name and Host Compute.  That's all.

---COMPUTE.MANAGER---
_do / _check_live_migration_destination
	dest_check_data = xenops.can_live_migrate_destination

	(Checks for the Assert)
	try:
		migrate_data = check_can_live_migrate_source(dest_check_data)

	return migrate_data

---VMOPS--
check_can_migrate_source(self, ctxt, instance_ref, dest_check_data)
	if block_migration:
		_call_live_migration_command(assert_can_migrate)
			_generate_vdi_map()
				Does NOT return
			ALSO Does NOT return
return dest_check_data

The changes made to address this issue are a fairly simple oslo_utils
version check. To pull this data I created two new host related methods
within VMops as well as a new import of oslo_config.versionutils.
I believe these methods ultimately belong in the xenapi.host class, but
for two very small methods I believed it better to avoid such a large import
to get minimal data.

Finally, an adjustment to the Fake XenAPI driver had to be made as it currently
does not include the host details beyond hostname environment in the
create_host check.  The change amends the stub dictionary to include this data.

Change-Id: Ie5295564a68f877fc061376c00f3fa706370a251
Closes-Bug: #1704071

nova/tests/unit/virt/xenapi/test_vmops.py

@@ -1794,6 +1794,78 @@ class LiveMigrateHelperTestCase(VMOpsTestBase):
                 self._call_live_migrate_command_with_migrate_send_data,
                 migrate_data)
 
+    @mock.patch.object(vmops.VMOps, '_call_live_migrate_command')
+    def test_check_can_live_migrate_source_with_xcp2(self, mock_call_migrate):
+        ctxt = 'ctxt'
+        fake_instance = {"name": "fake_instance"}
+        fake_dest_check_data = objects.XenapiLiveMigrateData()
+        fake_dest_check_data.block_migration = True
+        mock_call_migrate.side_effect = \
+            xenapi_fake.xenapi_session.XenAPI.Failure(['VDI_NOT_IN_MAP'])
+
+        with mock.patch.object(self.vmops,
+                               '_get_iscsi_srs') as mock_iscsi_srs, \
+            mock.patch.object(self.vmops,
+                              '_get_vm_opaque_ref') as mock_vm, \
+            mock.patch.object(self.vmops,
+                              '_get_host_software_versions') as mock_host_sw:
+            mock_iscsi_srs.return_value = []
+            mock_vm.return_value = 'vm_ref'
+            mock_host_sw.return_value = {'platform_name': 'XCP',
+                                         'platform_version': '2.1.0'}
+            fake_returned_data = self.vmops.check_can_live_migrate_source(
+                ctxt, fake_instance, fake_dest_check_data)
+
+        self.assertEqual(fake_returned_data, fake_dest_check_data)
+
+    @mock.patch.object(vmops.VMOps, '_call_live_migrate_command')
+    def test_check_can_live_migrate_source_with_xcp2_vif_raise(self,
+                                                            mock_call_migrate):
+        ctxt = 'ctxt'
+        fake_instance = {"name": "fake_instance"}
+        fake_dest_check_data = objects.XenapiLiveMigrateData()
+        fake_dest_check_data.block_migration = True
+        mock_call_migrate.side_effect = \
+            xenapi_fake.xenapi_session.XenAPI.Failure(['VIF_NOT_IN_MAP'])
+
+        with mock.patch.object(self.vmops,
+                               '_get_iscsi_srs') as mock_iscsi_srs, \
+            mock.patch.object(self.vmops,
+                              '_get_vm_opaque_ref') as mock_vm, \
+            mock.patch.object(self.vmops,
+                              '_get_host_software_versions') as mock_host_sw:
+            mock_iscsi_srs.return_value = []
+            mock_vm.return_value = 'vm_ref'
+            mock_host_sw.return_value = {'platform_name': 'XCP',
+                                         'platform_version': '2.1.0'}
+            self.assertRaises(exception.MigrationPreCheckError,
+                              self.vmops.check_can_live_migrate_source, ctxt,
+                              fake_instance, fake_dest_check_data)
+
+    @mock.patch.object(vmops.VMOps, '_call_live_migrate_command')
+    def test_check_can_live_migrate_source_with_xcp2_sw_raise(self,
+                                                            mock_call_migrate):
+        ctxt = 'ctxt'
+        fake_instance = {"name": "fake_instance"}
+        fake_dest_check_data = objects.XenapiLiveMigrateData()
+        fake_dest_check_data.block_migration = True
+        mock_call_migrate.side_effect = \
+            xenapi_fake.xenapi_session.XenAPI.Failure(['VDI_NOT_IN_MAP'])
+
+        with mock.patch.object(self.vmops,
+                               '_get_iscsi_srs') as mock_iscsi_srs, \
+            mock.patch.object(self.vmops,
+                              '_get_vm_opaque_ref') as mock_vm, \
+            mock.patch.object(self.vmops,
+                              '_get_host_software_versions') as mock_host_sw:
+            mock_iscsi_srs.return_value = []
+            mock_vm.return_value = 'vm_ref'
+            mock_host_sw.return_value = {'platform_name': 'XCP',
+                                         'platform_version': '1.1.0'}
+            self.assertRaises(exception.MigrationPreCheckError,
+                              self.vmops.check_can_live_migrate_source, ctxt,
+                              fake_instance, fake_dest_check_data)
+
     def test_generate_vif_network_map(self):
         with mock.patch.object(self._session.VIF,
                                'get_other_config') as mock_other_config, \

nova/virt/xenapi/fake.py

@@ -109,11 +109,14 @@ def _create_pool(name_label):
                           {'name_label': name_label})
 
 
-def create_host(name_label, hostname='fake_name', address='fake_addr'):
+def create_host(name_label, hostname='fake_name', address='fake_addr',
+                software_version={'platform_name': 'fake_platform',
+                                  'platform_version': '1.0.0'}):
     host_ref = _create_object('host',
-                               {'name_label': name_label,
+                              {'name_label': name_label,
-                                'hostname': hostname,
+                               'hostname': hostname,
-                                'address': address})
+                               'address': address,
+                               'software_version': software_version})
     host_default_sr_ref = _create_local_srs(host_ref)
     _create_local_pif(host_ref)
 

nova/virt/xenapi/vmops.py

@@ -36,6 +36,7 @@ from oslo_utils import netutils
 from oslo_utils import strutils
 from oslo_utils import timeutils
 from oslo_utils import units
+from oslo_utils import versionutils
 import six
 
 from nova import block_device
@@ -2227,6 +2228,22 @@ class VMOps(object):
         host_uuid = self._get_host_uuid_from_aggregate(context, hostname)
         return self._session.call_xenapi("host.get_by_uuid", host_uuid)
 
+    def _get_host_ref_no_aggr(self):
+        # Pull the current host ref from Dom0's resident_on field.  This
+        # allows us a simple way to pull the accurate host without aggregates
+        dom0_rec = self._session.call_xenapi("VM.get_all_records_where",
+                                             'field "domid"="0"')
+        dom0_ref = list(dom0_rec.keys())[0]
+
+        return dom0_rec[dom0_ref]['resident_on']
+
+    def _get_host_software_versions(self):
+        # Get software versions from host.get_record.
+        # Works around aggregate checking as not all places use aggregates.
+        host_ref = self._get_host_ref_no_aggr()
+        host_rec = self._session.call_xenapi("host.get_record", host_ref)
+        return host_rec['software_version']
+
     def _get_network_ref(self):
         # Get the network to for migrate.
         # This is the one associated with the pif marked management. From cli:
@@ -2345,14 +2362,27 @@ class VMOps(object):
                 raise exception.MigrationError(reason=_('XAPI supporting '
                                 'relax-xsm-sr-check=true required'))
 
+        # TODO(bkaminski): This entire block needs to be removed from this
+        # if statement. Live Migration should assert_can_migrate either way.
         if ('block_migration' in dest_check_data and
                 dest_check_data.block_migration):
             vm_ref = self._get_vm_opaque_ref(instance_ref)
+            host_sw = self._get_host_software_versions()
+            host_pfv = host_sw['platform_version']
             try:
                 self._call_live_migrate_command(
                     "VM.assert_can_migrate", vm_ref, dest_check_data)
             except self._session.XenAPI.Failure as exc:
                 reason = exc.details[0]
+                # XCP>=2.1 Will error on this assert call if iSCSI are attached
+                # as the SR has not been configured on the hypervisor at this
+                # point in the migration. We swallow this exception until a
+                # more intensive refactor can be done to correct this.
+                if ("VDI_NOT_IN_MAP" in reason and
+                        host_sw['platform_name'] == "XCP" and
+                        versionutils.is_compatible("2.1.0", host_pfv)):
+                    LOG.debug("Skipping exception for XCP>=2.1.0, %s", reason)
+                    return dest_check_data
                 msg = _('assert_can_migrate failed because: %s') % reason
                 LOG.debug(msg, exc_info=True)
                 raise exception.MigrationPreCheckError(reason=msg)