From fe7f6bcd6a72caaff1f80f8b6c9a80fd09253c69 Mon Sep 17 00:00:00 2001 From: lisali Date: Wed, 9 Mar 2016 02:57:48 +0000 Subject: [PATCH] Host selection in backup service With scaling backup service, it introduces a config called backup_use_same_backend which indicates whether customers use same backup driver in their environments. If the value is set to True, Cinder selects any backup host to do backup task. If the value is False, cinder can only select backup.host for a backup task. Currently the default value is set to False. This patch is to change above config to backup_use_same_host which means whether selecting same host for tasks of a backup. The default value is set to False. It indicates that we don't care about backup.host when running backup task, and choose any available host in same az. As currently Cinder doesn't support multiple backup drivers, the change is to make the config more sensible. Change-Id: I1a43df251a18006363162ee8e7c5aa891f44fa3a Closes-bug: #1554845 --- cinder/backup/api.py | 8 +- .../unit/api/contrib/test_admin_actions.py | 6 +- cinder/tests/unit/api/contrib/test_backups.py | 120 ++++++++++-------- cinder/tests/unit/test_backup.py | 6 +- 4 files changed, 78 insertions(+), 62 deletions(-) diff --git a/cinder/backup/api.py b/cinder/backup/api.py index 9ac21ebab..c447859ef 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -43,7 +43,7 @@ import cinder.volume from cinder.volume import utils as volume_utils backup_api_opts = [ - cfg.BoolOpt('backup_use_same_backend', + cfg.BoolOpt('backup_use_same_host', default=False, help='Backup services use same backend.') ] @@ -209,10 +209,10 @@ class API(base.Base): raise exception.ServiceNotFound(service_id='cinder-backup') backup_host = None - if host and self._is_backup_service_enabled(az, host): - backup_host = host - if not backup_host and (not host or CONF.backup_use_same_backend): + if (not host or not CONF.backup_use_same_host): backup_host = self._get_any_available_backup_service(az) + elif self._is_backup_service_enabled(az, host): + backup_host = host if not backup_host: raise exception.ServiceNotFound(service_id='cinder-backup') return backup_host diff --git a/cinder/tests/unit/api/contrib/test_admin_actions.py b/cinder/tests/unit/api/contrib/test_admin_actions.py index beb6cb1f4..6860a133b 100644 --- a/cinder/tests/unit/api/contrib/test_admin_actions.py +++ b/cinder/tests/unit/api/contrib/test_admin_actions.py @@ -106,9 +106,9 @@ class AdminActionsTest(BaseAdminTest): req.headers['content-type'] = 'application/json' req.body = jsonutils.dump_as_bytes({'os-reset_status': updated_status}) req.environ['cinder.context'] = ctx - with mock.patch('cinder.backup.api.API._is_backup_service_enabled') \ - as mock_is_service_available: - mock_is_service_available.return_value = True + with mock.patch('cinder.backup.api.API._get_available_backup_service_host') \ + as mock_get_backup_host: + mock_get_backup_host.return_value = 'testhost' resp = req.get_response(app()) return resp diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index 031ffb4cd..33b6c4ff7 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -1064,30 +1064,44 @@ class BackupsAPITestCase(test.TestCase): def test_get_available_backup_service(self, _mock_service_get_all_by_topic): _mock_service_get_all_by_topic.return_value = [ - {'availability_zone': 'az1', 'host': 'testhost', + {'availability_zone': 'az1', 'host': 'testhost1', 'disabled': 0, 'updated_at': timeutils.utcnow()}, - {'availability_zone': 'az2', 'host': 'fakehost', - 'disabled': 0, 'updated_at': timeutils.utcnow()}] + {'availability_zone': 'az2', 'host': 'testhost2', + 'disabled': 0, 'updated_at': timeutils.utcnow()}, + {'availability_zone': 'az2', 'host': 'testhost3', + 'disabled': 0, 'updated_at': timeutils.utcnow()}, ] actual_host = self.backup_api._get_available_backup_service_host( - 'testhost', 'az1') - self.assertEqual('testhost', actual_host) - self.assertRaises(exception.ServiceNotFound, - self.backup_api._get_available_backup_service_host, - 'testhost', 'az2') - self.assertRaises(exception.ServiceNotFound, - self.backup_api._get_available_backup_service_host, - 'testhost2', 'az1') - self.override_config('backup_use_same_backend', True) + None, 'az1') + self.assertEqual('testhost1', actual_host) + actual_host = self.backup_api._get_available_backup_service_host( + 'testhost2', 'az2') + self.assertIn(actual_host, ['testhost2', 'testhost3']) + actual_host = self.backup_api._get_available_backup_service_host( + 'testhost4', 'az1') + self.assertEqual('testhost1', actual_host) + + @mock.patch('cinder.db.service_get_all_by_topic') + def test_get_available_backup_service_with_same_host( + self, _mock_service_get_all_by_topic): + _mock_service_get_all_by_topic.return_value = [ + {'availability_zone': 'az1', 'host': 'testhost1', + 'disabled': 0, 'updated_at': timeutils.utcnow()}, + {'availability_zone': 'az2', 'host': 'testhost2', + 'disabled': 0, 'updated_at': timeutils.utcnow()}, ] + self.override_config('backup_use_same_host', True) actual_host = self.backup_api._get_available_backup_service_host( None, 'az1') - self.assertEqual('testhost', actual_host) + self.assertEqual('testhost1', actual_host) actual_host = self.backup_api._get_available_backup_service_host( - 'testhost2', 'az1') - self.assertEqual('testhost', actual_host) + 'testhost2', 'az2') + self.assertEqual('testhost2', actual_host) + self.assertRaises(exception.ServiceNotFound, + self.backup_api._get_available_backup_service_host, + 'testhost4', 'az1') @mock.patch('cinder.db.service_get_all_by_topic') - def test_delete_backup_available(self, - _mock_service_get_all_by_topic): + def test_delete_backup_available( + self, _mock_service_get_all_by_topic): _mock_service_get_all_by_topic.return_value = [ {'availability_zone': 'az1', 'host': 'testhost', 'disabled': 0, 'updated_at': timeutils.utcnow()}] @@ -1219,10 +1233,10 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') def test_restore_backup_volume_id_specified_json( - self, _mock_is_service_enabled): - _mock_is_service_enabled.return_value = True + self, _mock_get_backup_host): + _mock_get_backup_host.return_value = 'testhost' backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE) # need to create the volume referenced below first volume_name = 'test1' @@ -1244,10 +1258,10 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(volume_id, res_dict['restore']['volume_id']) self.assertEqual(volume_name, res_dict['restore']['volume_name']) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') def test_restore_backup_volume_id_specified_xml( - self, _mock_is_service_enabled): - _mock_is_service_enabled.return_value = True + self, _mock_get_backup_host): + _mock_get_backup_host.return_value = 'testhost' volume_name = 'test1' backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE) volume_id = utils.create_volume(self.context, @@ -1314,19 +1328,20 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual("Missing required element 'restore' in request body.", res_dict['badRequest']['message']) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.db.service_get_all_by_topic') @mock.patch('cinder.volume.api.API.create') - def test_restore_backup_volume_id_unspecified(self, - _mock_volume_api_create, - _mock_is_service_enabled): - + def test_restore_backup_volume_id_unspecified( + self, _mock_volume_api_create, + _mock_service_get_all_by_topic): # intercept volume creation to ensure created volume # has status of available def fake_volume_api_create(context, size, name, description): volume_id = utils.create_volume(self.context, size=size)['id'] return db.volume_get(context, volume_id) - _mock_is_service_enabled.return_value = True + _mock_service_get_all_by_topic.return_value = [ + {'availability_zone': 'az1', 'host': 'testhost', + 'disabled': 0, 'updated_at': timeutils.utcnow()}] _mock_volume_api_create.side_effect = fake_volume_api_create backup_id = self._create_backup(size=5, @@ -1344,11 +1359,11 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(202, res.status_int) self.assertEqual(backup_id, res_dict['restore']['backup_id']) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.db.service_get_all_by_topic') @mock.patch('cinder.volume.api.API.create') def test_restore_backup_name_specified(self, _mock_volume_api_create, - _mock_is_service_enabled): + _mock_service_get_all_by_topic): # Intercept volume creation to ensure created volume # has status of available def fake_volume_api_create(context, size, name, description): @@ -1357,7 +1372,9 @@ class BackupsAPITestCase(test.TestCase): return db.volume_get(context, volume_id) _mock_volume_api_create.side_effect = fake_volume_api_create - _mock_is_service_enabled.return_value = True + _mock_service_get_all_by_topic.return_value = [ + {'availability_zone': 'az1', 'host': 'testhost', + 'disabled': 0, 'updated_at': timeutils.utcnow()}] backup_id = self._create_backup(size=5, status=fields.BackupStatus.AVAILABLE) @@ -1382,10 +1399,10 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(202, res.status_int) self.assertEqual(backup_id, res_dict['restore']['backup_id']) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') def test_restore_backup_name_volume_id_specified( - self, _mock_is_service_enabled): - _mock_is_service_enabled.return_value = True + self, _mock_get_backup_host): + _mock_get_backup_host.return_value = 'testhost' backup_id = self._create_backup(size=5, status=fields.BackupStatus.AVAILABLE) orig_vol_name = "vol-00" @@ -1602,12 +1619,11 @@ class BackupsAPITestCase(test.TestCase): db.volume_destroy(context.get_admin_context(), volume_id) db.backup_destroy(context.get_admin_context(), backup_id) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') - def test_restore_backup_to_oversized_volume( - self, _mock_is_service_enabled): - _mock_is_service_enabled.return_value = True + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') + def test_restore_backup_to_oversized_volume(self, _mock_get_backup_host): backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE, size=10) + _mock_get_backup_host.return_value = 'testhost' # need to create the volume referenced below first volume_name = 'test1' volume_id = utils.create_volume(self.context, @@ -1632,8 +1648,8 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id) @mock.patch('cinder.backup.rpcapi.BackupAPI.restore_backup') - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') - def test_restore_backup_with_different_host(self, mock_is_backup_available, + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') + def test_restore_backup_with_different_host(self, _mock_get_backup_host, mock_restore_backup): volume_name = 'test1' backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE, @@ -1642,7 +1658,7 @@ class BackupsAPITestCase(test.TestCase): host='HostB@BackendB#PoolB', display_name=volume_name)['id'] - mock_is_backup_available.return_value = True + _mock_get_backup_host.return_value = 'testhost' body = {"restore": {"volume_id": volume_id, }} req = webob.Request.blank('/v2/fake/backups/%s/restore' % backup_id) @@ -1656,7 +1672,7 @@ class BackupsAPITestCase(test.TestCase): self.assertEqual(backup_id, res_dict['restore']['backup_id']) self.assertEqual(volume_id, res_dict['restore']['volume_id']) self.assertEqual(volume_name, res_dict['restore']['volume_name']) - mock_restore_backup.assert_called_once_with(mock.ANY, u'HostA', + mock_restore_backup.assert_called_once_with(mock.ANY, u'testhost', mock.ANY, volume_id) # Manually check if restore_backup was called with appropriate backup. self.assertEqual(backup_id, mock_restore_backup.call_args[0][2].id) @@ -1676,11 +1692,11 @@ class BackupsAPITestCase(test.TestCase): # request is not authorized self.assertEqual(403, res.status_int) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') @mock.patch('cinder.backup.rpcapi.BackupAPI.export_record') def test_export_backup_record_id_specified_json(self, _mock_export_record_rpc, - _mock_service_enabled): + _mock_get_backup_host): backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE, size=10) ctx = context.RequestContext('admin', 'fake', is_admin=True) @@ -1689,7 +1705,7 @@ class BackupsAPITestCase(test.TestCase): _mock_export_record_rpc.return_value = \ {'backup_service': backup_service, 'backup_url': backup_url} - _mock_service_enabled.return_value = True + _mock_get_backup_host.return_value = 'testhost' req = webob.Request.blank('/v2/fake/backups/%s/export_record' % backup_id) req.method = 'GET' @@ -1705,11 +1721,11 @@ class BackupsAPITestCase(test.TestCase): res_dict['backup-record']['backup_url']) db.backup_destroy(context.get_admin_context(), backup_id) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') @mock.patch('cinder.backup.rpcapi.BackupAPI.export_record') def test_export_record_backup_id_specified_xml(self, _mock_export_record_rpc, - _mock_service_enabled): + _mock_get_backup_host): backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE, size=10) ctx = context.RequestContext('admin', 'fake', is_admin=True) @@ -1718,7 +1734,7 @@ class BackupsAPITestCase(test.TestCase): _mock_export_record_rpc.return_value = \ {'backup_service': backup_service, 'backup_url': backup_url} - _mock_service_enabled.return_value = True + _mock_get_backup_host.return_value = 'testhost' req = webob.Request.blank('/v2/fake/backups/%s/export_record' % backup_id) req.method = 'GET' @@ -1769,15 +1785,15 @@ class BackupsAPITestCase(test.TestCase): res_dict['badRequest']['message']) db.backup_destroy(context.get_admin_context(), backup_id) - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') @mock.patch('cinder.backup.rpcapi.BackupAPI.export_record') def test_export_record_with_unavailable_service(self, _mock_export_record_rpc, - _mock_service_enabled): + _mock_get_backup_host): msg = 'fake unavailable service' _mock_export_record_rpc.side_effect = \ exception.InvalidBackup(reason=msg) - _mock_service_enabled.return_value = True + _mock_get_backup_host.return_value = 'testhost' backup_id = self._create_backup(status=fields.BackupStatus.AVAILABLE) ctx = context.RequestContext('admin', 'fake', is_admin=True) req = webob.Request.blank('/v2/fake/backups/%s/export_record' % diff --git a/cinder/tests/unit/test_backup.py b/cinder/tests/unit/test_backup.py index 7e7152ed7..8c33e13fd 100644 --- a/cinder/tests/unit/test_backup.py +++ b/cinder/tests/unit/test_backup.py @@ -1361,16 +1361,16 @@ class BackupAPITestCase(BaseBackupTest): volume_id=volume_id, container='volumebackups') - @mock.patch('cinder.backup.api.API._is_backup_service_enabled') + @mock.patch('cinder.backup.api.API._get_available_backup_service_host') @mock.patch('cinder.backup.rpcapi.BackupAPI.restore_backup') def test_restore_volume(self, mock_rpcapi_restore, - mock_is_service_enabled): + mock_get_backup_host): volume_id = self._create_volume_db_entry(status='available', size=1) backup = self._create_backup_db_entry(size=1, status='available') - mock_is_service_enabled.return_value = True + mock_get_backup_host.return_value = 'testhost' self.api.restore(self.ctxt, backup.id, volume_id) backup = objects.Backup.get_by_id(self.ctxt, backup.id) self.assertEqual(volume_id, backup.restore_volume_id) -- 2.45.2