From 860a40f299134fac895a5a68bd171a401c540acb Mon Sep 17 00:00:00 2001 From: Nate Potter Date: Tue, 17 Nov 2015 16:50:24 +0000 Subject: [PATCH] Check backup service before backup delete Right now, if a user tries to delete a backup and the cinder-backup service is down, the backup status will be error and no message will be provided to the user. This patch will add a check to see that the service is running before issuing the rpcapi cast to delete the backup, and inform the user that the service is down if the check fails. However, it is still possible with this fix for the service to go down during the time between the check and when the backup is actually deleted. Change-Id: Ia9914bfaae8ed9b6972953fc1e78c4471fa1661a Closes-bug: #1321519 --- cinder/backup/api.py | 13 ++- .../unit/api/contrib/test_admin_actions.py | 7 +- cinder/tests/unit/api/contrib/test_backups.py | 80 +++++++++++++++---- 3 files changed, 79 insertions(+), 21 deletions(-) diff --git a/cinder/backup/api.py b/cinder/backup/api.py index aa1a722c9..56d7e0b7c 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -77,6 +77,7 @@ class API(base.Base): :param force: indicate force delete or not :raises: InvalidBackup :raises: BackupDriverException + :raises: ServiceNotFound """ check_policy(context, 'delete') if not force and backup.status not in ['available', 'error']: @@ -86,6 +87,9 @@ class API(base.Base): backup.host): msg = _('force delete') raise exception.NotSupportedOperation(operation=msg) + if not self._is_backup_service_enabled(backup['availability_zone'], + backup.host): + raise exception.ServiceNotFound(service_id='cinder-backup') # Don't allow backup to be deleted if there are incremental # backups dependent on it. @@ -121,15 +125,15 @@ class API(base.Base): return backups - def _is_backup_service_enabled(self, volume, volume_host): + def _is_backup_service_enabled(self, availability_zone, host): """Check if there is a backup service available.""" topic = CONF.backup_topic ctxt = context.get_admin_context() services = objects.ServiceList.get_all_by_topic( ctxt, topic, disabled=False) for srv in services: - if (srv.availability_zone == volume['availability_zone'] and - srv.host == volume_host and + if (srv.availability_zone == availability_zone and + srv.host == host and utils.service_is_up(srv)): return True return False @@ -163,7 +167,8 @@ class API(base.Base): previous_status = volume['status'] volume_host = volume_utils.extract_host(volume['host'], 'host') - if not self._is_backup_service_enabled(volume, volume_host): + if not self._is_backup_service_enabled(volume['availability_zone'], + volume_host): raise exception.ServiceNotFound(service_id='cinder-backup') # Reserve a quota before setting volume status and backup status diff --git a/cinder/tests/unit/api/contrib/test_admin_actions.py b/cinder/tests/unit/api/contrib/test_admin_actions.py index a6da59f1a..af4bbb35c 100644 --- a/cinder/tests/unit/api/contrib/test_admin_actions.py +++ b/cinder/tests/unit/api/contrib/test_admin_actions.py @@ -989,8 +989,13 @@ class AdminActionsTest(test.TestCase): vac.validate_update, {'status': 'creating'}) + @mock.patch('cinder.db.service_get_all_by_topic') @mock.patch('cinder.backup.api.API._check_support_to_force_delete') - def _force_delete_backup_util(self, test_status, mock_check_support): + def _force_delete_backup_util(self, test_status, mock_check_support, + _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()}] # admin context ctx = context.RequestContext('admin', 'fake', True) mock_check_support.return_value = True diff --git a/cinder/tests/unit/api/contrib/test_backups.py b/cinder/tests/unit/api/contrib/test_backups.py index 27273b218..7832036da 100644 --- a/cinder/tests/unit/api/contrib/test_backups.py +++ b/cinder/tests/unit/api/contrib/test_backups.py @@ -925,30 +925,46 @@ class BackupsAPITestCase(test.TestCase): volume = self.volume_api.get(context.get_admin_context(), volume_id) # test empty service - self.assertEqual(False, self.backup_api._is_backup_service_enabled( - volume, test_host)) + self.assertEqual(False, + self.backup_api._is_backup_service_enabled( + volume['availability_zone'], + test_host)) # test host not match service - self.assertEqual(False, self.backup_api._is_backup_service_enabled( - volume, test_host)) + self.assertEqual(False, + self.backup_api._is_backup_service_enabled( + volume['availability_zone'], + test_host)) # test az not match service - self.assertEqual(False, self.backup_api._is_backup_service_enabled( - volume, test_host)) + self.assertEqual(False, + self.backup_api._is_backup_service_enabled( + volume['availability_zone'], + test_host)) # test disabled service - self.assertEqual(False, self.backup_api._is_backup_service_enabled( - volume, test_host)) + self.assertEqual(False, + self.backup_api._is_backup_service_enabled( + volume['availability_zone'], + test_host)) # test dead service - self.assertEqual(False, self.backup_api._is_backup_service_enabled( - volume, test_host)) + self.assertEqual(False, + self.backup_api._is_backup_service_enabled( + volume['availability_zone'], + test_host)) # test multi services and the last service matches - self.assertTrue(self.backup_api._is_backup_service_enabled(volume, - test_host)) + self.assertTrue(self.backup_api._is_backup_service_enabled( + volume['availability_zone'], + test_host)) - def test_delete_backup_available(self): + @mock.patch('cinder.db.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()}] backup_id = self._create_backup(status='available') req = webob.Request.blank('/v2/fake/backups/%s' % backup_id) @@ -962,7 +978,12 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id) - def test_delete_delta_backup(self): + @mock.patch('cinder.db.service_get_all_by_topic') + def test_delete_delta_backup(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()}] backup_id = self._create_backup(status='available') delta_id = self._create_backup(status='available', incremental=True) @@ -979,7 +1000,12 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), delta_id) db.backup_destroy(context.get_admin_context(), backup_id) - def test_delete_backup_error(self): + @mock.patch('cinder.db.service_get_all_by_topic') + def test_delete_backup_error(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()}] backup_id = self._create_backup(status='error') req = webob.Request.blank('/v2/fake/backups/%s' % backup_id) @@ -1022,7 +1048,12 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), backup_id) - def test_delete_backup_with_InvalidBackup2(self): + @mock.patch('cinder.db.service_get_all_by_topic') + def test_delete_backup_with_InvalidBackup2(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()}] volume_id = utils.create_volume(self.context, size=5)['id'] backup_id = self._create_backup(volume_id, status="available") delta_backup_id = self._create_backup(status='available', @@ -1044,6 +1075,23 @@ class BackupsAPITestCase(test.TestCase): db.backup_destroy(context.get_admin_context(), delta_backup_id) db.backup_destroy(context.get_admin_context(), backup_id) + @mock.patch('cinder.db.service_get_all_by_topic') + def test_delete_backup_service_down(self, + _mock_service_get_all_by_topic): + _mock_service_get_all_by_topic.return_value = [ + {'availability_zone': "az1", 'host': 'testhost', + 'disabled': 0, 'updated_at': '1775-04-19 05:00:00'}] + backup_id = self._create_backup(status='available') + req = webob.Request.blank('/v2/fake/backups/%s' % + backup_id) + req.method = 'DELETE' + req.headers['Content-Type'] = 'application/json' + res = req.get_response(fakes.wsgi_app()) + + self.assertEqual(404, res.status_int) + + db.backup_destroy(context.get_admin_context(), backup_id) + def test_restore_backup_volume_id_specified_json(self): backup_id = self._create_backup(status='available') # need to create the volume referenced below first -- 2.45.2