loader-entries: Add set-options-for-source for source-tracked kargs#2114
loader-entries: Add set-options-for-source for source-tracked kargs#2114jmarrero wants to merge 2 commits intobootc-dev:mainfrom
Conversation
|
Draft while I continue testing. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the loader-entries subcommand to manage kernel arguments from multiple independent sources by tracking them via extension keys in BLS configuration files. The implementation includes logic for merging source-specific options and staging new deployments. Review feedback identifies a logic gap in source discovery for staged deployments that could lead to incorrect merging or duplicate arguments, and also recommends replacing blocking synchronous I/O with asynchronous alternatives to prevent performance issues within the async executor.
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
I only did a quick skim, mostly style stuff so far
When set-options-for-source is called multiple times before rebooting, the second call needs to find the target source's previous value in the staged deployment's bootconfig (not the booted BLS, since we haven't rebooted). Without this, the old kargs from the first call would not be removed from the options line, causing duplication. Fix by explicitly checking the target source in the staged bootconfig after iterating known sources from the booted BLS entry. Addresses review feedback from PR bootc-dev#2114. Assisted-by: OpenCode (Claude claude-opus-4-6)
jmarrero
changed the title
|
/gemini review |
Add support for building the bootc test image with a custom ostree built from source. When BOOTC_ostree_src is set to a path to an ostree source tree, the build system: 1. Builds ostree RPMs inside a container matching the base image distro (via contrib/packaging/Dockerfile.ostree-override) 2. Installs ostree-devel into the buildroot so bootc links against the patched libostree 3. Installs ostree + ostree-libs into the final image The override ostree is built as version 2026.1 by default (configurable via BOOTC_ostree_version) to ensure it is always newer than the stock package. This is important for runtime version checks like ostree::check_version(). The Dockerfile includes a `FROM scratch as ostree-packages` fallback stage so that builds without BOOTC_ostree_src (including CI) continue to work unchanged — the empty stage satisfies the mount reference without requiring a build context. The same pattern can be reused for other dependency overrides (e.g. composefs) in the future. Usage: BOOTC_ostree_src=/path/to/ostree just build BOOTC_ostree_src=/path/to/ostree just test-tmt loader-entries-source Assisted-by: OpenCode (Claude Opus 4.6) Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the bootc loader-entries set-options-for-source subcommand, enabling independent management of kernel arguments from multiple sources (e.g., TuneD, admin) via custom x-options-source-* BLS extension keys. The changes include a new loader_entries module, CLI integration, documentation, and integration tests, along with a mechanism to build and inject a patched ostree for development. Feedback identifies a fragile string matching logic for identifying BLS entries, a logic bug that could lead to dropped source keys when multiple updates occur before a reboot, and the requirement to enable the ostree version check to ensure necessary bootconfig-extra support.
Add a new `bootc loader-entries set-options-for-source` command that
manages kernel arguments from independent sources (e.g. TuneD, admin)
by tracking ownership via `x-options-source-<name>` extension keys in
BLS config files. This solves the problem of karg accumulation on bootc
systems with transient /etc, where tools like TuneD lose their state
files on reboot and can't track which kargs they previously set.
The command stages a new deployment with the updated kargs and source
keys. The kargs diff is computed by removing the old source's args and
adding the new ones, preserving all untracked options. Source keys
survive the staging roundtrip via ostree's `bootconfig-extra`
serialization (ostree >= 2026.1, version check present but commented
out until release).
Staged deployment handling:
- No staged deployment: stages based on the booted commit
- Staged deployment exists (e.g. from `bootc upgrade`): replaces it
using the staged commit and origin, preserving pending upgrades while
layering the source kargs change on top
Includes a multi-reboot TMT integration test covering: input validation,
kargs surviving staging roundtrip, source replacement, multiple sources
coexisting, source removal, idempotency, system kargs preservation,
empty --options vs no --options, and staged deployment interaction with
bootc switch.
Example usage:
bootc loader-entries set-options-for-source --source tuned \
--options "isolcpus=1-3 nohz_full=1-3"
See: ostreedev/ostree#3570
Assisted-by: OpenCode (Claude Opus 4.6)
Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
Is how this was tested locally. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to manage kernel arguments from multiple independent sources (such as TuneD or system administrators) by tracking ownership via x-options-source-<name> extension keys in Boot Loader Specification (BLS) entries. It adds a new bootc loader-entries set-options-for-source command, updates the build system to allow injecting custom-built ostree RPMs (required for the new bootconfig-extra support), and includes comprehensive integration tests and documentation. Feedback focuses on the ostree version check logic, potential edge cases in BLS key parsing, and maintaining consistency when handling empty 'tombstone' values in the boot configuration.
| let Some((source_name, value)) = rest.split_once(|c: char| c.is_ascii_whitespace()) else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
The current logic for parsing x-options-source-* keys relies on split_once with whitespace. If a key exists in the BLS file but has no value (e.g., just x-options-source-tuned at the end of a line), it will be silently ignored. While a source without options might seem useless, it could be used to track the presence of a tool. Consider handling the case where no whitespace is present to at least recognize the source name.
| if content.lines().any(|line| { | ||
| line.starts_with("options ") | ||
| && line.split_ascii_whitespace().any(|arg| { | ||
| arg.strip_prefix("ostree=") | ||
| .is_some_and(|path| path.ends_with(&ostree_match)) | ||
| }) | ||
| }) { |
There was a problem hiding this comment.
Matching the BLS entry by manually parsing lines for options and checking for ostree= is somewhat fragile. While BootconfigParser doesn't expose key iteration, it might be safer to use it to parse the options line properly rather than relying on split_ascii_whitespace which might fail if arguments contain escaped spaces or other complex shell-like syntax.
| // them to empty string. BootconfigParser has no remove() API, so "" | ||
| // acts as a tombstone. An empty x-options-source-* key is harmless: | ||
| // extract_source_options_from_bls will parse it as an empty value, | ||
| // and the idempotency check skips empty values (!val.is_empty()). |
There was a problem hiding this comment.
Using an empty string as a 'tombstone' for BootconfigParser is a clever workaround for the lack of a remove() API. However, ensure that extract_source_options_from_bls consistently ignores these empty values. Currently, it inserts them into the map, and they are only filtered out during discovery in set_options_for_source_staged (line 252). It might be cleaner to filter them out directly in extract_source_options_from_bls if the value is empty after trimming.
References
- Maintain consistent handling of empty or 'tombstone' values across different parts of the module to avoid unexpected behavior.
2 participants
Add a new
bootc loader-entries set-options-for-sourcecommand thatmanages kernel arguments from independent sources (e.g. TuneD, admin)
by tracking ownership via
x-options-source-<name>extension keys inBLS config files. This solves the problem of karg accumulation on bootc
systems with transient /etc, where tools like TuneD lose their state
files on reboot and can't track which kargs they previously set.
The command stages a new deployment with the updated kargs and source
keys. The kargs diff is computed by removing the old source's args and
adding the new ones, preserving all untracked options. Source keys
survive the staging roundtrip via ostree's
bootconfig-extraserialization (ostree >= 2026.1, version check).
Staged deployment handling:
bootc upgrade): replaces itusing the staged commit and origin, preserving pending upgrades while
layering the source kargs change on top
Includes a multi-reboot TMT integration test covering: input validation,
kargs surviving staging roundtrip, source replacement, multiple sources
coexisting, source removal, idempotency, system kargs preservation,
empty --options vs no --options, and staged deployment interaction with
bootc switch.
Example usage:
bootc loader-entries set-options-for-source --source tuned
--options "isolcpus=1-3 nohz_full=1-3"
See: ostreedev/ostree#3570
Assisted-by: OpenCode (Claude Opus 4.6)
Signed-off-by: Joseph Marrero Corchado jmarrero@redhat.com