Add checks for volume status when rebuilding

When rebuilding, we should only allow detaching
the volume with 'in-use' status, volume in status
such as 'retyping' should not allowed.

Conflicts:
    nova/api/openstack/compute/servers.py
    nova/compute/api.py
    nova/tests/unit/api/openstack/compute/test_server_actions.py
Modified:
    nova/tests/unit/compute/test_compute_api.py

NOTE(elod.illes):
* conflicts in servers.py and test_server_actions.py are due to bug
  fixing patch I25eff0271c856a8d3e83867b448e1dec6f6732ab is not
  backported to stable/train
* api.py conflict is due to Ic2ad1468d31b7707b7f8f2b845a9cf47d9d076d5
  is part of a feature introduced in Ussuri
* modification of test_compute_api.py is also required due to patch
  I25eff0271c856a8d3e83867b448e1dec6f6732ab is not backported and
  another patch, Ide8eb9e09d22f20165474d499ef0524aefc67854, that
  cannot be backported to stable/train

Change-Id: I7f93cfd18f948134c9cb429dea55740d2cf97994
Closes-Bug: #1489304
(cherry picked from commit 10e9a9b9fc62a3cf72c3717e3621ed95d3cf5519)
(cherry picked from commit bcbeae2c605f4ab4ad805dddccac802928a180b6)

4 files changed, 95 insertions, 4 deletions

diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py
index 42308af931..68af4750d4 100644
--- a/nova/api/openstack/compute/servers.py
+++ b/nova/api/openstack/compute/servers.py
@@ -1139,6 +1139,7 @@ class ServersController(wsgi.Controller):
                 exception.ImageNotActive,
                 exception.ImageUnacceptable,
                 exception.InvalidMetadata,
+                exception.InvalidVolume,
                 ) as error:
             raise exc.HTTPBadRequest(explanation=error.format_message())
         except INVALID_FLAVOR_IMAGE_EXCEPTIONS as error:
diff --git a/nova/compute/api.py b/nova/compute/api.py
index 69837f62d7..08ca453d04 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -3346,6 +3346,12 @@ class API(base.Base):
         self._checks_for_create_and_rebuild(context, image_id, image,
                 flavor, metadata, files_to_inject, root_bdm)
 
+        # Check the state of the volume. If it is not in-use, an exception
+        # will occur when creating attachment during reconstruction,
+        # resulting in the failure of reconstruction and the instance
+        # turning into an error state.
+        self._check_volume_status(context, bdms)
+
         # NOTE(sean-k-mooney): When we rebuild with a new image we need to
         # validate that the NUMA topology does not change as we do a NOOP claim
         # in resource tracker. As such we cannot allow the resource usage or
@@ -3445,6 +3451,17 @@ class API(base.Base):
                 preserve_ephemeral=preserve_ephemeral, host=host,
                 request_spec=request_spec)
 
+    def _check_volume_status(self, context, bdms):
+        """Check whether the status of the volume is "in-use".
+
+        :param context: A context.RequestContext
+        :param bdms: BlockDeviceMappingList of BDMs for the instance
+        """
+        for bdm in bdms:
+            if bdm.volume_id:
+                vol = self.volume_api.get(context, bdm.volume_id)
+                self.volume_api.check_attached(context, vol)
+
     @staticmethod
     def _validate_numa_rebuild(instance, image, flavor):
         """validates that the NUMA constraints do not change on rebuild.
@@ -3963,10 +3980,8 @@ class API(base.Base):
 
         bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
                     context, instance.uuid)
-        for bdm in bdms:
-            if bdm.volume_id:
-                vol = self.volume_api.get(context, bdm.volume_id)
-                self.volume_api.check_attached(context, vol)
+        self._check_volume_status(context, bdms)
+
         if compute_utils.is_volume_backed_instance(context, instance, bdms):
             reason = _("Cannot rescue a volume-backed instance")
             raise exception.InstanceNotRescuable(instance_id=instance.uuid,
diff --git a/nova/tests/unit/api/openstack/compute/test_server_actions.py b/nova/tests/unit/api/openstack/compute/test_server_actions.py
index 2c5feb9c4b..1f6d632dfd 100644
--- a/nova/tests/unit/api/openstack/compute/test_server_actions.py
+++ b/nova/tests/unit/api/openstack/compute/test_server_actions.py
@@ -642,6 +642,21 @@ class ServerActionsControllerTestV21(test.TestCase):
                           self.controller._action_rebuild,
                           self.req, FAKE_UUID, body=body)
 
+    @mock.patch.object(compute_api.API, 'rebuild')
+    def test_rebuild_raise_invalid_volume_exc(self, mock_rebuild):
+        """Make sure that we can't rebuild with an InvalidVolume exception."""
+        body = {
+            "rebuild": {
+                "imageRef": self._image_href,
+            },
+        }
+
+        mock_rebuild.side_effect = exception.InvalidVolume('error')
+
+        self.assertRaises(webob.exc.HTTPBadRequest,
+                          self.controller._action_rebuild,
+                          self.req, FAKE_UUID, body=body)
+
     def test_resize_server(self):
 
         body = dict(resize=dict(flavorRef="http://localhost/3"))
diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py
index d17a9b5492..8db729b774 100644
--- a/nova/tests/unit/compute/test_compute_api.py
+++ b/nova/tests/unit/compute/test_compute_api.py
@@ -3620,6 +3620,66 @@ class _ComputeAPIUnitTestMixIn(object):
     @mock.patch.object(compute_api.API, '_check_auto_disk_config')
     @mock.patch.object(compute_api.API, '_checks_for_create_and_rebuild')
     @mock.patch.object(compute_api.API, '_record_action_start')
+    def test_rebuild_with_invalid_volume(self, _record_action_start,
+            _checks_for_create_and_rebuild, _check_auto_disk_config,
+            mock_get_image, mock_get_bdms, get_flavor,
+            instance_save, req_spec_get_by_inst_uuid):
+        """Test a negative scenario where the instance has an
+        invalid volume.
+        """
+        instance = fake_instance.fake_instance_obj(
+            self.context, vm_state=vm_states.ACTIVE, cell_name='fake-cell',
+            launched_at=timeutils.utcnow(),
+            system_metadata={}, image_ref='foo',
+            expected_attrs=['system_metadata'])
+
+        bdms = objects.BlockDeviceMappingList(objects=[
+                objects.BlockDeviceMapping(
+                    boot_index=None, image_id=None,
+                    source_type='volume', destination_type='volume',
+                    volume_type=None, snapshot_id=None,
+                    volume_id=uuids.volume_id, volume_size=None)])
+        mock_get_bdms.return_value = bdms
+
+        get_flavor.return_value = test_flavor.fake_flavor
+        flavor = instance.get_flavor()
+
+        image_href = 'foo'
+        image = {
+            "min_ram": 10, "min_disk": 1,
+            "properties": {
+                'architecture': fields_obj.Architecture.X86_64}}
+        mock_get_image.return_value = (None, image)
+
+        fake_spec = objects.RequestSpec()
+        req_spec_get_by_inst_uuid.return_value = fake_spec
+
+        fake_volume = {'id': uuids.volume_id, 'status': 'retyping'}
+        with mock.patch.object(self.compute_api.volume_api, 'get',
+                               return_value=fake_volume) as mock_get_volume:
+            self.assertRaises(exception.InvalidVolume,
+                              self.compute_api.rebuild,
+                              self.context,
+                              instance,
+                              image_href,
+                              "new password")
+            self.assertIsNone(instance.task_state)
+            mock_get_bdms.assert_called_once_with(self.context,
+                                                  instance.uuid)
+            mock_get_volume.assert_called_once_with(self.context,
+                                                    uuids.volume_id)
+            _check_auto_disk_config.assert_called_once_with(image=image)
+            _checks_for_create_and_rebuild.assert_called_once_with(
+                self.context, None, image, flavor, {}, [], None)
+
+    @mock.patch.object(objects.RequestSpec, 'get_by_instance_uuid')
+    @mock.patch.object(objects.Instance, 'save')
+    @mock.patch.object(objects.Instance, 'get_flavor')
+    @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_instance_uuid')
+    @mock.patch.object(compute_api.API, '_get_image')
+    @mock.patch.object(compute_api.API, '_check_auto_disk_config')
+    @mock.patch.object(compute_api.API, '_checks_for_create_and_rebuild')
+    @mock.patch.object(compute_api.API, '_record_action_start')
     def test_rebuild(self, _record_action_start,
             _checks_for_create_and_rebuild, _check_auto_disk_config,
             _get_image, bdm_get_by_instance_uuid, get_flavor, instance_save,