openstack/cinder
Code Issues Proposed changes

NetApp cDOT driver fails to clone from NFS cache

Browse Source 
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
This commit is contained in:
Clinton Knight 2016-10-17 12:55:47 -04:00 committed by Goutham Pacha Ravi
parent 366acd02b4
commit beed5c7789
6 changed files with 151 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_base.py
View File

@ -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)

16
cinder/tests/unit/volume/drivers/netapp/dataontap/utils/fakes.py
View File

@ -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',
},
}

92
cinder/tests/unit/volume/drivers/netapp/dataontap/utils/test_capabilities.py
View File

@ -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 = {

2
cinder/volume/drivers/netapp/dataontap/nfs_base.py
View File

@ -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')

49
cinder/volume/drivers/netapp/dataontap/utils/capabilities.py
View File

@ -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 '<is> True' or '<is> 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('<is>\s+True', value, re.I):
modified_extra_specs[key] = True
elif re.match('<is>\s+False', value, re.I):
modified_extra_specs[key] = False
if isinstance(value, six.string_types):
if re.match('<is>\s+True', value, re.I):
modified_extra_specs[key] = True
elif re.match('<is>\s+False', value, re.I):
modified_extra_specs[key] = False
return modified_extra_specs

4
releasenotes/notes/bug-1634203-netapp-cdot-fix-clone-from-nfs-image-cache-2218fb402783bc20.yaml Normal file
View File

@ -0,0 +1,4 @@
---
fixes:
- Fixed an issue where the NetApp cDOT NFS driver failed to clone new
volumes from the image cache.