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

diff --git a/cinder/tests/unit/test_netapp_nfs.py b/cinder/tests/unit/test_netapp_nfs.py
index ac9567799ed8747e74741d9d1d46a7e98a1272b8..221da44e63f7908a664452e1ed0357a700184293 100644 (file)
--- 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 894ef21586867f6651e8967ccc4717a1b606782c..bb22feff5363bae3fba430e9bf5f75d309cc3c2b 100644 (file)
--- 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 9c855de3b02fe041b48048bee76c4f63fade4274..07c0d7ee12bc903bbe9e5074b3f5e79c515a1d7f 100644 (file)
--- 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 4d714fd9b64f81daba60a4b9e2bf3209bd675939..13536fb33dfdefe9df45cfe799e24f35ed0e7639 100644 (file)
--- 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 1960b6b7f0ce2568e8f868dabfc0d37d9f874965..3e776f225af1e74e813160509bcebd51fdf2d6be 100644 (file)
--- 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 e3f562b3c7a952cb5b68b73d0de2479f718af6e3..bc89adedcb29c369297530394db2951edc4157b7 100644 (file)
--- 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 0dcb6d9d9858feb26e1e1db5f844df14e36cd1ca..5c9d99e2e3f55440311308049b5c9897030b138e 100644 (file)
--- 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)