fix(network-manager): disable tty output if the console is not usable #1611

bengal · 2021-09-24T11:47:04Z

The network-manager module also writes logs to the console, so that it's easier to debug network-related boot issues. If systemd can't open the console, the service fails and network doesn't get configured.

Add a check to disable tty output when the console is not present or not usable.

coreos/fedora-coreos-tracker#943

Checklist

I have tested it locally

johannbg · 2021-09-24T12:35:00Z

Hmm this seems somewhat backwards.
I expect/would think that the NM module by default should not be started in debugging mode thus outputting things to the console unless either rd.debug is found on the kernel commandline and or some nm.debug kernel commandline parameter is in use so perhaps a better fix might be to include two unit files, one that starts normall and another that starts if debug options are enabled as in the "standard" one would have ConditionKernelCommandLine=!FOO.NM.DEBUG and starts NM without debug being enabled while the debug one would have ConditionKernelCommandLine=FOO.NM.DEBUG in them.

bengal · 2021-09-27T11:43:09Z

I expect/would think that the NM module by default should not be started in debugging mode thus outputting things to the console unless either rd.debug is found on the kernel commandline and or some nm.debug kernel commandline parameter is in use

Good point, there should be no output on console unless the command line contains rd.debug. I've added a commit to change that.

a better fix might be to include two unit files, one that starts normally and another that starts if debug options are enabled

I'm not sure about this; having two units means that every change must be duplicated in both files and it's easy to get them out of sync by mistake. How about changing only the needed options through a drop-in file instead?

dustymabe · 2021-10-13T12:42:34Z

modules.d/35network-manager/nm-config.sh

+    if [ -n "$DRACUT_SYSTEMD" ]; then
+        # enable tty output if an usable console is found
+        # shellcheck disable=SC2217
+        if [ -w /dev/console ] && (echo < /dev/console) > /dev/null 2> /dev/null; then


I assume echo < /dev/console is a test and will fail if the console doesn't work? It seems weird to me to send anything to stdin of echo since it doesn't do anything with it. cat does.

Also:

Suggested change

if [ -w /dev/console ] && (echo < /dev/console) > /dev/null 2> /dev/null; then

if [ -w /dev/console ] && (echo < /dev/console) &> /dev/null; then

I think this is equivalent with less characters.

I assume echo < /dev/console is a test and will fail if the console doesn't work? It seems weird to me to send anything to stdin of echo since it doesn't do anything with it. cat does.

I copied this code from https://github.com/dracutdevs/dracut/blob/055/modules.d/99shutdown/shutdown.sh#L15

The idea is to try to open the console via shell redirection, but don't actually read or write anything from/to it. If /dev/console can't be opened, the command returns a non-zero value which is interpreted as false.

I think this is equivalent with less characters.

The interpreter of this script is /bin/sh, so it must be POSIX compatible.
&> is a bashism and is not POSIX.

Thanks for the reply/explanation.

dustymabe · 2021-10-13T12:44:39Z

modules.d/35network-manager/nm-config.sh

+[Service]
+StandardOutput=tty
+EOF
+            systemctl --no-block daemon-reload


any implications of running this daemon-reload here? If we ran this as part of a systemd generator would we need to do the daemon-reload?

I'm not aware of any side effect of calling daemon-reload here.

A generator would be an alternative and wouldn't require a daemon-reload; I think it requires to ship new service and a script. I like how the current code is self-contained, but I have no objections in turning it into a generator if that's the general consensus.

@lucab - any thoughts here on the tradeoffs?

Yes, reloading in the middle of a boot isn't the nicest, a generator would be advisable.
I don't think it directly breaks anything so not strictly a problem, but may trigger some unexpected second-order effects.
For reference, I see there are some dracut modules already shipping a generator in this repository, like modules.d/95nbd/.

Agree a generator would be slightly cleaner and would make a good follow-up, but if this is already tested IMO it's not worth the respin.

modules.d/35network-manager/nm-initrd.service

modules.d/35network-manager/nm-config.sh

The module should show the output on console only when initrd debugging is enabled.

modules.d/35network-manager/nm-initrd.service

dustymabe

seems fine to me

The network-manager module also writes logs to the console, so that it's easier to debug network-related boot issues. If systemd can't open the console, the service fails and network doesn't get configured. Add a check to disable tty output when the console is not present or not usable. coreos/fedora-coreos-tracker#943

dustymabe · 2021-10-19T15:57:28Z

@johannbg - can we merge this?

johannbg

LGTM

johannbg · 2021-10-19T20:29:30Z

@dustymabe I've ack this but it needs second review from @haraldh before it can be merged.

haraldh

This is so ugly... but if you insist...

dustymabe · 2021-10-21T13:43:54Z

This is so ugly

can you elaborate? obviously we're interested in an expert's opinion on best way to achieve something.

haraldh · 2021-10-22T07:34:50Z

All output of services is normally logged to the systemd journal anyway. There exist kernel command line options to display the output of the journal like systemd.journald.forward_to_console or IIRC, rd.systemd.journald.forward_to_console.

You can even get a systemd driven shell with rd.systemd.debug_shell and display the nm-initrd.service log entries.

If StandardOutput is going to the tty this information is lost for the journal. journal+console would be a better config anyway.

dustymabe · 2021-10-23T01:56:56Z

Thanks for the explanation.

…t usable

bengal requested review from danimo, haraldh and johannbg as code owners September 24, 2021 11:47

github-actions bot added modules Issue tracker for all modules network-manager Issues related to the network-manager module labels Sep 24, 2021

bengal force-pushed the bg/no-console branch from 4ad0834 to 7e5d828 Compare September 27, 2021 11:38

dustymabe reviewed Oct 13, 2021

View reviewed changes

modules.d/35network-manager/nm-initrd.service Outdated Show resolved Hide resolved

dustymabe reviewed Oct 13, 2021

View reviewed changes

modules.d/35network-manager/nm-config.sh Outdated Show resolved Hide resolved

fix(network-manager): show output on console only with rd.debug enabled

5ddcd63

The module should show the output on console only when initrd debugging is enabled.

bengal force-pushed the bg/no-console branch from 7e5d828 to 7235674 Compare October 14, 2021 13:47

dustymabe reviewed Oct 14, 2021

View reviewed changes

modules.d/35network-manager/nm-initrd.service Outdated Show resolved Hide resolved

dustymabe approved these changes Oct 14, 2021

View reviewed changes

bengal force-pushed the bg/no-console branch from 7235674 to aa55f51 Compare October 19, 2021 08:07

johannbg approved these changes Oct 19, 2021

View reviewed changes

johannbg enabled auto-merge (rebase) October 19, 2021 20:28

dustymabe mentioned this pull request Oct 21, 2021

initrd: nm-initrd.service fails to spawn depending on console setup coreos/fedora-coreos-tracker#943

Closed

haraldh approved these changes Oct 21, 2021

View reviewed changes

johannbg merged commit f6e6be2 into dracutdevs:master Oct 21, 2021

LaszloGombos pushed a commit to LaszloGombos/dracut that referenced this pull request Mar 24, 2023

Backport PR dracutdevs#1611 to fix network manager when console is no…

59c2556

…t usable

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(network-manager): disable tty output if the console is not usable #1611

fix(network-manager): disable tty output if the console is not usable #1611

bengal commented Sep 24, 2021

johannbg commented Sep 24, 2021

bengal commented Sep 27, 2021

dustymabe Oct 13, 2021

bengal Oct 14, 2021 •

edited

Loading

dustymabe Oct 14, 2021

dustymabe Oct 13, 2021

bengal Oct 14, 2021

dustymabe Oct 14, 2021

lucab Oct 18, 2021

jlebon Oct 18, 2021

dustymabe left a comment

dustymabe commented Oct 19, 2021

johannbg left a comment

johannbg commented Oct 19, 2021

haraldh left a comment

dustymabe commented Oct 21, 2021

haraldh commented Oct 22, 2021

dustymabe commented Oct 23, 2021

	if [ -w /dev/console ] && (echo < /dev/console) > /dev/null 2> /dev/null; then
	if [ -w /dev/console ] && (echo < /dev/console) &> /dev/null; then

fix(network-manager): disable tty output if the console is not usable #1611

fix(network-manager): disable tty output if the console is not usable #1611

Conversation

bengal commented Sep 24, 2021

Checklist

johannbg commented Sep 24, 2021

bengal commented Sep 27, 2021

dustymabe Oct 13, 2021

Choose a reason for hiding this comment

bengal Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

dustymabe Oct 14, 2021

Choose a reason for hiding this comment

dustymabe Oct 13, 2021

Choose a reason for hiding this comment

bengal Oct 14, 2021

Choose a reason for hiding this comment

dustymabe Oct 14, 2021

Choose a reason for hiding this comment

lucab Oct 18, 2021

Choose a reason for hiding this comment

jlebon Oct 18, 2021

Choose a reason for hiding this comment

dustymabe left a comment

Choose a reason for hiding this comment

dustymabe commented Oct 19, 2021

johannbg left a comment

Choose a reason for hiding this comment

johannbg commented Oct 19, 2021

haraldh left a comment

Choose a reason for hiding this comment

dustymabe commented Oct 21, 2021

haraldh commented Oct 22, 2021

dustymabe commented Oct 23, 2021

bengal Oct 14, 2021 •

edited

Loading