Fix various issues in the anaconda deploy interface

The kickstart template expects a dictionary with 'ks_options'
as the key. Instead build_kickstart_config_options function
returns a dict with keys 'liveimg_url', 'agent_token' and
'heartbeat_url'.

This change fixes this problem by returning a dictionary of
dict with 'ks_options' as key and the dictionary with
keys 'liveimg_url', 'agent_token' and heartbeat_url' as
value.

Fix a bug where the deploy() method of anaconda deploy
interface where it did not return states.DEPLOYWAIT instead
it returned 'None' which caused the instance to go straight to
'active' instead of 'wait call-back'.

Fix issues in the default kickstart template where heartbeat was
missing 'callback_url' parameter and the HTTP method should be
'POST' not 'PUT'.

Fix issues with automated cleaning when anaconda deploy interface
is used.

Anaconda deploy interface could not deploy tarballs as
the disk image sent to the anaconda interface via liveimg --url
kickstart command does not include any file extension. When
no file extension is present the kickstart command liveimg --url
assumes the disk is a mountable partiton image. We fix this
problem by enabling the user to specify file extensions using
a glance image property named 'disk_file_extension' on the OS
image.

Co-Authored-By: Ruby Loo <opensrloo@gmail.com>
Change-Id: I556f8c9efbc5ab0941513c3ecaa2aa3ca7f346ae

9 files changed, 112 insertions, 33 deletions

diff --git a/doc/source/admin/anaconda-deploy-interface.rst b/doc/source/admin/anaconda-deploy-interface.rst
index c72c4378e..041bd9c31 100644
--- a/doc/source/admin/anaconda-deploy-interface.rst
+++ b/doc/source/admin/anaconda-deploy-interface.rst
@@ -21,6 +21,16 @@ option in ironic.conf. For example:
 This change takes effect after all the ironic conductors have been
 restarted.
 
+The default kickstart template is specified via the configuration option
+``[anaconda]default_ks_template``. It is set to this `ks.cfg.template`_
+but can be modified to be some other template.
+
+.. code-block::  ini
+
+   [anaconda]
+   default_ks_template = file:///etc/ironic/ks.cfg.template
+
+
 When creating an ironic node, specify ``anaconda`` as the deploy interface.
 For example:
 
@@ -92,12 +102,19 @@ The kernel and ramdisk can be found at ``/images/pxeboot/vmlinuz`` and
 image can be normally found at ``/LiveOS/squashfs.img`` or
 ``/images/install.img``.
 
-The OS tarball must be configured with the following properties in glance, in order
-to be used with the anaconda deploy driver:
+The OS tarball must be configured with the following properties in glance, in
+order to be used with the anaconda deploy driver:
 
 * ``kernel_id``
 * ``ramdisk_id``
 * ``stage2_id``
+* ``disk_file_extension`` (optional)
+
+Valid ``disk_file_extension`` values are ``.img``, ``.tar``, ``.tbz``,
+``.tgz``, ``.txz``, ``.tar.gz``, ``.tar.bz2``, and ``.tar.xz``. When
+``disk_file_extension`` property is not set to one of the above valid values
+the anaconda installer will assume that the image provided is a mountable
+OS disk.
 
 This is an example of adding the anaconda-related images and the OS tarball to
 glance:
@@ -114,7 +131,8 @@ glance:
             compressed --disk-format raw --shared \
             --property kernel_id=<glance_uuid_vmlinuz> \
             --property ramdisk_id=<glance_uuid_ramdisk> \
-            --property stage2_id=<glance_uuid_stage2> <disto-name-version>
+            --property stage2_id=<glance_uuid_stage2> disto-name-version \
+            --property disk_file_extension=.tgz
 
 Creating a bare metal server
 ----------------------------
@@ -127,10 +145,6 @@ specified via the OS image in glance. If no kickstart template is specified
 (via the node's ``instance_info``  or ``ks_template`` glance image property),
 the default kickstart template will be used to deploy the OS.
 
-The default kickstart template is specified via the configuration option
-``[anaconda]default_ks_template``. It is set to this `ks.cfg.template`_
-but can be modified to be some other template.
-
 This is an example of how to set the kickstart template for a specific
 ironic node:
 
diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py
index c886360d3..08d6d3f98 100644
--- a/ironic/common/pxe_utils.py
+++ b/ironic/common/pxe_utils.py
@@ -963,14 +963,14 @@ def build_kickstart_config_options(task):
     :returns: A dictionary of kickstart options to be used in the kickstart
               template.
     """
-    ks_options = {}
+    params = {}
     node = task.node
     manager_utils.add_secret_token(node, pregenerated=True)
     node.save()
-    ks_options['liveimg_url'] = node.instance_info['image_url']
-    ks_options['agent_token'] = node.driver_internal_info['agent_secret_token']
-    ks_options['heartbeat_url'] = _build_heartbeat_url(node.uuid)
-    return ks_options
+    params['liveimg_url'] = node.instance_info['image_url']
+    params['agent_token'] = node.driver_internal_info['agent_secret_token']
+    params['heartbeat_url'] = _build_heartbeat_url(node.uuid)
+    return {'ks_options': params}
 
 
 def get_volume_pxe_options(task):
diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py
index 1a1b51aad..50e78051b 100644
--- a/ironic/drivers/modules/deploy_utils.py
+++ b/ironic/drivers/modules/deploy_utils.py
@@ -573,7 +573,7 @@ def validate_image_properties(task, deploy_info):
         properties = ['kernel_id', 'ramdisk_id']
         boot_option = get_boot_option(task.node)
         if boot_option == 'kickstart':
-            properties.append('squashfs_id')
+            properties.append('stage2_id')
     else:
         properties = ['kernel', 'ramdisk']
 
@@ -1121,6 +1121,24 @@ def _cache_and_convert_image(task, instance_info, image_info=None):
     symlink_dir = _get_http_image_symlink_dir_path()
     fileutils.ensure_tree(symlink_dir)
     symlink_path = _get_http_image_symlink_file_path(task.node.uuid)
+    file_extension = None
+    if get_boot_option(task.node) == 'kickstart':
+        # 'liveimg --url' kickstart command uses the file extension to
+        # identify the OS image type. Without a valid file extension it will
+        # assume the disk image is a partition image and try to 'mount' it on
+        # the ramdisk. See 'liveimg' command for more details
+        # https://pykickstart.readthedocs.io/en/latest/kickstart-docs.html
+        valid_file_extensions = ['.img', '.tar', '.tbz', '.tgz', '.txz',
+                                 '.tar.gz', '.tar.bz2', '.tar.xz']
+        if image_info and 'disk_file_extension' in image_info['properties']:
+            ext = image_info['properties']['disk_file_extension']
+            file_extension = ext if ext in valid_file_extensions else None
+        if file_extension:
+            symlink_path = symlink_path + file_extension
+        else:
+            LOG.warning("The 'disk_file_extension' property was not set on "
+                        "the glance image or not a valid extension so the "
+                        "anaconda installer will try to 'mount' the OS image.")
     utils.create_link_without_raise(image_path, symlink_path)
 
     base_url = CONF.deploy.http_url
@@ -1129,6 +1147,8 @@ def _cache_and_convert_image(task, instance_info, image_info=None):
     http_image_url = '/'.join(
         [base_url, CONF.deploy.http_image_subdir,
          task.node.uuid])
+    if file_extension:
+        http_image_url = http_image_url + file_extension
     _validate_image_url(task.node, http_image_url, secret=False)
     instance_info['image_url'] = http_image_url
 
diff --git a/ironic/drivers/modules/ks.cfg.template b/ironic/drivers/modules/ks.cfg.template
index 1a2cecaf3..40552377b 100644
--- a/ironic/drivers/modules/ks.cfg.template
+++ b/ironic/drivers/modules/ks.cfg.template
@@ -6,8 +6,9 @@ text
 cmdline
 reboot
 selinux --enforcing
-firewall --enabled
+firewall --disabled
 firstboot --disabled
+rootpw --lock
 
 bootloader --location=mbr --append="rhgb quiet crashkernel=auto"
 zerombr
@@ -19,19 +20,18 @@ liveimg --url {{ ks_options.liveimg_url }}
 
 # Following %pre, %onerror and %trackback sections are mandatory
 %pre
-/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'X-OpenStack-Ironic-API-Version: 1.72' -d '{"agent_token": "{{ ks_options.agent_token }}", "agent_status": "start", "agent_status_message": "Deployment starting. Running pre-installation scripts."}' {{ ks_options.heartbeat_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": "start", "agent_status_message": "Deployment starting. Running pre-installation scripts."}' {{ ks_options.heartbeat_url }}
 %end
 
 %onerror
-/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'X-OpenStack-Ironic-API-Version: 1.72' -d '{"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 }}
+/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 PUT -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'X-OpenStack-Ironic-API-Version: 1.72' -d '{"agent_token": "{{ ks_options.agent_token }}", "agent_status": "error", "agent_status_message": "Error: Installer crashed unexpectedly."}' {{ ks_options.heartbeat_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: Installer crashed unexpectedly."}' {{ ks_options.heartbeat_url }}
 %end
 
 # Sending callback after the installation is mandatory
 %post
-/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'X-OpenStack-Ironic-API-Version: 1.72' -d '{"agent_token": "{{ ks_options.agent_token }}", "agent_status": "end", "agent_status_message": "Deployment completed successfully."}' {{ ks_options.heartbeat_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": "end", "agent_status_message": "Deployment completed successfully."}' {{ ks_options.heartbeat_url }}
 %end
-
diff --git a/ironic/drivers/modules/pxe.py b/ironic/drivers/modules/pxe.py
index ea89700be..f3172b150 100644
--- a/ironic/drivers/modules/pxe.py
+++ b/ironic/drivers/modules/pxe.py
@@ -61,7 +61,7 @@ class PXEAnacondaDeploy(agent_base.AgentBaseMixin, agent_base.HeartbeatMixin,
         # Power-on the instance, with PXE prepared, we're done.
         manager_utils.node_power_action(task, states.POWER_ON)
         LOG.info('Deployment setup for node %s done', task.node.uuid)
-        return None
+        return states.DEPLOYWAIT
 
     @METRICS.timer('AnacondaDeploy.prepare')
     @task_manager.require_exclusive_lock
@@ -95,7 +95,11 @@ class PXEAnacondaDeploy(agent_base.AgentBaseMixin, agent_base.HeartbeatMixin,
         return False
 
     def should_manage_boot(self, task):
-        return False
+        if task.node.provision_state in (
+                states.DEPLOYING, states.DEPLOYWAIT, states.DEPLOYFAIL):
+            return False
+        # For cleaning and rescue, we use IPA, not anaconda
+        return agent_base.AgentBaseMixin.should_manage_boot(self, task)
 
     def reboot_to_instance(self, task):
         node = task.node
diff --git a/ironic/tests/unit/common/test_pxe_utils.py b/ironic/tests/unit/common/test_pxe_utils.py
index 2ae878edb..2c16e7eb7 100644
--- a/ironic/tests/unit/common/test_pxe_utils.py
+++ b/ironic/tests/unit/common/test_pxe_utils.py
@@ -1443,9 +1443,9 @@ class PXEBuildKickstartConfigOptionsTestCase(db_base.DbTestCase):
             expected['heartbeat_url'] = (
                 'http://ironic-api/v1/heartbeat/%s' % task.node.uuid
             )
-            ks_options = pxe_utils.build_kickstart_config_options(task)
-            self.assertTrue(ks_options.pop('agent_token'))
-            self.assertEqual(expected, ks_options)
+            params = pxe_utils.build_kickstart_config_options(task)
+            self.assertTrue(params['ks_options'].pop('agent_token'))
+            self.assertEqual(expected, params['ks_options'])
 
     @mock.patch('ironic.common.utils.render_template', autospec=True)
     def test_prepare_instance_kickstart_config_not_anaconda_boot(self,
@@ -1467,15 +1467,16 @@ class PXEBuildKickstartConfigOptionsTestCase(db_base.DbTestCase):
             'ks_cfg': ['', '/http_root/node_uuid/ks.cfg'],
             'ks_template': ['tmpl_id', '/http_root/node_uuid/ks.cfg.template']
         }
-        ks_options = {'liveimg_url': 'http://fake', 'agent_token': 'faketoken',
-                      'heartbeat_url': 'http://fake_hb'}
+        params = {'liveimg_url': 'http://fake', 'agent_token': 'faketoken',
+                  'heartbeat_url': 'http://fake_hb'}
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
-            ks_options_mock.return_value = ks_options
+            ks_options_mock.return_value = {'ks_options': params}
             pxe_utils.prepare_instance_kickstart_config(task, image_info,
                                                         anaconda_boot=True)
-            render_mock.assert_called_with(image_info['ks_template'][1],
-                                           ks_options)
+            render_mock.assert_called_with(
+                image_info['ks_template'][1], {'ks_options': params}
+            )
             write_mock.assert_called_with(image_info['ks_cfg'][1],
                                           render_mock.return_value)
 
diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py
index cf6c8484e..bfa662fa8 100644
--- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py
+++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py
@@ -1392,7 +1392,7 @@ class ValidateImagePropertiesTestCase(db_base.DbTestCase):
     @mock.patch.object(utils, 'get_boot_option', autospec=True,
                        return_value='kickstart')
     @mock.patch.object(image_service, 'get_image_service', autospec=True)
-    def test_validate_image_properties_glance_image_missing_squashfs_id(
+    def test_validate_image_properties_glance_image_missing_stage2_id(
             self, image_service_mock, boot_options_mock):
         inst_info = utils.get_image_instance_info(self.node)
         image_service_mock.return_value.show.return_value = {
diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py
index d1e2c2e7b..ce40b8dd8 100644
--- a/ironic/tests/unit/drivers/modules/test_pxe.py
+++ b/ironic/tests/unit/drivers/modules/test_pxe.py
@@ -236,7 +236,7 @@ class PXEBootTestCase(db_base.DbTestCase):
                               task.driver.boot.validate_inspection, task)
 
     @mock.patch.object(image_service.GlanceImageService, 'show', autospec=True)
-    def test_validate_kickstart_has_squashfs_id(self, mock_glance):
+    def test_validate_kickstart_missing_stage2_id(self, mock_glance):
         mock_glance.return_value = {'properties': {'kernel_id': 'fake-kernel',
                                                    'ramdisk_id': 'fake-initr'}}
         self.node.deploy_interface = 'anaconda'
@@ -244,7 +244,7 @@ class PXEBootTestCase(db_base.DbTestCase):
         self.config(http_url='http://fake_url', group='deploy')
         with task_manager.acquire(self.context, self.node.uuid) as task:
             self.assertRaisesRegex(exception.MissingParameterValue,
-                                   'squashfs_id',
+                                   'stage2_id',
                                    task.driver.boot.validate, task)
 
     def test_validate_kickstart_fail_http_url_not_set(self):
@@ -1011,7 +1011,9 @@ class PXEAnacondaDeployTestCase(db_base.DbTestCase):
                       'ks_cfg': ('', '/path/to/ks_cfg')}
         mock_image_info.return_value = image_info
         with task_manager.acquire(self.context, self.node.uuid) as task:
-            self.assertIsNone(task.driver.deploy.deploy(task))
+            self.assertEqual(
+                states.DEPLOYWAIT, task.driver.deploy.deploy(task)
+            )
             mock_image_info.assert_called_once_with(task, ipxe_enabled=False)
             mock_cache.assert_called_once_with(
                 task, image_info, ipxe_enabled=False)
@@ -1050,6 +1052,8 @@ class PXEAnacondaDeployTestCase(db_base.DbTestCase):
                       'ks_template': ('', '/path/to/ks_template'),
                       'ks_cfg': ('', '/path/to/ks_cfg')}
         mock_image_info.return_value = image_info
+        self.node.provision_state = states.DEPLOYWAIT
+        self.node.save()
         with task_manager.acquire(self.context, self.node.uuid) as task:
             task.driver.deploy.reboot_to_instance(task)
             mock_set_boot_dev.assert_called_once_with(task, boot_devices.DISK)
@@ -1112,6 +1116,17 @@ class PXEAnacondaDeployTestCase(db_base.DbTestCase):
                 task.node.driver_internal_info['agent_status'])
             self.assertTrue(mock_reboot_to_instance.called)
 
+    @mock.patch.object(deploy_utils, 'prepare_inband_cleaning', autospec=True)
+    def test_prepare_cleaning(self, prepare_inband_cleaning_mock):
+        prepare_inband_cleaning_mock.return_value = states.CLEANWAIT
+        self.node.provision_state = states.CLEANING
+        self.node.save()
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            self.assertEqual(
+                states.CLEANWAIT, self.deploy.prepare_cleaning(task))
+            prepare_inband_cleaning_mock.assert_called_once_with(
+                task, manage_boot=True)
+
 
 class PXEValidateRescueTestCase(db_base.DbTestCase):
 
diff --git a/releasenotes/notes/fix-anaconda-deploy-interface-bfa2cfca22b04680.yaml b/releasenotes/notes/fix-anaconda-deploy-interface-bfa2cfca22b04680.yaml
new file mode 100644
index 000000000..e791d7fdd
--- /dev/null
+++ b/releasenotes/notes/fix-anaconda-deploy-interface-bfa2cfca22b04680.yaml
@@ -0,0 +1,25 @@
+---
+fixes:
+  - |
+    Fixes a bug in the anaconda deploy interface where the 'ks_options'
+    key was not found when rendering the default kickstart template.
+  - |
+    Fixes issue where PXEAnacondaDeploy interface's deploy() method did not
+    return states.DEPLOYWAIT so the instance went straight to 'active' instead
+    of 'wait call-back'.
+  - |
+    Fixes an issue where the anaconda deploy interface mistakenly expected
+    'squashfs_id' instead of 'stage2_id'  property on the image.
+  - |
+    Fixes the heartbeat mechanism in the default kickstart template
+    ks.cfg.template as the heartbeat API only accepts 'POST' and expects a
+    mandatory 'callback_url' parameter.
+  - |
+    Fixes handling of tarball images in anaconda deploy interface. Allows user
+    specified file extensions to be appended to the disk image symlink. Users
+    can now set the file extensions by setting the 'disk_file_extension'
+    property on the OS image. This enables users to deploy tarballs with
+    anaconda deploy interface.
+  - |
+    Fixes issue where automated cleaning was not supported when anaconda deploy
+    interface is used.