From 53b91e513edfd57dc8d712f43b19378f4675d82e Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Tue, 31 Jan 2017 17:07:47 -0200 Subject: [PATCH] Fix multiple export locations during migration During a migration, the destination share instance export location was being shown, thus the user could get confused as to which export location to choose when mounting his share, since only the source could be mounted. This patch addresses this by filtering the destination export location out. APIImpact Change-Id: I20cf1a76399693d751fa87fc4df375162e7c5f1d Closes-bug: #1660726 --- manila/api/v2/share_export_locations.py | 3 +- manila/db/api.py | 6 ++- manila/db/sqlalchemy/api.py | 9 ++++- manila/tests/db/sqlalchemy/test_api.py | 37 +++++++++++++++---- ...ion-export-locations-5670734670435015.yaml | 4 ++ 5 files changed, 47 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/bug-1660726-migration-export-locations-5670734670435015.yaml diff --git a/manila/api/v2/share_export_locations.py b/manila/api/v2/share_export_locations.py index 17fcc25760..5ac88bac79 100644 --- a/manila/api/v2/share_export_locations.py +++ b/manila/api/v2/share_export_locations.py @@ -45,7 +45,8 @@ class ShareExportLocationController(wsgi.Controller): context = req.environ['manila.context'] self._verify_share(context, share_id) export_locations = db_api.share_export_locations_get_by_share_id( - context, share_id, include_admin_only=context.is_admin) + context, share_id, include_admin_only=context.is_admin, + ignore_migration_destination=True) return self._view_builder.summary_list(req, export_locations) @wsgi.Controller.api_version('2.9') diff --git a/manila/db/api.py b/manila/db/api.py index 570a02997e..1f58099aa1 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -676,10 +676,12 @@ def share_export_locations_get(context, share_id): def share_export_locations_get_by_share_id(context, share_id, - include_admin_only=True): + include_admin_only=True, + ignore_migration_destination=False): """Get all export locations of a share by its ID.""" return IMPL.share_export_locations_get_by_share_id( - context, share_id, include_admin_only=include_admin_only) + context, share_id, include_admin_only=include_admin_only, + ignore_migration_destination=ignore_migration_destination) def share_export_locations_get_by_share_instance_id(context, diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index bb942bfd95..34f8b7394f 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -2643,9 +2643,14 @@ def _share_export_locations_get(context, share_instance_ids, @require_context @require_share_exists def share_export_locations_get_by_share_id(context, share_id, - include_admin_only=True): + include_admin_only=True, + ignore_migration_destination=False): share = share_get(context, share_id) - ids = [instance.id for instance in share.instances] + if ignore_migration_destination: + ids = [instance.id for instance in share.instances + if instance['status'] != constants.STATUS_MIGRATING_TO] + else: + ids = [instance.id for instance in share.instances] rows = _share_export_locations_get( context, ids, include_admin_only=include_admin_only) return rows diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index e783793138..32271dcbcb 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -1396,11 +1396,27 @@ class ShareInstanceExportLocationsMetadataDatabaseAPITestCase(test.TestCase): def setUp(self): super(self.__class__, self).setUp() self.ctxt = context.get_admin_context() - self.share = db_utils.create_share() + share_id = 'fake_share_id' + instances = [ + db_utils.create_share_instance( + share_id=share_id, + status=constants.STATUS_AVAILABLE), + db_utils.create_share_instance( + share_id=share_id, + status=constants.STATUS_MIGRATING), + db_utils.create_share_instance( + share_id=share_id, + status=constants.STATUS_MIGRATING_TO), + ] + self.share = db_utils.create_share( + id=share_id, + instances=instances) self.initial_locations = ['/fake/foo/', '/fake/bar', '/fake/quuz'] - db_api.share_export_locations_update( - self.ctxt, self.share.instance['id'], self.initial_locations, - delete=False) + self.shown_locations = ['/fake/foo/', '/fake/bar'] + for i in range(0, 3): + db_api.share_export_locations_update( + self.ctxt, instances[i]['id'], self.initial_locations[i], + delete=False) def _get_export_location_uuid_by_path(self, path): els = db_api.share_export_locations_get_by_share_id( @@ -1416,14 +1432,21 @@ class ShareInstanceExportLocationsMetadataDatabaseAPITestCase(test.TestCase): els = db_api.share_export_locations_get_by_share_id( self.ctxt, self.share.id) self.assertEqual(3, len(els)) - for path in self.initial_locations: + for path in self.shown_locations: + self.assertTrue(any([path in el.path for el in els])) + + def test_get_export_locations_by_share_id_ignore_migration_dest(self): + els = db_api.share_export_locations_get_by_share_id( + self.ctxt, self.share.id, ignore_migration_destination=True) + self.assertEqual(2, len(els)) + for path in self.shown_locations: self.assertTrue(any([path in el.path for el in els])) def test_get_export_locations_by_share_instance_id(self): els = db_api.share_export_locations_get_by_share_instance_id( self.ctxt, self.share.instance.id) - self.assertEqual(3, len(els)) - for path in self.initial_locations: + self.assertEqual(1, len(els)) + for path in [self.shown_locations[1]]: self.assertTrue(any([path in el.path for el in els])) def test_export_location_metadata_update_delete(self): diff --git a/releasenotes/notes/bug-1660726-migration-export-locations-5670734670435015.yaml b/releasenotes/notes/bug-1660726-migration-export-locations-5670734670435015.yaml new file mode 100644 index 0000000000..78b9180f96 --- /dev/null +++ b/releasenotes/notes/bug-1660726-migration-export-locations-5670734670435015.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Export locations pertaining to migration destinations + are no longer shown until migration is complete.