From b565855250f4ff8a41ad58a997d26185ad35b6a4 Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Fri, 7 Jan 2022 12:07:09 -0500 Subject: [PATCH 1/6] 35coreos-ignition: skip reboot if changed kargs match current boot If the requested kernel arguments in the Ignition config already match the kernel arguments of the currently booted system then let's skip the reboot because the reboot won't change anything. One example of a use of this would be if someone is doing a PXE install and they want to persistently use `net.ifnames=0`. They apply `net.ifnames=0` on the PXE boot and coreos-installer transparently forwards it to the Ignition boot (for a single boot). Then the Ignition config has `net.ifnames=0` set in the kernel arguments section. ignition-kargs.service will take care of setting it persistently, but without this change the system will be rebooted. With this change we skip the reboot. --- .../35coreos-ignition/coreos-kargs.sh | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-kargs.sh b/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-kargs.sh index 1dcc3add13..9b7aebc1b5 100755 --- a/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-kargs.sh +++ b/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-kargs.sh @@ -1,12 +1,34 @@ #!/bin/bash set -euo pipefail +# First check to see if the requested state is satisfied by the current boot +/usr/bin/rdcore kargs --current --create-if-changed /run/coreos-kargs-thisboot-differ "$@" + if is-live-image; then - /usr/bin/rdcore kargs --current --create-if-changed /run/coreos-kargs-changed "$@" - if [ -e /run/coreos-kargs-changed ]; then + # If we're in a live system and the kargs don't match then we must error. + if [ -e /run/coreos-kargs-thisboot-differ ]; then + # Since we exit with error here the stderr will get shown by Ignition echo "Need to modify kernel arguments, but cannot affect live system." >&2 exit 1 fi else - /usr/bin/rdcore kargs --boot-device /dev/disk/by-label/boot --create-if-changed /run/coreos-kargs-reboot "$@" + /usr/bin/rdcore kargs --boot-device /dev/disk/by-label/boot --create-if-changed /run/coreos-kargs-changed "$@" + # If the bootloader was changed and the kernel arguments don't match this boot + # then we must reboot. If they do match this boot then we can skip the reboot. + # + # Note we write info messages to kmsg here because stdout gets swallowed + # by Ignition if there is no failure. This forces the info into the journal, + # but sometimes the journal will miss these messages because of ratelimiting. + # We've decided to accept this limitation rather than add the systemd-cat or + # logger utlities to the initramfs. + if [ -e /run/coreos-kargs-changed ]; then + if [ -e /run/coreos-kargs-thisboot-differ ]; then + msg="Kernel arguments were changed. Requesting reboot." + echo "$msg" > /dev/kmsg + touch /run/coreos-kargs-reboot + else + msg="Kernel arguments were changed, but they match this boot. Skipping reboot." + echo "$msg" > /dev/kmsg + fi + fi fi From a18120a5d392edba331b715844ff9d44790158e3 Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Wed, 2 Feb 2022 10:09:29 -0500 Subject: [PATCH 2/6] 35coreos-ignition: handle the case where booted kargs don't match desired Today when you boot a system it's possible a value for a karg you specified in the Ignition config matches the BLS config but not the kargs from the current boot (in /proc/cmdline). Let's detect this and reboot in that case too. --- .../35coreos-ignition/coreos-kargs.sh | 69 ++++++++++++++----- 1 file changed, 50 insertions(+), 19 deletions(-) diff --git a/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-kargs.sh b/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-kargs.sh index 9b7aebc1b5..655689673a 100755 --- a/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-kargs.sh +++ b/overlay.d/05core/usr/lib/dracut/modules.d/35coreos-ignition/coreos-kargs.sh @@ -1,34 +1,65 @@ #!/bin/bash set -euo pipefail -# First check to see if the requested state is satisfied by the current boot -/usr/bin/rdcore kargs --current --create-if-changed /run/coreos-kargs-thisboot-differ "$@" + +# There are a few cases we need to handle here. To illustrate this +# we'll use scenarios below where: +# +# - "booted": The kernel arguments from the currently booting system. +# - "ignition": The kernel arguments in the Ignition configuration. +# - "bls": The kernel arguments currently baked into the disk +# image BLS configs. +# +# The scenarios are: +# +# A. +# - Scenario: +# - booted: "" +# - ignition: "foobar" +# - bls: "" +# - Action: -> Update BLS configs, perform reboot +# B. +# - Scenario: +# - booted: "foobar" +# - ignition: "foobar" +# - bls: "" +# - Action: -> Update BLS configs, skip reboot +# C. +# - Scenario: +# - booted: "" +# - ignition: "foobar" +# - bls: "foobar" +# - Action: -> Skip update of BLS configs (they match already), perform reboot +# +# The logic here boils down to: +# if "ignition" != "booted"; then needreboot=1; fi +# if "ignition" != "bls"; then updatebls(); fi + +# NOTE: we write info messages to kmsg here because stdout gets swallowed +# by Ignition if there is no failure. This forces the info into the +# journal, but sometimes the journal will miss these messages because +# of ratelimiting. We've decided to accept this limitation rather than +# add the systemd-cat or logger utlities to the initramfs. + +# If the desired state isn't reflected by the current boot we'll need to reboot. +/usr/bin/rdcore kargs --current --create-if-changed /run/coreos-kargs-reboot "$@" +if [ -e /run/coreos-kargs-reboot ]; then + msg="Desired kernel arguments don't match current boot. Requesting reboot." + echo "$msg" > /dev/kmsg +fi if is-live-image; then # If we're in a live system and the kargs don't match then we must error. - if [ -e /run/coreos-kargs-thisboot-differ ]; then + if [ -e /run/coreos-kargs-reboot ]; then # Since we exit with error here the stderr will get shown by Ignition echo "Need to modify kernel arguments, but cannot affect live system." >&2 exit 1 fi else + # Update the BLS configs if they need to be updated. /usr/bin/rdcore kargs --boot-device /dev/disk/by-label/boot --create-if-changed /run/coreos-kargs-changed "$@" - # If the bootloader was changed and the kernel arguments don't match this boot - # then we must reboot. If they do match this boot then we can skip the reboot. - # - # Note we write info messages to kmsg here because stdout gets swallowed - # by Ignition if there is no failure. This forces the info into the journal, - # but sometimes the journal will miss these messages because of ratelimiting. - # We've decided to accept this limitation rather than add the systemd-cat or - # logger utlities to the initramfs. if [ -e /run/coreos-kargs-changed ]; then - if [ -e /run/coreos-kargs-thisboot-differ ]; then - msg="Kernel arguments were changed. Requesting reboot." - echo "$msg" > /dev/kmsg - touch /run/coreos-kargs-reboot - else - msg="Kernel arguments were changed, but they match this boot. Skipping reboot." - echo "$msg" > /dev/kmsg - fi + msg="Kernel arguments in BLS config were updated." + echo "$msg" > /dev/kmsg fi fi From 6d118829314aea6d3fec8ebf9c2c8a1a630c502e Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Wed, 2 Feb 2022 09:59:20 -0500 Subject: [PATCH 3/6] tests: rename the ignition.kargs test to ignition.kargs.basic We're going to add more tests here so let's put this one under a subheading. --- tests/kola/ignition/kargs/{ => basic}/config.ign | 0 tests/kola/ignition/kargs/{ => basic}/test.sh | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename tests/kola/ignition/kargs/{ => basic}/config.ign (100%) rename tests/kola/ignition/kargs/{ => basic}/test.sh (100%) diff --git a/tests/kola/ignition/kargs/config.ign b/tests/kola/ignition/kargs/basic/config.ign similarity index 100% rename from tests/kola/ignition/kargs/config.ign rename to tests/kola/ignition/kargs/basic/config.ign diff --git a/tests/kola/ignition/kargs/test.sh b/tests/kola/ignition/kargs/basic/test.sh similarity index 100% rename from tests/kola/ignition/kargs/test.sh rename to tests/kola/ignition/kargs/basic/test.sh From 2bcebfefe5adeb20fe8237e3acd151e66236f473 Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Wed, 2 Feb 2022 13:02:15 -0500 Subject: [PATCH 4/6] tests: ignition.kargs.basic: rework test - Add description - Convert Ignition to Butane - Use ok/fatal from commonlib.sh --- tests/kola/ignition/kargs/basic/config.bu | 7 +++++++ tests/kola/ignition/kargs/basic/config.ign | 9 --------- tests/kola/ignition/kargs/basic/data/commonlib.sh | 1 + tests/kola/ignition/kargs/basic/test.sh | 1 + 4 files changed, 9 insertions(+), 9 deletions(-) create mode 100644 tests/kola/ignition/kargs/basic/config.bu delete mode 100644 tests/kola/ignition/kargs/basic/config.ign create mode 120000 tests/kola/ignition/kargs/basic/data/commonlib.sh diff --git a/tests/kola/ignition/kargs/basic/config.bu b/tests/kola/ignition/kargs/basic/config.bu new file mode 100644 index 0000000000..9c5f82fb56 --- /dev/null +++ b/tests/kola/ignition/kargs/basic/config.bu @@ -0,0 +1,7 @@ +variant: fcos +version: 1.4.0 +kernel_arguments: + should_exist: + - foobar + should_not_exist: + - mitigations=auto,nosmt diff --git a/tests/kola/ignition/kargs/basic/config.ign b/tests/kola/ignition/kargs/basic/config.ign deleted file mode 100644 index 00816dc153..0000000000 --- a/tests/kola/ignition/kargs/basic/config.ign +++ /dev/null @@ -1,9 +0,0 @@ -{ - "ignition": { - "version": "3.3.0" - }, - "kernelArguments": { - "shouldExist": ["foobar"], - "shouldNotExist": ["mitigations=auto,nosmt"] - } -} diff --git a/tests/kola/ignition/kargs/basic/data/commonlib.sh b/tests/kola/ignition/kargs/basic/data/commonlib.sh new file mode 120000 index 0000000000..7028449b11 --- /dev/null +++ b/tests/kola/ignition/kargs/basic/data/commonlib.sh @@ -0,0 +1 @@ +../../../../data/commonlib.sh \ No newline at end of file diff --git a/tests/kola/ignition/kargs/basic/test.sh b/tests/kola/ignition/kargs/basic/test.sh index 1601fd71f8..dcfad77c3c 100755 --- a/tests/kola/ignition/kargs/basic/test.sh +++ b/tests/kola/ignition/kargs/basic/test.sh @@ -2,6 +2,7 @@ # TODO: Doc set -xeuo pipefail +# This test runs on all platforms and verifies Ignition kernel argument setting. . $KOLA_EXT_DATA/commonlib.sh From d6e945b98a42dcb675a6d77d350a71a2a2d4c861 Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Wed, 2 Feb 2022 13:23:50 -0500 Subject: [PATCH 5/6] tests: add ignition.kargs.skipreboot test This test verifies that if a kernel argument that is set as "should_exist" in the Ignition config already exists on the kernel command line of the machine then we can skip the reboot when applying kernel arguments but we must still update the BLS configs to make it permanent. --- .../kola/ignition/kargs/skipreboot/config.bu | 5 +++ .../kargs/skipreboot/data/commonlib.sh | 1 + tests/kola/ignition/kargs/skipreboot/test.sh | 43 +++++++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 tests/kola/ignition/kargs/skipreboot/config.bu create mode 120000 tests/kola/ignition/kargs/skipreboot/data/commonlib.sh create mode 100755 tests/kola/ignition/kargs/skipreboot/test.sh diff --git a/tests/kola/ignition/kargs/skipreboot/config.bu b/tests/kola/ignition/kargs/skipreboot/config.bu new file mode 100644 index 0000000000..4c530b7fac --- /dev/null +++ b/tests/kola/ignition/kargs/skipreboot/config.bu @@ -0,0 +1,5 @@ +variant: fcos +version: 1.4.0 +kernel_arguments: + should_exist: + - foobar diff --git a/tests/kola/ignition/kargs/skipreboot/data/commonlib.sh b/tests/kola/ignition/kargs/skipreboot/data/commonlib.sh new file mode 120000 index 0000000000..7028449b11 --- /dev/null +++ b/tests/kola/ignition/kargs/skipreboot/data/commonlib.sh @@ -0,0 +1 @@ +../../../../data/commonlib.sh \ No newline at end of file diff --git a/tests/kola/ignition/kargs/skipreboot/test.sh b/tests/kola/ignition/kargs/skipreboot/test.sh new file mode 100755 index 0000000000..ab41bb423a --- /dev/null +++ b/tests/kola/ignition/kargs/skipreboot/test.sh @@ -0,0 +1,43 @@ +#!/bin/bash +set -xeuo pipefail +# kola: { "platforms": "qemu", "appendFirstbootKernelArgs": "foobar" } +# This test verifies that if a kernel argument that is set as "should_exist" +# in the Ignition config already exists on the kernel command line of the machine +# then we can skip the reboot when applying kernel arguments but we must still +# update the BLS configs to make it permanent. This is Scenario B from +# the documentation in 35coreos-ignition/coreos-kargs.sh. +# +# - platforms: qemu +# - appendFirstbootKernelArgs is only supported on qemu. +# - appendFirstbootKernelArgs: foobar +# - The kernel argument to apply transiently only on the first boot. + +. $KOLA_EXT_DATA/commonlib.sh + +kargchecks() { + if ! grep foobar /proc/cmdline; then + fatal "missing expected kernel arg in kernel cmdline" + fi + if ! grep foobar /boot/loader/entries/*.conf; then + fatal "missing expected kernel arg in BLS config" + fi +} + +case "${AUTOPKGTEST_REBOOT_MARK:-}" in + "") + kargchecks + # If this file exists then reboot was skipped. See + # 35coreos-ignition/coreos-kargs.sh + if [ ! -e /run/coreos-kargs-changed ]; then + fatal "missing file that should exist if no reboot happened" + fi + # Now reboot the machine and verify the kernel argument persists + /tmp/autopkgtest-reboot nextboot + ;; + nextboot) + kargchecks + ;; + *) fatal "unexpected mark: ${AUTOPKGTEST_REBOOT_MARK}";; +esac + +ok "Ignition kargs skip reboot" From 83f3df2487b6c473f2d36da0ba44e089c31f84fc Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Wed, 2 Feb 2022 13:27:38 -0500 Subject: [PATCH 6/6] tests: add reboot check to ignition.kargs.basic This will further sanity check the karg persists even to subsequent boots. --- tests/kola/ignition/kargs/basic/test.sh | 27 +++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/tests/kola/ignition/kargs/basic/test.sh b/tests/kola/ignition/kargs/basic/test.sh index dcfad77c3c..c31163c6fe 100755 --- a/tests/kola/ignition/kargs/basic/test.sh +++ b/tests/kola/ignition/kargs/basic/test.sh @@ -6,10 +6,25 @@ set -xeuo pipefail . $KOLA_EXT_DATA/commonlib.sh -if ! grep foobar /proc/cmdline; then - fatal "missing foobar in kernel cmdline" -fi -if grep mitigations /proc/cmdline; then - fatal "found mitigations in kernel cmdline" -fi +kargchecks() { + if ! grep foobar /proc/cmdline; then + fatal "missing foobar in kernel cmdline" + fi + if grep mitigations /proc/cmdline; then + fatal "found mitigations in kernel cmdline" + fi +} + +case "${AUTOPKGTEST_REBOOT_MARK:-}" in + "") + kargchecks + # Now reboot the machine and verify the kernel argument persists + /tmp/autopkgtest-reboot nextboot + ;; + nextboot) + kargchecks + ;; + *) fatal "unexpected mark: ${AUTOPKGTEST_REBOOT_MARK}";; +esac + ok "Ignition kargs"