Merge "Check backup service before backup delete"

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
+            if (srv.availability_zone == availability_zone and
-                    srv.host == volume_host 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

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

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(
+        self.assertEqual(False,
-            volume, test_host))
+                         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(
+        self.assertEqual(False,
-            volume, test_host))
+                         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(
+        self.assertEqual(False,
-            volume, test_host))
+                         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(
+        self.assertEqual(False,
-            volume, test_host))
+                         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(
+        self.assertEqual(False,
-            volume, test_host))
+                         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,
+        self.assertTrue(self.backup_api._is_backup_service_enabled(
-                                                                   test_host))
+                        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