From d7d0f7042e5828c2da2ae7b91444feebcd0835d8 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Wed, 23 Jun 2021 08:32:56 -0700 Subject: [PATCH] Clean up ipxe support The puppet-ironic module's ipxe support does not align with current versions of ironic which delineated ipxe and pxe support quite some time ago. This is an attempt to reconcile this. Conflicts: manifests/pxe.pp Backport notes: - This change includes the following follow-up commits. Make ipxe bootfile name configurable commit 918b3116634d1be812aa5e0359eff9119ffe87a6 Fix incorrect handling/update about deprecated parameters commit b307c40f353d6f8e965e9c9d52382fe7b0145043 Fix usage of ironic::pxe::common commit c21b1e8db6ece50f19c09b38ead3f7d5ec84249c - This change keeps default of uefi_ipxe_bootfile_name to ipxe.efi to be aligned with the default in ironic stable/wallaby (and older). - The ip_version parameter is not depecated by this backport because the parameter is still valid. - The change in default values were reverted as much as possible. Change-Id: Ia30aff290ec24972f387612851f8f630ddc9403b (cherry picked from commit 3864e15998b5b1eec7d2b1b4911add9bb899fdb8) --- manifests/drivers/pxe.pp | 72 +++++++++++++------ manifests/pxe.pp | 27 ++++--- .../ipxe_bootfile_name-4f4dbc874f1c2e63.yaml | 6 ++ .../notes/pxe-cleanup-692c78cad322893d.yaml | 38 ++++++++++ spec/classes/ironic_drivers_pxe_spec.rb | 36 +++------- 5 files changed, 120 insertions(+), 59 deletions(-) create mode 100644 releasenotes/notes/ipxe_bootfile_name-4f4dbc874f1c2e63.yaml create mode 100644 releasenotes/notes/pxe-cleanup-692c78cad322893d.yaml diff --git a/manifests/drivers/pxe.pp b/manifests/drivers/pxe.pp index ce4e2daa..341df418 100644 --- a/manifests/drivers/pxe.pp +++ b/manifests/drivers/pxe.pp @@ -19,25 +19,31 @@ # # === Parameters # -# [*ipxe_enabled*] -# (optional) Enable ipxe support -# Defaults to false. -# # [*pxe_append_params*] # (optional) Additional append parameters for baremetal PXE boot. # Should be valid pxe parameters -# Defaults to $::os_service_default +# Defaults to $::os_service_default. # # [*pxe_bootfile_name*] # (optional) Bootfile DHCP parameter. -# If not set, its value is detected based on ipxe_enabled. -# Defaults to undef. +# If not set, its value is detected. +# Defaults to $::os_service_default. # # [*pxe_config_template*] # (optional) Template file for PXE configuration. -# If set, should be an valid template file. Otherwise, its value is detected -# based on ipxe_enabled. -# Defaults to undef. +# If set, should be an valid template file. Otherwise, its value is detected. +# Defaults to $::os_service_default. +# +# [*ipxe_bootfile_name*] +# (optional) Bootfile DHCP parameter when the ipxe boot interface is set +# for a baremetal node. If not set, its value is detected. +# Defaults to $::os_service_default. +# +# [*ipxe_config_template*] +# (optional) Template file for PXE configuration with the iPXE boot +# interface. If set, should be an valid template file. Otherwise, +# its value is detected. +# Defaults to $::os_service_default. # # [*tftp_server*] # (optional) IP address of Ironic compute node's tftp server. @@ -72,6 +78,12 @@ # (optional) Template file for PXE configuration for UEFI boot loader. # Defaults to $::os_service_default. # +# [*uefi_ipxe_bootfile_name*] +# (optional) Bootfile DHCP parameter for UEFI boot mode for the +# ipxe boot interface. No separate configuration template is required +# when using ipxe. +# Defaults to ipxe.efi +# # [*ipxe_timeout*] # (optional) ipxe timeout in second. # Should be an valid integer @@ -93,11 +105,20 @@ # (optional) The IP version that will be used for PXE booting. # Defaults to $::os_service_default. # +# DEPRECATED PARAMETERS +# +# [*ipxe_enabled*] +# DEPRECATED: This option is no longer used as support for the option was +# deprecated during Ironic's Stein development cycle and removed during +# Ironic's Train development cycle. +# If this setting is populated, a warning will be indicated. +# class ironic::drivers::pxe ( - $ipxe_enabled = false, $pxe_append_params = $::os_service_default, - $pxe_bootfile_name = undef, - $pxe_config_template = undef, + $pxe_bootfile_name = $::os_service_default, + $pxe_config_template = $::os_service_default, + $ipxe_bootfile_name = $::os_service_default, + $ipxe_config_template = $::os_service_default, $tftp_server = $::os_service_default, $tftp_root = '/tftpboot', $images_path = $::os_service_default, @@ -105,11 +126,14 @@ class ironic::drivers::pxe ( $instance_master_path = $::os_service_default, $uefi_pxe_bootfile_name = $::os_service_default, $uefi_pxe_config_template = $::os_service_default, + $uefi_ipxe_bootfile_name = 'ipxe.efi', $ipxe_timeout = $::os_service_default, $enable_ppc64le = false, $boot_retry_timeout = $::os_service_default, $boot_retry_check_interval = $::os_service_default, $ip_version = $::os_service_default, + # DEPRECATED PARAMETERS + $ipxe_enabled = undef, ) { include ironic::deps @@ -117,19 +141,17 @@ class ironic::drivers::pxe ( $tftp_root_real = pick($::ironic::pxe::common::tftp_root, $tftp_root) $ipxe_timeout_real = pick($::ironic::pxe::common::ipxe_timeout, $ipxe_timeout) - if $ipxe_enabled { - $pxe_bootfile_name_real = pick($pxe_bootfile_name, 'undionly.kpxe') - $pxe_config_template_real = pick($pxe_config_template, '$pybasedir/drivers/modules/ipxe_config.template') - } else { - $pxe_bootfile_name_real = pick($pxe_bootfile_name, 'pxelinux.0') - $pxe_config_template_real = pick($pxe_config_template, '$pybasedir/drivers/modules/pxe_config.template') + if $ipxe_enabled != undef { + warning('The ironic::drivers::pxe::ipxe_enabled parameter is deprecated and has no effect.') } # Configure ironic.conf ironic_config { 'pxe/pxe_append_params': value => $pxe_append_params; - 'pxe/pxe_bootfile_name': value => $pxe_bootfile_name_real; - 'pxe/pxe_config_template': value => $pxe_config_template_real; + 'pxe/pxe_bootfile_name': value => $pxe_bootfile_name; + 'pxe/pxe_config_template': value => $pxe_config_template; + 'pxe/ipxe_bootfile_name': value => $ipxe_bootfile_name; + 'pxe/ipxe_config_template': value => $ipxe_config_template; 'pxe/tftp_server': value => $tftp_server; 'pxe/tftp_root': value => $tftp_root_real; 'pxe/images_path': value => $images_path; @@ -137,6 +159,7 @@ class ironic::drivers::pxe ( 'pxe/instance_master_path': value => $instance_master_path; 'pxe/uefi_pxe_bootfile_name': value => $uefi_pxe_bootfile_name; 'pxe/uefi_pxe_config_template': value => $uefi_pxe_config_template; + 'pxe/uefi_ipxe_bootfile_name': value => $uefi_ipxe_bootfile_name; 'pxe/ipxe_timeout': value => $ipxe_timeout_real; 'pxe/boot_retry_timeout': value => $boot_retry_timeout; 'pxe/boot_retry_check_interval': value => $boot_retry_check_interval; @@ -150,7 +173,12 @@ class ironic::drivers::pxe ( # architecture ironic_config { # NOTE(tonyb): This first value shouldn't be needed but seems to be? - 'pxe/pxe_config_template_by_arch': value => "ppc64le:${pxe_config_template_real}"; + # NOTE(TheJulia): Likely not needed as this just points to the default, + # and when the explicit pxe driver is used everything should fall to + # it but in the interest of minimizing impact, the output result + # is preserved as we now just allow the default for normal template + # operation to be used. + 'pxe/pxe_config_template_by_arch': value => 'ppc64le:$pybasedir/drivers/modules/pxe_config.template'; 'pxe/pxe_bootfile_name_by_arch': value => 'ppc64le:config'; } } diff --git a/manifests/pxe.pp b/manifests/pxe.pp index 3fea90df..5527995f 100644 --- a/manifests/pxe.pp +++ b/manifests/pxe.pp @@ -57,16 +57,23 @@ # in the source file being /usr/share/ipxe/ipxe-snponly-x86_64.efi # Defaults to 'ipxe' # +# [*ipxe_bootfile_name*] +# (optional) Name of efi file used to boot servers with iPXE + UEFI. This +# should be consistent with the uefi_ipxe_bootfile_name parameter in pxe +# driver. +# Defaults to 'ipxe.efi' +# class ironic::pxe ( - $package_ensure = 'present', - $tftp_root = '/tftpboot', - $http_root = '/httpboot', - $http_port = '8088', - $syslinux_path = $::ironic::params::syslinux_path, - $syslinux_files = $::ironic::params::syslinux_files, - $tftp_bind_host = undef, - $enable_ppc64le = false, - $ipxe_name_base = 'ipxe', + $package_ensure = 'present', + $tftp_root = '/tftpboot', + $http_root = '/httpboot', + $http_port = '8088', + $syslinux_path = $::ironic::params::syslinux_path, + $syslinux_files = $::ironic::params::syslinux_files, + $tftp_bind_host = undef, + $enable_ppc64le = false, + $ipxe_name_base = 'ipxe', + $ipxe_bootfile_name = 'ipxe.efi' ) inherits ::ironic::params { include ironic::deps @@ -181,7 +188,7 @@ class ironic::pxe ( require => Anchor['ironic-inspector::install::end'], } - file { "${tftp_root_real}/ipxe.efi": + file { "${tftp_root_real}/${ipxe_bootfile_name}": ensure => 'file', seltype => 'tftpdir_t', owner => 'ironic', diff --git a/releasenotes/notes/ipxe_bootfile_name-4f4dbc874f1c2e63.yaml b/releasenotes/notes/ipxe_bootfile_name-4f4dbc874f1c2e63.yaml new file mode 100644 index 00000000..013a48bb --- /dev/null +++ b/releasenotes/notes/ipxe_bootfile_name-4f4dbc874f1c2e63.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + The new ``ironic::pxe::ipxe_bootfile_name`` parameter has been added. This + parameter is used to determine name of the efi file used to boot nodes with + UEFI + iPXE. diff --git a/releasenotes/notes/pxe-cleanup-692c78cad322893d.yaml b/releasenotes/notes/pxe-cleanup-692c78cad322893d.yaml new file mode 100644 index 00000000..83f7c6b5 --- /dev/null +++ b/releasenotes/notes/pxe-cleanup-692c78cad322893d.yaml @@ -0,0 +1,38 @@ +--- +features: + - Adds support for the ``ironic.conf`` parameters ``[pxe]ipxe_bootfile_name`` + and ``[pxe]ipxe_config_template`` which are utilized by the ``ipxe`` boot + interface in Ironic. These settings use the manifest parameters + ``ironic::drivers::pxe::ipxe_bootfile_name`` and + ``ironic::drivers::pxe::ipxe_config_template`` respectively. + - Adds support for the ``ironic.conf`` parameter + ``[pxe]uefi_ipxe_bootfile_name`` to be explicitly set using the + ``ironic::drivers::pxe::uefi_ipxe_bootfile_name`` manifest parameter. +upgrade: + - | + The Ironic project has in elevated support for ``ipxe`` to a top level + node boot_interface, and removed support for it's ``[pxe]ipxe_enabled`` + option as this is settable per node. Removal of this option has allowed + for the manifest to be cleaned up. + - The manifest now no longer applies override defaults for a number of + ``ironic.conf`` settings, as they match the default settings the project + utilizes. These manifest parameters are + ``ironic::drivers::pxe::pxe_bootfile_name``, + ``ironic::drivers::pxe::pxe_config_template``, + ``ironic::drivers::pxe::tftp_root``, + ``ironic::drivers::pxe::tftp_master_path``. +deprecations: + - | + The ``ironic::drivers::pxe::ipxe_enabled`` parameter has been deprecated + and has no effect moving forward. This is a result of the underlying + Ironic project deprecating and removing. Use of this option will raise + a warning. +fixes: + - | + Fixes a potential issue where use of the + ``ironic::drivers::pxe::ipxe_enabled`` parameter would + cause settings specific to ``ipxe`` to be set overriding the ``pxe`` boot + interface's defaults in Ironic. This resulted in operators being unable to + choose to boot a node from ``pxe`` and not ``ipxe``. Now the settings are + not overriden as the ``ironic::drivers::pxe::ipxe_enabled`` parameter for + the manifest has no effect, and raises a warning if used. diff --git a/spec/classes/ironic_drivers_pxe_spec.rb b/spec/classes/ironic_drivers_pxe_spec.rb index 4137be20..b8c81575 100644 --- a/spec/classes/ironic_drivers_pxe_spec.rb +++ b/spec/classes/ironic_drivers_pxe_spec.rb @@ -23,10 +23,9 @@ require 'spec_helper' describe 'ironic::drivers::pxe' do let :default_params do - { :pxe_bootfile_name => 'pxelinux.0', - :pxe_config_template => '$pybasedir/drivers/modules/pxe_config.template', - :tftp_root => '/tftpboot', + { :tftp_root => '/tftpboot', :tftp_master_path => '/tftpboot/master_images', + :uefi_ipxe_bootfile_name => 'ipxe.efi', } end @@ -41,8 +40,10 @@ describe 'ironic::drivers::pxe' do it 'configures ironic.conf' do is_expected.to contain_ironic_config('pxe/pxe_append_params').with_value('') - is_expected.to contain_ironic_config('pxe/pxe_bootfile_name').with_value(p[:pxe_bootfile_name]) - is_expected.to contain_ironic_config('pxe/pxe_config_template').with_value(p[:pxe_config_template]) + is_expected.to contain_ironic_config('pxe/pxe_bootfile_name').with_value('') + is_expected.to contain_ironic_config('pxe/pxe_config_template').with_value('') + is_expected.to contain_ironic_config('pxe/ipxe_bootfile_name').with_value('') + is_expected.to contain_ironic_config('pxe/ipxe_config_template').with_value('') is_expected.to contain_ironic_config('pxe/tftp_server').with_value('') is_expected.to contain_ironic_config('pxe/tftp_root').with_value(p[:tftp_root]) is_expected.to contain_ironic_config('pxe/images_path').with_value('') @@ -50,30 +51,10 @@ describe 'ironic::drivers::pxe' do is_expected.to contain_ironic_config('pxe/instance_master_path').with_value('') is_expected.to contain_ironic_config('pxe/uefi_pxe_bootfile_name').with_value('') is_expected.to contain_ironic_config('pxe/uefi_pxe_config_template').with_value('') + is_expected.to contain_ironic_config('pxe/uefi_ipxe_bootfile_name').with_value('ipxe.efi') is_expected.to contain_ironic_config('pxe/ipxe_enabled').with_ensure('absent') end - context 'when overriding only ipxe_enabled' do - before do - params.merge!( - :ipxe_enabled => true, - ) - end - - it 'detects correct boot parameters' do - is_expected.to contain_ironic_config('pxe/pxe_append_params').with_value('') - is_expected.to contain_ironic_config('pxe/pxe_bootfile_name').with_value('undionly.kpxe') - is_expected.to contain_ironic_config('pxe/pxe_config_template').with_value('$pybasedir/drivers/modules/ipxe_config.template') - is_expected.to contain_ironic_config('pxe/tftp_server').with_value('') - is_expected.to contain_ironic_config('pxe/tftp_root').with_value(p[:tftp_root]) - is_expected.to contain_ironic_config('pxe/images_path').with_value('') - is_expected.to contain_ironic_config('pxe/tftp_master_path').with_value(p[:tftp_master_path]) - is_expected.to contain_ironic_config('pxe/instance_master_path').with_value('') - is_expected.to contain_ironic_config('pxe/uefi_pxe_bootfile_name').with_value('') - is_expected.to contain_ironic_config('pxe/uefi_pxe_config_template').with_value('') - end - end - context 'when overriding only enable_ppc64le' do before do params.merge!( @@ -98,9 +79,9 @@ describe 'ironic::drivers::pxe' do :tftp_master_path => '/mnt/master_images', :instance_master_path => '/mnt/ironic/master_images', :uefi_pxe_bootfile_name => 'bootx64.efi', + :uefi_ipxe_bootfile_name => 'snponly.efi', :uefi_pxe_config_template => 'foo-uefi', :ipxe_timeout => '60', - :ipxe_enabled => true, :pxe_bootfile_name => 'bootx64', :boot_retry_timeout => 600, :boot_retry_check_interval => 120, @@ -118,6 +99,7 @@ describe 'ironic::drivers::pxe' do is_expected.to contain_ironic_config('pxe/instance_master_path').with_value(p[:instance_master_path]) is_expected.to contain_ironic_config('pxe/uefi_pxe_bootfile_name').with_value(p[:uefi_pxe_bootfile_name]) is_expected.to contain_ironic_config('pxe/uefi_pxe_config_template').with_value(p[:uefi_pxe_config_template]) + is_expected.to contain_ironic_config('pxe/uefi_ipxe_bootfile_name').with_value(p[:uefi_ipxe_bootfile_name]) is_expected.to contain_ironic_config('pxe/ipxe_timeout').with_value(p[:ipxe_timeout]) is_expected.to contain_ironic_config('pxe/pxe_bootfile_name').with_value(p[:pxe_bootfile_name]) is_expected.to contain_ironic_config('pxe/boot_retry_timeout').with_value(p[:boot_retry_timeout])