More fixes for anaconda deploy interface

The anaconda deploy interface has a few issues that are
addressed here:

- fixes logic in get_instance_image_info() for anaconda. If the
  ironic node's instance_info doesn't have both 'stage2' and
  'ks_template' specified, we weren't using any values from the
  instance_info. This has been fixed to use values from
  instance_info if specified. Otherwise, they are set as follows:
  The 'stage2' value is taken from the image properties.
  We use the value for 'ks_template' if it is specified in the
  image properties. If not (since it is optional), we use the
  config option's '[anaconda]default_ks_template' value.
  setting.
- For anaconda's stage2 directory, we were incorrectly creating a
  directory using the full path of the stage2 file. It now
  correctly creates the right directory.
- The anaconda deploy interface expects the node's instance_info
  to be populated with the 'image_url'; added code to do that in
  PXEAnacondaDeploy's prepare() method.
- When the deploy is finished and the bm node is being rebooted,
  we incorrectly set the node's provision state to 'active'
  instead of doing it via the provisioning state machine mechanism.
- The code that was doing the validation of the kickstart file was
  incorrect and resulted in errors; this has been addressed.
- The '%traceback' section in the packaged 'ks.cfg.template' file
  is deprecated and fails validation, so it has been removed.

Conflict: ironic/common/pxe_utils.py, cache_ramdisk_kernel(), due
to code changes in master that aren't in xena. Manually fixed.

Change-Id: I953e948bcfa108d4c8e7b145da2f52b652e52a10
(cherry picked from commit 06cc5d47dcdc49193e5e4e748471eb0b9ae97bbb)

5 files changed, 93 insertions, 45 deletions

diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py
index 08d6d3f98..13c27d718 100644
--- a/ironic/common/pxe_utils.py
+++ b/ironic/common/pxe_utils.py
@@ -694,42 +694,52 @@ def get_instance_image_info(task, ipxe_enabled=False):
         return image_info
 
     labels = ('kernel', 'ramdisk')
+    image_properties = None
     d_info = deploy_utils.get_image_instance_info(node)
     if not (i_info.get('kernel') and i_info.get('ramdisk')):
+        # NOTE(rloo): If both are not specified in instance_info
+        # we won't use any of them. We'll use the values specified
+        # with the image, which we assume have been set.
         glance_service = service.GlanceImageService(context=ctx)
-        iproperties = glance_service.show(d_info['image_source'])['properties']
+        image_properties = glance_service.show(
+            d_info['image_source'])['properties']
         for label in labels:
-            i_info[label] = str(iproperties[label + '_id'])
+            i_info[label] = str(image_properties[label + '_id'])
         node.instance_info = i_info
         node.save()
 
     anaconda_labels = ()
     if deploy_utils.get_boot_option(node) == 'kickstart':
-        # stage2  - Installer stage2 squashfs image
-        # ks_template - Anaconda kickstart template
+        # stage2: installer stage2 squashfs image
+        # ks_template: anaconda kickstart template
         # ks_cfg - rendered ks_template
         anaconda_labels = ('stage2', 'ks_template', 'ks_cfg')
-        if not (i_info.get('stage2') and i_info.get('ks_template')):
-            iproperties = glance_service.show(
-                d_info['image_source']
-            )['properties']
-            for label in anaconda_labels:
+        if not i_info.get('stage2') or not i_info.get('ks_template'):
+            if not image_properties:
+                glance_service = service.GlanceImageService(context=ctx)
+                image_properties = glance_service.show(
+                    d_info['image_source'])['properties']
+            if not i_info.get('ks_template'):
                 # ks_template is an optional property on the image
-                if (label == 'ks_template'
-                        and not iproperties.get('ks_template')):
-                    i_info[label] = CONF.anaconda.default_ks_template
-                elif label == 'ks_cfg':
-                    i_info[label] = ''
-                elif label == 'stage2' and 'stage2_id' not in iproperties:
-                    msg = ("stage2_id property missing on the image. "
-                           "The anaconda deploy interface requires stage2_id "
-                           "property to be associated with the os image. ")
+                if 'ks_template' not in image_properties:
+                    i_info['ks_template'] = CONF.anaconda.default_ks_template
+                else:
+                    i_info['ks_template'] = str(
+                        image_properties['ks_template'])
+            if not i_info.get('stage2'):
+                if 'stage2_id' not in image_properties:
+                    msg = ("'stage2_id' property is missing from the OS image "
+                           "%s. The anaconda deploy interface requires this "
+                           "to be set with the OS image or in instance_info. "
+                           % d_info['image_source'])
                     raise exception.ImageUnacceptable(msg)
                 else:
-                    i_info[label] = str(iproperties['stage2_id'])
+                    i_info['stage2'] = str(image_properties['stage2_id'])
+        # NOTE(rloo): This is internally generated; cannot be specified.
+        i_info['ks_cfg'] = ''
 
-            node.instance_info = i_info
-            node.save()
+        node.instance_info = i_info
+        node.save()
 
     for label in labels + anaconda_labels:
         image_info[label] = (
@@ -1119,16 +1129,17 @@ def validate_kickstart_file(ks_cfg):
         return
 
     with tempfile.NamedTemporaryFile(
-            dir=CONF.tempdir, suffix='.cfg') as ks_file:
-        ks_file.writelines(ks_cfg)
+            dir=CONF.tempdir, suffix='.cfg', mode='wt') as ks_file:
+        ks_file.write(ks_cfg)
+        ks_file.flush()
         try:
-            result = utils.execute(
+            utils.execute(
                 'ksvalidator', ks_file.name, check_on_exit=[0], attempts=1
             )
-        except processutils.ProcessExecutionError:
+        except processutils.ProcessExecutionError as e:
             msg = _(("The kickstart file generated does not pass validation. "
-                     "The ksvalidator tool returned following error(s): %s") %
-                    (result))
+                     "The ksvalidator tool returned the following error: %s") %
+                    (e))
             raise exception.InvalidKickstartFile(msg)
 
 
@@ -1225,17 +1236,14 @@ def cache_ramdisk_kernel(task, pxe_info, ipxe_enabled=False):
     else:
         path = os.path.join(get_root_dir(), node.uuid)
     fileutils.ensure_tree(path)
-    # anconda deploy will have 'stage2' as one of the labels in pxe_info dict
+    # anaconda deploy will have 'stage2' as one of the labels in pxe_info dict
     if 'stage2' in pxe_info.keys():
-        # stage2  will be stored in ipxe http directory. So make sure they
-        # exist.
-        fileutils.ensure_tree(
-            get_file_path_from_label(
-                node.uuid,
-                get_ipxe_root_dir(),
-                'stage2'
-            )
-        )
+        # stage2 will be stored in ipxe http directory so make sure the
+        # directory exists.
+        file_path = get_file_path_from_label(node.uuid,
+                                             get_ipxe_root_dir(),
+                                             'stage2')
+        fileutils.ensure_tree(os.path.dirname(file_path))
         # ks_cfg is rendered later by the driver using ks_template. It cannot
         # be fetched and cached.
         t_pxe_info.pop('ks_cfg')
diff --git a/ironic/drivers/modules/ks.cfg.template b/ironic/drivers/modules/ks.cfg.template
index 40552377b..f7ef75e1c 100644
--- a/ironic/drivers/modules/ks.cfg.template
+++ b/ironic/drivers/modules/ks.cfg.template
@@ -18,7 +18,7 @@ autopart
 # Downloading and installing OS image using liveimg section is mandatory
 liveimg --url {{ ks_options.liveimg_url }}
 
-# Following %pre, %onerror and %trackback sections are mandatory
+# Following %pre and %onerror sections are mandatory
 %pre
 /usr/bin/curl -X POST -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'X-OpenStack-Ironic-API-Version: 1.72' -d '{"callback_url": "", "agent_token": "{{ ks_options.agent_token }}", "agent_status": "start", "agent_status_message": "Deployment starting. Running pre-installation scripts."}' {{ ks_options.heartbeat_url }}
 %end
@@ -27,10 +27,6 @@ liveimg --url {{ ks_options.liveimg_url }}
 /usr/bin/curl -X POST -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'X-OpenStack-Ironic-API-Version: 1.72' -d '{"callback_url": "", "agent_token": "{{ ks_options.agent_token }}", "agent_status": "error", "agent_status_message": "Error: Deploying using anaconda. Check console for more information."}' {{ ks_options.heartbeat_url }}
 %end
 
-%traceback
-/usr/bin/curl -X POST -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'X-OpenStack-Ironic-API-Version: 1.72' -d '{"callback_url": "", "agent_token": "{{ ks_options.agent_token }}", "agent_status": "error", "agent_status_message": "Error: Installer crashed unexpectedly."}' {{ ks_options.heartbeat_url }}
-%end
-
 # Sending callback after the installation is mandatory
 %post
 /usr/bin/curl -X POST -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'X-OpenStack-Ironic-API-Version: 1.72' -d '{"callback_url": "", "agent_token": "{{ ks_options.agent_token }}", "agent_status": "end", "agent_status_message": "Deployment completed successfully."}' {{ ks_options.heartbeat_url }}
diff --git a/ironic/drivers/modules/pxe.py b/ironic/drivers/modules/pxe.py
index f3172b150..9df7354a5 100644
--- a/ironic/drivers/modules/pxe.py
+++ b/ironic/drivers/modules/pxe.py
@@ -78,6 +78,9 @@ class PXEAnacondaDeploy(agent_base.AgentBaseMixin, agent_base.HeartbeatMixin,
             # NOTE(TheJulia): If this was any other interface, we would
             # unconfigure tenant networks, add provisioning networks, etc.
             task.driver.storage.attach_volumes(task)
+            node.instance_info = deploy_utils.build_instance_info_for_deploy(
+                task)
+            node.save()
         if node.provision_state in (states.ACTIVE, states.UNRESCUING):
             # In the event of takeover or unrescue.
             task.driver.boot.prepare_instance(task)
@@ -116,13 +119,13 @@ class PXEAnacondaDeploy(agent_base.AgentBaseMixin, agent_base.HeartbeatMixin,
             agent_base.log_and_raise_deployment_error(task, msg)
 
         try:
+            task.process_event('resume')
             self.clean_up(task)
             manager_utils.node_power_action(task, states.POWER_OFF)
             task.driver.network.remove_provisioning_network(task)
             task.driver.network.configure_tenant_networks(task)
             manager_utils.node_power_action(task, states.POWER_ON)
-            node.provision_state = states.ACTIVE
-            node.save()
+            task.process_event('done')
         except Exception as e:
             msg = (_('Error rebooting node %(node)s after deploy. '
                      'Error: %(error)s') %
diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py
index f57a0105b..fecc0b374 100644
--- a/ironic/tests/unit/drivers/modules/test_pxe.py
+++ b/ironic/tests/unit/drivers/modules/test_pxe.py
@@ -895,15 +895,23 @@ class PXEAnacondaDeployTestCase(db_base.DbTestCase):
             mock_prepare_ks_config.assert_called_once_with(task, image_info,
                                                            anaconda_boot=True)
 
+    @mock.patch.object(deploy_utils, 'build_instance_info_for_deploy',
+                       autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True)
-    def test_prepare(self, mock_prepare_instance):
+    def test_prepare(self, mock_prepare_instance, mock_build_instance):
+
         node = self.node
         node.provision_state = states.DEPLOYING
         node.instance_info = {}
         node.save()
+        updated_instance_info = {'image_url': 'foo'}
+        mock_build_instance.return_value = updated_instance_info
         with task_manager.acquire(self.context, node.uuid) as task:
             task.driver.deploy.prepare(task)
             self.assertFalse(mock_prepare_instance.called)
+            mock_build_instance.assert_called_once_with(task)
+            node.refresh()
+            self.assertEqual(updated_instance_info, node.instance_info)
 
     @mock.patch.object(pxe.PXEBoot, 'prepare_instance', autospec=True)
     def test_prepare_active(self, mock_prepare_instance):
diff --git a/releasenotes/notes/anaconda-deploy-more-fixes-58d996c7031c8c4b.yaml b/releasenotes/notes/anaconda-deploy-more-fixes-58d996c7031c8c4b.yaml
new file mode 100644
index 000000000..9298c3036
--- /dev/null
+++ b/releasenotes/notes/anaconda-deploy-more-fixes-58d996c7031c8c4b.yaml
@@ -0,0 +1,33 @@
+---
+fixes:
+  - |
+    Fixes the logic for the anaconda deploy interface. If the
+    ironic node's instance_info doesn't have both 'stage2' and
+    'ks_template' specified, we weren't using the instance_info
+    at all. This has been fixed to use the instance_info if it
+    was specified. Otherwise, 'stage2' is taken from the
+    image's properties (assumed that it is set there).
+    'ks_template' value is from the image properties if specified
+    there (since it is optional); else we use the config setting
+    '[anaconda] default_ks_template'.
+  - |
+    For the anaconda deploy interface, the 'stage2' directory was
+    incorrectly being created using the full path of the stage2 file;
+    this has been fixed.
+  - |
+    The anaconda deploy interface expects the node's instance_info
+    to be populated with the 'image_url'; this is now populated
+    (via PXEAnacondaDeploy's prepare() method).
+  - |
+    For the anaconda deploy interface, when the deploy was finished
+    and the bm node was being rebooted, the node's provision state was
+    incorrectly being set to 'active' -- the provisioning state-machine
+    mechanism now handles that.
+  - |
+    For the anaconda deploy interface, the code that was doing the
+    validation of the kickstart file was incorrect and resulted in
+    errors; this has been addressed.
+  - |
+    For the anaconda deploy interface, the '%traceback' section in the
+    packaged 'ks.cfg.template' file is deprecated and fails validation,
+    so it has been removed.