From d6c527bb6b0ee63fc07b91855fcf4e15f7a03821 Mon Sep 17 00:00:00 2001
From: Eoghan Glynn <eglynn@redhat.com>
Date: Tue, 22 Jan 2013 15:44:21 +0000
Subject: Avoid stuck task_state on snapshot image failure

Fixes bug LP 1101136

Previously if the glance interaction failed prior to an
instance being snapshot'd or backed up, the task state
remained stuck at image_snapshot/backup.

The normal task state reversion logic did not kick in,
as this is limited to the compute layer, whereas the
intial glance interaction occurs within the API layer.

Now, we avoid this problem by delaying setting the task
state until the initial initial image creation/retrieval
has completed.

Change-Id: Id498ae6b3674306743013e4fe99837da8e2031b5
---
 nova/compute/api.py                | 16 ++++++-----
 nova/tests/compute/test_compute.py | 54 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 6 deletions(-)

(limited to 'nova')

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 4b15a3e27..9092a001a 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1353,9 +1353,6 @@ class API(base.Base):
             None if rotation shouldn't be used (as in the case of snapshots)
         :param extra_properties: dict of extra image properties to include
         """
-        instance = self.update(context, instance,
-                               task_state=task_states.IMAGE_BACKUP,
-                               expected_task_state=None)
         if image_id:
             # The image entry has already been created, so just pull the
             # metadata.
@@ -1364,6 +1361,11 @@ class API(base.Base):
             image_meta = self._create_image(context, instance, name,
                     'backup', backup_type=backup_type,
                     rotation=rotation, extra_properties=extra_properties)
+
+        instance = self.update(context, instance,
+                               task_state=task_states.IMAGE_BACKUP,
+                               expected_task_state=None)
+
         self.compute_rpcapi.snapshot_instance(context, instance=instance,
                 image_id=image_meta['id'], image_type='backup',
                 backup_type=backup_type, rotation=rotation)
@@ -1381,9 +1383,6 @@ class API(base.Base):
 
         :returns: A dict containing image metadata
         """
-        instance = self.update(context, instance,
-                               task_state=task_states.IMAGE_SNAPSHOT,
-                               expected_task_state=None)
         if image_id:
             # The image entry has already been created, so just pull the
             # metadata.
@@ -1391,6 +1390,11 @@ class API(base.Base):
         else:
             image_meta = self._create_image(context, instance, name,
                     'snapshot', extra_properties=extra_properties)
+
+        instance = self.update(context, instance,
+                               task_state=task_states.IMAGE_SNAPSHOT,
+                               expected_task_state=None)
+
         self.compute_rpcapi.snapshot_instance(context, instance=instance,
                 image_id=image_meta['id'], image_type='snapshot')
         return image_meta
diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py
index 092fd940a..865288f95 100644
--- a/nova/tests/compute/test_compute.py
+++ b/nova/tests/compute/test_compute.py
@@ -4688,6 +4688,60 @@ class ComputeAPITestCase(BaseTestCase):
         self.assertEqual(properties['d'], 'd')
         self.assertFalse('spam' in properties)
 
+    def _do_test_snapshot_image_service_fails(self, method, image_id):
+        # Ensure task_state remains at None if image service fails.
+        def fake_fails(*args, **kwargs):
+            raise test.TestingException()
+
+        restore = getattr(fake_image._FakeImageService, method)
+        self.stubs.Set(fake_image._FakeImageService, method, fake_fails)
+
+        instance = self._create_fake_instance()
+        self.assertRaises(test.TestingException,
+                          self.compute_api.snapshot,
+                          self.context,
+                          instance,
+                          'no_image_snapshot',
+                          image_id=image_id)
+
+        self.stubs.Set(fake_image._FakeImageService, method, restore)
+        db_instance = db.instance_get_all(self.context)[0]
+        self.assertIsNone(db_instance['task_state'])
+
+    def test_snapshot_image_creation_fails(self):
+        self._do_test_snapshot_image_service_fails('create', None)
+
+    def test_snapshot_image_show_fails(self):
+        self._do_test_snapshot_image_service_fails('show', 'image')
+
+    def _do_test_backup_image_service_fails(self, method, image_id):
+        # Ensure task_state remains at None if image service fails.
+        def fake_fails(*args, **kwargs):
+            raise test.TestingException()
+
+        restore = getattr(fake_image._FakeImageService, method)
+        self.stubs.Set(fake_image._FakeImageService, method, fake_fails)
+
+        instance = self._create_fake_instance()
+        self.assertRaises(test.TestingException,
+                          self.compute_api.backup,
+                          self.context,
+                          instance,
+                          'no_image_backup',
+                          'DAILY',
+                          0,
+                          image_id=image_id)
+
+        self.stubs.Set(fake_image._FakeImageService, method, restore)
+        db_instance = db.instance_get_all(self.context)[0]
+        self.assertIsNone(db_instance['task_state'])
+
+    def test_backup_image_creation_fails(self):
+        self._do_test_backup_image_service_fails('create', None)
+
+    def test_backup_image_show_fails(self):
+        self._do_test_backup_image_service_fails('show', 'image')
+
     def test_backup(self):
         # Can't backup an instance which is already being backed up.
         instance = self._create_fake_instance()
-- 
cgit