From: Vipin Balachandran Date: Fri, 14 Mar 2014 12:20:27 +0000 (+0530) Subject: vmware:Ignore inaccessible/inMaintenance datastore X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=36b5782dea6d0e627c65c73dbfee39d55dbee318;p=openstack-build%2Fcinder-build.git vmware:Ignore inaccessible/inMaintenance datastore The vmdk driver uses the number of hosts connected to a datastore as one of the criteria for selecting a datastore for volume creation. It might incorrectly consider a host to which the datastore is inaccessible while computing the number of connected hosts. This is because, the driver sometimes uses the aggregated accessible status of a datastore while determining its accessibility to a host. Also, the driver is ignoring the maintenance status of the datastore and might create a volume in a datastore which is entering maintenance or already in maintenance. This change fixes these issues in datastore filtering. Change-Id: Ibd5c1865d42638a735fd588e9c10e8f714c9c030 Closes-Bug: #1291346 Closes-Bug: #1291291 --- diff --git a/cinder/tests/test_vmware_volumeops.py b/cinder/tests/test_vmware_volumeops.py index ab71dcb63..a4b2019bf 100644 --- a/cinder/tests/test_vmware_volumeops.py +++ b/cinder/tests/test_vmware_volumeops.py @@ -161,32 +161,24 @@ class VolumeOpsTestCase(test.TestCase): mount_info.accessMode = "readWrite" mount_info.mounted = True mount_info.accessible = True - datastore = mock.sentinel.datastore - self.assertTrue(self.vops._is_usable(datastore, mount_info)) + self.assertTrue(self.vops._is_usable(mount_info)) del mount_info.mounted - self.assertTrue(self.vops._is_usable(datastore, mount_info)) + self.assertTrue(self.vops._is_usable(mount_info)) mount_info.accessMode = "readonly" - self.assertFalse(self.vops._is_usable(datastore, mount_info)) + self.assertFalse(self.vops._is_usable(mount_info)) mount_info.accessMode = "readWrite" mount_info.mounted = False - self.assertFalse(self.vops._is_usable(datastore, mount_info)) + self.assertFalse(self.vops._is_usable(mount_info)) mount_info.mounted = True mount_info.accessible = False - self.assertFalse(self.vops._is_usable(datastore, mount_info)) - - with mock.patch.object(self.vops, 'get_summary') as get_summary: - del mount_info.accessible - summary = mock.Mock(spec=object) - summary.accessible = True - get_summary.return_value = summary - self.assertTrue(self.vops._is_usable(datastore, mount_info)) + self.assertFalse(self.vops._is_usable(mount_info)) - summary.accessible = False - self.assertFalse(self.vops._is_usable(datastore, mount_info)) + del mount_info.accessible + self.assertFalse(self.vops._is_usable(mount_info)) def _create_host_mounts(self, access_mode, host, set_accessible=True, is_accessible=True, mounted=True): @@ -218,94 +210,118 @@ class VolumeOpsTestCase(test.TestCase): return host_mounts def test_get_connected_hosts(self): - datastore = mock.sentinel.datastore - host = mock.Mock(spec=object) - host.value = mock.sentinel.host - host_mounts = self._create_host_mounts("readWrite", host) - self.session.invoke_api.return_value = host_mounts - - hosts = self.vops.get_connected_hosts(datastore) - self.assertEqual([mock.sentinel.host], hosts) - self.session.invoke_api.assert_called_once_with(vim_util, - 'get_object_property', - self.session.vim, - datastore, - 'host') + with mock.patch.object(self.vops, 'get_summary') as get_summary: + datastore = mock.sentinel.datastore + summary = mock.Mock(spec=object) + get_summary.return_value = summary - def test_is_valid(self): - datastore = mock.sentinel.datastore - host = mock.Mock(spec=object) - host.value = mock.sentinel.host + summary.accessible = False + hosts = self.vops.get_connected_hosts(datastore) + self.assertEqual([], hosts) - def _is_valid(host_mounts, is_valid): + summary.accessible = True + host = mock.Mock(spec=object) + host.value = mock.sentinel.host + host_mounts = self._create_host_mounts("readWrite", host) self.session.invoke_api.return_value = host_mounts - result = self.vops._is_valid(datastore, host) - self.assertEqual(is_valid, result) - self.session.invoke_api.assert_called_with(vim_util, - 'get_object_property', - self.session.vim, - datastore, - 'host') - # Test with accessible attr - _is_valid(self._create_host_mounts("readWrite", host), True) - - # Test without accessible attr, and use summary instead + hosts = self.vops.get_connected_hosts(datastore) + self.assertEqual([mock.sentinel.host], hosts) + self.session.invoke_api.assert_called_once_with( + vim_util, + 'get_object_property', + self.session.vim, + datastore, + 'host') + + del host_mounts.DatastoreHostMount + hosts = self.vops.get_connected_hosts(datastore) + self.assertEqual([], hosts) + + def test_is_valid(self): with mock.patch.object(self.vops, 'get_summary') as get_summary: summary = mock.Mock(spec=object) - summary.accessible = True get_summary.return_value = summary - _is_valid(self._create_host_mounts("readWrite", host, False), - True) - - # Test negative cases for is_valid - _is_valid(self._create_host_mounts("Inaccessible", host), False) - _is_valid(self._create_host_mounts("readWrite", host, True, False), - False) - _is_valid(self._create_host_mounts("readWrite", host, True, True, - False), False) - with mock.patch.object(self.vops, 'get_summary') as get_summary: - summary = mock.Mock(spec=object) + + datastore = mock.sentinel.datastore + host = mock.Mock(spec=object) + host.value = mock.sentinel.host + + def _is_valid(host_mounts, is_valid): + self.session.invoke_api.return_value = host_mounts + result = self.vops._is_valid(datastore, host) + self.assertEqual(is_valid, result) + self.session.invoke_api.assert_called_with( + vim_util, + 'get_object_property', + self.session.vim, + datastore, + 'host') + + # Test positive cases + summary.maintenanceMode = 'normal' + summary.accessible = True + _is_valid(self._create_host_mounts("readWrite", host), True) + + # Test negative cases + _is_valid(self._create_host_mounts("Inaccessible", host), False) + _is_valid(self._create_host_mounts("readWrite", host, True, False), + False) + _is_valid(self._create_host_mounts("readWrite", host, True, True, + False), False) + summary.accessible = False - get_summary.return_value = summary _is_valid(self._create_host_mounts("readWrite", host, False), False) + summary.accessible = True + summary.maintenanceMode = 'inMaintenance' + _is_valid(self._create_host_mounts("readWrite", host), False) + def test_get_dss_rp(self): - # build out props to be returned by 1st invoke_api call - datastore_prop = mock.Mock(spec=object) - datastore_prop.name = 'datastore' - datastore_prop.val = mock.Mock(spec=object) - datastore_prop.val.ManagedObjectReference = [mock.sentinel.ds1, - mock.sentinel.ds2] - compute_resource_prop = mock.Mock(spec=object) - compute_resource_prop.name = 'parent' - compute_resource_prop.val = mock.sentinel.compute_resource - elem = mock.Mock(spec=object) - elem.propSet = [datastore_prop, compute_resource_prop] - props = [elem] - # build out host_mounts to be returned by 2nd invoke_api call - host = mock.Mock(spec=object) - host.value = mock.sentinel.host - host_mounts = self._create_host_mounts("readWrite", host) - # build out resource_pool to be returned by 3rd invoke_api call - resource_pool = mock.sentinel.resource_pool - # set return values for each call of invoke_api - self.session.invoke_api.side_effect = [props, - host_mounts, - host_mounts, - resource_pool] - # invoke function and verify results - (dss_actual, rp_actual) = self.vops.get_dss_rp(host) - self.assertEqual([mock.sentinel.ds1, mock.sentinel.ds2], dss_actual) - self.assertEqual(resource_pool, rp_actual) - - # invoke function with no valid datastore and verify exception raised - host_mounts = self._create_host_mounts("inaccessible", host) - self.session.invoke_api.side_effect = [props, - host_mounts, - host_mounts, - resource_pool] - self.assertRaises(error_util.VimException, self.vops.get_dss_rp, host) + with mock.patch.object(self.vops, 'get_summary') as get_summary: + summary = mock.Mock(spec=object) + summary.accessible = True + summary.maintenanceModel = 'normal' + get_summary.return_value = summary + + # build out props to be returned by 1st invoke_api call + datastore_prop = mock.Mock(spec=object) + datastore_prop.name = 'datastore' + datastore_prop.val = mock.Mock(spec=object) + datastore_prop.val.ManagedObjectReference = [mock.sentinel.ds1, + mock.sentinel.ds2] + compute_resource_prop = mock.Mock(spec=object) + compute_resource_prop.name = 'parent' + compute_resource_prop.val = mock.sentinel.compute_resource + elem = mock.Mock(spec=object) + elem.propSet = [datastore_prop, compute_resource_prop] + props = [elem] + # build out host_mounts to be returned by 2nd invoke_api call + host = mock.Mock(spec=object) + host.value = mock.sentinel.host + host_mounts = self._create_host_mounts("readWrite", host) + # build out resource_pool to be returned by 3rd invoke_api call + resource_pool = mock.sentinel.resource_pool + # set return values for each call of invoke_api + self.session.invoke_api.side_effect = [props, + host_mounts, + host_mounts, + resource_pool] + # invoke function and verify results + (dss_actual, rp_actual) = self.vops.get_dss_rp(host) + self.assertEqual([mock.sentinel.ds1, mock.sentinel.ds2], + dss_actual) + self.assertEqual(resource_pool, rp_actual) + + # invoke function with no valid datastore + summary.maintenanceMode = 'inMaintenance' + self.session.invoke_api.side_effect = [props, + host_mounts, + host_mounts, + resource_pool] + self.assertRaises(error_util.VimException, + self.vops.get_dss_rp, + host) def test_get_parent(self): # Not recursive diff --git a/cinder/volume/drivers/vmware/volumeops.py b/cinder/volume/drivers/vmware/volumeops.py index 8ba7b01f1..87eac1cca 100644 --- a/cinder/volume/drivers/vmware/volumeops.py +++ b/cinder/volume/drivers/vmware/volumeops.py @@ -138,30 +138,20 @@ class VMwareVolumeOps(object): self._session.invoke_api(vim_util, 'cancel_retrieval', self._session.vim, retrieve_result) - def _is_usable(self, datastore, mount_info): - """Check if the given datastore is usable as per the given mount info. + def _is_usable(self, mount_info): + """Check if a datastore is usable as per the given mount info. The datastore is considered to be usable for a host only if it is writable, mounted and accessible. - :param datastore: Reference to the datastore entity - :param mount_info: host mount information + :param mount_info: Host mount information :return: True if datastore is usable """ - - writable = mount_info.accessMode == "readWrite" - + writable = mount_info.accessMode == 'readWrite' # If mounted attribute is not set, then default is True - mounted = True - if hasattr(mount_info, "mounted"): - mounted = mount_info.mounted - - if hasattr(mount_info, "accessible"): - accessible = mount_info.accessible - else: - # If accessible attribute is not set, we look at summary - summary = self.get_summary(datastore) - accessible = summary.accessible + mounted = getattr(mount_info, 'mounted', True) + # If accessible attribute is not set, then default is False + accessible = getattr(mount_info, 'accessible', False) return writable and mounted and accessible @@ -175,31 +165,57 @@ class VMwareVolumeOps(object): :return: List of managed object references of all connected hosts """ + summary = self.get_summary(datastore) + if not summary.accessible: + return [] host_mounts = self._session.invoke_api(vim_util, 'get_object_property', self._session.vim, datastore, 'host') + if not hasattr(host_mounts, 'DatastoreHostMount'): + return [] + connected_hosts = [] for host_mount in host_mounts.DatastoreHostMount: - if self._is_usable(datastore, host_mount.mountInfo): + if self._is_usable(host_mount.mountInfo): connected_hosts.append(host_mount.key.value) return connected_hosts + def _in_maintenance(self, summary): + """Check if a datastore is entering maintenance or in maintenance. + + :param summary: Summary information about the datastore + :return: True if the datastore is entering maintenance or in + maintenance + """ + if hasattr(summary, 'maintenanceMode'): + return summary.maintenanceMode in ['enteringMaintenance', + 'inMaintenance'] + return False + def _is_valid(self, datastore, host): - """Check if host's datastore is accessible, mounted and writable. + """Check if the datastore is valid for the given host. + + A datastore is considered valid for a host only if the datastore is + writable, mounted and accessible. Also, the datastore should not be + in maintenance mode. :param datastore: Reference to the datastore entity :param host: Reference to the host entity :return: True if datastore can be used for volume creation """ + summary = self.get_summary(datastore) + in_maintenance = self._in_maintenance(summary) + if not summary.accessible or in_maintenance: + return False host_mounts = self._session.invoke_api(vim_util, 'get_object_property', self._session.vim, datastore, 'host') for host_mount in host_mounts.DatastoreHostMount: if host_mount.key.value == host.value: - return self._is_usable(datastore, host_mount.mountInfo) + return self._is_usable(host_mount.mountInfo) return False def get_dss_rp(self, host):