iso: add `kargs` commands to manage kernel args #341

jlebon · 2020-08-04T21:19:11Z

The new commands are coreos-installer iso kargs modify/reset/show.
These allow modifying the baked in kernel arguments in a CoreOS live
ISO.

My primary motivation for this is for embedding the right serial console
argument into the ISO before booting it:

$ coreos-installer iso kargs modify --append console=ttyS0 my.iso
$ coreos-installer iso kargs show my.iso
mitigations=auto,nosmt ... console=ttyS0
$ cosa run --qemu-iso my.iso -c
...
[    0.000000] Command line: BOOT_IMAGE=/images/vmlinuz ...  console=ttyS0
...

This example specifically demonstrates what cosa will do automatically
itself once it can rely on this functionality. This will for example
allow testiso tests to be much more useful because we'll actually have
console logs.

But it's of course useful generically. Another way to look at it is that
it mirrors/brings to parity the provisioning capabilities of the live
PXE path.

Co-authored-by: Nikita Dubrovskii nikita@linux.ibm.com

jlebon · 2020-08-04T21:21:18Z

The command requires coreos/coreos-assembler#1637 to work so that ISOs have the necessary info in the header, but the PR itself can of course be merged independently.

lucab · 2020-08-05T08:13:58Z

UX concern: it is my understanding that iso show-kargs only shows custom-embedded kargs, omitting the baked ones. That may be a surprising semantics for casual users, so it may be good to make it more explicit in the choice of sub-command names.

jlebon · 2020-08-05T14:24:41Z

UX concern: it is my understanding that iso show-kargs only shows custom-embedded kargs, omitting the baked ones. That may be a surprising semantics for casual users, so it may be good to make it more explicit in the choice of sub-command names.

Yeah, good point. Open to bikeshed on names. 🖌️
@bgilbert suggested splitting out two separate commands:

$ coreos-installer iso ignition {embed|show|remove}
$ coreos-installer iso kargs {embed|show|remove}

(And deprecate/forward coreos-installer iso {embed|show|remove} to the first one.)

Maybe we could make that second command be coreos-installer iso appended-kargs or something?

bgilbert · 2020-08-05T18:28:38Z

Maybe we could make that second command be coreos-installer iso appended-kargs or something?

coreos-installer iso kargs appended or show-appended?

dustymabe · 2020-08-05T18:49:12Z

Maybe we could make that second command be coreos-installer iso appended-kargs or something?

coreos-installer iso kargs appended or show-appended?

I don't mind show so much. We could output a message to the user explaining the limitation. Is there any way to show the baked ones even though we can't change them? If so, maybe something like:

coreos-installer iso kargs {append|show|show-all|remove}

bgilbert · 2020-08-05T18:50:23Z

We could output a message to the user explaining the limitation.

In help, presumably, not at runtime.

@jlebon Remember to update the subcommand list in .cci.jenkinsfile.

bgilbert · 2020-08-05T20:49:21Z

@bgilbert suggested splitting out two separate commands

#343

lucab · 2020-08-06T10:36:44Z

coreos-installer iso kargs {append|show-appended|unappend}

This follows from @dustymabe suggestion. Some approach like this feels good to me because:

it is clear that kargs are appended (i.e. postfix-joined) to something else
it is clear that it can only show appended entries
it is clear that it can only remove previously appended entries

jlebon · 2020-08-06T14:20:17Z

My argument for coreos-installer iso appended-kargs {embed|show|remove} is that the subcommands match exactly the ones for coreos-installer iso ignition, which I think is a nice property.

@lucab Does that meet the criteria list you have in #341 (comment)?

lucab · 2020-08-06T14:29:19Z

@jlebon yes, not strongly attached to specific keywords as long as the intent is clear.

cgwalters · 2020-08-06T20:27:50Z

src/cmdline.rs

@@ -438,6 +457,40 @@ pub fn parse_args() -> Result<Config> {
                                .takes_value(true),
                        ),
                )
+                .subcommand(
+                    SubCommand::with_name("embed-kargs")
+                        .about("Embed additional kernel args in an ISO image")


Maybe Add kernel arguments to the ISO image?

One concern: we also have the same use case for the ISO of needing to remove the default nosmt in FCOS?

The wording here matches the wording used for the ignition subcommand. It's more or less s/embedded Ignition config/embedded additional kernel args/. Thought that was nice, but not against rewording entirely if folks prefer!

Re. nosmt, yeah right now this is limited to managing additional kernel args. I think we could redesign things so that it handles base kargs as well, though it's definitely easier technically (and I think somewhat conceptually too) to operate in an "overlay" pattern, again like the ignition command. For example, appended-kargs remove will change the file back to a binary match of the original file.

I was looking at the kargs we bake in today, and the two big ones that one might want to revert are the smt one and the cgroupsv2 one. Both of those can be done by just overriding with another karg (i.e. mitigations=off systemd.unified_cgroup_hierarchy=1).

Actually hmm, for the mitigations=off test, my test might not be valid because I was testing in a VM with a single core & thread anyway. But I definitely observed cat /sys/devices/system/cpu/vulnerabilities/* change output. I'll do another sanity-check that SMT can be re-enabled this way.

I'll do another sanity-check that SMT can be re-enabled this way.

OK, so this test was slightly wrong because the default kernel behaviour is mitigations=auto, not off. But anyway, I did verify using qemu-kvm -smp threads=2 and lscpu that appending mitigations=auto is indeed equivalent to removing the mitigations=auto,nosmt flag.

src/iso.rs

cgwalters · 2020-08-06T20:34:01Z

src/iso.rs

+    let s = std::str::from_utf8(&area[..area_size]).chain_err(|| "invalid UTF-8 area")?;
+    let s = s.trim_end_matches('#');
+    if s.is_empty() {
+        bail!("No embedded additional kernel args.");


Is that an error?

This matches the semantics for iso ignition show. :)

This was changed to `coreos-installer iso appended-kargs` in later reviews: coreos/coreos-installer#341

jlebon · 2020-08-06T22:19:59Z

OK, rebased this on top of #343 (that was quite the conflict-fest) and moved to coreos-installer iso appended-kargs {embed|show|remove}!

bgilbert · 2020-08-08T02:38:51Z

coreos/coreos-assembler#1638 will need to be updated.

This was changed to `coreos-installer iso appended-kargs` in later reviews: coreos/coreos-installer#341

jlebon · 2020-08-08T15:23:37Z

coreos/coreos-assembler#1638 will need to be updated.

Yup, that's part of coreos/coreos-assembler#1637!

If we merge the cosa PR first, then we should be able to see this in action in CI here by inspecting the testiso console logs.

jlebon · 2020-10-23T20:15:23Z

How about renaming the command to just kargs since it's not just about appended kargs anymore?

nikita-dubrovskii · 2020-10-26T09:50:07Z

How about renaming the command to just kargs since it's not just about appended kargs anymore?

done

cgwalters · 2020-10-27T16:32:57Z

I think given coreos/fedora-coreos-tracker#567 we should probably also support explicitly reconfiguring the console, not just appending to the existing list.

nikita-dubrovskii · 2020-10-27T18:21:14Z

I think given coreos/fedora-coreos-tracker#567 we should probably also support explicitly reconfiguring the console, not just appending to the existing list.

didn't get your point. Now we can karg remove -k "console=X" ISO ; karg add -k "console=Y" ISO.

cgwalters · 2020-10-27T21:54:34Z

didn't get your point. Now we can karg remove -k "console=X" ISO ; karg add -k "console=Y" ISO.

OK, that sounds good. Is it possible to do in one commandline? Otherwise people without reflinks are going to feel some pain in rewriting the ISO twice.

It looks like some basic tests could be added here for CI.

nikita-dubrovskii · 2020-10-28T07:16:42Z

OK, that sounds good. Is it possible to do in one commandline?

not yet

It looks like some basic tests could be added here for CI.

exactly, but this depends on coreos/coreos-assembler#1637 , so it's better to do a bit later in a separate PR

jlebon · 2020-12-23T22:56:13Z

@nikita-dubrovskii Hope you don't mind, I picked this up again and pushed some refactors on top! I kept your co-ownership in the commit. Some things I did:

renamed modify to replace
used lazy_static! for the regex
removed embed since it duplicates add
changed replace so that it replaces the karg in-place since ordering matters for kargs (so we can't just remove the old karg and add the new one)
made the order in which the various commands are handled in code always be add, replace, remove, reset, show to be consistent
made add, replace, and remove take multiple kargs instead of just one (on that subject, I think I'd like to change the CLI further so that the commands are just modify/show/reset and we then have modify be able to take multiple --add/--replace/--remove switches; that way you can do all your modifications in one invocation -- WDYT?)
made replace just support KEY=OLD=NEW without having to surround OLD and NEW with single quotes since it's more natural. I'm guessing you originally did it that way to avoid space escaping issues? Worth noting rpm-ostree kargs has been using that format for a while and that hasn't come up yet (and I guess in the worst case if there really is some space escaping issues, users can always fall back to remove and add)
standardized all the operations to use the same set of four helper functions: find_karg_embed_areas, get_current_kargs, modify_kargs, and iso_kargs_write.
split out the default file offset as an explicit separate return variable instead of as part of the other file offsets
squashed all the commits down into a single one to keep the final git history clean

There are some more tweaks I'd like to do, but the code works well as is!

Requires: coreos/coreos-assembler#1985

nikita-dubrovskii · 2020-12-24T09:45:53Z

Hope you don't mind, I picked this up again and pushed some refactors on top!

Np, thx for the updates

* removed `embed` since it duplicates `add`

embed could be better just to replace any existing kargs, but i don't mind

* made `add`, `replace`, and `remove` take multiple kargs instead of just one (on that subject, I think I'd like to change the CLI further so that the commands are just `modify/show/reset` and we then have `modify` be able to take multiple `--add`/`--replace`/`--remove` switches; that way you can do all your modifications in one invocation -- WDYT?)

Hmm, i'm not a fan of many --xyz switches, but here it looks reasonable

* made `replace` just support `KEY=OLD=NEW` without having to surround `OLD` and `NEW` with single quotes since it's more natural. I'm guessing you originally did it that way to avoid space escaping issues? Worth noting `rpm-ostree kargs` has been using that format for a while and that hasn't come up yet (and I guess in the worst case if there really is some space escaping issues, users can always fall back to `remove` and `add`)

Yep, quotes were used to handle spaces, at least i was testing it.

LGTM

jlebon · 2021-01-04T17:34:39Z

OK, split out some prep patches into separate commits and simplified it down to having just modify, reset, and show!

This is mostly a revert of coreos#1652, though with a spin on it: we automatically detect if the `coreos-installer` in the cosa container has coreos/coreos-installer#341. This will allow us to test the functionality in that PR's CI before merging, as well as avoiding ugly warnings until it gets into cosa proper. (This addresses the original reason for the revert.) Another difference is that we capture the stderr and make it part of the warning to make it less scary if operating on an older CoreOS ISO and to make it clear that it's non-fatal.

That way, cosa itself uses the fresh binaries when calling out to `coreos-installer` (e.g. for downloading images, injecting Ignition configs, etc... and soon also injecting ISO kargs).

Then, `bls_entry_options_delete_and_append_kargs` is a thin wrapper around that new function to convert to the callback semantics we need. Prep for future patch.

jlebon · 2021-01-06T19:41:09Z

To clarify, once we're agreed on the general direction here, my suggestion is to merge coreos/coreos-assembler#1989 and then we should be able to verify in CI here that it's working by looking at the console log output for kola testiso tests. I can also add some more tests to exercise reset and show too.

This is mostly a revert of coreos#1652, though with a spin on it: we automatically detect if the `coreos-installer` in the cosa container has coreos/coreos-installer#341. This will allow us to test the functionality in that PR's CI before merging, as well as avoiding ugly warnings until it gets into cosa proper. (This addresses the original reason for the revert.) Another difference is that we capture the stderr and make it part of the warning to make it less scary if operating on an older CoreOS ISO and to make it clear that it's non-fatal.

bgilbert

LGTM generally!

Cargo.toml

src/install.rs

.cci.jenkinsfile

src/live.rs

src/cmdline.rs

src/live.rs

This is mostly a revert of #1652, though with a spin on it: we automatically detect if the `coreos-installer` in the cosa container has coreos/coreos-installer#341. This will allow us to test the functionality in that PR's CI before merging, as well as avoiding ugly warnings until it gets into cosa proper. (This addresses the original reason for the revert.) Another difference is that we capture the stderr and make it part of the warning to make it less scary if operating on an older CoreOS ISO and to make it clear that it's non-fatal.

This matches rpm-ostree's `kargs --replace` syntax as well. This functionality isn't actually used by anything, but it's prep for that.

Prep for future patch.

We were already using this shorter help text for `iso ignition embed --output`. Let's use that for all the other similar `--output` flags to be consistent, and also because the next patch introduces another command which will use this and it will help staying within the 80 character column limit.

jlebon · 2021-01-08T20:27:06Z

Updated and now with a test on the FCOS ISO! Once I moved the embed area-related values into a struct, I refactored things a bit more to just put the functions in the struct's impl which made things even nicer!

jlebon · 2021-01-08T21:39:18Z

Cool! We can see coreos/coreos-assembler#1989 in action in the iso-install console logs:

[    0.000000] Command line: BOOT_IMAGE=/images/pxeboot/vmlinuz initrd=/images/pxeboot/initrd.img,
/images/ignition.img mitigations=auto,nosmt systemd.unified_cgroup_hierarchy=0 coreos.liveiso=fedo
ra-coreos-33.20210108.dev.0 ignition.firstboot ignition.platform.id=metal console=ttyS0
...

tests/test-iso-kargs.sh

src/live.rs

The new commands are `coreos-installer iso kargs modify/reset/show`. These allow modifying the baked in kernel arguments in a CoreOS live ISO. My primary motivation for this is for embedding the right serial console argument into the ISO before booting it: ``` $ coreos-installer iso kargs modify --append console=ttyS0 my.iso $ coreos-installer iso kargs show my.iso mitigations=auto,nosmt ... console=ttyS0 $ cosa run --qemu-iso my.iso -c ... [ 0.000000] Command line: BOOT_IMAGE=/images/vmlinuz ... console=ttyS0 ... ``` This example specifically demonstrates what cosa will do automatically itself once it can rely on this functionality. This will for example allow `testiso` tests to be much more useful because we'll actually have console logs. But it's of course useful generically. Another way to look at it is that it mirrors/brings to parity the provisioning capabilities of the live PXE path. Co-authored-by: Nikita Dubrovskii <nikita@linux.ibm.com>

jlebon · 2021-01-11T14:59:33Z

Updated!

Document additions from coreos#341

This was referenced Aug 4, 2020

qemu: Automatically inject serial console karg into ISO coreos/coreos-assembler#1638

Merged

live: add karg embed areas in GRUB/ISOLINUX configs coreos/fedora-coreos-config#545

Merged

bgilbert mentioned this pull request Aug 5, 2020

Deprecate "iso (.*)" in favor of "iso ignition $1" #343

Merged

cgwalters approved these changes Aug 6, 2020

View reviewed changes

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Aug 6, 2020

qemu: fix coreos-installer karg injection command

62d3155

This was changed to `coreos-installer iso appended-kargs` in later reviews: coreos/coreos-installer#341

jlebon changed the title ~~iso: add commands to manage additional kernel args~~ iso: add appended-kargs commands to manage additional kernel args Aug 6, 2020

jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Aug 8, 2020

qemu: fix coreos-installer karg injection command

bfcbb1b

This was changed to `coreos-installer iso appended-kargs` in later reviews: coreos/coreos-installer#341

jlebon mentioned this pull request Aug 10, 2020

Issue installing KVM guest from FCOS Live ISO (have to add console=ttyS0) coreos/fedora-coreos-tracker#597

Open

jlebon added the status/on-hold label Aug 11, 2020

nikita-dubrovskii mentioned this pull request Oct 22, 2020

buildextend-live: add karg embed area offsets to header coreos/coreos-assembler#1637

Merged

nikita-dubrovskii requested a review from cgwalters November 2, 2020 10:24

cgwalters mentioned this pull request Nov 4, 2020

Provide minimal ISO coreos/fedora-coreos-tracker#661

Closed

jlebon changed the title ~~iso: add appended-kargs commands to manage additional kernel args~~ iso: add kargs commands to manage kernel args Dec 23, 2020

jlebon mentioned this pull request Jan 4, 2021

buildextend-live: make default kargs file look the same coreos/coreos-assembler#1985

Merged

jlebon mentioned this pull request Jan 5, 2021

qemu: automatically inject serial console karg into ISO coreos/coreos-assembler#1989

Merged

jlebon added 2 commits January 5, 2021 11:31

ci: install built binaries into buildroot

f9a3bed

That way, cosa itself uses the fresh binaries when calling out to `coreos-installer` (e.g. for downloading images, injecting Ignition configs, etc... and soon also injecting ISO kargs).

install: factor out function to modify kargs

d312d7a

Then, `bls_entry_options_delete_and_append_kargs` is a thin wrapper around that new function to convert to the callback semantics we need. Prep for future patch.

bgilbert reviewed Jan 8, 2021

View reviewed changes

bgilbert removed the status/on-hold label Jan 8, 2021

bgilbert reviewed Jan 8, 2021

View reviewed changes

src/live.rs Outdated Show resolved Hide resolved

jlebon added 3 commits January 8, 2021 14:54

install: support replacing kargs using key=oldval=newval syntax

0f27b49

This matches rpm-ostree's `kargs --replace` syntax as well. This functionality isn't actually used by anything, but it's prep for that.

live: make Ignition header size and magic constants

00dfcda

Prep for future patch.

bgilbert reviewed Jan 11, 2021

View reviewed changes

bgilbert approved these changes Jan 11, 2021

View reviewed changes

bgilbert merged commit ed57f21 into coreos:master Jan 11, 2021

travier added a commit to travier/coreos-installer that referenced this pull request Jul 1, 2021

docs: Add iso kargs modify/reset/show

15bc92e

Document additions from coreos#341

travier added a commit to travier/coreos-installer that referenced this pull request Jul 1, 2021

docs: Add iso kargs modify/reset/show

0d4f3d4

Document additions from coreos#341

travier mentioned this pull request Jul 1, 2021

docs: Add iso kargs modify/reset/show #567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iso: add `kargs` commands to manage kernel args #341

iso: add `kargs` commands to manage kernel args #341

jlebon commented Aug 4, 2020 •

edited

Loading

jlebon commented Aug 4, 2020

lucab commented Aug 5, 2020

jlebon commented Aug 5, 2020

bgilbert commented Aug 5, 2020

dustymabe commented Aug 5, 2020

bgilbert commented Aug 5, 2020

bgilbert commented Aug 5, 2020

lucab commented Aug 6, 2020

jlebon commented Aug 6, 2020

lucab commented Aug 6, 2020

cgwalters Aug 6, 2020

jlebon Aug 6, 2020

jlebon Aug 6, 2020

jlebon Aug 7, 2020

cgwalters Aug 6, 2020

jlebon Aug 6, 2020

jlebon commented Aug 6, 2020

bgilbert commented Aug 8, 2020

jlebon commented Aug 8, 2020

jlebon commented Oct 23, 2020

nikita-dubrovskii commented Oct 26, 2020

cgwalters commented Oct 27, 2020

nikita-dubrovskii commented Oct 27, 2020

cgwalters commented Oct 27, 2020

nikita-dubrovskii commented Oct 28, 2020

jlebon commented Dec 23, 2020 •

edited

Loading

nikita-dubrovskii commented Dec 24, 2020

jlebon commented Jan 4, 2021

jlebon commented Jan 6, 2021

bgilbert left a comment

jlebon commented Jan 8, 2021

jlebon commented Jan 8, 2021

jlebon commented Jan 11, 2021

iso: add kargs commands to manage kernel args #341

iso: add kargs commands to manage kernel args #341

Conversation

jlebon commented Aug 4, 2020 • edited Loading

jlebon commented Aug 4, 2020

lucab commented Aug 5, 2020

jlebon commented Aug 5, 2020

bgilbert commented Aug 5, 2020

dustymabe commented Aug 5, 2020

bgilbert commented Aug 5, 2020

bgilbert commented Aug 5, 2020

lucab commented Aug 6, 2020

jlebon commented Aug 6, 2020

lucab commented Aug 6, 2020

cgwalters Aug 6, 2020

Choose a reason for hiding this comment

jlebon Aug 6, 2020

Choose a reason for hiding this comment

jlebon Aug 6, 2020

Choose a reason for hiding this comment

jlebon Aug 7, 2020

Choose a reason for hiding this comment

cgwalters Aug 6, 2020

Choose a reason for hiding this comment

jlebon Aug 6, 2020

Choose a reason for hiding this comment

jlebon commented Aug 6, 2020

bgilbert commented Aug 8, 2020

jlebon commented Aug 8, 2020

jlebon commented Oct 23, 2020

nikita-dubrovskii commented Oct 26, 2020

cgwalters commented Oct 27, 2020

nikita-dubrovskii commented Oct 27, 2020

cgwalters commented Oct 27, 2020

nikita-dubrovskii commented Oct 28, 2020

jlebon commented Dec 23, 2020 • edited Loading

nikita-dubrovskii commented Dec 24, 2020

jlebon commented Jan 4, 2021

jlebon commented Jan 6, 2021

bgilbert left a comment

Choose a reason for hiding this comment

jlebon commented Jan 8, 2021

jlebon commented Jan 8, 2021

jlebon commented Jan 11, 2021

iso: add `kargs` commands to manage kernel args #341

iso: add `kargs` commands to manage kernel args #341

jlebon commented Aug 4, 2020 •

edited

Loading

jlebon commented Dec 23, 2020 •

edited

Loading