#1874 PR `merged`: Add code to recognize persistent LAN interface and manipulate KERNEL_CMDLINE¶

Labels: enhancement, fixed / solved / done

gdha opened issue at 2018-07-18 16:24:¶

Type: Enhancement
Impact: Normal
Reference to related issue (URL): https://github.com/gdha/rear-automated-testing/issues/61 and #1400
How was this pull request tested? via ReaR-Automated-Testing platform
Brief description of the changes in this pull request: new code was added to recognize persistent naming of LAN interfaces. If required it will manipulate the KERNEL_CMDLINE

jsmeix commented at 2018-07-19 08:50:¶

@gdha
I have no experience with the particular net.ifnames=0 kernel command line parameter
but that particular case does not matter for what I do not like in general with your
current implementation as far as I understand it from plain looking at the code.

As far as I understand it from plain looking at the code
with your current implementation things would work as follows:

(1.)
In default.conf

COPY_KERNEL_PARAMETERS=( 'net.ifnames' 'biosdevname' )

results that in rescue/GNU/Linux/290_kernel_cmdline.sh
a net.ifnames=0 kernel command line parameter setting
will be added to the KERNEL_CMDLINE that will be used
when booting the recovery system kernel.
Up to that point there are no changes as things worked in the past.

(2.)
Now via your current implementation there is your new
subsequent script rescue/GNU/Linux/700_persistent_interface.sh
that again removes the net.ifnames=0 kernel command line parameter
if persistent network interface naming is used on the original system
from the KERNEL_CMDLINE that will be used
when booting the recovery system kernel.

This means the user has no way to specify that the recovery system kernel
must boot with the net.ifnames=0 kernel command line parameter set
if persistent network interface naming is used on his original system.

This contradicts in general my favourite "final power to the user" principle and
it contradicts in particular what default.conf tells about KERNEL_CMDLINE

# You can add arbitrary kernel command line parameters
# when booting the rescue/recovery system
# as you need it

I.e. when the user has in his etc/rear/local.conf

KERNEL_CMDLINE="net.ifnames=0"

then ReaR must obey the user's commandment and boot the recovery system kernel
with the net.ifnames=0 kernel command line parameter set.

To solve the issue I suggest to not implement it in a new
separated subsequent script but to add code to the existsing
rescue/GNU/Linux/290_kernel_cmdline.sh
that does by default (i.e. unless net.ifnames is specified in KERNEL_CMDLINE)
not add net.ifnames=0 to KERNEL_CMDLINE
if persistent network interface naming is used on the original system.

Simply put I suggest:
By default
if persistent network interface naming is used on the original system
skip adding net.ifnames=0 to KERNEL_CMDLINE
in rescue/GNU/Linux/290_kernel_cmdline.sh

jsmeix commented at 2018-07-19 09:04:¶

Only an untested proposal to show what I mean:

I think the code in rescue/GNU/Linux/290_kernel_cmdline.sh
can be enhanced to support what is needed here
and simplified (no longer the circuitous kernel_option_superseeded)
to something like (excerpt - only the second for loop):

# Verify if the kernel option we want to add to KERNEL_CMDLINE are not already set/force by the user in the rear configuration.
# If yes, the parameter set in the configuration file have the priority and superseed the current kernel option.
for new_kernel_option in "${new_kernel_options_to_add[@]}" ; do
    new_kernel_option_keyword="${new_kernel_option%%=*}"

    for rear_kernel_option in $KERNEL_CMDLINE; do
        # Check if a kernel option key without value parameter (everything before =) is not already present in rear KERNEL_CMDLINE array.
        if test "$new_kernel_option_keyword = "${rear_kernel_option%%=*}" ; then
            Log "Current kernel option [$new_kernel_option] supperseeded by [$rear_kernel_option] in your rear configuration: (KERNEL_CMDLINE)"
            # Continue with the next new_kernel_option (i.e. continue the outer 'for' loop):
            continue 2
        fi
    done

    if test "net.ifnames" = "$new_kernel_option_keyword" ; then
        # If we are using persistent naming do not add net.ifnames to KERNEL_CMDLINE
        # see https://github.com/rear/rear/pull/1874
        # and continue with the next new_kernel_option:
        is_persistent_ethernet_name $( ip r | awk '$2 == "dev" && $8 == "src" { print $3 }' | sort -u | head -1 ) && continue
    fi

    LogPrint "Adding $new_kernel_option to KERNEL_CMDLINE"
    KERNEL_CMDLINE="$KERNEL_CMDLINE $new_kernel_option"
done

jsmeix commented at 2018-07-19 09:10:¶

@schabrolles
I added you as reviewer here
because rescue/GNU/Linux/290_kernel_cmdline.sh
is originally your script, cf. https://github.com/rear/rear/pull/1495

gdha commented at 2018-07-19 09:20:¶

@jsmeix Thank you for the review! As you see by reviewing each others code it can only get better. Thanks for the suggestion as it is indeed easier and cleaner. Will take a day or two before I can change it due to being off-site.

jsmeix commented at 2018-07-19 10:47:¶

I have a nitpicking question:

What do you think is best how to split
the kernel option keyword from the kernel option value:

# new_kernel_option="foo=bar=baz"

# echo "keyword='${new_kernel_option%=*}'"
keyword='foo=bar'

# echo "keyword='${new_kernel_option%%=*}'"
keyword='foo'

I guess ${new_kernel_option%%=*} would be better because
I assume a '=' character never happens in a kernel option keyword
but a '=' character might happen in a kernel option value
because one cannot know what arbitrary values could be.
For example think about values that could contain arbitrary characters
like some_password=arbitrary=characters

gdha commented at 2018-07-20 09:30:¶

@jsmeix @schabrolles I reworked the code in 290_kernel_cmdline.sh script to deal with persistent naming (I also included jsmeix suggestion of above). It works fine as I tested it via rear-automated-testing.

jsmeix commented at 2018-07-20 10:03:¶

@gdha
I approve it because you tested it and it works well for you
and you have perfectly well documented what the code
is meant to do.

But - from plain looking at the code - it still seems
as if the added part at the end of
usr/share/rear/rescue/GNU/Linux/290_kernel_cmdline.sh
makes it impossible for the user to enforce via
KERNEL_CMDLINE="$KERNEL_CMDLINE net.ifnames=0"
that the recovery system kernel gets booted with the net.ifnames=0
kernel option set.

Regardless if that works or not I think when the user has explicity
specified it (all in local.conf is what the user has explicity specified
in contrast to what we have in default.conf) ReaR must obey.

Simply put:
I think ReaR is for root and runs as root and then the user
can do destructive things if he is careless, e.g. when there is
rm -rf --no-preserve-root / in local.conf the user gets that.

Or I misunderstand what the added part at the end of
usr/share/rear/rescue/GNU/Linux/290_kernel_cmdline.sh
actually does?

schabrolles commented at 2018-07-20 10:04:¶

@gdha, I don't currently don't have time to deeply look into the code; but had a quick look on it. If I understand well, the purpose of this PR is to remove KERNEL_CMDLINE parameters from local.conf that could be set by "mistake" like net.ifname=0 when biosdevname are currently in use on the real system. Am I right ?

jsmeix commented at 2018-07-20 10:15:¶

I think it could happen in a migration mode use case
(i.e. when rear recover is run on different replacement hardware
or when rear recover is run in a different new environment)
that a KERNEL_CMDLINE setting in local.conf is wrong
for the replacement hardware or in the new environment.

But in this case the user can edit the kernel command line
in the recovery system boot menue so that a
wrong KERNEL_CMDLINE setting in local.conf
is not a dead end for the user.

gdha commented at 2018-07-23 09:32:¶

@jsmeix @schabrolles Indeed users can add it anyhow without fully understanding the meaning of it or the side-effects (=no interfaces found in recovery mode). This protects them from such mistakes and in case they really need it they can still add it manually or on the command level at boot up time.
Furthermore, it makes my ReaR Automated Testing environment much easier (less config sets required)

[Export of Github issue for rear/rear.]

#1874 PR merged: Add code to recognize persistent LAN interface and manipulate KERNEL_CMDLINE¶