diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py index e403589aaf..3884362d09 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -1831,6 +1831,10 @@ class NetAppCmodeFileStorageLibrary(object): self._get_backend_snapshot_name(snapshot['id'])) LOG.debug('Restoring snapshot %s', snapshot_name) vserver_client.restore_snapshot(share_name, snapshot_name) + volume = vserver_client.get_volume(share_name) + + # When calculating the size, round up to the next GB. + return int(math.ceil(float(volume['size']) / units.Gi)) @na_utils.trace def delete_snapshot(self, context, snapshot, share_server=None, diff --git a/manila/share/manager.py b/manila/share/manager.py index 706caa1855..8c8e89d316 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -3463,11 +3463,12 @@ class ShareManager(manager.SchedulerDependentManager): context, snapshot_instance, snapshot=snapshot) try: - self.driver.revert_to_snapshot(context, - snapshot_instance_dict, - share_access_rules, - snapshot_access_rules, - share_server=share_server) + updated_share_size = self.driver.revert_to_snapshot( + context, + snapshot_instance_dict, + share_access_rules, + snapshot_access_rules, + share_server=share_server) except Exception as excep: with excutils.save_and_reraise_exception(): @@ -3496,15 +3497,37 @@ class ShareManager(manager.SchedulerDependentManager): resource_id=share_id, exception=excep) + # fail-safe in case driver returned size is None or invalid + if not updated_share_size: + updated_share_size = snapshot['size'] + else: + try: + int(updated_share_size) + except ValueError: + updated_share_size = snapshot['size'] + if reservations: - QUOTAS.commit( - context, reservations, project_id=project_id, user_id=user_id, - share_type_id=share_type_id, - ) + if updated_share_size == snapshot['size']: + QUOTAS.commit( + context, reservations, project_id=project_id, + user_id=user_id, share_type_id=share_type_id, + ) + else: + # NOTE(kpdev): The driver tells us that the share size wasn't + # modified to the snapshot's size; so no need to commit quota + # changes + QUOTAS.rollback( + context, reservations, project_id=project_id, + user_id=user_id, share_type_id=share_type_id, + ) + if updated_share_size != share['size']: + LOG.error("Driver returned an unexpected size %d on " + "revert to snapshot operation. You need to " + "adjust the quota", updated_share_size) self.db.share_update( context, share_id, - {'status': constants.STATUS_AVAILABLE, 'size': snapshot['size']}) + {'status': constants.STATUS_AVAILABLE, 'size': updated_share_size}) self.db.share_snapshot_update( context, snapshot_id, {'status': constants.STATUS_AVAILABLE}) diff --git a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py index 2f4630c5d4..20816c38fd 100644 --- a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py +++ b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py @@ -2449,6 +2449,7 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): '_get_vserver', mock.Mock(return_value=(fake.VSERVER1, vserver_client))) + vserver_client.get_volume.return_value = fake.FLEXVOL_TO_MANAGE fake_snapshot = copy.deepcopy(fake.SNAPSHOT) if use_snap_provider_location: fake_snapshot['provider_location'] = 'fake-provider-location' @@ -2458,7 +2459,7 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): result = self.library.revert_to_snapshot( self.context, fake_snapshot, share_server=fake.SHARE_SERVER) - self.assertIsNone(result) + self.assertIsNotNone(result) share_name = self.library._get_backend_share_name( fake_snapshot['share_id']) snapshot_name = (self.library._get_backend_snapshot_name( diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index cb0a033a45..229c95f99b 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -7930,9 +7930,12 @@ class ShareManagerTestCase(test.TestCase): mock.ANY, share, snapshot, reservations, share_access_rules, snapshot_access_rules, share_id=share_id) - @ddt.data(None, 'fake_reservations') - def test__revert_to_snapshot(self, reservations): - + @ddt.data((None, False), + (None, True), + ('fake_reservations', False), + ('fake_reservations', True)) + @ddt.unpack + def test__revert_to_snapshot(self, reservations, revert_return_share_size): mock_quotas_rollback = self.mock_object(quota.QUOTAS, 'rollback') mock_quotas_commit = self.mock_object(quota.QUOTAS, 'commit') self.mock_object( @@ -7944,14 +7947,15 @@ class ShareManagerTestCase(test.TestCase): share = fakes.fake_share( id=share_id, instance={'id': 'fake_instance_id', 'share_type_id': 'fake_share_type_id'}, - project_id='fake_project', user_id='fake_user', size=2) + project_id='fake_project', user_id='fake_user', + size=5 if revert_return_share_size else 3) snapshot_instance = fakes.fake_snapshot_instance( share_id=share_id, share=share, name='fake_snapshot', share_instance=share['instance']) snapshot = fakes.fake_snapshot( id='fake_snapshot_id', share_id=share_id, share=share, instance=snapshot_instance, project_id='fake_project', - user_id='fake_user', size=1) + user_id='fake_user', size=4) share_access_rules = [] snapshot_access_rules = [] @@ -7965,6 +7969,8 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.db, 'share_update') mock_share_snapshot_update = self.mock_object( self.share_manager.db, 'share_snapshot_update') + mock_driver.revert_to_snapshot.return_value = ( + 5 if revert_return_share_size else None) self.share_manager._revert_to_snapshot(self.context, share, snapshot, reservations, @@ -7978,19 +7984,27 @@ class ShareManagerTestCase(test.TestCase): share_access_rules, snapshot_access_rules, share_server=None) - self.assertFalse(mock_quotas_rollback.called) if reservations: - mock_quotas_commit.assert_called_once_with( - mock.ANY, reservations, project_id='fake_project', - user_id='fake_user', - share_type_id=( - snapshot_instance['share_instance']['share_type_id'])) + if revert_return_share_size: + mock_quotas_rollback.assert_called_once_with( + mock.ANY, reservations, project_id='fake_project', + user_id='fake_user', + share_type_id=( + snapshot_instance['share_instance']['share_type_id'])) + else: + self.assertFalse(mock_quotas_rollback.called) + mock_quotas_commit.assert_called_once_with( + mock.ANY, reservations, project_id='fake_project', + user_id='fake_user', + share_type_id=( + snapshot_instance['share_instance']['share_type_id'])) else: self.assertFalse(mock_quotas_commit.called) mock_share_update.assert_called_once_with( mock.ANY, share_id, - {'status': constants.STATUS_AVAILABLE, 'size': snapshot['size']}) + {'status': constants.STATUS_AVAILABLE, + 'size': 5 if revert_return_share_size else 4}) mock_share_snapshot_update.assert_called_once_with( mock.ANY, 'fake_snapshot_id', {'status': constants.STATUS_AVAILABLE}) diff --git a/releasenotes/notes/bug-2064502-netapp-fix-revert-to-snapshot-f23ab4dc325b2c42.yaml b/releasenotes/notes/bug-2064502-netapp-fix-revert-to-snapshot-f23ab4dc325b2c42.yaml new file mode 100644 index 0000000000..ce5fa25e59 --- /dev/null +++ b/releasenotes/notes/bug-2064502-netapp-fix-revert-to-snapshot-f23ab4dc325b2c42.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + When reverting shares to snapshots that are larger or smaller than + the share, some storage systems such as NetApp ONTAP always defer + to the larger of the two sizes. Manila's share manager interface now + accounts for this behavior, and adjusts project quotas appropriately. + For more details, please check + Launchpad `bug 2064502 `_