Pure Storage: check volumename length does not exceed maximum

Add in checks to ensure volume names in all cases, including
snapshotting and unmanaging, do not exceed the name length
limits on the FlashArray

NOTE: This had to be manually amended as the original commit used
a version of ddt not supported in Victoria

Change-Id: I6c153fd6c9c0f485092a82f0e6ae020ec5980fcc
Closes-Bug: 1870103
(cherry picked from commit bb219d6b9f)

cinder/tests/unit/volume/drivers/test_pure.py

@@ -52,6 +52,7 @@ BASE_DRIVER_OBJ = DRIVER_PATH + ".PureBaseVolumeDriver"
 ISCSI_DRIVER_OBJ = DRIVER_PATH + ".PureISCSIDriver"
 FC_DRIVER_OBJ = DRIVER_PATH + ".PureFCDriver"
 ARRAY_OBJ = DRIVER_PATH + ".FlashArray"
+UNMANAGED_SUFFIX = "-unmanaged"
 
 GET_ARRAY_PRIMARY = {"version": "99.9.9",
                      "revision": "201411230504+8a400f7",
@@ -495,6 +496,7 @@ MANAGEABLE_PURE_SNAP_REFS = [
         'source_reference': {'name': MANAGEABLE_PURE_SNAPS[2]['source']},
     }
 ]
+MAX_SNAP_LENGTH = 96
 
 
 class FakePureStorageHTTPError(Exception):
@@ -2042,7 +2044,7 @@ class PureBaseVolumeDriverTestCase(PureBaseSharedDriverTestCase):
 
     def test_unmanage(self):
         vol, vol_name = self.new_fake_vol()
-        unmanaged_vol_name = vol_name + "-unmanaged"
+        unmanaged_vol_name = vol_name + UNMANAGED_SUFFIX
 
         self.driver.unmanage(vol)
 
@@ -2057,7 +2059,7 @@ class PureBaseVolumeDriverTestCase(PureBaseSharedDriverTestCase):
 
     def test_unmanage_with_deleted_volume(self):
         vol, vol_name = self.new_fake_vol()
-        unmanaged_vol_name = vol_name + "-unmanaged"
+        unmanaged_vol_name = vol_name + UNMANAGED_SUFFIX
         self.array.rename_volume.side_effect = \
             self.purestorage_module.PureHTTPError(
                 text="Volume does not exist.",
@@ -2206,12 +2208,23 @@ class PureBaseVolumeDriverTestCase(PureBaseSharedDriverTestCase):
                           self.driver.manage_existing_snapshot_get_size,
                           snap, {'name': PURE_SNAPSHOT['name']})
 
-    def test_unmanage_snapshot(self):
+    @ddt.data(
-        snap, snap_name = self.new_fake_snap()
+        # 96 chars, will exceed allowable length
-        unmanaged_snap_name = snap_name + "-unmanaged"
+        'volume-1e5177e7-95e5-4a0f-b170-e45f4b469f6a-cinder.'
+        'snapshot-253b2878-ec60-4793-ad19-e65496ec7aab',
+        # short_name that will require no adjustment
+        'volume-1e5177e7-cinder.snapshot-e65496ec7aab')
+    @mock.patch(BASE_DRIVER_OBJ + "._get_snap_name")
+    def test_unmanage_snapshot(self, fake_name, mock_get_snap_name):
+        snap, _ = self.new_fake_snap()
+        mock_get_snap_name.return_value = fake_name
         self.driver.unmanage_snapshot(snap)
-        self.array.rename_volume.assert_called_with(snap_name,
+        self.array.rename_volume.assert_called_once()
-                                                    unmanaged_snap_name)
+        old_name = self.array.rename_volume.call_args[0][0]
+        new_name = self.array.rename_volume.call_args[0][1]
+        self.assertEqual(fake_name, old_name)
+        self.assertLessEqual(len(new_name), MAX_SNAP_LENGTH)
+        self.assertTrue(new_name.endswith(UNMANAGED_SUFFIX))
 
     def test_unmanage_snapshot_error_propagates(self):
         snap, _ = self.new_fake_snap()
@@ -2221,7 +2234,11 @@ class PureBaseVolumeDriverTestCase(PureBaseSharedDriverTestCase):
 
     def test_unmanage_snapshot_with_deleted_snapshot(self):
         snap, snap_name = self.new_fake_snap()
-        unmanaged_snap_name = snap_name + "-unmanaged"
+        if len(snap_name + UNMANAGED_SUFFIX) > MAX_SNAP_LENGTH:
+            unmanaged_snap_name = snap_name[:-len(UNMANAGED_SUFFIX)] + \
+                UNMANAGED_SUFFIX
+        else:
+            unmanaged_snap_name = snap_name
         self.array.rename_volume.side_effect = \
             self.purestorage_module.PureHTTPError(
                 text="Snapshot does not exist.",

cinder/volume/drivers/pure.py

@@ -125,6 +125,8 @@ ERR_MSG_ALREADY_IN_USE = "already in use"
 EXTRA_SPECS_REPL_ENABLED = "replication_enabled"
 EXTRA_SPECS_REPL_TYPE = "replication_type"
 
+MAX_VOL_LENGTH = 63
+MAX_SNAP_LENGTH = 96
 UNMANAGED_SUFFIX = '-unmanaged'
 SYNC_REPLICATION_REQUIRED_API_VERSIONS = ['1.13', '1.14']
 ASYNC_REPLICATION_REQUIRED_API_VERSIONS = [
@@ -1269,7 +1271,11 @@ class PureBaseVolumeDriver(san.SanDriver):
         """
 
         vol_name = self._get_vol_name(volume)
-        unmanaged_vol_name = vol_name + UNMANAGED_SUFFIX
+        if len(vol_name + UNMANAGED_SUFFIX) > MAX_VOL_LENGTH:
+            unmanaged_vol_name = vol_name[:-len(UNMANAGED_SUFFIX)] + \
+                UNMANAGED_SUFFIX
+        else:
+            unmanaged_vol_name = vol_name + UNMANAGED_SUFFIX
         LOG.info("Renaming existing volume %(ref_name)s to %(new_name)s",
                  {"ref_name": vol_name, "new_name": unmanaged_vol_name})
         self._rename_volume_object(vol_name, unmanaged_vol_name)
@@ -1326,7 +1332,11 @@ class PureBaseVolumeDriver(san.SanDriver):
         """
         self._verify_manage_snap_api_requirements()
         snap_name = self._get_snap_name(snapshot)
-        unmanaged_snap_name = snap_name + UNMANAGED_SUFFIX
+        if len(snap_name + UNMANAGED_SUFFIX) > MAX_SNAP_LENGTH:
+            unmanaged_snap_name = snap_name[:-len(UNMANAGED_SUFFIX)] + \
+                UNMANAGED_SUFFIX
+        else:
+            unmanaged_snap_name = snap_name + UNMANAGED_SUFFIX
         LOG.info("Renaming existing snapshot %(ref_name)s to "
                  "%(new_name)s", {"ref_name": snap_name,
                                   "new_name": unmanaged_snap_name})
@@ -1555,13 +1565,14 @@ class PureBaseVolumeDriver(san.SanDriver):
         base_name = volume.name
 
         # Some OpenStack deployments, eg PowerVC, create a volume.name that
-        # when appended with out '-cinder' string will exceed the maximum
+        # when appended with our '-cinder' string will exceed the maximum
         # volume name length for Pure, so here we left truncate the true volume
         # name before the opennstack volume_name_template affected it and
         # then put back the template format
         if len(base_name) > 56:
-            actual_name = base_name[7:]
+            actual_name = base_name[(len(CONF.volume_name_template) - 2):]
-            base_name = "volume-" + actual_name[-52:]
+            base_name = CONF.volume_name_template % \
+                actual_name[-(56 - len(CONF.volume_name_template)):]
 
         repl_type = self._get_replication_type_from_vol_type(
             volume.volume_type)

releasenotes/notes/bug-1870103-013e314e9a5b8e08.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Pure Storage driver `bug 1870103
+    <https://bugs.launchpad.net/cinder/+bug/1870103>`_:
+    Ensure that unmanaged volumes do not exceed maximum
+    character length on FlashArray.