From cc651ff8c658c4f8d4415d46a348136b653ab30a Mon Sep 17 00:00:00 2001 From: Tom Barron Date: Thu, 10 Sep 2015 19:53:29 -0400 Subject: [PATCH] Fix NetApp clone from glance failure When glance and cinder are backed by the same NetApp DOT flexvol container, exposed as an NFS share, a direct clone is attempted of the backing files rather than a copy. With the advent of kilo 2015.1.1, this clone operation fails with the message: "Image cloning unsuccessful ... need more than 2 values to unpack." The issue here is that the clone path code is calling the parent nfs class _is_share_eligible() method to see if there is enough space on the destination flexvol and this method calls _get_capacity_info() expecting a three-tuple to be returned. However, commit e52f304313efc695f7dd89c222041bffd53c131a deliberately changed the return values of _get_capacity_info() within the NetApp drivers to return a 2-tuple, based on API calls to obtain capacity information from the NetApp storage arrays. That commit missed that this method would also be invoked by the parent nfs class _is_share_eligible() method, which expects a different signature. The fix in this commit is to replace the call to the parent _is_share_eligible() method with a more appropriate NetApp class specific _share_has_space_for_clone() method. This new method uses the NetApp version of _get_capacity_info to get space information from the arrays via NetApp apis and expects a 2-tuple to be returned from this method. Change-Id: Ib0c69e5ea7b32d17930fe0bdcdb9357fd4e4a244 Closes-bug: 1490845 --- cinder/tests/unit/test_netapp_nfs.py | 36 +++++----- .../netapp/dataontap/test_nfs_7mode.py | 13 ++++ .../drivers/netapp/dataontap/test_nfs_base.py | 70 ++++++++++++++++++- .../netapp/dataontap/test_nfs_cmode.py | 56 +++++++++++++++ .../drivers/netapp/dataontap/nfs_7mode.py | 7 +- .../drivers/netapp/dataontap/nfs_base.py | 22 ++++-- .../drivers/netapp/dataontap/nfs_cmode.py | 16 ++++- 7 files changed, 190 insertions(+), 30 deletions(-) diff --git a/cinder/tests/unit/test_netapp_nfs.py b/cinder/tests/unit/test_netapp_nfs.py index ac9567799..221da44e6 100644 --- a/cinder/tests/unit/test_netapp_nfs.py +++ b/cinder/tests/unit/test_netapp_nfs.py @@ -516,13 +516,13 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): mox.StubOutWithMock(drv, '_find_image_in_cache') mox.StubOutWithMock(drv, '_do_clone_rel_img_cache') mox.StubOutWithMock(drv, '_post_clone_image') - mox.StubOutWithMock(drv, '_is_share_vol_compatible') + mox.StubOutWithMock(drv, '_is_share_clone_compatible') utils.get_volume_extra_specs(mox_lib.IgnoreArg()) drv._find_image_in_cache(mox_lib.IgnoreArg()).AndReturn( [('share', 'file_name')]) - drv._is_share_vol_compatible(mox_lib.IgnoreArg(), - mox_lib.IgnoreArg()).AndReturn(True) + drv._is_share_clone_compatible(mox_lib.IgnoreArg(), + mox_lib.IgnoreArg()).AndReturn(True) drv._do_clone_rel_img_cache('file_name', 'vol', 'share', 'file_name') drv._post_clone_image(volume) @@ -547,14 +547,14 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): mox.StubOutWithMock(utils, 'get_volume_extra_specs') mox.StubOutWithMock(drv, '_find_image_in_cache') mox.StubOutWithMock(drv, '_is_cloneable_share') - mox.StubOutWithMock(drv, '_is_share_vol_compatible') + mox.StubOutWithMock(drv, '_is_share_clone_compatible') utils.get_volume_extra_specs(mox_lib.IgnoreArg()) drv._find_image_in_cache(mox_lib.IgnoreArg()).AndReturn([]) drv._is_cloneable_share( mox_lib.IgnoreArg()).AndReturn('127.0.0.1:/share') - drv._is_share_vol_compatible(mox_lib.IgnoreArg(), - mox_lib.IgnoreArg()).AndReturn(False) + drv._is_share_clone_compatible(mox_lib.IgnoreArg(), + mox_lib.IgnoreArg()).AndReturn(False) mox.ReplayAll() (prop, cloned) = drv.clone_image( @@ -582,14 +582,14 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): mox.StubOutWithMock(drv, '_discover_file_till_timeout') mox.StubOutWithMock(drv, '_set_rw_permissions') mox.StubOutWithMock(drv, '_resize_image_file') - mox.StubOutWithMock(drv, '_is_share_vol_compatible') + mox.StubOutWithMock(drv, '_is_share_clone_compatible') utils.get_volume_extra_specs(mox_lib.IgnoreArg()) drv._find_image_in_cache(mox_lib.IgnoreArg()).AndReturn([]) drv._is_cloneable_share( mox_lib.IgnoreArg()).AndReturn('127.0.0.1:/share') - drv._is_share_vol_compatible(mox_lib.IgnoreArg(), - mox_lib.IgnoreArg()).AndReturn(True) + drv._is_share_clone_compatible(mox_lib.IgnoreArg(), + mox_lib.IgnoreArg()).AndReturn(True) drv._get_mount_point_for_share(mox_lib.IgnoreArg()).AndReturn('/mnt') image_utils.qemu_img_info('/mnt/img-id', run_as_root=True).\ AndReturn(self.get_img_info('raw')) @@ -624,14 +624,14 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): mox.StubOutWithMock(drv, '_resize_image_file') mox.StubOutWithMock(image_utils, 'convert_image') mox.StubOutWithMock(drv, '_register_image_in_cache') - mox.StubOutWithMock(drv, '_is_share_vol_compatible') + mox.StubOutWithMock(drv, '_is_share_clone_compatible') utils.get_volume_extra_specs(mox_lib.IgnoreArg()) drv._find_image_in_cache(mox_lib.IgnoreArg()).AndReturn([]) drv._is_cloneable_share('nfs://127.0.0.1/share/img-id').AndReturn( '127.0.0.1:/share') - drv._is_share_vol_compatible(mox_lib.IgnoreArg(), - mox_lib.IgnoreArg()).AndReturn(True) + drv._is_share_clone_compatible(mox_lib.IgnoreArg(), + mox_lib.IgnoreArg()).AndReturn(True) drv._get_mount_point_for_share('127.0.0.1:/share').AndReturn('/mnt') image_utils.qemu_img_info('/mnt/img-id', run_as_root=True).\ AndReturn(self.get_img_info('notraw')) @@ -668,7 +668,7 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): mox.StubOutWithMock(drv, '_discover_file_till_timeout') mox.StubOutWithMock(image_utils, 'convert_image') mox.StubOutWithMock(drv, '_register_image_in_cache') - mox.StubOutWithMock(drv, '_is_share_vol_compatible') + mox.StubOutWithMock(drv, '_is_share_clone_compatible') mox.StubOutWithMock(drv, '_do_qos_for_volume') mox.StubOutWithMock(drv, 'local_path') @@ -676,8 +676,8 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): drv._find_image_in_cache(mox_lib.IgnoreArg()).AndReturn([]) drv._is_cloneable_share('nfs://127.0.0.1/share/img-id').AndReturn( '127.0.0.1:/share') - drv._is_share_vol_compatible(mox_lib.IgnoreArg(), - mox_lib.IgnoreArg()).AndReturn(True) + drv._is_share_clone_compatible(mox_lib.IgnoreArg(), + mox_lib.IgnoreArg()).AndReturn(True) drv._get_mount_point_for_share('127.0.0.1:/share').AndReturn('/mnt') image_utils.qemu_img_info('/mnt/img-id', run_as_root=True).\ AndReturn(self.get_img_info('notraw')) @@ -720,15 +720,15 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): mox.StubOutWithMock(image_utils, 'convert_image') mox.StubOutWithMock(drv, '_do_qos_for_volume') mox.StubOutWithMock(drv, '_register_image_in_cache') - mox.StubOutWithMock(drv, '_is_share_vol_compatible') + mox.StubOutWithMock(drv, '_is_share_clone_compatible') mox.StubOutWithMock(drv, 'local_path') utils.get_volume_extra_specs(mox_lib.IgnoreArg()) drv._find_image_in_cache(mox_lib.IgnoreArg()).AndReturn([]) drv._is_cloneable_share('nfs://127.0.0.1/share/img-id').AndReturn( '127.0.0.1:/share') - drv._is_share_vol_compatible(mox_lib.IgnoreArg(), - mox_lib.IgnoreArg()).AndReturn(True) + drv._is_share_clone_compatible(mox_lib.IgnoreArg(), + mox_lib.IgnoreArg()).AndReturn(True) drv._get_mount_point_for_share('127.0.0.1:/share').AndReturn('/mnt') image_utils.qemu_img_info('/mnt/img-id', run_as_root=True).\ AndReturn(self.get_img_info('notraw')) diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_7mode.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_7mode.py index 894ef2158..bb22feff5 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_7mode.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_7mode.py @@ -122,3 +122,16 @@ class NetApp7modeNfsDriverTestCase(test.TestCase): fake.NFS_SHARE, []) self.assertEqual([], result) + + @ddt.data({'has_space': True, 'expected': True}, + {'has_space': False, 'expected': False}) + @ddt.unpack + def test_is_share_clone_compatible(self, has_space, expected): + mock_share_has_space_for_clone = self.mock_object( + self.driver, '_share_has_space_for_clone') + mock_share_has_space_for_clone.return_value = has_space + + result = self.driver._is_share_clone_compatible(fake.VOLUME, + fake.NFS_SHARE) + + self.assertEqual(expected, result) 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 9c855de3b..07c0d7ee1 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 @@ -20,6 +20,7 @@ Unit tests for the NetApp NFS storage driver import os import copy +import ddt import mock from os_brick.remotefs import remotefs as remotefs_brick from oslo_utils import units @@ -33,14 +34,15 @@ from cinder.volume.drivers.netapp import utils as na_utils from cinder.volume.drivers import nfs +@ddt.ddt class NetAppNfsDriverTestCase(test.TestCase): def setUp(self): super(NetAppNfsDriverTestCase, self).setUp() configuration = mock.Mock() configuration.reserved_percentage = 0 configuration.nfs_mount_point_base = '/mnt/test' - configuration.nfs_used_ratio = 0.89 - configuration.nfs_oversub_ratio = 3.0 + configuration.nfs_used_ratio = 1.0 + configuration.nfs_oversub_ratio = 1.1 kwargs = {'configuration': configuration} @@ -348,3 +350,67 @@ class NetAppNfsDriverTestCase(test.TestCase): result = self.driver._get_export_path(fake.VOLUME_ID) self.assertEqual(expected, result) + + def test_is_share_clone_compatible(self): + self.assertRaises(NotImplementedError, + self.driver._is_share_clone_compatible, + fake.NFS_VOLUME, + fake.NFS_SHARE) + + @ddt.data( + {'size': 12, 'thin': False, 'over': 1.0, 'res': 0, 'expected': True}, + {'size': 12, 'thin': False, 'over': 1.0, 'res': 5, 'expected': False}, + {'size': 12, 'thin': True, 'over': 1.0, 'res': 5, 'expected': False}, + {'size': 12, 'thin': True, 'over': 1.1, 'res': 5, 'expected': True}, + {'size': 240, 'thin': True, 'over': 20.0, 'res': 0, 'expected': True}, + {'size': 241, 'thin': True, 'over': 20.0, 'res': 0, 'expected': False}, + ) + @ddt.unpack + def test_share_has_space_for_clone(self, size, thin, over, res, expected): + total_bytes = 20 * units.Gi + available_bytes = 12 * units.Gi + + with mock.patch.object(self.driver, + '_get_capacity_info', + return_value=( + total_bytes, available_bytes)): + with mock.patch.object(self.driver, + 'over_subscription_ratio', + over): + with mock.patch.object(self.driver, + 'reserved_percentage', + res): + result = self.driver._share_has_space_for_clone( + fake.NFS_SHARE, + size, + thin=thin) + self.assertEqual(expected, result) + + @ddt.data( + {'size': 12, 'thin': False, 'over': 1.0, 'res': 0, 'expected': True}, + {'size': 12, 'thin': False, 'over': 1.0, 'res': 5, 'expected': False}, + {'size': 12, 'thin': True, 'over': 1.0, 'res': 5, 'expected': False}, + {'size': 12, 'thin': True, 'over': 1.1, 'res': 5, 'expected': True}, + {'size': 240, 'thin': True, 'over': 20.0, 'res': 0, 'expected': True}, + {'size': 241, 'thin': True, 'over': 20.0, 'res': 0, 'expected': False}, + ) + @ddt.unpack + @mock.patch.object(nfs_base.NetAppNfsDriver, '_get_capacity_info') + def test_share_has_space_for_clone2(self, + mock_get_capacity, + size, thin, over, res, expected): + total_bytes = 20 * units.Gi + available_bytes = 12 * units.Gi + mock_get_capacity.return_value = (total_bytes, available_bytes) + + with mock.patch.object(self.driver, + 'over_subscription_ratio', + over): + with mock.patch.object(self.driver, + 'reserved_percentage', + res): + result = self.driver._share_has_space_for_clone( + fake.NFS_SHARE, + size, + thin=thin) + self.assertEqual(expected, result) diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py index 4d714fd9b..13536fb33 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py @@ -442,3 +442,59 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): mock_loopingcall.assert_has_calls([ mock.call(mock_remove_unused_qos_policy_groups)]) self.assertTrue(harvest_qos_periodic_task.start.called) + + @ddt.data( + {'space': True, 'ssc': True, 'match': True, 'expected': True}, + {'space': True, 'ssc': True, 'match': False, 'expected': False}, + {'space': True, 'ssc': False, 'match': True, 'expected': True}, + {'space': True, 'ssc': False, 'match': False, 'expected': True}, + {'space': False, 'ssc': True, 'match': True, 'expected': False}, + {'space': False, 'ssc': True, 'match': False, 'expected': False}, + {'space': False, 'ssc': False, 'match': True, 'expected': False}, + {'space': False, 'ssc': False, 'match': False, 'expected': False}, + ) + @ddt.unpack + @mock.patch.object(nfs_cmode.NetAppCmodeNfsDriver, + '_is_share_vol_type_match') + @mock.patch.object(nfs_cmode.NetAppCmodeNfsDriver, + '_share_has_space_for_clone') + @mock.patch.object(nfs_cmode.NetAppCmodeNfsDriver, + '_is_volume_thin_provisioned') + def test_is_share_clone_compatible(self, + mock_is_volume_thin_provisioned, + mock_share_has_space_for_clone, + mock_is_share_vol_type_match, + space, ssc, match, expected): + mock_share_has_space_for_clone.return_value = space + mock_is_share_vol_type_match.return_value = match + + with mock.patch.object(self.driver, 'ssc_enabled', ssc): + result = self.driver._is_share_clone_compatible(fake.VOLUME, + fake.NFS_SHARE) + self.assertEqual(expected, result) + + @ddt.data( + {'sparsed': True, 'ssc': True, 'vol_thin': True, 'expected': True}, + {'sparsed': True, 'ssc': True, 'vol_thin': False, 'expected': True}, + {'sparsed': True, 'ssc': False, 'vol_thin': True, 'expected': True}, + {'sparsed': True, 'ssc': False, 'vol_thin': False, 'expected': True}, + {'sparsed': False, 'ssc': True, 'vol_thin': True, 'expected': True}, + {'sparsed': False, 'ssc': True, 'vol_thin': False, 'expected': False}, + {'sparsed': False, 'ssc': False, 'vol_thin': True, 'expected': False}, + {'sparsed': False, 'ssc': False, 'vol_thin': False, 'expected': False}, + ) + @ddt.unpack + def test_is_volume_thin_provisioned( + self, sparsed, ssc, vol_thin, expected): + fake_volume = object() + ssc_vols = {'thin': {fake_volume if vol_thin else None}} + + with mock.patch.object(self.driver, 'ssc_enabled', ssc): + with mock.patch.object(self.driver, 'ssc_vols', ssc_vols): + with mock.patch.object(self.driver.configuration, + 'nfs_sparsed_volumes', + sparsed): + result = self.driver._is_volume_thin_provisioned( + fake_volume) + + self.assertEqual(expected, result) diff --git a/cinder/volume/drivers/netapp/dataontap/nfs_7mode.py b/cinder/volume/drivers/netapp/dataontap/nfs_7mode.py index 1960b6b7f..3e776f225 100644 --- a/cinder/volume/drivers/netapp/dataontap/nfs_7mode.py +++ b/cinder/volume/drivers/netapp/dataontap/nfs_7mode.py @@ -176,9 +176,10 @@ class NetApp7modeNfsDriver(nfs_base.NetAppNfsDriver): LOG.debug('No share match found for ip %s', ip) return None - def _is_share_vol_compatible(self, volume, share): - """Checks if share is compatible with volume to host it.""" - return self._is_share_eligible(share, volume['size']) + def _is_share_clone_compatible(self, volume, share): + """Checks if share is compatible with volume to host its clone.""" + thin = self.configuration.nfs_sparsed_volumes + return self._share_has_space_for_clone(share, volume['size'], thin) def _check_volume_type(self, volume, share, file_name, extra_specs): """Matches a volume type for share file.""" diff --git a/cinder/volume/drivers/netapp/dataontap/nfs_base.py b/cinder/volume/drivers/netapp/dataontap/nfs_base.py index e3f562b3c..bc89adedc 100644 --- a/cinder/volume/drivers/netapp/dataontap/nfs_base.py +++ b/cinder/volume/drivers/netapp/dataontap/nfs_base.py @@ -493,7 +493,7 @@ class NetAppNfsDriver(driver.ManageableVD, (share, file_name) = res LOG.debug('Cache share: %s', share) if (share and - self._is_share_vol_compatible(volume, share)): + self._is_share_clone_compatible(volume, share)): try: self._do_clone_rel_img_cache( file_name, volume['name'], share, file_name) @@ -513,7 +513,7 @@ class NetAppNfsDriver(driver.ManageableVD, run_as_root = self._execute_as_root for loc in image_locations: share = self._is_cloneable_share(loc) - if share and self._is_share_vol_compatible(volume, share): + if share and self._is_share_clone_compatible(volume, share): LOG.debug('Share is cloneable %s', share) volume['provider_location'] = share (__, ___, img_file) = loc.rpartition('/') @@ -699,10 +699,24 @@ class NetAppNfsDriver(driver.ManageableVD, path = self.local_path(volume) self._resize_image_file(path, new_size) - def _is_share_vol_compatible(self, volume, share): - """Checks if share is compatible with volume to host it.""" + def _is_share_clone_compatible(self, volume, share): + """Checks if share is compatible with volume to host its clone.""" raise NotImplementedError() + def _share_has_space_for_clone(self, share, size_in_gib, thin=True): + """Is there space on the share for a clone given the original size?""" + requested_size = size_in_gib * units.Gi + + total_size, total_available = self._get_capacity_info(share) + + reserved_ratio = self.reserved_percentage / 100.0 + reserved = int(round(total_size * reserved_ratio)) + available = max(0, total_available - reserved) + if thin: + available = available * self.over_subscription_ratio + + return available >= requested_size + def _check_share_can_hold_size(self, share, size): """Checks if volume can hold image with size.""" _tot_size, tot_available = self._get_capacity_info( diff --git a/cinder/volume/drivers/netapp/dataontap/nfs_cmode.py b/cinder/volume/drivers/netapp/dataontap/nfs_cmode.py index 0dcb6d9d9..5c9d99e2e 100644 --- a/cinder/volume/drivers/netapp/dataontap/nfs_cmode.py +++ b/cinder/volume/drivers/netapp/dataontap/nfs_cmode.py @@ -301,14 +301,24 @@ class NetAppCmodeNfsDriver(nfs_base.NetAppNfsDriver): return vol return None - def _is_share_vol_compatible(self, volume, share): - """Checks if share is compatible with volume to host it.""" - compatible = self._is_share_eligible(share, volume['size']) + def _is_share_clone_compatible(self, volume, share): + """Checks if share is compatible with volume to host its clone.""" + thin = self._is_volume_thin_provisioned(volume) + compatible = self._share_has_space_for_clone(share, + volume['size'], + thin) if compatible and self.ssc_enabled: matched = self._is_share_vol_type_match(volume, share) compatible = compatible and matched return compatible + def _is_volume_thin_provisioned(self, volume): + if self.configuration.nfs_sparsed_volumes: + return True + if self.ssc_enabled and volume in self.ssc_vols['thin']: + return True + return False + def _is_share_vol_type_match(self, volume, share): """Checks if share matches volume type.""" netapp_vol = self._get_vol_for_share(share) -- 2.45.2