Skip to content

install: Add --karg-delete#2105

Merged
cgwalters merged 5 commits intobootc-dev:mainfrom
hone:install-karg-delete
Apr 3, 2026
Merged

install: Add --karg-delete#2105
cgwalters merged 5 commits intobootc-dev:mainfrom
hone:install-karg-delete

Conversation

@hone
Copy link
Copy Markdown
Contributor

@hone hone commented Mar 29, 2026

Following up from the contribfest session. This adds --karg-delete to install.

In some relatively rare use cases, one might want to remove a kernel argument shipped in the container image.

Fixes: #1229

@github-actions github-actions bot added the area/install Issues related to `bootc install` label Mar 29, 2026
@bootc-bot bootc-bot bot requested a review from jmarrero March 29, 2026 05:06
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the ability to remove kernel arguments during installation via a new --karg-delete CLI option and a corresponding karg-deletes field in the installation configuration. The implementation includes logic to process these deletions before applying new arguments, a helper function for precise or key-based removal, and comprehensive unit and integration tests to verify the functionality. I have no feedback to provide.

@hone
Copy link
Copy Markdown
Contributor Author

hone commented Mar 29, 2026

I wasn't sure about a few things when implementing this.

  1. What is the preferred order of precedence for how --karg and --karg-delete interface. kargs can come from root_setup, install_config, and the CLI. In order to let you delete kargs and allow you to replace it with a CLI flag, I deleted the kargs before applying the CLI flags.
  2. Do all install CLI flags also get applied to config? I modeled a configuration option based off of kargs.
  3. I wasn't sure which tests to add, so I added some unit tests, integration, and tmt tests.

@hone hone force-pushed the install-karg-delete branch 2 times, most recently from 22c5fa8 to 8cb46ce Compare March 29, 2026 09:42
hone added 4 commits March 31, 2026 03:21
This adds `--karg-delete` to install.

In some relatively rare use cases, one might want to remove a kernel argument shipped in the container image.

Fixes: bootc-dev#1229
Signed-off-by: Terence Lee <hone02@gmail.com>
Signed-off-by: Terence Lee <hone02@gmail.com>
Signed-off-by: Terence Lee <hone02@gmail.com>
Signed-off-by: Terence Lee <hone02@gmail.com>
@hone hone force-pushed the install-karg-delete branch from ccccf8a to ddbbb3f Compare March 31, 2026 08:21
@github-actions github-actions bot added the area/documentation Updates to the documentation label Mar 31, 2026
@hone
Copy link
Copy Markdown
Contributor Author

hone commented Mar 31, 2026

For my tmt test, I used fixme_skip_if_composefs: true. It didn't look like the composefs boot supported the CLI flags.

…plemented for composefs

Signed-off-by: Terence Lee <hone02@gmail.com>
@hone hone force-pushed the install-karg-delete branch from 35c3e5d to 87f13a7 Compare April 1, 2026 13:41
@henrywang
Copy link
Copy Markdown
Collaborator

Hi @hone, please skip Packit s390x rpm build issue. The build is successful, but status can't be updated in PR. I already ask for help in Packit channel.

# summary: Test bootc install --karg-delete
# duration: 30m
# extra:
# fixme_skip_if_composefs: true
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should work for composefs backend as well since the config gathering in the install path is pretty much the same for ostree and composefs. Were you getting some sort of error when testing this for composefs backend?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just that if you use --to-filesystem you need to setup the entire boot partition table, so you can't use --bootloader none.

I was looking through setup_composefs_boot and it didn't look like it handled the CLI karg/-delete args I was working on in install_container.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can only do this with type1 BLS though to be clear with composefs, not UKIs. We have to error out on any local karg manipulations there.

Copy link
Copy Markdown
Collaborator

@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.

Thanks! Looks sane offhand

Comment on lines +1157 to +1166
if let Some(install_config_karg_deletes) = install_config_karg_deletes {
for karg_delete in install_config_karg_deletes {
karg_deletes.push(karg_delete);
}
}
if let Some(deletes) = state.config_opts.karg_delete.as_ref() {
for karg_delete in deletes {
karg_deletes.push(karg_delete);
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Minor style nit I think this could all be one combinator chain via .chain().flatten().collect()


pub(crate) fn delete_kargs(existing: &mut Cmdline, deletes: &Vec<&str>) {
for delete in deletes {
if let Some(param) = utf8::Parameter::parse(&delete) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm when would we get None from parsing here? Just when it's an empty string right? We should probably have filtered those out or errored if I'm right

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Empty strings or strings with only whitespace will return None, anything else should successful parse.

@cgwalters cgwalters merged commit fa95cfd into bootc-dev:main Apr 3, 2026
61 of 65 checks passed
@hone hone deleted the install-karg-delete branch April 3, 2026 19:54
@cgwalters
Copy link
Copy Markdown
Collaborator

What is the preferred order of precedence for how --karg and --karg-delete interface. kargs can come from root_setup, install_config, and the CLI. In order to let you delete kargs and allow you to replace it with a CLI flag, I deleted the kargs before applying the CLI flags.

I think as implemented the CLI should always win, right? We should probably error with --karg=foo and --karg-delete=foo though.

Do all install CLI flags also get applied to config? I modeled a configuration option based off of kargs.

I'm not totally parsing this - it's more that the CLI and config all fold into the bootloader ocnfig.

I wasn't sure which tests to add, so I added some unit tests, integration, and tmt tests.

It's confusing, what you did is fine, but a lot of the install tests were already duplicated. It's a complex problem though, I'll write up a separate tracker issue for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/documentation Updates to the documentation area/install Issues related to `bootc install`

Projects

None yet

Development

Successfully merging this pull request may close these issues.

install: Add --karg-delete

5 participants