bootconfig: Preserve extension BLS keys across staged deployments#3570
bootconfig: Preserve extension BLS keys across staged deployments#3570jmarrero wants to merge 1 commit intoostreedev:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Code Review
The changes effectively address the problem of preserving custom bootconfig keys across staged deployment finalization. The introduction of _ostree_bootconfig_parser_get_extra_keys_variant and its integration into the staging and reloading process is well-implemented. The new test cases thoroughly validate the functionality, including edge cases and the full serialization/deserialization roundtrip. The code is clear, well-structured, and adheres to good practices for GVariant handling and memory management.
8b45dc4 to
95b81f1
Compare
cgwalters
left a comment
There was a problem hiding this comment.
Backing up a second...I had vaguely thought we'd have "magic comments" and not add new keys.
I have a concern that other tools might choke on unknown keys. I guess it's hard to know.
Bikeshed on naming, how about x-ostree-options-source-<name> e.g. x-ostree-options-tuned
e1ab545 to
4294e40
Compare
cgwalters
left a comment
There was a problem hiding this comment.
Looks sane to me! I guess a question here is...do we actually push the logic for kargs here like ostree kargs --source=foo here and then rpm-ostree could reuse that?
If so, then we could add more proper integration tests here.
|
|
||
| /* Magic comments preserved across parse/write roundtrips. | ||
| * Each entry is the comment body (without the leading "# "), | ||
| * e.g. "x-ostree-options-source-tuned nohz=full isolcpus=1-3". |
There was a problem hiding this comment.
In theory, this could be used outside of ostree (like bootc w/composefs non-sealed UKI). In fact we almost certainly should support it eventually there. It doesn't matter that the naming has ostree- as the prefix, but...in theory the workflow on the bootc upgrade side would need similar logic to preserve comments. Should be straightforward.
Hmm, feels like it's worth doing a bit more investigation before settling on magic comments. FWIW, throwing AI against systemd-boot and rhboot/grub2 and asking "in the BLS implementation, what happens if we encounter a key we don't know about", and both came back saying it would just be ignored. Obviously not hard to test either. Though really, since ostree owns the BLS entries, couldn't we just colocate a file in whatever preferred format in the |
Hmm. You made me think here...since actually what we just need here is data lifecycle bound to the deployment, we could store it ~anywhere. But probably the most obvious one is the origin file. And if we do that, no new APIs are needed here at all right? |
|
Also looks like zipl ignores unknown keys https://github.com/ibm-s390-linux/s390-tools/blob/d0046257b60c4f79ec506aa5a664fad471dae247/zipl/src/job.c#L1674 So yeah I'm fine with going with x-ostree instead of magic comments. |
|
Just thinking about the medium term, what I think would be nice is:
-- We could head towards that place by:
|
|
We would still need to modify ostree no? To make sure don't drop these |
Adds support for merging disparate kernel argument sources into the single BLS entry `options` field. Each source (e.g., TuneD, admin, bootc kargs.d) can independently manage its own set of kernel arguments, tracked via `# x-ostree-options-source-<name>` magic comments in BLS config files. Changes: - Extend BLS parser to parse/preserve source-tracking magic comments - Add new `loader_entries` module with set-options-for-source logic - Add `bootc loader-entries set-options-for-source` CLI command - Add unit tests for merge logic and BLS roundtrip See: ostreedev/ostree#3570 See: #899 Co-authored-by: cgwalters <244096+cgwalters@users.noreply.github.com> Agent-Logs-Url: https://github.com/bootc-dev/bootc/sessions/ce7bf603-8a51-4bd9-8653-6e1221a2bdc6
Yeah, you're right definitely.
Right that gets to a tricky point which is staged deployments. Definitely those should go through the ostree APIs. And the staged thing is really a big wrinkle in this. |
|
So let's say, we revert this PR back to using keys, we call those keys But then, side step rpm-ostree and have |
Yes, though...there's a very special thing going on here which is that for ostree, when we do a staged deployment just to change kernel arguments it's kind of...suboptimal because we end up making a whole new deployment (hardlink farm and a new copy of But with composefs - there's no hardlinks, which is already more efficient. We could take the next step there in theory support having 2 bootloader entries point to one deployment - i.e. we skip staging there. Anyways...that's a detail to take over to the bootc side really. |
a25095a to
4933ce6
Compare
…kargs
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
Example usage:
bootc loader-entries set-options-for-source --source tuned \
--options "isolcpus=1-3 nohz_full=1-3"
See: ostreedev/ostree#3570
See: bootc-dev#899
Assisted-by: OpenCode (Claude claude-opus-4-6)
cgwalters
left a comment
There was a problem hiding this comment.
We'll also need to regenerate the Rust bindings after this.
| cancellable, error); | ||
| } | ||
|
|
||
| /* Allowlist of key prefixes that should be preserved across the staged |
There was a problem hiding this comment.
I am not sure we need an allowlist, was there a rationale for it? In the end we may as well just allow writing arbitrary keys, we kind of have to trust our callers.
There was a problem hiding this comment.
We are trying to serialize the current known keys as we have always been doing until now, then add any new one into bootconfig-extra. If so we would need an block list like a previous iteration so we don't serialize the known ones twice, or we have an allowlist like you suggested on #3570 (comment) which was the intent here.
We could have no block or allow list but when all keys go into bootconfig-extra. Which if I follow correctly could end up in a nasty bug such as:
1. bootc upgrade stages a deployment with a new kernel vmlinuz-6.9.0
2. set-options-for-source re-stages, serializing bootconfig-extra which includes linux /vmlinuz-6.8.0 (from the merge/booted deployment)
3. At finalization, install_deployment_kernel() correctly sets linux /vmlinuz-6.9.0
4. Then _reload_staged() restores bootconfig-extra and overwrites linux back to /vmlinuz-6.8.0
I pushed the updated code using blocklist, let me know if I am missing something.
There was a problem hiding this comment.
Yeah reading back I think I asked for an allowlist before, I'm sorry, I'm inconsistent
When a deployment is staged via ostree_sysroot_stage_tree_with_options(),
the deployment metadata is serialized to /run/ostree/staged-deployment
as a GVariant. During finalization at shutdown,
_ostree_sysroot_reload_staged() creates a fresh OstreeBootconfigParser
and only restores the "options" key from the serialized kargs. Any
additional BLS keys that were set on the bootconfig are silently dropped.
The parse/write/clone paths in OstreeBootconfigParser already handle
unknown keys generically (the "Write unknown fields" loop in
write_at()), so keys survive direct deployments and in-memory
operations. The gap is exclusively in the staged deployment roundtrip,
where a fresh bootconfig is rebuilt from just the kargs strv.
This matters for the upcoming bootc `loader-entries set-options-for-source`
feature, which stores kernel argument ownership as extension BLS keys
(e.g. `x-options-source-tuned nohz=full isolcpus=1-3`). On bootc
systems with transient /etc, tools like TuneD lose track of which kargs
they own because their state files are wiped on reboot. Tracking
ownership directly in the BLS config on /boot solves this, but only if
the keys survive staging. systemd-boot, GRUB, and zipl all ignore
unknown BLS keys, so extension keys are safe.
Fix this by following the same pattern used for overlay-initrds:
1. Add _ostree_bootconfig_parser_get_extra_keys_variant() which returns
all non-standard BLS keys as an a{ss} GVariant. Standard keys
(title, version, options, linux, initrd, devicetree) are excluded
since they are rebuilt from scratch during finalization. All other
keys are preserved, trusting the caller.
2. In ostree_sysroot_stage_tree_with_options(), serialize any extra
keys as "bootconfig-extra" in the staged GVariant dict. Since
_ostree_deployment_set_bootconfig_from_kargs() creates a fresh
bootconfig with only the "options" key, the code falls back to
the merge deployment's bootconfig for extra keys. This ensures
keys are inherited across staged deployments without the caller
needing to re-set them.
3. In _ostree_sysroot_reload_staged(), restore extra keys from the
"bootconfig-extra" dict onto the deployment's bootconfig via
ostree_bootconfig_parser_set().
The function is private (_ostree_ prefix) since only ostree's own
staging code uses it. No new public API, no changes to .sym files,
no changes to GIR or Rust bindings.
Backwards compatibility:
- Old ostree ignores the unknown "bootconfig-extra" key in the a{sv}
dict (extension keys silently lost, same as before this patch).
- New ostree gracefully handles the absence of "bootconfig-extra" in
staged data written by older versions (g_variant_dict_lookup returns
FALSE, no restoration attempted).
Assisted-by: OpenCode (Claude claude-opus-4-6)
Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
cgwalters
left a comment
There was a problem hiding this comment.
I had expected a new option to e.g. ostree_sysroot_stage_tree_with_options or so that allowed injecting new keys; as is how would this be used by callers like bootc?
When a deployment is staged via ostree_sysroot_stage_tree_with_options(),
the deployment metadata is serialized to /run/ostree/staged-deployment
as a GVariant. During finalization at shutdown,
_ostree_sysroot_reload_staged() creates a fresh OstreeBootconfigParser
and only restores the "options" key from the serialized kargs. Any
additional BLS keys that were set on the bootconfig are silently dropped.
The parse/write/clone paths in OstreeBootconfigParser already handle
unknown keys generically (the "Write unknown fields" loop in
write_at()), so keys survive direct deployments and in-memory
operations. The gap is exclusively in the staged deployment roundtrip,
where a fresh bootconfig is rebuilt from just the kargs strv.
This matters for the upcoming bootc
loader-entries set-options-for-sourcefeature, which stores kernel argument ownership as extension BLS keys
(e.g.
x-options-source-tuned nohz=full isolcpus=1-3). On bootcsystems with transient /etc, tools like TuneD lose track of which kargs
they own because their state files are wiped on reboot. Tracking
ownership directly in the BLS config on /boot solves this, but only if
the keys survive staging. systemd-boot, GRUB, and zipl all ignore
unknown BLS keys, so extension keys are safe.
Fix this by following the same pattern used for overlay-initrds:
Add _ostree_bootconfig_parser_get_extra_keys_variant() which returns
all non-standard BLS keys as an a{ss} GVariant. Standard keys
(title, version, options, linux, initrd, devicetree) are excluded
since they are rebuilt from scratch during finalization. All other
keys are preserved, trusting the caller.
In ostree_sysroot_stage_tree_with_options(), serialize any extra
keys as "bootconfig-extra" in the staged GVariant dict. Since
_ostree_deployment_set_bootconfig_from_kargs() creates a fresh
bootconfig with only the "options" key, the code falls back to
the merge deployment's bootconfig for extra keys. This ensures
keys are inherited across staged deployments without the caller
needing to re-set them.
In _ostree_sysroot_reload_staged(), restore extra keys from the
"bootconfig-extra" dict onto the deployment's bootconfig via
ostree_bootconfig_parser_set().
The function is private (ostree prefix) since only ostree's own
staging code uses it. No new public API, no changes to .sym files,
no changes to GIR or Rust bindings.
Backwards compatibility:
dict (extension keys silently lost, same as before this patch).
staged data written by older versions (g_variant_dict_lookup returns
FALSE, no restoration attempted).
Assisted-by: OpenCode (Claude claude-opus-4-6)
Signed-off-by: Joseph Marrero Corchado jmarrero@redhat.com