Update nova rebuild to account for new image

Update the Nova driver's rebuild() to account for changes in the instance image
during rebuild.  Currently, rebuild ignores user requests to rebuild with a
new image, making it impossible to do image-based upgrades.  This adds logic
to the patcher to account for the preserve_ephemeral flag and updates rebuild()
to use the patcher to re-patch all driver_info during rebuild.

This also adds missing test coverage of the rebuild code.

Change-Id: I6c8aacd36e0095d0590a1dc552730f2d52036225
Closes-bug: #1334905.

4 files changed, 139 insertions, 18 deletions

diff --git a/ironic/nova/tests/virt/ironic/test_driver.py b/ironic/nova/tests/virt/ironic/test_driver.py
index a70493238..d2ca7320d 100644
--- a/ironic/nova/tests/virt/ironic/test_driver.py
+++ b/ironic/nova/tests/virt/ironic/test_driver.py
@@ -28,6 +28,7 @@ from ironic.nova.virt.ironic import driver as ironic_driver
 from ironic.nova.virt.ironic import ironic_states
 
 from nova.compute import power_state as nova_states
+from nova.compute import task_states
 from nova import context as nova_context
 from nova import exception
 from nova.objects import flavor as flavor_obj
@@ -933,3 +934,96 @@ class IronicDriverTestCase(test.NoDBTestCase):
         fake_group = 'fake-security-group-members'
         self.driver.refresh_instance_security_rules(fake_group)
         mock_risr.assert_called_once_with(fake_group)
+
+    @mock.patch.object(ironic_driver.IronicDriver, '_wait_for_active')
+    @mock.patch.object(loopingcall, 'FixedIntervalLoopingCall')
+    @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state')
+    @mock.patch.object(flavor_obj.Flavor, 'get_by_id')
+    @mock.patch.object(ironic_driver.IronicDriver, '_add_driver_fields')
+    @mock.patch.object(FAKE_CLIENT.node, 'get')
+    @mock.patch.object(instance_obj.Instance, 'save')
+    def _test_rebuild(self, mock_save, mock_get, mock_driver_fields,
+                      mock_fg_bid, mock_set_pstate, mock_looping,
+                      mock_wait_active, preserve=False):
+        node_uuid = uuidutils.generate_uuid()
+        instance_uuid = uuidutils.generate_uuid()
+        node = ironic_utils.get_test_node(uuid=node_uuid,
+                                          instance_uuid=instance_uuid,
+                                          instance_type_id=5)
+        mock_get.return_value = node
+
+        image_meta = ironic_utils.get_test_image_meta()
+        flavor_id = 5
+        flavor = {'id': flavor_id, 'name': 'baremetal'}
+        mock_fg_bid.return_value = flavor
+
+        instance = fake_instance.fake_instance_obj(self.ctx,
+                                                   uuid=instance_uuid,
+                                                   node=node_uuid,
+                                                   instance_type_id=flavor_id)
+
+
+        fake_looping_call = FakeLoopingCall()
+        mock_looping.return_value = fake_looping_call
+
+        self.driver.rebuild(
+            context=self.ctx, instance=instance, image_meta=image_meta,
+            injected_files=None, admin_password=None, bdms=None,
+            detach_block_devices=None, attach_block_devices=None,
+            preserve_ephemeral=preserve)
+
+        mock_save.assert_called_once_with(
+            expected_task_state=[task_states.REBUILDING])
+        mock_driver_fields.assert_called_once_with(node, instance, image_meta,
+                                                   flavor, preserve)
+        mock_set_pstate.assert_called_once_with(node_uuid,
+                                                ironic_states.REBUILD)
+        mock_looping.assert_called_once_with(mock_wait_active,
+                                             FAKE_CLIENT_WRAPPER,
+                                             instance)
+        fake_looping_call.start.assert_called_once_with(
+            interval=CONF.ironic.api_retry_interval)
+        fake_looping_call.wait.assert_called_once()
+
+    def test_rebuild_preserve_ephemeral(self):
+        self._test_rebuild(preserve=True)
+
+    def test_rebuild_no_preserve_ephemeral(self):
+        self._test_rebuild(preserve=False)
+
+    @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state')
+    @mock.patch.object(flavor_obj.Flavor, 'get_by_id')
+    @mock.patch.object(ironic_driver.IronicDriver, '_add_driver_fields')
+    @mock.patch.object(FAKE_CLIENT.node, 'get')
+    @mock.patch.object(instance_obj.Instance, 'save')
+    def test_rebuild_failures(self, mock_save, mock_get, mock_driver_fields,
+                              mock_fg_bid, mock_set_pstate):
+        node_uuid = uuidutils.generate_uuid()
+        instance_uuid = uuidutils.generate_uuid()
+        node = ironic_utils.get_test_node(uuid=node_uuid,
+                                          instance_uuid=instance_uuid,
+                                          instance_type_id=5)
+        mock_get.return_value = node
+
+        image_meta = ironic_utils.get_test_image_meta()
+        flavor_id = 5
+        flavor = {'id': flavor_id, 'name': 'baremetal'}
+        mock_fg_bid.return_value = flavor
+
+        instance = fake_instance.fake_instance_obj(self.ctx,
+                                                   uuid=instance_uuid,
+                                                   node=node_uuid,
+                                                   instance_type_id=flavor_id)
+
+        exceptions = [
+            exception.NovaException(),
+            ironic_exception.BadRequest(),
+            ironic_exception.InternalServerError(),
+        ]
+        for e in exceptions:
+            mock_set_pstate.side_effect = e
+            self.assertRaises(exception.InstanceDeployFailure,
+                self.driver.rebuild,
+                context=self.ctx, instance=instance, image_meta=image_meta,
+                injected_files=None, admin_password=None, bdms=None,
+                detach_block_devices=None, attach_block_devices=None)
diff --git a/ironic/nova/tests/virt/ironic/test_patcher.py b/ironic/nova/tests/virt/ironic/test_patcher.py
index e3a58a781..6ee7d7217 100644
--- a/ironic/nova/tests/virt/ironic/test_patcher.py
+++ b/ironic/nova/tests/virt/ironic/test_patcher.py
@@ -72,6 +72,29 @@ class IronicDriverFieldsTestCase(test.NoDBTestCase):
         self.assertIn(expected1, patch)
         self.assertIn(expected2, patch)
 
+    def test_pxe_get_deploy_patch_preserve_ephemeral(self):
+        node = ironic_utils.get_test_node(driver='pxe_fake')
+        instance = fake_instance.fake_instance_obj(
+                        self.ctx, node=node.uuid, ephemeral_gb=10)
+        for preserve in [True, False]:
+            patch = patcher.create(node).get_deploy_patch(
+                    instance, self.image_meta, self.flavor,
+                    preserve_ephemeral=preserve)
+            expected =  {'path': '/instance_info/preserve_ephemeral',
+                         'value': str(preserve), 'op': 'add', }
+            self.assertIn(expected, patch)
+
+    def test_pxe_get_deploy_patch_no_preserve_ephemeral(self):
+        node = ironic_utils.get_test_node(driver='pxe_fake')
+        instance = fake_instance.fake_instance_obj(
+                        self.ctx, node=node.uuid, ephemeral_gb=10)
+        patch = patcher.create(node).get_deploy_patch(
+                    instance, self.image_meta, self.flavor)
+        for preserve in [True, False]:
+            unexpected =  {'path': '/instance_info/preserve_ephemeral',
+                           'value': str(preserve), 'op': 'add', }
+            self.assertNotIn(unexpected, patch)
+
     def test_pxe_get_deploy_patch_no_flavor_kernel_ramdisk_ids(self):
         self.flavor = ironic_utils.get_test_flavor(extra_specs={})
         node = ironic_utils.get_test_node(driver='pxe_fake')
diff --git a/ironic/nova/virt/ironic/driver.py b/ironic/nova/virt/ironic/driver.py
index 949ec393b..8de4fa3cf 100644
--- a/ironic/nova/virt/ironic/driver.py
+++ b/ironic/nova/virt/ironic/driver.py
@@ -242,11 +242,13 @@ class IronicDriver(virt_driver.ComputeDriver):
     def _stop_firewall(self, instance, network_info):
         self.firewall_driver.unfilter_instance(instance, network_info)
 
-    def _add_driver_fields(self, node, instance, image_meta, flavor):
+    def _add_driver_fields(self, node, instance, image_meta, flavor,
+                           preserve_ephemeral=None):
         icli = client_wrapper.IronicClientWrapper()
         patch = patcher.create(node).get_deploy_patch(instance,
                                                       image_meta,
-                                                      flavor)
+                                                      flavor,
+                                                      preserve_ephemeral)
 
         # Associate the node with an instance
         patch.append({'path': '/instance_uuid', 'op': 'add',
@@ -726,21 +728,14 @@ class IronicDriver(virt_driver.ComputeDriver):
         instance.task_state = task_states.REBUILD_SPAWNING
         instance.save(expected_task_state=[task_states.REBUILDING])
 
-        node_uuid = instance.get('node')
-
+        node_uuid = instance['node']
         icli = client_wrapper.IronicClientWrapper()
+        node = icli.call("node.get", node_uuid)
+        flavor = flavor_obj.Flavor.get_by_id(context,
+                                             instance['instance_type_id'])
 
-        # Update instance_info for the ephemeral preservation value.
-        patch = []
-        patch.append({'path': '/instance_info/preserve_ephemeral',
-                      'op': 'add', 'value': str(preserve_ephemeral)})
-        try:
-            icli.call('node.update', node_uuid, patch)
-        except ironic_exception.BadRequest:
-            msg = (_("Failed to add deploy parameters on node %(node)s "
-                     "when rebuilding the instance %(instance)s")
-                   % {'node': node_uuid, 'instance': instance['uuid']})
-            raise exception.InstanceDeployFailure(msg)
+        self._add_driver_fields(node, instance, image_meta, flavor,
+                                preserve_ephemeral)
 
         # Trigger the node rebuild/redeploy.
         try:
diff --git a/ironic/nova/virt/ironic/patcher.py b/ironic/nova/virt/ironic/patcher.py
index e1bbb7c22..c76f1af6a 100644
--- a/ironic/nova/virt/ironic/patcher.py
+++ b/ironic/nova/virt/ironic/patcher.py
@@ -47,7 +47,8 @@ class GenericDriverFields(object):
     def __init__(self, node):
         self.node = node
 
-    def get_deploy_patch(self, instance, image_meta, flavor):
+    def get_deploy_patch(self, instance, image_meta, flavor,
+                         preserve_ephemeral=None):
         return []
 
     def get_cleanup_patch(self, instance, network_info, flavor):
@@ -74,7 +75,8 @@ class PXEDriverFields(GenericDriverFields):
             deploy_ids['pxe_deploy_ramdisk'] = deploy_ramdisk
         return deploy_ids
 
-    def get_deploy_patch(self, instance, image_meta, flavor):
+    def get_deploy_patch(self, instance, image_meta, flavor,
+                         preserve_ephemeral=None):
         """Build a patch to add the required fields to deploy a node.
 
         Build a json-patch to add the required fields to deploy a node
@@ -82,8 +84,10 @@ class PXEDriverFields(GenericDriverFields):
 
         :param instance: the instance object.
         :param image_meta: the metadata associated with the instance
-                            image.
+                           image.
         :param flavor: the flavor object.
+        :param preserve_ephemeral: preserve_ephemeral status (bool) to be
+                                   specified during rebuild.
         :returns: a json-patch with the fields that needs to be updated.
 
         """
@@ -108,6 +112,11 @@ class PXEDriverFields(GenericDriverFields):
                 patch.append({'path': '/instance_info/ephemeral_format',
                               'op': 'add',
                               'value': CONF.default_ephemeral_format})
+
+        if preserve_ephemeral is not None:
+            patch.append({'path': '/instance_info/preserve_ephemeral',
+                          'op': 'add', 'value': str(preserve_ephemeral)})
+
         return patch
 
     def get_cleanup_patch(self, instance, network_info, flavor):