Skip to content

loader-entries: Add composefs support for set-options-for-source#2161

Draft
jmarrero wants to merge 1 commit intobootc-dev:mainfrom
jmarrero:composefs-loader-entries-source
Draft

loader-entries: Add composefs support for set-options-for-source#2161
jmarrero wants to merge 1 commit intobootc-dev:mainfrom
jmarrero:composefs-loader-entries-source

Conversation

@jmarrero
Copy link
Copy Markdown
Contributor

The ostree path for set-options-for-source (merged in fb8c668) stages a new deployment via ostree APIs. On composefs-booted systems this approach cannot work because the status detection logic treats any entry whose verity matches the booted deployment as "booted" rather than "staged", making same-image kargs-only changes invisible to finalization.

Instead, the composefs path directly modifies BLS entry files on /boot. This is architecturally correct because bootc already manages BLS entries directly on composefs systems, and the BLSConfig.extra HashMap already preserves x-options-source-* extension keys through parse/write roundtrips. No GVariant serialization, ostree version gating, or finalization service is needed.

Changes:

  • Add composefs implementation in bootc_composefs/loader_entries.rs that reads the booted BLS entry, computes the merged options via the shared parsing functions, and writes the updated entry back atomically. When staged entries exist (e.g. from bootc upgrade), they are also updated so finalization does not overwrite the kargs changes.
  • Refactor CLI dispatch to use match storage.kind() for routing between the ostree and composefs backends.
  • Make SourceName, OPTIONS_SOURCE_KEY_PREFIX, extract_source_options_from_bls, and compute_merged_options pub(crate) for reuse by the composefs module.
  • Bail with a clear error on UKI boot type since kargs are embedded in the PE binary.
  • Add composefs-specific TMT test (test-43) with 4 phases / 3 reboots covering input validation, kargs persistence, source replacement, multiple sources, idempotency, and source removal.
  • Add missing fixme_skip_if_composefs metadata to the existing ostree test file header so the TMT generator preserves the skip flag.
  • 8 new unit tests for the composefs path, all existing tests unchanged.

Closes: #899
Assisted-by: OpenCode (Claude Opus 4.6)

The ostree path for set-options-for-source (merged in fb8c668) stages a
new deployment via ostree APIs. On composefs-booted systems this approach
cannot work because the status detection logic treats any entry whose
verity matches the booted deployment as "booted" rather than "staged",
making same-image kargs-only changes invisible to finalization.

Instead, the composefs path directly modifies BLS entry files on /boot.
This is architecturally correct because bootc already manages BLS entries
directly on composefs systems, and the BLSConfig.extra HashMap already
preserves x-options-source-* extension keys through parse/write
roundtrips. No GVariant serialization, ostree version gating, or
finalization service is needed.

Changes:
- Add composefs implementation in bootc_composefs/loader_entries.rs that
  reads the booted BLS entry, computes the merged options via the shared
  parsing functions, and writes the updated entry back atomically. When
  staged entries exist (e.g. from bootc upgrade), they are also updated
  so finalization does not overwrite the kargs changes.
- Refactor CLI dispatch to use match storage.kind() for routing between
  the ostree and composefs backends.
- Make SourceName, OPTIONS_SOURCE_KEY_PREFIX, extract_source_options_from_bls,
  and compute_merged_options pub(crate) for reuse by the composefs module.
- Bail with a clear error on UKI boot type since kargs are embedded in
  the PE binary.
- Add composefs-specific TMT test (test-43) with 4 phases / 3 reboots
  covering input validation, kargs persistence, source replacement,
  multiple sources, idempotency, and source removal.
- Add missing fixme_skip_if_composefs metadata to the existing ostree
  test file header so the TMT generator preserves the skip flag.
- 8 new unit tests for the composefs path, all existing tests unchanged.

Closes: bootc-dev#899
Assisted-by: OpenCode (Claude Opus 4.6)
@bootc-bot bootc-bot Bot requested a review from ckyrouac April 23, 2026 19:34
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 implements a composefs backend for source-tracked kernel arguments, enabling direct modification of BLS entry files on /boot. This approach is more efficient for composefs-booted systems as it avoids ostree-specific staging and finalization. The changes include a new loader_entries module for composefs, CLI updates to route commands based on storage type, and comprehensive integration tests. Feedback focuses on improving the robustness of BLS entry updates: specifically, ensuring all matching entries are updated when multiple files share a version string and properly propagating errors when updating staged entries to prevent inconsistent system states.

Comment on lines +110 to +146
) -> Result<bool> {
for entry in entries_dir.entries_utf8()? {
let entry = entry?;
let file_name = entry.file_name()?;
if !file_name.ends_with(".conf") {
continue;
}
let content = entries_dir
.read_to_string(&file_name)
.with_context(|| format!("Reading BLS entry {file_name}"))?;
let mut bls =
parse_bls_config(&content).with_context(|| format!("Parsing BLS entry {file_name}"))?;

if bls.version().to_string() != target_version {
continue;
}

// Skip EFI/UKI entries — can't modify their options
if !matches!(bls.cfg_type, BLSConfigType::NonEFI { .. }) {
continue;
}

let current_options = get_options_str(&bls)?;
let source_options = extract_source_options_from_extra(&bls);
let merged = compute_merged_options(&current_options, &source_options, source, new_options);

update_bls_config(&mut bls, &merged, source, new_options, &source_options)?;

entries_dir
.atomic_write(&file_name, bls.to_string().as_bytes())
.with_context(|| format!("Writing updated BLS entry {file_name}"))?;

tracing::info!("Updated BLS entry '{file_name}' with kargs for source '{source}'");
return Ok(true);
}
Ok(false)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The update_bls_entry_in_dir function currently returns early after finding the first entry that matches target_version. If multiple BLS entries share the same version (which can happen with rollback entries as noted in your comments on line 234), only the first one encountered in the directory iteration will be updated. This can lead to inconsistent kernel arguments across deployments that share a version string.

Consider iterating through all entries and updating every match to ensure consistency.

fn update_bls_entry_in_dir(
    entries_dir: &Dir,
    target_version: &str,
    source: &SourceName,
    new_options: Option<&str>,
) -> Result<bool> {
    let mut found = false;
    for entry in entries_dir.entries_utf8()? {
        let entry = entry?;
        let file_name = entry.file_name()?;
        if !file_name.ends_with(".conf") {
            continue;
        }
        let content = entries_dir
            .read_to_string(&file_name)
            .with_context(|| format!("Reading BLS entry {file_name}"))?;
        let mut bls =
            parse_bls_config(&content).with_context(|| format!("Parsing BLS entry {file_name}"))?;

        if bls.version().to_string() != target_version {
            continue;
        }

        // Skip EFI/UKI entries — can't modify their options
        if !matches!(bls.cfg_type, BLSConfigType::NonEFI { .. }) {
            continue;
        }

        let current_options = get_options_str(&bls)?;
        let source_options = extract_source_options_from_extra(&bls);
        let merged = compute_merged_options(&current_options, &source_options, source, new_options);

        update_bls_config(&mut bls, &merged, source, new_options, &source_options)?;

        entries_dir
            .atomic_write(&file_name, bls.to_string().as_bytes())
            .with_context(|| format!("Writing updated BLS entry {file_name}"))?;

        tracing::info!("Updated BLS entry '{file_name}' with kargs for source '{source}'");
        found = true;
    }
    Ok(found)
}

let staged_version = staged_bls.version().to_string();
// Update each staged entry (best effort — some may be rollback
// entries that share our version)
let _ = update_bls_entry_in_dir(&staged_dir, &staged_version, &source, new_options);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Errors encountered while updating staged BLS entries are currently ignored. If an update fails (e.g., due to I/O issues), the system might be left in an inconsistent state where the booted entry is updated but staged entries are not. This could lead to the kernel argument changes being lost after a subsequent upgrade finalization. It is recommended to propagate these errors.

            update_bls_entry_in_dir(&staged_dir, &staged_version, &source, new_options)
                .with_context(|| format!("Updating staged BLS entry (version '{staged_version}')"))?;

@cgwalters
Copy link
Copy Markdown
Collaborator

I don't like the "Changes" section in commit messages like this. I did bootc-dev/infra#171 to try to tweak that.

Comment on lines +67 to +72
match &mut bls.cfg_type {
BLSConfigType::NonEFI { options, .. } => {
*options = Some(merged_options.clone());
}
_ => anyhow::bail!("BLS entry is not a NonEFI (BLS) type"),
}
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.

Some overlap with get_options_str above

@@ -0,0 +1,209 @@
# number: 43
# tmt:
# summary: Test bootc loader-entries set-options-for-source on composefs
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.

But can't we have just one test that works on both backends?

//! # Composefs backend for source-tracked kernel arguments
//!
//! This module implements `set-options-for-source` for composefs-booted systems.
//! Unlike the ostree path (which stages a new deployment via ostree APIs), the
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.

That's not true, but we were missing docs for it. I took a stab at this in #2168

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.

/etc/bootc/kargs.d

2 participants