Fixes #14920 - Support for booting UEFI machines #82

ManfredP · 2016-09-06T13:07:03Z

Hi, thanks for the module. I added the possibility to boot UEFI machines and have different boot loaders for different architectures. I did this by introducing a new parameter for the base class called bootfiles, which defaults to an empty hash. If you don't add anything to bootfiles everything stays the same, so the change will not break compatibility for existing users. The keys in bootfiles are the 16 bit architecture codes in hex and the values are the file names. The filename in pxefilename becomes the default which is used if no architecture code matches. In most of the cases you will want to set bootfiles to
'00:00': 'pxelinux.0'
'00:06': 'shim.efi'
'00:07': 'shim.efi'
'00:09': 'shim.efi'

mmoll · 2016-09-06T13:08:37Z

@lzap @timogoebel might be interesting for you

lzap · 2016-09-12T12:07:11Z

This essentially implements http://projects.theforeman.org/issues/14920 which is great.

I think we should provide sane defaults:

RHEL6

00:00 = pxelinux.0
00:07 and 00:09 = grub/grubx64.efi

RHEL7

00:00 = pxelinux.0
00:06 = grub2/shim.efi
00:07 and 00:09 = grub2/shim.efi

Debian/Ubuntu

00:00 = pxelinux.0
00:06 = grub2/bootia32.efi
00:07 and 00:09 = grub2/bootx64.efi

Also reword the commit message to Fixes #14920 - xxxx as it fully provides the feature described there.

Big +1 from me other than that, thanks!

lzap · 2016-09-12T12:09:02Z

For the record, here is the bit which deploys the files: https://github.com/theforeman/puppet-foreman_proxy/pull/276/files

ekohl · 2016-09-12T12:17:41Z

The puppet code looks fine so 👍 on that but I'm unfamiliar with the dhcp specifics so I'm trusting @lzap on that.

ManfredP · 2016-09-13T16:33:52Z

Thanks for the feedback, I changed the commit message and added the defaults, but I changed two things:

I added pxelinux.0 as a default for the pxefilename parameter, because it wouldn't make much sense to me to have defaults for the EFI loaders, and none for PXE.
I removed 00:00 from the default hashes, because pxelinux.0 is the default boot file, so it will be used if nothing matches.

lzap · 2016-09-14T09:18:27Z

I agree with the changes you made, looks good to me. This needs a puppet expert, I am not merging any Puppet code :-) Big thank you for this. @ekohl @mmoll ?

ekohl · 2016-09-14T09:26:26Z

manifests/params.pp

    }

    'RedHat': {
      $dhcp_dir    = '/etc/dhcp'
      $packagename = 'dhcp'
      $servicename = 'dhcpd'
      $root_group  = 'root'
+      if 0 + $::operatingsystemmajrelease > 6 {


We still avoid using this fact because older facter versions don't support it, but I haven't seen this workaround before. Will puppet automatically convert the string to an int?

Yes, it's a common workaround for "casting to int". However this does fail if the fact operatingsystemmajrelease is not available.

OK, I will change this to use the operatingsystemrelease fact.

ekohl · 2016-09-14T09:34:35Z

I added pxelinux.0 as a default for the pxefilename parameter, because it wouldn't make much sense to me to have defaults for the EFI loaders, and none for PXE.

This makes sense to me and I don't think it'll have any impact on users. The whole block is still behind an if @pxeserver which still has no default.

ManfredP · 2016-09-15T16:33:37Z

I changed all tests to use rspec-puppet-facts and changed the params.pp code to use $::operatingsystemrelease instead of $::operatingsystemmajrelease

ekohl

Other than one small thing I think this looks good to go.

ekohl · 2016-09-16T14:26:26Z

spec/classes/failover_spec.rb

+        let(:facts) do
+          facts.merge({
+            :concat_basedir => '/doesnotexist',
+            :domain         => 'example.org',


The default facts also have a domain which we can use. IMHO there's no need to override it.

Fully agree with that. I kept it, because I thought that the "fragment content check" also checks the domain name, but this is only the case in init_spec.rb. I will correct this.

ManfredP · 2016-09-16T17:02:26Z

Removed the override of the domain fact in failover_spec.rb

mmoll · 2016-09-20T20:13:30Z

Merged, danke @ManfredP!
I guess wel'll need an installer migration for $pxefilename?

lzap · 2016-09-21T08:16:28Z

I don't think so, when I install 1.13 stable RC1 with default installer options, this is what I get:

# PXE Handoff.
next-server 192.168.222.1;
filename "pxelinux.0";

log-facility local7;

include "/etc/dhcp/dhcpd.hosts";

# zzz.lan
subnet 192.168.222.0 netmask 255.255.255.0 {
  pool
  {
    range 192.168.222.2 192.168.222.200;
  }

  option subnet-mask 255.255.255.0;
  option routers 192.168.222.1;
}

The filename is set, so what migration would you expect?

ekohl · 2016-09-21T11:50:18Z

@lzap the migrations are for upgrades, so if you install 1.12 and then upgrade it to 1.13 with the installer then you have an old answer in your answers file.

domcleal · 2016-09-21T11:51:51Z

dhcp isn't a top-level installer module, the installer doesn't allow these parameters to be changed and doesn't store them.

ekohl · 2016-09-21T11:52:26Z

Good point.

lzap · 2016-09-21T13:28:42Z

@ekohl I understand but what I am trying to say is there is nothing to migrate, it currently renders the template with:

filename "pxelinux.0";

The patch does not change the behaviour. Please explain.

mmoll · 2016-09-21T13:42:36Z

before this change the $pxefilename paramater of the dhcp calss was set to "undef". If that's saved in the answer file (I'm unsure if undefs do get saved) and not re-evaluated, users updating from an older version will still have the undef value on the next installer run, even with an updated installer.

lzap · 2016-09-22T08:00:33Z

before this change the $pxefilename paramater of the dhcp calss was set to "undef". If that's saved in the answer file (I'm unsure if undefs do get saved) and not re-evaluated, users updating from an older version will still have the undef value on the next installer run, even with an updated installer.

Sure, I understand the process, but why I still have the correct entry
in the deployed dhcpd.conf on 1.12? What I am saying is the migration
could be useless.

Someone explain to me please.

Later,
Lukas #lzap Zapletal

ekohl · 2016-09-22T09:13:28Z

As @domcleal pointed out, the installer never sets this parameter. It's explicitly set here. So you're right that in this case the migration would not be needed.

lzap · 2016-09-22T10:01:00Z

As @domcleal pointed out, the installer never sets this parameter. It's explicitly set here. So you're right that in this case the migration would not be needed.

Aaah thanks, yay!

Later,
Lukas #lzap Zapletal

ManfredP · 2016-09-22T13:58:58Z

Thanks to everyone for the support and for merging this.

ekohl · 2016-09-23T07:57:19Z

Thanks for your contribution! As you could see it was already on the todo list.

ekohl reviewed Sep 14, 2016
View reviewed changes

ekohl approved these changes Sep 16, 2016

View reviewed changes

ekohl changed the title ~~Support for booting UEFI machines~~ Fixes #14920 - Support for booting UEFI machines Sep 16, 2016

ManfredP added 2 commits September 16, 2016 18:33

all tests now use rspec-puppet-facts

9155142

Fixes #14920 - support for bootfile per client architecture

f76d766

mmoll closed this in 6d01407 Sep 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #14920 - Support for booting UEFI machines #82

Fixes #14920 - Support for booting UEFI machines #82

ManfredP commented Sep 6, 2016

mmoll commented Sep 6, 2016

lzap commented Sep 12, 2016

lzap commented Sep 12, 2016

ekohl commented Sep 12, 2016

ManfredP commented Sep 13, 2016

lzap commented Sep 14, 2016

ekohl Sep 14, 2016

timogoebel Sep 14, 2016

ManfredP Sep 14, 2016

ekohl commented Sep 14, 2016

ManfredP commented Sep 15, 2016

ekohl left a comment

ekohl Sep 16, 2016

ManfredP Sep 16, 2016

ManfredP commented Sep 16, 2016

mmoll commented Sep 20, 2016

lzap commented Sep 21, 2016

ekohl commented Sep 21, 2016

domcleal commented Sep 21, 2016

ekohl commented Sep 21, 2016

lzap commented Sep 21, 2016

mmoll commented Sep 21, 2016

lzap commented Sep 22, 2016

ekohl commented Sep 22, 2016

lzap commented Sep 22, 2016

ManfredP commented Sep 22, 2016

ekohl commented Sep 23, 2016

Fixes #14920 - Support for booting UEFI machines #82

Fixes #14920 - Support for booting UEFI machines #82

Conversation

ManfredP commented Sep 6, 2016

mmoll commented Sep 6, 2016

lzap commented Sep 12, 2016

lzap commented Sep 12, 2016

ekohl commented Sep 12, 2016

ManfredP commented Sep 13, 2016

lzap commented Sep 14, 2016

ekohl Sep 14, 2016

Choose a reason for hiding this comment

timogoebel Sep 14, 2016

Choose a reason for hiding this comment

ManfredP Sep 14, 2016

Choose a reason for hiding this comment

ekohl commented Sep 14, 2016

ManfredP commented Sep 15, 2016

ekohl left a comment

Choose a reason for hiding this comment

ekohl Sep 16, 2016

Choose a reason for hiding this comment

ManfredP Sep 16, 2016

Choose a reason for hiding this comment

ManfredP commented Sep 16, 2016

mmoll commented Sep 20, 2016

lzap commented Sep 21, 2016

ekohl commented Sep 21, 2016

domcleal commented Sep 21, 2016

ekohl commented Sep 21, 2016

lzap commented Sep 21, 2016

mmoll commented Sep 21, 2016

lzap commented Sep 22, 2016

ekohl commented Sep 22, 2016

lzap commented Sep 22, 2016

ManfredP commented Sep 22, 2016

ekohl commented Sep 23, 2016