From 7d18029d52398080525f67383bbdadfa0786c788 Mon Sep 17 00:00:00 2001 From: Simon Dodsley Date: Tue, 5 Jan 2021 12:05:29 -0500 Subject: [PATCH] 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 bb219d6b9f62cc549cb9fe38b60e5e0b5aa450cf) --- cinder/tests/unit/volume/drivers/test_pure.py | 33 ++++++++++++++----- cinder/volume/drivers/pure.py | 21 +++++++++--- .../notes/bug-1870103-013e314e9a5b8e08.yaml | 7 ++++ 3 files changed, 48 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/bug-1870103-013e314e9a5b8e08.yaml diff --git a/cinder/tests/unit/volume/drivers/test_pure.py b/cinder/tests/unit/volume/drivers/test_pure.py index 08ff0b6e6fd..37a527d8c4c 100644 --- a/cinder/tests/unit/volume/drivers/test_pure.py +++ b/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): - snap, snap_name = self.new_fake_snap() - unmanaged_snap_name = snap_name + "-unmanaged" + @ddt.data( + # 96 chars, will exceed allowable length + '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, - unmanaged_snap_name) + self.array.rename_volume.assert_called_once() + 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.", diff --git a/cinder/volume/drivers/pure.py b/cinder/volume/drivers/pure.py index 12706c6be4d..3ba40c6bb35 100644 --- a/cinder/volume/drivers/pure.py +++ b/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:] - base_name = "volume-" + actual_name[-52:] + actual_name = base_name[(len(CONF.volume_name_template) - 2):] + 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) diff --git a/releasenotes/notes/bug-1870103-013e314e9a5b8e08.yaml b/releasenotes/notes/bug-1870103-013e314e9a5b8e08.yaml new file mode 100644 index 00000000000..5d8751f5b05 --- /dev/null +++ b/releasenotes/notes/bug-1870103-013e314e9a5b8e08.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Pure Storage driver `bug 1870103 + `_: + Ensure that unmanaged volumes do not exceed maximum + character length on FlashArray.