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 ab71dcb63eaa66e4fcb0514ae7e1e8e82c2accb8..a4b2019bf014932ee1079d5d14e70254dd85fa98 100644 (file)
--- 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 8ba7b01f153ae5d2a8dced5c0b2eae953da33a99..87eac1cca53ed06ac8533df1549ee46e2f508b6d 100644 (file)
--- 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):