Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Linux kernel arguments for Grub command line #123

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Fix Linux kernel arguments for Grub command line #123

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 9, 2018

This pull request addresses two things:

  1. Before, the code in 80-mkbootable that generated the Grub command line which finally starts the Linux kernel in a stateful setup, ignored the contents of the variable WWKARGS as long as the variable CONSOLE was empty.

    In fact, the command line was build via

    echo "GRUB_CMDLINE_LINUX='console=tty0 console=${CONSOLE} ${KARGS}'" >> "${CONF}"

    thus people had to specify an additional console other than tty0.

    This threw away even useful kernel arguments such as net.ifnames=0 or ipv6.disable=1 as there was no GRUB_CMDLINE_LINUX in the file $GRUBDEFAULTCONF, if one was not aware that an additional console was necessary.

    Now, kernel arguments are added properly, even if CONSOLE is empty, as long as WWKARGS is not.
    The latter is usually the case, as the script 80-mkbootable adds the argument quiet as a default value.

  2. The kernel expects its arguments to be separated by spaces.
    However, the moment the script 80-mkbootable is executed, the variable WWKARGS contains the kernel arguments as a comma separated list.
    This is true, even if the kernel arguments are explicitly specified as separated by spaces while configuring a cluster.
    Thus, the function update_default_grub did not work properly before.

    I do not know where the commas are finally added, I just observed that they are there, so the new code strips them away and replaces them with spaces.

    Note, however, that this might introduce subtle bugs, where kernel arguments take a comma separated list themselve as a parameter.

    I do not know how to solve this, as I can not figure out where WWKARGS is populated with commas.

Best regards
Wolfgang

Before, the code generating the Grub command calling the Linux kernel
ignored the WWKARGS variable when no CONSOLE was given.

Furthermore, even if an additional CONSOLE was given, the arguments were
passed as a comma separated list, while the kernel expects them to be
separated by spaces.

Signed-off-by: Wolfgang Hoelzl <wolfgang.hoelzl(at)tum.de>
@bensallen bensallen added the bug label Jul 23, 2018
@bensallen bensallen added this to the 3.9 milestone Jul 23, 2018
@bensallen bensallen self-assigned this Jul 23, 2018
@bensallen
Copy link
Member

bensallen commented Jul 23, 2018

Hi Wolfgang,

Thanks for finding this issue and the pull request.

Note, however, that this might introduce subtle bugs, where kernel arguments take a comma separated list themselve as a parameter.

We should figure out a way to parse WWKARGS so we can handle this case. I can think of a few places where one could need commas in the parameter of a kernel cmdline arguments.

I'm not sure how we could actually go about parsing this as-is, however... There would be no way to tell a bare argument versus an argument with comma separated parameters.

I do not know how to solve this, as I can not figure out where WWKARGS is populated with commas.

The output from http://$master/WW/nodeconfig?hwaddr=$WWINIT_HWADDR is sourced via https://github.com/warewulf/warewulf3/blob/master/provision/initramfs/capabilities/transport-http/wwgetnodeconfig#L14. Example:

WWKARGS="nosplash,nmi_watchdog=0,nopti"
export WWKARGS

nodeconfig.pl joins arrays with commas here: https://github.com/warewulf/warewulf3/blob/master/provision/cgi-bin/nodeconfig.pl#L67

KARGS is stored as an array here: https://github.com/warewulf/warewulf3/blob/master/provision/lib/Warewulf/Provision.pm#L165

@ghost
Copy link
Author

ghost commented Jul 24, 2018

Isn't it enough to simply join WWKARGS using a space instead of a comma?
So https://github.com/warewulf/warewulf3/blob/master/provision/cgi-bin/nodeconfig.pl#L67 needs to check for WWKARGS in some way and then has to call a different join command.
Then, 80-mkbootable can use WWKARGS as it is and it's up to the user to provide proper kernel parameters.

However, I'm not proficient enough with warewulf in particular and perl in general to detect WWKARGS at the abovementioned place in the code.

Best regards,
Wolfgang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant