From beed5c7789d4d05137a1f8dce87c56a7c3500cdf Mon Sep 17 00:00:00 2001 From: Clinton Knight Date: Mon, 17 Oct 2016 12:55:47 -0400 Subject: [PATCH] NetApp cDOT driver fails to clone from NFS cache In some cases, the NetApp cDOT NFS driver fails to clone from an NFS image cache. Root cause is code that compares extra specs of the new volume to capability lists that the driver reports to the scheduler (such as a list of disk types). It fails because a list is mutable, so not hashable, so the code cannot do set-based comparison operations against extra specs. The simple fix is to stop doing set operations on extra specs. Also, update the time on the image cache file so that our tests can distinguish between a fast clone from cache and the slower copy-from-Glance fallback. Change-Id: I1dfacd24d49b11967eeb0c6b8560114d01952d74 Closes-Bug: #1634203 --- .../drivers/netapp/dataontap/test_nfs_base.py | 5 + .../drivers/netapp/dataontap/utils/fakes.py | 16 ++-- .../dataontap/utils/test_capabilities.py | 92 +++++++++++++++++++ .../drivers/netapp/dataontap/nfs_base.py | 2 + .../netapp/dataontap/utils/capabilities.py | 49 ++++++++-- ...from-nfs-image-cache-2218fb402783bc20.yaml | 4 + 6 files changed, 151 insertions(+), 17 deletions(-) create mode 100644 releasenotes/notes/bug-1634203-netapp-cdot-fix-clone-from-nfs-image-cache-2218fb402783bc20.yaml diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py index 667f10c39e7..c3fa715f39f 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py @@ -439,6 +439,7 @@ class NetAppNfsDriverTestCase(test.TestCase): self.mock_object(os.path, 'exists', mock.Mock(return_value=path_exists)) self.mock_object(self.driver, '_clone_backing_file_for_volume') + self.mock_object(os, 'utime') retval = self.driver._do_clone_rel_img_cache( fake.CLONE_SOURCE_NAME, fake.CLONE_DESTINATION_NAME, @@ -450,8 +451,12 @@ class NetAppNfsDriverTestCase(test.TestCase): self.driver._clone_backing_file_for_volume.assert_called_once_with( fake.CLONE_SOURCE_NAME, fake.CLONE_DESTINATION_NAME, share=fake.NFS_SHARE, volume_id=None) + os.utime.assert_called_once_with( + 'dir/' + fake.CLONE_SOURCE_NAME, None) else: self.driver._clone_backing_file_for_volume.assert_not_called() + os.utime.assert_not_called() + os.path.exists.assert_called_once_with( 'dir/' + fake.CLONE_DESTINATION_NAME) diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/utils/fakes.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/utils/fakes.py index e104f9098ed..adb4bc25052 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/utils/fakes.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/utils/fakes.py @@ -39,8 +39,8 @@ SSC = { 'netapp_dedup': 'true', 'netapp_mirrored': 'false', 'netapp_raid_type': 'raid_dp', - 'netapp_disk_type': 'SSD', - 'netapp_hybrid_aggregate': False, + 'netapp_disk_type': ['SSD'], + 'netapp_hybrid_aggregate': 'false', 'pool_name': 'volume1', }, 'volume2': { @@ -52,8 +52,8 @@ SSC = { 'netapp_dedup': 'true', 'netapp_mirrored': 'true', 'netapp_raid_type': 'raid_dp', - 'netapp_disk_type': 'FCAL', - 'netapp_hybrid_aggregate': True, + 'netapp_disk_type': ['FCAL', 'SSD'], + 'netapp_hybrid_aggregate': 'true', 'pool_name': 'volume2', }, } @@ -95,14 +95,14 @@ SSC_MIRROR_INFO = { SSC_AGGREGATE_INFO = { 'volume1': { - 'netapp_disk_type': 'SSD', + 'netapp_disk_type': ['SSD'], 'netapp_raid_type': 'raid_dp', - 'netapp_hybrid_aggregate': False, + 'netapp_hybrid_aggregate': 'false', }, 'volume2': { - 'netapp_disk_type': 'FCAL', + 'netapp_disk_type': ['FCAL', 'SSD'], 'netapp_raid_type': 'raid_dp', - 'netapp_hybrid_aggregate': True, + 'netapp_hybrid_aggregate': 'true', }, } diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/utils/test_capabilities.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/utils/test_capabilities.py index d8df5833af2..b99c5b77620 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/utils/test_capabilities.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/utils/test_capabilities.py @@ -91,6 +91,12 @@ class CapabilitiesLibraryTestCase(test.TestCase): self.assertEqual(fake.SSC, result) self.assertIsNot(fake.SSC, result) + def test_get_ssc_flexvol_names(self): + + result = self.ssc_library.get_ssc_flexvol_names() + + self.assertItemsEqual(fake.SSC_VOLUMES, result) + def test_get_ssc_for_flexvol(self): result = self.ssc_library.get_ssc_for_flexvol(fake.SSC_VOLUMES[0]) @@ -350,12 +356,98 @@ class CapabilitiesLibraryTestCase(test.TestCase): 'netapp_mirrored': 'true', 'netapp_raid_type': 'raid_dp', 'netapp_disk_type': 'FCAL', + 'non_ssc_key': 'fake_value', } result = self.ssc_library.get_matching_flexvols_for_extra_specs(specs) self.assertEqual(['volume2'], result) + @ddt.data( + { + 'flexvol_info': { + 'netapp_dedup': 'true', + }, + 'extra_specs': { + 'netapp_dedup': 'true', + 'non_ssc_key': 'fake_value', + } + }, + { + 'flexvol_info': fake.SSC['volume1'], + 'extra_specs': { + 'netapp_disk_type': 'SSD', + 'pool_name': 'volume1', + } + }, + { + 'flexvol_info': fake.SSC['volume2'], + 'extra_specs': { + 'netapp_disk_type': 'SSD', + 'netapp_hybrid_aggregate': 'true', + } + } + ) + @ddt.unpack + def test_flexvol_matches_extra_specs(self, flexvol_info, extra_specs): + + result = self.ssc_library._flexvol_matches_extra_specs(flexvol_info, + extra_specs) + + self.assertTrue(result) + + @ddt.data( + { + 'flexvol_info': { + 'netapp_dedup': 'true', + }, + 'extra_specs': { + 'netapp_dedup': 'false', + 'non_ssc_key': 'fake_value', + } + }, + { + 'flexvol_info': fake.SSC['volume2'], + 'extra_specs': { + 'netapp_disk_type': 'SSD', + 'pool_name': 'volume1', + } + }, + { + 'flexvol_info': fake.SSC['volume2'], + 'extra_specs': { + 'netapp_disk_type': 'SATA', + } + } + ) + @ddt.unpack + def test_flexvol_matches_extra_specs_no_match(self, flexvol_info, + extra_specs): + + result = self.ssc_library._flexvol_matches_extra_specs(flexvol_info, + extra_specs) + + self.assertFalse(result) + + @ddt.data(('SSD', 'SSD'), ('SSD', ['SSD', 'FCAL'])) + @ddt.unpack + def test_extra_spec_matches(self, extra_spec_value, ssc_flexvol_value): + + result = self.ssc_library._extra_spec_matches(extra_spec_value, + ssc_flexvol_value) + + self.assertTrue(result) + + @ddt.data(('SSD', 'FCAL'), ('SSD', ['FCAL'])) + @ddt.unpack + def test_extra_spec_matches_no_match(self, extra_spec_value, + ssc_flexvol_value): + + result = self.ssc_library._extra_spec_matches(extra_spec_value, + ssc_flexvol_value) + + self.assertFalse(result) + def test_modify_extra_specs_for_comparison(self): specs = { diff --git a/cinder/volume/drivers/netapp/dataontap/nfs_base.py b/cinder/volume/drivers/netapp/dataontap/nfs_base.py index e0a317fe29b..694289e6785 100644 --- a/cinder/volume/drivers/netapp/dataontap/nfs_base.py +++ b/cinder/volume/drivers/netapp/dataontap/nfs_base.py @@ -403,6 +403,8 @@ class NetAppNfsDriver(driver.ManageableVD, LOG.info(_LI('Cloning from cache to destination %s'), dst) self._clone_backing_file_for_volume(src, dst, volume_id=None, share=share) + src_path = '%s/%s' % (dir, src) + os.utime(src_path, None) _do_clone() @utils.synchronized('clean_cache') diff --git a/cinder/volume/drivers/netapp/dataontap/utils/capabilities.py b/cinder/volume/drivers/netapp/dataontap/utils/capabilities.py index 8447b8dc9a4..3df17f331c7 100644 --- a/cinder/volume/drivers/netapp/dataontap/utils/capabilities.py +++ b/cinder/volume/drivers/netapp/dataontap/utils/capabilities.py @@ -215,20 +215,49 @@ class CapabilitiesLibrary(object): """Return a list of flexvol names that match a set of extra specs.""" extra_specs = self._modify_extra_specs_for_comparison(extra_specs) - extra_specs_set = set(extra_specs.items()) - ssc = self.get_ssc() matching_flexvols = [] - for flexvol_name, flexvol_info in ssc.items(): - if extra_specs_set.issubset(set(flexvol_info.items())): + for flexvol_name, flexvol_info in self.get_ssc().items(): + + if self._flexvol_matches_extra_specs(flexvol_info, extra_specs): matching_flexvols.append(flexvol_name) return matching_flexvols + def _flexvol_matches_extra_specs(self, flexvol_info, extra_specs): + """Check whether the SSC data for a FlexVol matches extra specs. + + A set of extra specs is considered a match for a FlexVol if, for each + extra spec, the value matches what is in SSC or the key is unknown to + SSC. + """ + + for extra_spec_key, extra_spec_value in extra_specs.items(): + + if extra_spec_key in flexvol_info: + if not self._extra_spec_matches(extra_spec_value, + flexvol_info[extra_spec_key]): + return False + + return True + + def _extra_spec_matches(self, extra_spec_value, ssc_flexvol_value): + """Check whether an extra spec value matches something in the SSC. + + The SSC values may be scalars or lists, so the extra spec value must be + compared to the SSC value if it is a scalar, or it must be sought in + the list. + """ + + if isinstance(ssc_flexvol_value, list): + return extra_spec_value in ssc_flexvol_value + else: + return extra_spec_value == ssc_flexvol_value + def _modify_extra_specs_for_comparison(self, extra_specs): """Adjust extra spec values for simple comparison to SSC values. - Most extra-spec key-value tuples may be directly compared. But the + Most extra-spec key-value tuples may be directly compared. But the boolean values that take the form ' True' or ' False' must be modified to allow comparison with the values we keep in the SSC and report to the scheduler. @@ -237,9 +266,11 @@ class CapabilitiesLibrary(object): modified_extra_specs = copy.deepcopy(extra_specs) for key, value in extra_specs.items(): - if re.match('\s+True', value, re.I): - modified_extra_specs[key] = True - elif re.match('\s+False', value, re.I): - modified_extra_specs[key] = False + + if isinstance(value, six.string_types): + if re.match('\s+True', value, re.I): + modified_extra_specs[key] = True + elif re.match('\s+False', value, re.I): + modified_extra_specs[key] = False return modified_extra_specs diff --git a/releasenotes/notes/bug-1634203-netapp-cdot-fix-clone-from-nfs-image-cache-2218fb402783bc20.yaml b/releasenotes/notes/bug-1634203-netapp-cdot-fix-clone-from-nfs-image-cache-2218fb402783bc20.yaml new file mode 100644 index 00000000000..aefc36b1829 --- /dev/null +++ b/releasenotes/notes/bug-1634203-netapp-cdot-fix-clone-from-nfs-image-cache-2218fb402783bc20.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Fixed an issue where the NetApp cDOT NFS driver failed to clone new + volumes from the image cache.