Merge ~smoser/curtin:fix/1527664-grub-cmdline-linux-default-in-default-grub into curtin:master

[Scott Moser]

Adjust helpers/common to edit GRUB_CMDLINE_LINUX_DEFAULT in place.

This will help us avoid hitting upgrade prompts as seen
and described in bug 564853. The change here is only to move
GRUB_CMDLINE_LINUX_DEFAULT to /etc/default/grub from
/etc/default/grub.d/50-curtin-settings.cfg.
Currently, other changes will still to into 50-curtin-settings.cfg.

Also collect /etc/default/grub files to artifacts and fix small issue
that resulted in the carryover args having an additional white space
at the end.

LP: #1527664

see commit message

[Scott Moser (smoser)]

tempted to write the shell parser in python and either
 a.) have the helper call 'curtin edit-shell-file /etc/default/grub ...'
 b.) have curtin python code edit the file itslef and drop that function from install-grub

if 'b', we'd have to do the change before calling 'install-grub' so that changes would
be seen.

[Ryan Harper (raharper)]

I'm find with this shell. Long term, I'd like to drop install-grub from shell and write that in python; if/when we do that, we could fold this into that transition. Sound reasonable?

[Ryan Harper (raharper)]

Some in-line comments/questions

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:bd29bd9d2efb4cee46ea15e3644554309da1f3a8
https://jenkins.ubuntu.com/server/job/curtin-ci/1137/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1137
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1137
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1137
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1137

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/1137/rebuild

review: Approve (continuous-integration)

71c0260... by Scott Moser on 2018-11-30

take Ryan's suggestions for code readability

605b250... by Scott Moser on 2018-11-30

collect etc/default/grub files

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:605b25070fb8d9907d4abf71f398766729f0f237
https://jenkins.ubuntu.com/server/job/curtin-ci/1138/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1138
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1138
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1138
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1138

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/1138/rebuild

review: Approve (continuous-integration)

4d0086b... by Scott Moser on 2018-11-30

fix a bug, add some debug

d8b2f6c... by Scott Moser on 2018-11-30

more debug

[Ryan Harper (raharper)]

I'm missing something with the ret=3 case, see inline comment.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:d8b2f6c3a3f3b493e0c4f5c568ebd0af4cc0d87e
https://jenkins.ubuntu.com/server/job/curtin-ci/1139/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1139
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1139
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1139
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1139

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/1139/rebuild

review: Approve (continuous-integration)

ce22923... by Scott Moser on 2018-11-30

remove trailing white space when carrying over params.

when carrying over parameters that occurred before the --
and no other params, we would see a trailing space on the end
of the 'carry over' parameters.

3259873... by Scott Moser on 2018-11-30

cleanup, add some quoting to add obviousness

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:32598730042b3e22a567b5546c5d9af4020d8cff
https://jenkins.ubuntu.com/server/job/curtin-ci/1140/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1140
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1140
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1140
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1140

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/1140/rebuild

review: Approve (continuous-integration)

a232743... by Scott Moser on 2018-11-30

simplify a bit.

[Scott Moser (smoser)]

@Ryan,
I simplified the code a bit.
I was over-engineering the shell_config_update. I was trying to account for the case where the /etc/default/grub file did not exist and to let the caller handle the behavior of what to do if it did not.

We can add that function later if we want.
For now, the behavior is
 - if /etc/default/grub exists and has GRUB_CMDLINE_LINUX_DEFAULT= in it, then edit in place.
 - otherwise append to it

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:ee63445c3d291052ff6b8ad858e3bfbb32c84129
https://jenkins.ubuntu.com/server/job/curtin-ci/1141/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1141
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1141
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1141
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1141

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/1141/rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:a232743c8f653414a2db15318e555cd2116114fe
https://jenkins.ubuntu.com/server/job/curtin-ci/1142/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1142
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1142
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1142
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1142

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/1142/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Yes, that looks good. Approved, let's get a jenkins run and then we can merge.

review: Approve

[Scott Moser (smoser)]

Jenkins run at
 https://jenkins.ubuntu.com/server/job/curtin-vmtest-devel-debug/138/

./tools/jenkins-runner -p4 tests/vmtests/test_basic.py tests/vmtests/test_lvm_raid.py

[Scott Moser (smoser)]

----------------------------------------------------------------------
Ran 343 tests in 2595.668s

OK (SKIP=12)
Fri, 30 Nov 2018 22:44:54 +0000: vmtest end [0] in 2600s

[Scott Moser (smoser)]

approving based on ryan's approval above.

review: Approve

diff --git a/helpers/common b/helpers/common
index f9217b7..34f0870 100644
--- a/helpers/common
+++ b/helpers/common
@@ -537,7 +537,44 @@ get_carryover_params() {
         done
         echo "${c# }"
     )
-    _RET="${carry_lead:+${carry_lead} }${carry_extra}"
+    [ -n "${carry_lead}" -a -n "${carry_extra}" ] &&
+        carry_lead="${carry_lead} "
+    _RET="${carry_lead}${carry_extra}"
+}
+
+shell_config_update() {
+    # shell_config_update(file, name, value)
+    # update variable 'name' setting value to 'val' in shell syntax 'file'.
+    # if 'name' is not present, then append declaration.
+    local file="$1" name="$2" val="$3"
+    if ! [ -f "$file" ] || ! grep -q "^$name=" "$file"; then
+        debug 2 "appending to $file shell $name=\"$val\""
+        echo "$name=\"$val\"" >> "$file"
+        return
+    fi
+    local cand="" del=""
+    for cand in "|" "," "/"; do
+        [ "${val#*${del}}" = "${val}" ] && del="$cand" && break
+    done
+    [ -n "$del" ] || {
+        error "Couldn't find a sed delimiter for '$val'";
+        return 1;
+    }
+
+    sed -i -e "s${del}^$name=.*${del}$name=\"$val\"${del}" "$file" ||
+        { error "Failed editing '$file' to set $name=$val"; return 1; }
+    debug 2 "updated $file to set $name=\"$val\""
+    return 0
+}
+
+apply_grub_cmdline_linux_default() {
+    local mp="$1" newargs="$2" edg="${3:-etc/default/grub}"
+    local gcld="GRUB_CMDLINE_LINUX_DEFAULT"
+    debug 1 "setting $gcld to '$newargs' in $edg"
+    shell_config_update "$mp/$edg" "$gcld" "$newargs" || {
+        error "Failed to set '$gcld=$newargs' in $edg"
+        return 1
+    }
 }
 
 install_grub() {
@@ -709,6 +746,8 @@ install_grub() {
     esac
 
     local grub_d="etc/default/grub.d"
+    # ubuntu writes to /etc/default/grub.d/50-curtin-settings.cfg
+    # to avoid tripping prompts on upgrade LP: #564853
     local mygrub_cfg="$grub_d/50-curtin-settings.cfg"
     case $os_family in
         redhat)
@@ -736,9 +775,9 @@ install_grub() {
     # always append rd.auto=1 for centos
     case $os_family in
         redhat)
-            newargs="$newargs rd.auto=1";;
+            newargs="${newargs:+${newargs} }rd.auto=1";;
     esac
-    debug 1 "carryover command line params: $newargs"
+    debug 1 "carryover command line params '$newargs'"
 
     case $os_family in
        debian)
@@ -746,9 +785,15 @@ install_grub() {
             { error "Failed to write '$mygrub_cfg'"; return 1; }
         ;;
     esac
+
+    if [ "${REPLACE_GRUB_LINUX_DEFAULT:-1}" != "0" ]; then
+        apply_grub_cmdline_linux_default "$mp" "$newargs" || {
+            error "Failed to apply grub cmdline."
+            return 1
+        }
+    fi
+
     {
-        [ "${REPLACE_GRUB_LINUX_DEFAULT:-1}" = "0" ] ||
-            echo "GRUB_CMDLINE_LINUX_DEFAULT=\"$newargs\""
         echo "# Curtin disable grub os prober that might find other OS installs."
         echo "GRUB_DISABLE_OS_PROBER=true"
         echo "GRUB_TERMINAL=console"
diff --git a/tests/vmtests/__init__.py b/tests/vmtests/__init__.py
index 5ffa632..bc4a87b 100644
--- a/tests/vmtests/__init__.py
+++ b/tests/vmtests/__init__.py
@@ -527,6 +527,9 @@ DEFAULT_COLLECT_SCRIPTS = {
         if [ -e /sys/firmware/efi ]; then
             efibootmgr -v | cat >efibootmgr.out;
         fi
+        [ ! -d /etc/default/grub.d ] ||
+            cp -a /etc/default/grub.d etc_default_grub_d
+        [ ! -f /etc/default/grub ] || cp /etc/default/grub etc_default_grub
 
         exit 0
         """)],