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
e52f304313 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
This commit is contained in:
Tom Barron
parent
39ae7a23eb
commit
cc651ff8c6
|
|
@ -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'))
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Reference in New Issue