Merge ~alkisg/ubuntu/+source/ipxe:lp1811496 into ubuntu/+source/ipxe:ubuntu/devel

[Alkis Georgopoulos]

c93d8f5... by Alkis Georgopoulos on 2019-12-16

Save default entry when iPXE is selected

[Christian Ehrhardt  (paelzer)]

Hi Alkis,
if no one beats me to handle it I'll be taking a look at ipxe in January anyway and will consider this MP then.

[Alkis Georgopoulos (alkisg)]

The main goal is to have this in 20.04, so January is great (maybe Debian will also have it by then).
Thank you Christian!

[Rafael David Tinoco (rafaeldtinoco)]

Hello Alkis, I'm reviewing this and will give it a try (and test).

Based on:

https://bugs.launchpad.net/ubuntu/+source/ipxe/+bug/1811496/comments/18

where is the "save_default_entry" entries ?

I'll review the main functionality, meanwhile, could you re-push this with that change ?

Thank you!

[Alkis Georgopoulos (alkisg)]

Thank you Rafael,

save_default_entries is an internal function provided by /usr/share/grub/grub-mkconfig to grub scripts. AFAIK it's the official way to implement the save default functionality:

$ grep -r save_default_entry /usr/share/grub/ /etc/grub.d
/usr/share/grub/grub-mkconfig_lib:save_default_entry ()
/etc/grub.d/30_os-prober: save_default_entry | grub_add_tab
/etc/grub.d/30_os-prober: save_default_entry | grub_add_tab
/etc/grub.d/30_os-prober: save_default_entry | sed -e "s/^/\t/"
/etc/grub.d/30_os-prober: save_default_entry | grub_add_tab
/etc/grub.d/30_os-prober: save_default_entry | sed -e "s/^/$grub_tab$grub_tab/"
/etc/grub.d/30_os-prober: save_default_entry | grub_add_tab
/etc/grub.d/30_os-prober: save_default_entry | sed -e "s/^/\t/"
/etc/grub.d/20_linux_xen: save_default_entry | grub_add_tab | sed "s/^/$submenu_indentation/"
/etc/grub.d/10_linux: save_default_entry | grub_add_tab
/etc/grub.d/20_ipxe: save_default_entry | grub_add_tab

[Rafael David Tinoco (rafaeldtinoco)]

Hey Alkis,

Yep, I know, I failed to see your merge proposal had it included:

/etc/grub.d/20_ipxe: save_default_entry | grub_add_tab

...
+ save_default_entry | grub_add_tab
...

my bad, thanks for the reply!

[Rafael David Tinoco (rafaeldtinoco)]

Alright, I have tested your change and, despite a missing changelog entry, it makes total sense. I haven't created a PXE boot entirely, but, instead, I sniffed the virtio interface of the VM using the EFI menu entry:

19:29:32.878687 IP 0.0.0.0.68 > 255.255.255.255.67: BOOTP/DHCP, Request from 52:54:00:dd:d0:18, length 406
19:29:32.878741 IP6 fe80::5054:ff:fedd:d018 > ff02::2: ICMP6, router solicitation, length 16
19:29:32.878768 IP6 fe80::216:3eff:fe3f:8b66 > ff02::1:ff00:0: ICMP6, neighbor solicitation, who has ::, length 32
19:29:32.878777 IP6 fe80::216:3eff:fe80:bcd9 > ff02::1:ff00:0: ICMP6, neighbor solicitation, who has ::, length 32
19:29:32.878891 IP 10.250.99.1.67 > 10.250.99.220.68: BOOTP/DHCP, Reply, length 300
19:29:33.144534 IP6 fe80::5054:ff:fedd:d018 > ff02::2: ICMP6, router solicitation, length 16
19:29:33.693789 IP6 fe80::5054:ff:fedd:d018 > ff02::2: ICMP6, router solicitation, length 16
19:29:33.913529 IP 0.0.0.0.68 > 255.255.255.255.67: BOOTP/DHCP, Request from 52:54:00:dd:d0:18, length 406
19:29:33.913714 IP 10.250.99.1.67 > 10.250.99.220.68: BOOTP/DHCP, Reply, length 300
19:29:34.737371 IP6 fe80::5054:ff:fedd:d018 > ff02::2: ICMP6, router solicitation, length 16
19:29:35.945745 IP 0.0.0.0.68 > 255.255.255.255.67: BOOTP/DHCP, Request from 52:54:00:dd:d0:18, length 418
19:29:35.945973 IP 10.250.99.1.67 > 10.250.99.220.68: BOOTP/DHCP, Reply, length 300
19:29:35.946042 ARP, Request who-has 10.250.99.220 tell 10.250.99.220, length 28
19:29:36.769615 IP6 fe80::5054:ff:fedd:d018 > ff02::2: ICMP6, router solicitation, length 16
19:29:40.432536 IP6 fe80::216:3eff:fe3f:8b66 > ff02::2: ICMP6, router solicitation, length 16
19:29:40.834099 IP6 fe80::5054:ff:fedd:d018 > ff02::2: ICMP6, router solicitation, length 16
19:29:41.033953 ARP, Request who-has 10.250.99.220 tell 10.250.99.1, length 28
19:29:41.034082 ARP, Reply 10.250.99.220 is-at 52:54:00:dd:d0:18, length 30

And it got the IP but no file was offered (As I didnt offer one).

One thing to notice:

I had serial=console in my grub and it doesn't work with ttyS0 as output (I know its obvious, but, it looked like it didnt work in my setup in an initial look). After having a VNC graphics set, I could see the iPXE 20190109.133f4c4-0ubuntu3 (the version I created) in the console before the PXE attempt.

So, I'm +1 on this code change.

I'll update SALSA merge request as well to see if we accelerate merge over there. Nevertheless, on our side, I think its great to add this change.

review: Approve

[Rafael David Tinoco (rafaeldtinoco)]

Alkis,

Would you mind creating a debian/changelog entry... example:

commit 360c9d8 (HEAD -> lp1811496)
Author: Rafael David Tinoco <email address hidden>
Date: Mon Jan 6 19:00:07 2020

    changelog

diff --git a/debian/changelog b/debian/changelog
index c11bbfa..8f4b12e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+ipxe (1.0.0+git-20190109.133f4c4-0ubuntu3) focal; urgency=medium
+
+ [Alkis Georgopoulos]
+ * Use ipxe.efi under UEFI (LP: #1811496)
+ * Save default entry when iPXE is selected
+
+ -- Rafael David Tinoco <email address hidden> Mon, 06 Jan 2020 18:59:25 +0000
+
 ipxe (1.0.0+git-20190109.133f4c4-0ubuntu2) disco; urgency=medium

   * Add e1000e firmware for qemu. (closes: #884240)

and re-pushing your change to this branch ?

This way I can ask someone to sponsor the push to usd-importer ?

I have upload rights for this package but won't upload until usd-importer git branch is also updated.

Thanks a lot!

Rafael

[Rafael David Tinoco (rafaeldtinoco)]

Ah, and could you add, again, as reviewers:

canonical-server
paelzer

So its back in our queue and christian can sponsor the push ?

Thank you!

3850978... by Alkis Georgopoulos on 2020-01-06

changelog: Use ipxe.efi under UEFI

[Alkis Georgopoulos (alkisg)]

I added a changelog entry and I requested review from canonical-server and paelzer.

Thank you Rafael!

[Christian Ehrhardt  (paelzer)]

Thanks Rafael and Alkis, now this is ready for pickup once I'm through the rest of the virst stack.

[Christian Ehrhardt  (paelzer)]

review: Approve

[Rafael David Tinoco (rafaeldtinoco)]

I'm merging this fix together with fix for: LP: #1858374 in a new merge request, respecting credits, and will inform it here.

[Bryce Harrington (bryce)]

Running shellcheck on 20_ipxe would likely suggest some stylistic improvements such as quoting and bracketing of variables. I think that's good practice, but not critical.

I'm not keen on splitting the heredoc with the `save_default_entry | grub_add_tab` call in the middle, but there's nothing technically wrong with it. I might try doing something like:

   echo "Found iPXE image: $IPXE" >&2
   entry=$(save_default_entry | grub_add_tab)
   cat << EOF
menuentry "Network boot (iPXE)" --users "" --class network {
${prepare_boot_cache}
${entry}
 if [ "\$grub_platform" = "efi" ]; then
  chainloader $IPXEPATH.efi
 else
  linux16 $IPXEPATH.lkrn
  # If the user provided an iPXE script, load it
  if [ -f $IPXEPATH.ipxe ]; then
   initrd16 $IPXEPATH.ipxe
  fi
 fi
 }
 EOF
 fi

Unfortunately I don't know enough about ipxe to provide meaningful feedback on the grub code, but presuming it works in testing, the syntax looks ok.

review: Approve

[Alkis Georgopoulos (alkisg)]

Hi, regarding "EOF", I imitated what the other scripts in /etc/grub.d where doing for two reasons:
One is for code consistency, and the second is that I think they're doing that to avoid an empty line if ${entry} is completely empty. It's possible to avoid empty lines while using variables in HEREDOCs, but it's less readable.

The shellcheck error about the unquoted ${GRUB_DEVICE_BOOT} isn't part of the diff, we didn't touch it...

Thanks!

[Christian Ehrhardt  (paelzer)]

Hi,
I'm good to follow the "code consistency" argument here.

FYI - Rafael combined it with another iPXE fix in a new MP for an upload to Focal.
See - https://code.launchpad.net/~rafaeldtinoco/ubuntu/+source/ipxe/+git/ipxe/+merge/377540

Once that one closes (soon I'd hope) we can also close this MP here as merged.

[Christian Ehrhardt  (paelzer)]

I'm merging the MP that Rafael used to combine the two fixes now.

[Christian Ehrhardt  (paelzer)]

https://launchpad.net/ubuntu/+source/ipxe/1.0.0+git-20190109.133f4c4-0ubuntu3 started to build, lets consider this one merged as well.

diff --git a/debian/changelog b/debian/changelog
index c11bbfa..557c249 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+ipxe (1.0.0+git-20190109.133f4c4-0ubuntu3) focal; urgency=medium
+
+  [Alkis Georgopoulos]
+  * Use ipxe.efi under UEFI (LP: #1811496)
+  * Save default entry when iPXE is selected
+
+ -- Rafael David Tinoco <[e-mail]> Mon, 06 Jan 2020 18:59:25 +0000
+
 ipxe (1.0.0+git-20190109.133f4c4-0ubuntu2) disco; urgency=medium
 
   * Add e1000e firmware for qemu. (closes: #884240)
diff --git a/debian/tree/ipxe/etc/grub.d/20_ipxe b/debian/tree/ipxe/etc/grub.d/20_ipxe
index 8be37c8..8f22d13 100755
--- a/debian/tree/ipxe/etc/grub.d/20_ipxe
+++ b/debian/tree/ipxe/etc/grub.d/20_ipxe
@@ -15,11 +15,24 @@ IPXE=/boot/ipxe.lkrn
 
 if test -e "$IPXE" ; then
   IPXEPATH=$( make_system_path_relative_to_its_root "$IPXE" )
+  # Remove the .lkrn extension
+  IPXEPATH=${IPXEPATH%.lkrn}
   echo "Found iPXE image: $IPXE" >&2
   cat << EOF
 menuentry "Network boot (iPXE)" --users "" --class network {
 ${prepare_boot_cache}
-	linux16 $IPXEPATH
+EOF
+  save_default_entry | grub_add_tab
+  cat << EOF
+	if [ "\$grub_platform" = "efi" ]; then
+		chainloader $IPXEPATH.efi
+	else
+		linux16 $IPXEPATH.lkrn
+		# If the user provided an iPXE script, load it
+		if [ -f $IPXEPATH.ipxe ]; then
+			initrd16 $IPXEPATH.ipxe
+		fi
+	fi
 }
 EOF
 fi