providers: setup network kernel arguments in initrd #390

lucab · 2020-04-14T18:28:10Z

This adds initial/experimental support for providing supplemental
network kernel arguments in the initrd.
It is meant to support environment where custom network kargs may
be provided via a back-channel.
Such arguments are only retrieved and applied on first boot, and
their format is defined by dracut [0].
The feature is currently reachable as a dedicated subcommand,
but this does not contain any platform-specific implementation.

[0] https://mirrors.edge.kernel.org/pub/linux/utils/boot/dracut/dracut.html#_network

lucab · 2020-04-14T18:30:22Z

This only contains CLI glue, but no actual logic (neither a default nor platform-specific one), in order to first land some scaffolding which I already have in other WIP patches.

/cc @jlebon

cgwalters · 2020-04-16T12:46:18Z

From the coreos.auth.verify test:

    3.080761] afterburn[280]: Error: failed to parse command-line arguments
[    3.085988] afterburn[280]: Caused by: error: Found argument '' which wasn't expected, or isn't valid in this context
[    3.092709] afterburn[280]: USAGE:
[    3.095982] afterburn[280]:     afterburn exp rd-network-kargs --cmdline
[    3.099981] afterburn[280]: For more information try --help
[    3.104302] systemd[1]: Startup finished in 2.059s (kernel) + 0 (initrd) + 779ms (userspace) = 2.838s.

cgwalters · 2020-04-16T12:48:24Z

Ahh and we need to update ignition in Fedora to pick up coreos/ignition-dracut#146 - that would have made debugging this instant and obvious.

jlebon

LGTM overall, just a few comments!

dracut/30afterburn/afterburn-network-kargs.service

src/cli/mod.rs

src/cli/exp.rs

lucab · 2020-04-17T13:38:10Z

@jlebon ack, good suggestions. I've rebased this on master, it should contain all the basic logic required for the default-juggling in coreos/fedora-coreos-tracker#460.
I'm going to give this a bunch of manual spins with the reduced grub config.

jlebon

LGTM!

lucab · 2020-04-20T14:10:18Z

src/initrd/mod.rs

+use std::io::Write;
+
+/// Path to cmdline.d fragment for network kernel arguments.
+static KARGS_PATH: &str = "/etc/cmdline.d/50-afterburn-network-kargs";


Self-note: .conf suffix was missing here.

lucab · 2020-04-20T15:11:20Z

Spot one silly issue when testing manually with coreos/coreos-assembler#1373. Fixed that, I didn't notice other issues and changing --default-value= actually change the behavior on firstboot.

cgwalters · 2020-04-20T15:25:03Z

dracut/30afterburn/afterburn-network-kargs.service

+OnFailureJobMode=isolate
+
+[Service]
+Environment=AFTERBURN_OPT_DEFAULT_VALUE=--default-value=ip=dhcp,dhcp6


The kernel networking defaults are critically important value, and this feels like a rather obscure place for them to live.
How about we add them to fedora-coreos-config in e.g. /usr/share/coreos-default-network-kargs or something, and then here do:
EnvironmentFile=/usr/share/coreos-default-network-kargs
?

I agree.
Though, this introduces an hard-requirement on a file which only lives in some -config repo.
Should I keep this current line and just add the EnvironmentFile=- afterwards, so that the explicit entry under -config is only overriding it if it exists?

But then we have (potentially) two versions; we'd silently use this one if the other one happened to not exist or we typo'd a path.

Perhaps a build-time option is best? Would help enable other distributions.

Yes, that's how overriding works by design.
There is also an in-code default here (which can be turned into a build-time option, yes, but I think the unit override is more useful and with lower complexity).

Unless you are actually thinking of making this a required parameter and dropping all the defaults from this repo. In which case, I can rework some bits so that the unit will actually fail if --default-value is missing, and inject that only via a -config dropin.
Is this UX closer to the flow you had in mind? That is, your preference is for a single definition with hard-fail, not multiple levels of overrides?

I can rework some bits so that the unit will actually fail if --default-value is missing, and inject that only via a -config dropin.
Is this UX closer to the flow you had in mind? That is, your preference is for a single definition with hard-fail, not multiple levels of overrides?

I think so yes. This feels like a global property of the operating system that I wouldn't expect to be hardcoded in afterburn.

Think about how many gyrations we went through with the switch from dhcp to dhcp,dhcp6, having that be revs to afterburn.rpm just feels weird.

So yes, my proposal is that this is a file in some format (text, env foo=bar, JSON, whatever) that we ship in fedora-coreos-config.

(Although, complicating things is that it needs to end up in the initramfs so we'd need a new dracut "module" just to copy the file, but eh.)

This all said...I'm not opposed to landing this as is either to unblock further work and iterating on this as a followup.

No worries, I'd rather get the integration/design part right upfront. I've pushed a fixup for the flow above. Thanks for the feedback!

lucab · 2020-04-21T12:37:46Z

@cgwalters @jlebon I've pushed a fixup to align the default-flow as suggested in review. It goes together with coreos/fedora-coreos-config#362. Feel free to review either here or there.

dustymabe · 2020-04-21T13:52:49Z

dracut/30afterburn/afterburn-network-kargs.service

+# This service may produce additional kargs fragments,
+# which are then consumed by dracut-cmdline(8).
+DefaultDependencies=no
+Before=dracut-cmdline.service


is there anything complicated that needs to happen before this service runs (i.e. device loading or anything to query information (like VMware guestinfo)? If so this might be pretty early.

For VMware specifically, no (it goes via an IOPort, the logic here does not require the vm-tool daemon running).

In general yes, this is very-painfully-early. But this needs to run before the cmdline is assembled by dracut and then fed into the initqueue networking.

I didn't find any better locations given the current status of things, do you have alternative places?

This is fine for now. If we hit a constraint in the future we can do what I do in the copy_firstboot_network code where we run after cmdline, but before initqueue and then just blank out the results of the earlier call to nm-initrd-generator. If we switch to that model we'll need to add some more ordering to the copy_firstboot_network service to run after afterburn.

(Or, rework NM to run as a unit and not in the initqueue)

dustymabe · 2020-04-21T13:54:10Z

is the case where a user provides their own ip= arguments handled here? A simple "yes" will suffice.

lucab · 2020-04-21T14:05:44Z

@dustymabe if the bootloader command line already contains an ip= parameter, the exclusion logic at https://github.com/coreos/afterburn/pull/390/files#diff-2f2a33f6cbc0b21a19815f89505a9f8cR63-R66 short-circuits the rest of the logic.

dracut/30afterburn/afterburn-network-kargs.service

lucab · 2020-05-04T16:24:17Z

Bump. May I ask for another review round and making sure that everybody is happy with the new approach to default kargs? If so, I think we can start merging coreos/fedora-coreos-config#362 to make CI happy here.

This adds initial/experimental support for providing supplemental network kernel arguments in the initrd. It is meant to support environment where custom network kargs may be provided via a back-channel. Such arguments are only retrieved and applied on first boot, and their format is defined by dracut [0]. The feature is currently reachable as a dedicated subcommand, but this does not contain any platform-specific implementation. [0] https://mirrors.edge.kernel.org/pub/linux/utils/boot/dracut/dracut.html#_network

lucab · 2020-05-18T12:21:02Z

The default config landed in both FCOS and RHCOS -config repos. I manually tested this on both with static and DHCP configuration, it works as designed. Merging.

lucab added the kind/design label Apr 14, 2020

lucab requested a review from jlebon April 14, 2020 18:28

This comment has been minimized.

Sign in to view

lucab force-pushed the ups/rd-network-kargs branch 2 times, most recently from 0a840d7 to be5e1a7 Compare April 16, 2020 10:54

lucab closed this Apr 16, 2020

lucab reopened this Apr 16, 2020

lucab force-pushed the ups/rd-network-kargs branch from be5e1a7 to 3b8c4fe Compare April 16, 2020 13:12

lucab mentioned this pull request Apr 16, 2020

providers: add a default catch-all provider #392

Closed

lucab force-pushed the ups/rd-network-kargs branch from 3b8c4fe to 8c00f24 Compare April 16, 2020 15:17

jlebon reviewed Apr 16, 2020

View reviewed changes

dracut/30afterburn/afterburn-network-kargs.service Outdated Show resolved Hide resolved

src/cli/mod.rs Outdated Show resolved Hide resolved

src/cli/exp.rs Outdated Show resolved Hide resolved

lucab force-pushed the ups/rd-network-kargs branch from 8c00f24 to c4c38de Compare April 17, 2020 11:16

jlebon approved these changes Apr 17, 2020

View reviewed changes

lucab mentioned this pull request Apr 20, 2020

grub: de-hardcode firstboot network configuration coreos/coreos-assembler#1373

Merged

lucab commented Apr 20, 2020

View reviewed changes

lucab force-pushed the ups/rd-network-kargs branch from c4c38de to f78f03a Compare April 20, 2020 14:12

cgwalters reviewed Apr 20, 2020

View reviewed changes

lucab mentioned this pull request Apr 21, 2020

dracut/firstboot-network: add default network kargs coreos/fedora-coreos-config#362

Merged

lucab force-pushed the ups/rd-network-kargs branch from c386684 to 9367282 Compare April 21, 2020 13:17

dustymabe reviewed Apr 21, 2020

View reviewed changes

jlebon reviewed Apr 21, 2020

View reviewed changes

dracut/30afterburn/afterburn-network-kargs.service Show resolved Hide resolved

lucab force-pushed the ups/rd-network-kargs branch from 9367282 to 71cca18 Compare April 24, 2020 12:48

lucab force-pushed the ups/rd-network-kargs branch from 71cca18 to 538a66b Compare April 24, 2020 13:07

lucab mentioned this pull request Apr 24, 2020

Release 4.3.3 #403

Merged

lucab force-pushed the ups/rd-network-kargs branch 2 times, most recently from 9ac90fc to 01b0a9e Compare April 28, 2020 12:49

jlebon approved these changes May 4, 2020

View reviewed changes

lucab force-pushed the ups/rd-network-kargs branch from 01b0a9e to bd6e461 Compare May 12, 2020 15:13

lucab merged commit 480f068 into coreos:master May 18, 2020

lucab deleted the ups/rd-network-kargs branch May 18, 2020 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

providers: setup network kernel arguments in initrd #390

providers: setup network kernel arguments in initrd #390

lucab commented Apr 14, 2020

lucab commented Apr 14, 2020

This comment has been minimized.

cgwalters commented Apr 16, 2020

cgwalters commented Apr 16, 2020

jlebon left a comment

lucab commented Apr 17, 2020

jlebon left a comment

lucab Apr 20, 2020

lucab commented Apr 20, 2020

cgwalters Apr 20, 2020

lucab Apr 20, 2020

cgwalters Apr 20, 2020

lucab Apr 20, 2020

cgwalters Apr 20, 2020

lucab Apr 21, 2020

lucab commented Apr 21, 2020

dustymabe Apr 21, 2020

lucab Apr 21, 2020

dustymabe Apr 21, 2020

cgwalters Apr 24, 2020

dustymabe commented Apr 21, 2020

lucab commented Apr 21, 2020

lucab commented May 4, 2020

lucab commented May 18, 2020

providers: setup network kernel arguments in initrd #390

providers: setup network kernel arguments in initrd #390

Conversation

lucab commented Apr 14, 2020

lucab commented Apr 14, 2020

This comment has been minimized.

cgwalters commented Apr 16, 2020

cgwalters commented Apr 16, 2020

jlebon left a comment

Choose a reason for hiding this comment

lucab commented Apr 17, 2020

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Apr 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe commented Apr 21, 2020

lucab commented Apr 21, 2020

lucab commented May 4, 2020

lucab commented May 18, 2020