From 8e5937e66803b134b06f8c186e1682c7654502d5 Mon Sep 17 00:00:00 2001 From: Alexandru Muresan Date: Tue, 31 Oct 2017 11:49:27 +0200 Subject: [PATCH] Validate SAN policy when having passthrough disks One common cause for passthrough disk attachment issues is an incorrectly configured san policy. Having it set to 'online' will prevevent disks from being attached to VMs. This change allows the hyperv driver to check the san policy and log a warning message when the driver loads and when a passthrough disk is being attached. Depends-On: I25033c15b77f494a417c9cd01d194a8bfb3cfe13 Change-Id: Ia123d98286fa99f45f91bfc47a4f0d468fa7d13e Closes-Bug: #1727257 --- compute_hyperv/nova/volumeops.py | 29 ++++++++++++ compute_hyperv/tests/unit/test_volumeops.py | 49 ++++++++++++++++++++- 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/compute_hyperv/nova/volumeops.py b/compute_hyperv/nova/volumeops.py index 03e0063a..32763de6 100644 --- a/compute_hyperv/nova/volumeops.py +++ b/compute_hyperv/nova/volumeops.py @@ -104,6 +104,17 @@ class VolumeOps(object): raise exception.VolumeDriverNotFound(driver_type=driver_type) return self.volume_drivers[driver_type] + def validate_host_configuration(self): + for protocol, volume_driver in self.volume_drivers.items(): + try: + volume_driver.validate_host_configuration() + except exception.ValidationError as ex: + LOG.warning( + "Volume driver %(protocol)s reported a validation " + "error. Attaching such volumes will probably fail. " + "Error message: %(err_msg)s.", + dict(protocol=protocol, err_msg=ex.message)) + def attach_volumes(self, context, volumes, instance): for vol in volumes: self.attach_volume(context, vol['connection_info'], instance) @@ -406,6 +417,10 @@ class BaseVolumeDriver(object): return self._get_disk_res_path(disk_paths[0]) + def validate_host_configuration(self): + if self._is_block_dev: + self._check_san_policy() + def _get_disk_res_path(self, disk_path): if self._is_block_dev: # We need the Msvm_DiskDrive resource path as this @@ -423,8 +438,22 @@ class BaseVolumeDriver(object): raise exception.DiskNotFound(err_msg) return disk_res_path + def _check_san_policy(self): + disk_policy = self._diskutils.get_new_disk_policy() + + accepted_policies = [os_win_const.DISK_POLICY_OFFLINE_SHARED, + os_win_const.DISK_POLICY_OFFLINE_ALL] + + if disk_policy not in accepted_policies: + err_msg = _("Invalid SAN policy. The SAN policy " + "must be set to 'Offline Shared' or 'Offline All' " + "in order to attach passthrough disks to instances.") + raise exception.ValidationError(message=err_msg) + def attach_volume(self, connection_info, instance_name, disk_bus=constants.CTRL_TYPE_SCSI): + self.validate_host_configuration() + dev_info = self.connect_volume(connection_info) serial = connection_info['serial'] diff --git a/compute_hyperv/tests/unit/test_volumeops.py b/compute_hyperv/tests/unit/test_volumeops.py index 67fa7fba..c7d28974 100644 --- a/compute_hyperv/tests/unit/test_volumeops.py +++ b/compute_hyperv/tests/unit/test_volumeops.py @@ -85,6 +85,20 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase): self._volumeops._get_volume_driver, connection_info=fake_conn_info) + def test_validate_host_configuration(self): + self._volumeops.volume_drivers = { + constants.STORAGE_PROTOCOL_SMBFS: mock.Mock( + side_effect=exception.ValidationError), + constants.STORAGE_PROTOCOL_ISCSI: mock.Mock( + side_effect=exception.ValidationError), + constants.STORAGE_PROTOCOL_FC: mock.Mock() + } + + self._volumeops.validate_host_configuration() + + for volume_drv in self._volumeops.volume_drivers.values(): + volume_drv.validate_host_configuration.assert_called_once_with() + @mock.patch.object(volumeops.VolumeOps, 'attach_volume') def test_attach_volumes(self, mock_attach_volume): block_device_info = get_fake_block_dev_info() @@ -624,12 +638,44 @@ class BaseVolumeDriverTestCase(test_base.HyperVBaseTestCase): mock.sentinel.disk_path) self.assertEqual(mock.sentinel.disk_path, path) + @mock.patch.object(volumeops.BaseVolumeDriver, + '_check_san_policy') + @ddt.data(True, False) + def test_validate_host_configuration(self, is_block_dev, + fake_check_san_policy): + self._base_vol_driver._is_block_dev = is_block_dev + + self._base_vol_driver.validate_host_configuration() + + if is_block_dev: + fake_check_san_policy.assert_called_once_with() + else: + fake_check_san_policy.assert_not_called() + + @ddt.data(os_win_const.DISK_POLICY_OFFLINE_ALL, + os_win_const.DISK_POLICY_ONLINE_ALL) + def test_check_san_policy(self, disk_policy): + self._diskutils.get_new_disk_policy.return_value = disk_policy + + accepted_policies = [os_win_const.DISK_POLICY_OFFLINE_SHARED, + os_win_const.DISK_POLICY_OFFLINE_ALL] + + if disk_policy not in accepted_policies: + self.assertRaises( + exception.ValidationError, + self._base_vol_driver._check_san_policy) + else: + self._base_vol_driver._check_san_policy() + @mock.patch.object(volumeops.BaseVolumeDriver, '_get_disk_res_path') @mock.patch.object(volumeops.BaseVolumeDriver, '_get_disk_ctrl_and_slot') @mock.patch.object(volumeops.BaseVolumeDriver, 'connect_volume') - def _test_attach_volume(self, mock_connect_volume, + @mock.patch.object(volumeops.BaseVolumeDriver, + 'validate_host_configuration') + def _test_attach_volume(self, mock_validate_host_config, + mock_connect_volume, mock_get_disk_ctrl_and_slot, mock_get_disk_res_path, is_block_dev=True): @@ -666,6 +712,7 @@ class BaseVolumeDriverTestCase(test_base.HyperVBaseTestCase): mock.sentinel.raw_path) mock_get_disk_ctrl_and_slot.assert_called_once_with( mock.sentinel.instance_name, mock.sentinel.disk_bus) + mock_validate_host_config.assert_called_once_with() def test_attach_volume_image_file(self): self._test_attach_volume(is_block_dev=False)