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

install: add --delete-karg and --append-karg #268

Merged
merged 4 commits into from Jun 26, 2020
Merged

install: add --delete-karg and --append-karg #268

merged 4 commits into from Jun 26, 2020

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jun 11, 2020

This allows users to delete and append kernel arguments from the default
set. The major use cases for this are (1) avoiding an additional reboot
which might be costly in some situations, and (2) making it easier to
troubleshoot the system on firstboot.

For more details, see
coreos/fedora-coreos-tracker#533 (comment).

Closes: coreos/fedora-coreos-tracker#533

src/install.rs Outdated
@@ -254,6 +259,31 @@ fn write_firstboot_kargs(mountpoint: &Path, args: &str) -> Result<()> {
Ok(())
}

/// Write persistent kernel arguments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we need to keep in mind is this needs to trigger a rerun of zipl on s390x.

Hmm...I am increasingly thinking what we want is like a special systemd unit for zipl that checks the mtime of the BLS config and runs zipl if it's changed or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, after installation we do write some s390x specific args parsed from /proc/cmdline

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yup, goot catch. Hmm, let's circle back on this once #153 merges? It'll make it easier to run zipl. I can open an issue once this merges.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, haven't seen this message begore. So, i've already started working on patch to remove all logic in #153 zipl.rs in favour of using --append-karg.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brokenjacobs
Copy link

Is there also a way to remove persistent arguments as well? (console=ttyS0,1152008n1).

It's a good idea in my opinion to emulate the various arguments to rpm-ostree kargs. They are all there for a reason.

@lucab
Copy link
Contributor

lucab commented Jun 12, 2020

I like this but also have a bad feeling about it; I fear this is going to trip many people.

In particular, it looks like the logic implemented here is really an --append-persistent-kargs. Also, it implicitly assumes that users already know what are the current kargs (is that a static set?) and that previous values can be overridden in a last-key-wins ways (I know that's not the case generally, see for example hugepages).

src/install.rs Outdated Show resolved Hide resolved
@jlebon
Copy link
Member Author

jlebon commented Jun 15, 2020

In particular, it looks like the logic implemented here is really an --append-persistent-kargs.

Yeah, see coreos/fedora-coreos-tracker#533 (comment) related to this. Will rename the switch.

Also, it implicitly assumes that users already know what are the current kargs (is that a static set?)

Hmm, though that's no different from eventual kargs-via-Ignition support, right?

and that previous values can be overridden in a last-key-wins ways (I know that's not the case generally, see for example hugepages).

Yes, agreed (turns out console is another such example as mentioned in the linked comment above). Combined with --delete-persistent-kargs, I think it provides enough flexibility.

(See also ostreedev/ostree#1859 -- we'll need to make sure that the implementation of kargs-via-Ignition is ordering aware).

@jlebon jlebon changed the title install: add --persistent-args install: add --delete-karg and --append-karg Jun 16, 2020
@jlebon
Copy link
Member Author

jlebon commented Jun 16, 2020

OK, split out prep patches in #271!

src/install.rs Outdated Show resolved Hide resolved
Factor out the closure and add some basic unit tests.
We don't really want users to use this now. The primary use case was
configuring the network from the kernel cmdline. This has been obsoleted
by the nicer `--copy-network` approach.

We still need to keep supporting it for now though. It's used at least
by `coreos-installer.service` to auto-forward some kargs.
This allows users to delete and append kernel arguments from the default
set. The major use cases for this are (1) avoiding an additional reboot
which might be costly in some situations, and (2) making it easier to
troubleshoot the system on firstboot.

For more details, see
coreos/fedora-coreos-tracker#533 (comment).

Closes: coreos/fedora-coreos-tracker#533
@jlebon jlebon merged commit 844b7e6 into coreos:master Jun 26, 2020
@jlebon jlebon deleted the pr/persistent-args branch June 26, 2020 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support setting kernel args for the installed OS image
6 participants