From 8f82bb7966986f8857913ed2366fcf42c134e027 Mon Sep 17 00:00:00 2001 From: Adriano Rosso Date: Wed, 21 Dec 2016 09:18:04 -0200 Subject: [PATCH] HNAS: Change snapshot names Currently, HNAS NFS driver creates snapshots named as snapshot- in the backend. This is not very effective because there is no precise way to relate the snapshot with the volume that originated it using HNAS features. This patch changes the snapshot related features to work with snapshots named as . in order to make easier to relate the snapshots with their parent volumes. The operations for snapshots named the old way are still supported, but deprecated. Co-Authored-By: Erlon Cruz Change-Id: I188c7a3f67558e17ccc1c50e0f00dbb076395b18 --- .../drivers/hitachi/test_hitachi_hnas_nfs.py | 81 +++++++-- cinder/volume/drivers/hitachi/hnas_nfs.py | 166 ++++++++++++------ ...hange-snapshot-names-8153b043eb7e99fc.yaml | 6 + 3 files changed, 186 insertions(+), 67 deletions(-) create mode 100644 releasenotes/notes/hnas-change-snapshot-names-8153b043eb7e99fc.yaml diff --git a/cinder/tests/unit/volume/drivers/hitachi/test_hitachi_hnas_nfs.py b/cinder/tests/unit/volume/drivers/hitachi/test_hitachi_hnas_nfs.py index 80960bec213..7ca98678a9f 100644 --- a/cinder/tests/unit/volume/drivers/hitachi/test_hitachi_hnas_nfs.py +++ b/cinder/tests/unit/volume/drivers/hitachi/test_hitachi_hnas_nfs.py @@ -47,7 +47,7 @@ _SNAPSHOT = { 'id': fake.SNAPSHOT_ID, 'size': 128, 'volume_type': None, - 'provider_location': None, + 'provider_location': 'hnas', 'volume_size': 128, 'volume': _VOLUME, 'volume_name': _VOLUME['name'], @@ -321,25 +321,66 @@ class HNASNFSDriverTest(test.TestCase): self.assertEqual('NFS', out['storage_protocol']) def test_create_volume_from_snapshot(self): - self.mock_object(backend.HNASSSHBackend, 'file_clone') + expected_out = {'provider_location': 'hnas'} - self.driver.create_volume_from_snapshot(self.volume, self.snapshot) + self.mock_object(self.driver, '_file_not_present', + mock.Mock(return_value=False)) + self.mock_object(backend.HNASSSHBackend, 'file_clone') + result = self.driver.create_volume_from_snapshot(self.volume, + self.snapshot) + + self.assertEqual(expected_out, result) + + def test_create_volume_from_snapshot_legacy(self): + expected_out = {'provider_location': 'hnas'} + + self.mock_object(self.driver, '_file_not_present', + mock.Mock(return_value=True)) + self.mock_object(backend.HNASSSHBackend, 'file_clone') + result = self.driver.create_volume_from_snapshot(self.volume, + self.snapshot) + + self.assertEqual(expected_out, result) def test_create_snapshot(self): + expected_out = {'provider_location': 'hnas'} self.mock_object(backend.HNASSSHBackend, 'file_clone') - self.driver.create_snapshot(self.snapshot) + result = self.driver.create_snapshot(self.snapshot) + + self.assertEqual(expected_out, result) def test_delete_snapshot(self): + nfs_mount = "/opt/stack/data/cinder/mnt/" + path = nfs_mount + self.driver._get_snapshot_name(self.snapshot) + + self.mock_object(self.driver, '_file_not_present', + mock.Mock(return_value=False)) + + self.mock_object(self.driver, '_get_file_path', + mock.Mock(return_value=path)) self.mock_object(self.driver, '_execute') self.driver.delete_snapshot(self.snapshot) - def test_delete_snapshot_execute_exception(self): - self.mock_object(self.driver, '_execute', - side_effect=putils.ProcessExecutionError) + self.driver._execute.assert_called_with('rm', path, run_as_root=True) + + def test_delete_snapshot_legacy(self): + nfs_mount = "/opt/stack/data/cinder/mnt/" + legacy_path = nfs_mount + self.snapshot.name + + self.mock_object(self.driver, '_file_not_present', + mock.Mock(return_value=True)) + self.mock_object(self.driver, '_file_not_present', + mock.Mock(return_value=False)) + self.mock_object(self.driver, '_get_file_path', + mock.Mock(return_value=legacy_path)) + self.mock_object(self.driver, '_execute') self.driver.delete_snapshot(self.snapshot) + self.driver._execute.assert_called_with('rm', legacy_path, + run_as_root=True) + def test_extend_volume(self): share_mount_point = '/fs-cinder' data = image_utils.imageutils.QemuImgInfo @@ -539,7 +580,7 @@ class HNASNFSDriverTest(test.TestCase): def test_manage_existing_snapshot(self): nfs_share = "172.24.49.21:/fs-cinder" nfs_mount = "/opt/stack/data/cinder/mnt/" + fake.SNAPSHOT_ID - path = "unmanage-snapshot-" + fake.SNAPSHOT_ID + path = "unmanage-%s.%s" % (self.snapshot.volume.name, self.snapshot.id) loc = {'provider_location': '172.24.49.21:/fs-cinder'} existing_ref = {'source-name': '172.24.49.21:/fs-cinder/' + fake.SNAPSHOT_ID} @@ -557,10 +598,30 @@ class HNASNFSDriverTest(test.TestCase): self.assertEqual(loc, out) + def test_manage_existing_snapshot_legacy(self): + nfs_share = "172.24.49.21:/fs-cinder" + nfs_mount = "/opt/stack/data/cinder/mnt/" + fake.SNAPSHOT_ID + path = "unmanage-snapshot-%s" % self.snapshot.id + loc = {'provider_location': '172.24.49.21:/fs-cinder'} + existing_ref = { + 'source-name': '172.24.49.21:/fs-cinder/' + fake.SNAPSHOT_ID} + + self.mock_object(self.driver, '_get_share_mount_and_vol_from_vol_ref', + return_value=(nfs_share, nfs_mount, path)) + self.mock_object(backend.HNASSSHBackend, 'check_snapshot_parent', + return_value=True) + self.mock_object(self.driver, '_execute') + self.mock_object(backend.HNASSSHBackend, 'get_export_path', + return_value='fs-cinder') + + out = self.driver.manage_existing_snapshot(self.snapshot, existing_ref) + + self.assertEqual(loc, out) + def test_manage_existing_snapshot_not_parent_exception(self): nfs_share = "172.24.49.21:/fs-cinder" nfs_mount = "/opt/stack/data/cinder/mnt/" + fake.SNAPSHOT_ID - path = "unmanage-snapshot-" + fake.SNAPSHOT_ID + path = "unmanage-%s.%s" % (fake.VOLUME_ID, self.snapshot.id) existing_ref = {'source-name': '172.24.49.21:/fs-cinder/' + fake.SNAPSHOT_ID} @@ -601,7 +662,7 @@ class HNASNFSDriverTest(test.TestCase): def test_unmanage_snapshot(self): path = '/opt/stack/cinder/mnt/826692dfaeaf039b1f4dcc1dacee2c2e' - snapshot_name = 'snapshot-' + self.snapshot.id + snapshot_name = "%s.%s" % (self.snapshot.volume.name, self.snapshot.id) old_path = os.path.join(path, snapshot_name) new_path = os.path.join(path, 'unmanage-' + snapshot_name) diff --git a/cinder/volume/drivers/hitachi/hnas_nfs.py b/cinder/volume/drivers/hitachi/hnas_nfs.py index 67725dbf91b..889ce1142ec 100644 --- a/cinder/volume/drivers/hitachi/hnas_nfs.py +++ b/cinder/volume/drivers/hitachi/hnas_nfs.py @@ -83,6 +83,7 @@ class HNASNFSDriver(nfs.NfsDriver): Fixed driver stats reporting Version 6.0.0: Deprecated hnas_svcX_vol_type configuration Added list-manageable volumes/snapshots support + Rename snapshots to link with its original volume """ # ThirdPartySystems wiki page CI_WIKI_NAME = "Hitachi_HNAS_CI" @@ -146,6 +147,12 @@ class HNASNFSDriver(nfs.NfsDriver): return service + def _get_snapshot_name(self, snapshot): + snap_file_name = ("%(vol_name)s.%(snap_id)s" % + {'vol_name': snapshot.volume.name, + 'snap_id': snapshot.id}) + return snap_file_name + @cutils.trace def extend_volume(self, volume, new_size): """Extend an existing volume. @@ -155,7 +162,7 @@ class HNASNFSDriver(nfs.NfsDriver): :raises: InvalidResults """ nfs_mount = volume.provider_location - path = self._get_volume_path(nfs_mount, volume.name) + path = self._get_file_path(nfs_mount, volume.name) # Resize the image file on share to new size. LOG.info(_LI("Checking file for resize.")) @@ -190,10 +197,18 @@ class HNASNFSDriver(nfs.NfsDriver): :param snapshot: source snapshot :returns: the provider_location of the volume created """ - self._clone_volume(snapshot.volume, volume.name, snapshot.name) - share = snapshot.volume.provider_location + nfs_mount = snapshot.volume.provider_location + snapshot_name = self._get_snapshot_name(snapshot) - return {'provider_location': share} + if self._file_not_present(nfs_mount, snapshot_name): + LOG.info(_LI("Creating volume %(vol)s from legacy " + "snapshot %(snap)s."), + {'vol': volume.name, 'snap': snapshot.name}) + snapshot_name = snapshot.name + + self._clone_volume(snapshot.volume, volume.name, snapshot_name) + + return {'provider_location': nfs_mount} @cutils.trace def create_snapshot(self, snapshot): @@ -202,7 +217,8 @@ class HNASNFSDriver(nfs.NfsDriver): :param snapshot: dictionary snapshot reference :returns: the provider_location of the snapshot created """ - self._clone_volume(snapshot.volume, snapshot.name) + snapshot_name = self._get_snapshot_name(snapshot) + self._clone_volume(snapshot.volume, snapshot_name) share = snapshot.volume.provider_location LOG.debug('Share: %(shr)s', {'shr': share}) @@ -217,39 +233,48 @@ class HNASNFSDriver(nfs.NfsDriver): :param snapshot: dictionary snapshot reference """ nfs_mount = snapshot.volume.provider_location + snapshot_name = self._get_snapshot_name(snapshot) - if self._volume_not_present(nfs_mount, snapshot.name): - return True + if self._file_not_present(nfs_mount, snapshot_name): + # Snapshot with new name does not exist. The verification + # for a file with legacy name will be done. + snapshot_name = snapshot.name - self._execute('rm', self._get_volume_path(nfs_mount, snapshot.name), - run_as_root=True) + if self._file_not_present(nfs_mount, snapshot_name): + # The file does not exist. Nothing to do. + return - def _volume_not_present(self, nfs_mount, volume_name): - """Check if volume does not exist. + self._execute('rm', self._get_file_path( + nfs_mount, snapshot_name), run_as_root=True) + + def _file_not_present(self, nfs_mount, volume_name): + """Check if file does not exist. :param nfs_mount: string path of the nfs share :param volume_name: string volume name - :returns: boolean (true for volume not present and false otherwise) + :returns: boolean (true for file not present and false otherwise) """ try: - self._try_execute('ls', - self._get_volume_path(nfs_mount, volume_name)) - except processutils.ProcessExecutionError: - # If the volume isn't present - return True + self._execute('ls', self._get_file_path(nfs_mount, volume_name)) + except processutils.ProcessExecutionError as e: + if "No such file or directory" in e.stderr: + # If the file isn't present + return True + else: + raise return False - def _get_volume_path(self, nfs_share, volume_name): - """Get volume path (local fs path) for given name on given nfs share. + def _get_file_path(self, nfs_share, file_name): + """Get file path (local fs path) for given name on given nfs share. :param nfs_share string, example 172.18.194.100:/var/nfs - :param volume_name string, + :param file_name string, example volume-91ee65ec-c473-4391-8c09-162b00c68a8c :returns: the local path according to the parameters """ return os.path.join(self._get_mount_point_for_share(nfs_share), - volume_name) + file_name) @cutils.trace def create_cloned_volume(self, volume, src_vref): @@ -680,7 +705,6 @@ class HNASNFSDriver(nfs.NfsDriver): return size def _check_snapshot_parent(self, volume, old_snap_name, share): - volume_name = 'volume-' + volume.id (fs, path, fs_label) = self._get_service(volume) # 172.24.49.34:/nfs_cinder @@ -692,6 +716,13 @@ class HNASNFSDriver(nfs.NfsDriver): return self.backend.check_snapshot_parent(volume_path, old_snap_name, fs_label) + def _get_snapshot_origin_from_name(self, snap_name): + """Gets volume name from snapshot names""" + if 'unmanage' in snap_name: + return snap_name.split('.')[0][9:] + + return snap_name.split('.')[0] + @cutils.trace def manage_existing_snapshot(self, snapshot, existing_ref): """Brings an existing backend storage object under Cinder management. @@ -712,29 +743,31 @@ class HNASNFSDriver(nfs.NfsDriver): 'ref': existing_ref['source-name']}) volume = snapshot.volume + parent_name = self._get_snapshot_origin_from_name(src_snapshot_name) - # Check if the snapshot belongs to the volume - real_parent = self._check_snapshot_parent(volume, src_snapshot_name, - nfs_share) + if parent_name != volume.name: + # Check if the snapshot belongs to the volume for the legacy case + if not self._check_snapshot_parent( + volume, src_snapshot_name, nfs_share): + msg = (_("This snapshot %(snap)s doesn't belong " + "to the volume parent %(vol)s.") % + {'snap': src_snapshot_name, 'vol': volume.id}) + raise exception.ManageExistingInvalidReference( + existing_ref=existing_ref, reason=msg) - if not real_parent: - msg = (_("This snapshot %(snap)s doesn't belong " - "to the volume parent %(vol)s.") % - {'snap': snapshot.id, 'vol': volume.id}) - raise exception.ManageExistingInvalidReference( - existing_ref=existing_ref, reason=msg) + snapshot_name = self._get_snapshot_name(snapshot) - if src_snapshot_name == snapshot.name: + if src_snapshot_name == snapshot_name: LOG.debug("New Cinder snapshot %(snap)s name matches reference " - "name. No need to rename.", {'snap': snapshot.name}) + "name. No need to rename.", {'snap': snapshot_name}) else: src_snap = os.path.join(nfs_mount, src_snapshot_name) - dst_snap = os.path.join(nfs_mount, snapshot.name) + dst_snap = os.path.join(nfs_mount, snapshot_name) try: self._try_execute("mv", src_snap, dst_snap, run_as_root=False, check_exit_code=True) LOG.info(_LI("Setting newly managed Cinder snapshot name " - "to %(snap)s."), {'snap': snapshot.name}) + "to %(snap)s."), {'snap': snapshot_name}) self._set_rw_permissions_for_all(dst_snap) except (OSError, processutils.ProcessExecutionError) as err: msg = (_("Failed to manage existing snapshot " @@ -760,10 +793,16 @@ class HNASNFSDriver(nfs.NfsDriver): """ path = self._get_mount_point_for_share(snapshot.provider_location) + snapshot_name = self._get_snapshot_name(snapshot) - new_name = "unmanage-" + snapshot.name + if self._file_not_present(snapshot.provider_location, snapshot_name): + LOG.info(_LI("Unmanaging legacy snapshot %(snap)s."), + {'snap': snapshot.name}) + snapshot_name = snapshot.name - old_path = os.path.join(path, snapshot.name) + new_name = "unmanage-" + snapshot_name + + old_path = os.path.join(path, snapshot_name) new_path = os.path.join(path, new_name) try: @@ -863,10 +902,12 @@ class HNASNFSDriver(nfs.NfsDriver): for resource in bend_rsrc[exp]: # Ignoring resources of unwanted types - if ((resource_type == 'volume' and 'snapshot' in resource) or - (resource_type == 'snapshot' and - 'volume' in resource)): + if ((resource_type == 'volume' and + ('.' in resource or 'snapshot' in resource)) or + (resource_type == 'snapshot' and '.' not in resource and + 'snapshot' not in resource)): continue + path = '%s/%s' % (exp, resource) mnt_path = '%s/%s' % (mnt_point, resource) size = self._get_file_size(mnt_path) @@ -877,9 +918,12 @@ class HNASNFSDriver(nfs.NfsDriver): if resource_type == 'volume': potential_id = utils.extract_id_from_volume_name(resource) - else: + elif 'snapshot' in resource: + # This is for the snapshot legacy case potential_id = utils.extract_id_from_snapshot_name( resource) + else: + potential_id = resource.split('.')[1] # When a resource is already managed by cinder, it's not # recommended to manage it again. So we set safe_to_manage = @@ -898,24 +942,32 @@ class HNASNFSDriver(nfs.NfsDriver): # source-reference, throw a warning message and fill the # extra-info. if resource_type == 'snapshot': - path = path.split(':')[1] - origin = self._get_snapshot_origin(path, exports[exp]) - - if not origin: - # if origin is empty, the file is not a clone - continue - elif len(origin) == 1: - origin = origin[0].split('/')[2] - origin = utils.extract_id_from_volume_name(origin) + if 'snapshot' not in resource: + origin = self._get_snapshot_origin_from_name(resource) + if 'unmanage' in origin: + origin = origin[16:] + else: + origin = origin[7:] rsrc_inf['source_reference'] = {'id': origin} else: - LOG.warning(_LW("Could not determine the volume that " - "owns the snapshot %(snap)s"), - {'snap': resource}) - rsrc_inf['source_reference'] = {'id': 'unknown'} - rsrc_inf['extra_info'] = ('Could not determine the ' - 'volume that owns the ' - 'snapshot') + path = path.split(':')[1] + origin = self._get_snapshot_origin(path, exports[exp]) + + if not origin: + # if origin is empty, the file is not a clone + continue + elif len(origin) == 1: + origin = origin[0].split('/')[2] + origin = utils.extract_id_from_volume_name(origin) + rsrc_inf['source_reference'] = {'id': origin} + else: + LOG.warning(_LW("Could not determine the volume " + "that owns the snapshot %(snap)s"), + {'snap': resource}) + rsrc_inf['source_reference'] = {'id': 'unknown'} + rsrc_inf['extra_info'] = ('Could not determine ' + 'the volume that owns ' + 'the snapshot') entries.append(rsrc_inf) diff --git a/releasenotes/notes/hnas-change-snapshot-names-8153b043eb7e99fc.yaml b/releasenotes/notes/hnas-change-snapshot-names-8153b043eb7e99fc.yaml new file mode 100644 index 00000000000..c940e4eba62 --- /dev/null +++ b/releasenotes/notes/hnas-change-snapshot-names-8153b043eb7e99fc.yaml @@ -0,0 +1,6 @@ +--- +deprecations: + - Support for snapshots named in the backend as ``snapshot-`` + is deprecated. Snapshots are now named in the backend as + ``.``. +