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

diff --git a/cinder/backup/api.py b/cinder/backup/api.py
index aa1a722c9a14ee679d18155a36e4f0ccd2b3f8c9..56d7e0b7c9b9a9a78e9a6869d030b25a3c64f8aa 100644 (file)
--- 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 a6da59f1a60e00068523b0e7d8bcc25ca057012c..af4bbb35cf5c061ccc7458f5ef438eec9feb998d 100644 (file)
--- 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 27273b218e3e31fa417a7a4bb8f325bca126bd2b..7832036dad042570d7b4b8321af15b30e45a08c8 100644 (file)
--- 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