From 06cc5d47dcdc49193e5e4e748471eb0b9ae97bbb Mon Sep 17 00:00:00 2001 From: Ruby Loo Date: Fri, 4 Feb 2022 21:26:26 +0000 Subject: [PATCH] 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. Change-Id: I953e948bcfa108d4c8e7b145da2f52b652e52a10 --- ironic/common/pxe_utils.py | 82 ++++++++++--------- ironic/drivers/modules/ks.cfg.template | 6 +- ironic/drivers/modules/pxe.py | 7 +- ironic/tests/unit/drivers/modules/test_pxe.py | 10 ++- ...da-deploy-more-fixes-58d996c7031c8c4b.yaml | 33 ++++++++ 5 files changed, 93 insertions(+), 45 deletions(-) create mode 100644 releasenotes/notes/anaconda-deploy-more-fixes-58d996c7031c8c4b.yaml diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py index e87f731855..c2d97df62f 100644 --- a/ironic/common/pxe_utils.py +++ b/ironic/common/pxe_utils.py @@ -668,42 +668,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] = ( @@ -1090,16 +1100,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) @@ -1197,17 +1208,14 @@ def cache_ramdisk_kernel(task, pxe_info, ipxe_enabled=False): else: path = os.path.join(CONF.pxe.tftp_root, node.uuid) 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. - ensure_tree( - get_file_path_from_label( - node.uuid, - CONF.deploy.http_root, - 'stage2' - ) - ) + # stage2 will be stored in ipxe http directory so make sure the + # directory exists. + file_path = get_file_path_from_label(node.uuid, + CONF.deploy.http_root, + 'stage2') + 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 40552377be..f7ef75e1cd 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 50d962fcf6..6eccc1938c 100644 --- a/ironic/drivers/modules/pxe.py +++ b/ironic/drivers/modules/pxe.py @@ -85,6 +85,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) @@ -123,13 +126,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 e1559b6f69..d999a8f7ae 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/ironic/tests/unit/drivers/modules/test_pxe.py @@ -1022,15 +1022,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 0000000000..9298c30363 --- /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.