HP 3PAR: Don't ignore extra-specs snap_cpg when missing cpg

When snap_cpg is specified in extra-specs, it should be used.
For some reason, it was being ignored when extra-specs did not
also specify a user cpg.

When using a volume-type, the snapCPG should come from (in this order
of preference):
1. extra specs snap_cpg,
2. extra specs cpg,
3. config hp3par_cpg_snap,
4. config hp3par_cpg.

Change-Id: I649e3dc49f7775a8071229fc315f0ee5f23aa833
Closes-Bug: 1368972

diff --git a/cinder/tests/test_hp3par.py b/cinder/tests/test_hp3par.py
index 78f2d94701228d415cfeaf0b9597659df84558d6..f11ea6116fffa30e004cc0456d984b1472b376b5 100644 (file)
--- a/cinder/tests/test_hp3par.py
+++ b/cinder/tests/test_hp3par.py
@@ -559,6 +559,72 @@ class HP3PARBaseDriver(object):
         mock_client.assert_has_calls(expected)
         self.assertEqual(return_model, None)
 
+    @mock.patch.object(volume_types, 'get_volume_type')
+    def test_get_snap_cpg_from_volume_type(self, _mock_volume_types):
+
+        self.setup_driver()
+        expected_type_snap_cpg = "type_snap_cpg"
+        _mock_volume_types.return_value = {
+            'name': 'gold',
+            'extra_specs': {
+                'cpg': HP3PAR_CPG,
+                'snap_cpg': expected_type_snap_cpg,
+                'volume_type': self.volume_type}}
+
+        result = self.driver.common.get_volume_settings_from_type_id(
+            "mock", self.driver.configuration.hp3par_cpg)
+
+        self.assertEqual(expected_type_snap_cpg, result['snap_cpg'])
+
+    @mock.patch.object(volume_types, 'get_volume_type')
+    def test_get_snap_cpg_from_volume_type_cpg(self, _mock_volume_types):
+
+        self.setup_driver()
+        expected_cpg = 'use_extra_specs_cpg'
+        _mock_volume_types.return_value = {
+            'name': 'gold',
+            'extra_specs': {
+                'cpg': expected_cpg,
+                'volume_type': self.volume_type}}
+
+        result = self.driver.common.get_volume_settings_from_type_id(
+            "mock", self.driver.configuration.hp3par_cpg)
+
+        self.assertEqual(expected_cpg, result['snap_cpg'])
+
+    @mock.patch.object(volume_types, 'get_volume_type')
+    def test_get_snap_cpg_from_volume_type_conf_snap_cpg(
+            self, _mock_volume_types):
+        _mock_volume_types.return_value = {
+            'name': 'gold',
+            'extra_specs': {
+                'volume_type': self.volume_type}}
+
+        conf = self.setup_configuration()
+        expected_snap_cpg = conf.hp3par_cpg_snap
+        self.setup_driver(config=conf)
+        result = self.driver.common.get_volume_settings_from_type_id(
+            "mock", self.driver.configuration.hp3par_cpg)
+
+        self.assertEqual(expected_snap_cpg, result['snap_cpg'])
+
+    @mock.patch.object(volume_types, 'get_volume_type')
+    def test_get_snap_cpg_from_volume_type_conf_cpg(
+            self, _mock_volume_types):
+        _mock_volume_types.return_value = {
+            'name': 'gold',
+            'extra_specs': {
+                'volume_type': self.volume_type}}
+
+        conf = self.setup_configuration()
+        conf.hp3par_cpg_snap = None
+        expected_cpg = conf.hp3par_cpg
+        self.setup_driver(config=conf)
+        result = self.driver.common.get_volume_settings_from_type_id(
+            "mock", self.driver.configuration.hp3par_cpg)
+
+        self.assertEqual(expected_cpg, result['snap_cpg'])
+
     @mock.patch.object(volume_types, 'get_volume_type')
     def test_create_volume_qos(self, _mock_volume_types):
         # setup_mock_client drive with default configuration
@@ -1016,7 +1082,7 @@ class HP3PARBaseDriver(object):
             mock.call.modifyVolume(
                 osv_matcher,
                 {'comment': '{"qos": {}, "display_name": "Foo Volume"}',
-                 'snapCPG': 'CPG-FC1'}),
+                 'snapCPG': HP3PAR_CPG_SNAP}),
             mock.call.modifyVolume(osv_matcher,
                                    {'action': 6,
                                     'userCPG': 'CPG-FC1',
@@ -1171,7 +1237,7 @@ class HP3PARBaseDriver(object):
             mock.call.modifyVolume(
                 osv_matcher,
                 {'comment': '{"qos": {}, "display_name": "Foo Volume"}',
-                 'snapCPG': 'CPG-FC1'}),
+                 'snapCPG': HP3PAR_CPG_SNAP}),
             mock.call.modifyVolume(osv_matcher,
                                    {'action': 6,
                                     'userCPG': 'CPG-FC1',
@@ -1884,11 +1950,12 @@ class HP3PARBaseDriver(object):
             }
         }
 
+        expected_snap_cpg = self.volume_type['extra_specs']['cpg']
         expected_retype_modify = [
             mock.call.modifyVolume(osv_matcher,
                                    {'comment': self.CommentMatcher(
                                        self.assertEqual, retype_comment_qos),
-                                    'snapCPG': 'OpenStackCPGSnap'}),
+                                    'snapCPG': expected_snap_cpg}),
             mock.call.deleteVolumeSet(vvs_matcher),
         ]
 
@@ -2151,7 +2218,7 @@ class HP3PARBaseDriver(object):
             mock.call.getVolume(unm_matcher),
             mock.call.modifyVolume(
                 unm_matcher, {'newName': osv_matcher, 'comment': mock.ANY}),
-            mock.call.getCPG('POOL1'),
+            mock.call.getCPG('OpenStackCPG'),
             mock.call.getVolume(osv_matcher),
             mock.call.getCPG('testUserCpg0'),
             mock.call.getCPG('OpenStackCPG'),

diff --git a/cinder/volume/drivers/san/hp/hp_3par_common.py b/cinder/volume/drivers/san/hp/hp_3par_common.py
index f4c75f651353fdc3c86cec4f18e9b035cafe0282..b93e204fd85f6293c214df00705872a98bb05193 100644 (file)
--- a/cinder/volume/drivers/san/hp/hp_3par_common.py
+++ b/cinder/volume/drivers/san/hp/hp_3par_common.py
@@ -156,10 +156,11 @@ class HP3PARCommon(object):
         2.0.23 - Increase the hostname size from 23 to 31  Bug #1371242
         2.0.24 - Add pools (hp3par_cpg now accepts a list of CPGs)
         2.0.25 - Migrate without losing type settings bug #1356608
+        2.0.26 - Don't ignore extra-specs snap_cpg when missing cpg #1368972
 
     """
 
-    VERSION = "2.0.25"
+    VERSION = "2.0.26"
 
     stats = {}
 
@@ -965,7 +966,7 @@ class HP3PARCommon(object):
         default_cpg = pool or self.config.hp3par_cpg[0]
 
         cpg = self._get_key_value(hp3par_keys, 'cpg', default_cpg)
-        if cpg not in self.config.hp3par_cpg:
+        if cpg is not default_cpg:
             # The cpg was specified in a volume type extra spec so it
             # needs to be validated that it's in the correct domain.
             self.validate_cpg(cpg)
@@ -974,12 +975,13 @@ class HP3PARCommon(object):
             # default.
             snap_cpg = self._get_key_value(hp3par_keys, 'snap_cpg', cpg)
         else:
-            # default snap_cpg to hp3par_cpg_snap if it's not specified
-            # in the volume type extra specs.
+            # Look to see if the snap_cpg was specified in volume type
+            # extra spec, if not use hp3par_cpg_snap from config as the
+            # default.
             snap_cpg = self.config.hp3par_cpg_snap
-            # if it's still not set or empty then set it to the cpg
-            # specified in the cinder.conf file.
-            if not self.config.hp3par_cpg_snap:
+            snap_cpg = self._get_key_value(hp3par_keys, 'snap_cpg', snap_cpg)
+            # If it's still not set or empty then set it to the cpg.
+            if not snap_cpg:
                 snap_cpg = cpg
 
         # if provisioning is not set use thin