NetApp cmode nfs: Fix QOS extra spec

This patch fixes the incorrect behavior where the NetApp cmode
nfs driver will choose to create a volume on a flexVol that has
a QOS policy group that matches the specified QOS policy group
intended for the cinder volume. The correct behavior is to ignore
the QOS policy group of the flexVol and instead assign the QOS policy
group to the newly created cinder volume.

Change-Id: I45d905da2a9a07b3ae8c00a225ab3b7f7ceb12d8
Closes-Bug: #1288283

diff --git a/cinder/tests/test_netapp_nfs.py b/cinder/tests/test_netapp_nfs.py
index d6ceb635cd9333c34f42cbce1895b17be625df97..bf520b78383d1f2496c9f1eb4ef09c37cb03f3fd 100644 (file)
--- a/cinder/tests/test_netapp_nfs.py
+++ b/cinder/tests/test_netapp_nfs.py
@@ -811,6 +811,46 @@ class NetappDirectCmodeNfsDriverOnlyTestCase(test.TestCase):
         self._driver.ssc_enabled = True
         self._driver.configuration.netapp_copyoffload_tool_path = 'cof_path'
 
+    @mock.patch.object(netapp_nfs, 'get_volume_extra_specs')
+    def test_create_volume(self, mock_volume_extra_specs):
+        drv = self._driver
+        drv.ssc_enabled = False
+        extra_specs = {}
+        mock_volume_extra_specs.return_value = extra_specs
+        fake_share = 'localhost:myshare'
+        fake_qos_policy = 'qos_policy_1'
+        with mock.patch.object(drv, '_ensure_shares_mounted'):
+            with mock.patch.object(drv, '_find_shares',
+                                   return_value=['localhost:myshare']):
+                with mock.patch.object(drv, '_do_create_volume'):
+                    volume_info = self._driver.create_volume(FakeVolume(1))
+                    self.assertEqual(volume_info.get('provider_location'),
+                                     fake_share)
+
+    @mock.patch.object(netapp_nfs, 'get_volume_extra_specs')
+    def test_create_volume_with_qos_policy(self, mock_volume_extra_specs):
+        drv = self._driver
+        drv.ssc_enabled = False
+        extra_specs = {'netapp:qos_policy_group': 'qos_policy_1'}
+        fake_volume = FakeVolume(1)
+        fake_share = 'localhost:myshare'
+        fake_qos_policy = 'qos_policy_1'
+        mock_volume_extra_specs.return_value = extra_specs
+
+        with mock.patch.object(drv, '_ensure_shares_mounted'):
+            with mock.patch.object(drv, '_find_shares',
+                                   return_value=['localhost:myshare']):
+                with mock.patch.object(drv, '_do_create_volume'):
+                    with mock.patch.object(drv,
+                                           '_set_qos_policy_group_on_volume'
+                                           ) as mock_set_qos:
+                        volume_info = self._driver.create_volume(fake_volume)
+                        self.assertEqual(volume_info.get('provider_location'),
+                                         'localhost:myshare')
+                        mock_set_qos.assert_called_once_with(fake_volume,
+                                                             fake_share,
+                                                             fake_qos_policy)
+
     def test_copy_img_to_vol_copyoffload_success(self):
         drv = self._driver
         context = object()

diff --git a/cinder/volume/drivers/netapp/nfs.py b/cinder/volume/drivers/netapp/nfs.py
index 1e5fa3fe10284e95f7e52dce79db72412b15aa13..630bd4c64b228b65d82a26d857489bfff3c4876f 100644 (file)
--- a/cinder/volume/drivers/netapp/nfs.py
+++ b/cinder/volume/drivers/netapp/nfs.py
@@ -774,6 +774,9 @@ class NetAppDirectCmodeNfsDriver (NetAppDirectNfsDriver):
         """
         self._ensure_shares_mounted()
         extra_specs = get_volume_extra_specs(volume)
+        qos_policy_group = None
+        if extra_specs:
+            qos_policy_group = extra_specs.pop('netapp:qos_policy_group', None)
         eligible = self._find_shares(volume['size'], extra_specs)
         if not eligible:
             raise exception.NfsNoSuitableShareFound(
@@ -783,12 +786,16 @@ class NetAppDirectCmodeNfsDriver (NetAppDirectNfsDriver):
                 volume['provider_location'] = sh
                 LOG.info(_('casted to %s') % volume['provider_location'])
                 self._do_create_volume(volume)
+                if qos_policy_group:
+                    self._set_qos_policy_group_on_volume(volume, sh,
+                                                         qos_policy_group)
                 return {'provider_location': volume['provider_location']}
-            except Exception:
-                LOG.warn(_("Exception creating vol %(name)s"
-                           " on share %(share)s")
-                         % {'name': volume['name'],
-                             'share': volume['provider_location']})
+            except Exception as ex:
+                LOG.error(_("Exception creating vol %(name)s on "
+                            "share %(share)s. Details: %(ex)s")
+                          % {'name': volume['name'],
+                             'share': volume['provider_location'],
+                             'ex': ex})
                 volume['provider_location'] = None
             finally:
                 if self.ssc_enabled:
@@ -796,6 +803,19 @@ class NetAppDirectCmodeNfsDriver (NetAppDirectNfsDriver):
         msg = _("Volume %s could not be created on shares.")
         raise exception.VolumeBackendAPIException(data=msg % (volume['name']))
 
+    def _set_qos_policy_group_on_volume(self, volume, share, qos_policy_group):
+        target_path = '%s' % (volume['name'])
+        export_path = share.split(':')[1]
+        flex_vol_name = self._get_vol_by_junc_vserver(self.vserver,
+                                                      export_path)
+        file_assign_qos = NaElement.create_node_with_children(
+            'file-assign-qos',
+            **{'volume': flex_vol_name,
+               'qos-policy-group-name': qos_policy_group,
+               'file': target_path,
+               'vserver': self.vserver})
+        self._invoke_successfully(file_assign_qos)
+
     def _find_shares(self, size, extra_specs):
         """Finds suitable shares for given params."""
         shares = []

diff --git a/cinder/volume/drivers/netapp/ssc_utils.py b/cinder/volume/drivers/netapp/ssc_utils.py
index ebd058885fe2f64bbfc080945e2d9b0eef1444c6..a588b388e7ff8af2924f7395a27a7155b6dd438e 100644 (file)
--- a/cinder/volume/drivers/netapp/ssc_utils.py
+++ b/cinder/volume/drivers/netapp/ssc_utils.py
@@ -533,7 +533,6 @@ def get_volumes_for_specs(ssc_vols, specs):
     result = copy.deepcopy(ssc_vols['all'])
     raid_type = specs.get('netapp:raid_type')
     disk_type = specs.get('netapp:disk_type')
-    qos_policy_group = specs.get('netapp:qos_policy_group')
     bool_specs_list = ['netapp_mirrored', 'netapp_unmirrored',
                        'netapp_dedup', 'netapp_nodedup',
                        'netapp_compression', 'netapp_nocompression',
@@ -582,7 +581,7 @@ def get_volumes_for_specs(ssc_vols, specs):
             result = result & ssc_vols['thin']
         else:
             result = result - ssc_vols['thin']
-    if raid_type or disk_type or qos_policy_group:
+    if raid_type or disk_type:
         tmp = copy.deepcopy(result)
         for vol in tmp:
             if raid_type:
@@ -595,11 +594,6 @@ def get_volumes_for_specs(ssc_vols, specs):
                 vol_dtype = vol_dtype.lower() if vol_dtype else None
                 if disk_type.lower() != vol_dtype:
                     result.discard(vol)
-            if qos_policy_group:
-                vol_qos = vol.qos['qos_policy_group']
-                vol_qos = vol_qos.lower() if vol_qos else None
-                if qos_policy_group.lower() != vol_qos:
-                    result.discard(vol)
     return result