composefs: Refactoring to use storage #1718

cgwalters · 2025-10-31T13:06:06Z

A key goal here is to avoid global state and have as much as possible be fd-relative as Storage encourages, so that things can work consistently in the install and update paths.

gemini-code-assist

Code Review

This pull request introduces a significant and valuable refactoring to eliminate global state by introducing a Storage struct. This change greatly improves the code's testability and maintainability by making dependencies explicit and using file-descriptor-relative operations. The refactoring has been applied consistently across the composefs and ostree paths.

My review focuses on the correctness of this refactoring. I've found one logical issue in the command-line interface where backend-specific logic is not correctly dispatched, and a minor opportunity for code simplification. Overall, this is a great step forward for the codebase.

crates/lib/src/cli.rs

crates/lib/src/status.rs

crates/lib/src/cli.rs

Johan-Liebert1 · 2025-11-03T07:18:18Z

crates/lib/src/cli.rs

+            switch_ostree(opts, storage, &booted_ostree).await
+        }
+        BootedStorageKind::Composefs(booted_cfs) => {
+            if opts.mutate_in_place {


For composefs we would require a hard reboot for any switch right? I'm not sure if we can switch without rebooting. Maybe we could for Type1 booted systems, but we will definitely need to reboot for UKIs

No I think the same principle applies - yes a new UKI would have a new rootfs digest always and hence be different, but I think what we want here is to check the kernel version, and if that's the same, then we can do a soft reboot.

Johan-Liebert1 · 2025-11-04T08:28:58Z

So

bootc/crates/tests-integration/src/container.rs

Line 12 in 27b6189

pub(crate) fn test_bootc_status() -> Result<()> {

fails. This test is supposed to be run inside a container and should be expected to fail. I'm not sure how it was passing until now

Johan-Liebert1 · 2025-11-04T09:36:53Z

My bad, the issue was not the test failing, but the fact that previously we didn't error out if the system wasn't deployed via bootc, but now we do.

Johan-Liebert1 · 2025-11-05T13:29:01Z

@cgwalters Could you review my changes?

cgwalters · 2025-11-05T13:48:31Z

Hmmm. Yes this is complicated. I am not quite happy with having storage call back into the CLI code, we should clean that up.

I think we need to have a higher level wrapper here that returns Result<Option<BootedStorage>>

cgwalters · 2025-11-05T14:54:52Z

Also this shows another bug, we definitely need to do the "re-exec self" dance even in the composefs path.

Distinguishing isn't useful and we want to get away from having ostree repos in images anyways. Signed-off-by: Colin Walters <[email protected]>

We detect things later regardless, and this is prep for composefs. Signed-off-by: Colin Walters <[email protected]>

Use implicit return expressions instead of explicit `return` statements for the final expression in functions, following Rust conventions. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

Refactor the storage layer to introduce new types that better encapsulate boot context: - Add BootedStorage wrapper that detects ostree vs composefs boot - Add BootedOstree struct holding sysroot reference and deployment - Add BootedComposefs struct holding repository and cmdline - Add BootedStorageKind enum to discriminate between backends This allows passing boot context as a cohesive unit rather than separate parameters. Update the upgrade code path to use these new types: - Change get_status() to accept &BootedOstree instead of separate params - Update handle_staged_soft_reboot() similarly - Update upgrade() to use structs throughout Add comprehensive documentation to all new types and methods in store/mod.rs. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

Apply the same pattern used in upgrade() to switch, rollback, and edit commands. This eliminates redundant composefs_booted() calls from the main CLI code paths by consistently using storage.kind() for dispatch. Changes: - Split switch/rollback/edit into dispatcher + _ostree() helper functions - Update get_status_require_booted() to return BootedOstree instead of separate deployment/sysroot components - Update soft_reboot_rollback() to accept &BootedOstree - Update deploy::rollback() and install_reset() for new types - All command verbs now follow consistent pattern: storage.kind()? → match BootedStorageKind → call helper with &BootedOstree This change eliminates all composefs_booted() calls from crates/lib/src/cli.rs main code paths, passing boot context data down from BootedStorage instead. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

…state Update composefs command functions to accept and use Storage and BootedComposefs parameters instead of calling composefs_deployment_status() which scrapes global state. Changes: - Add get_composefs_status(storage, booted_cfs) helper in status.rs - Update upgrade_composefs() to use passed composefs.repo and get_composefs_status() - Update switch_composefs() to accept and use storage/booted_cfs parameters - Update composefs_rollback() to use storage.physical_root for opening directories - Update delete_composefs_deployment() to accept and pass through parameters - Update CLI dispatchers to pass storage and booted_cfs to all composefs functions This eliminates composefs_deployment_status() calls from command functions, completing the pattern established with BootedOstree for ostree commands. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

…tedComposefs Update composefs_gc(), composefs_backend_finalize(), and get_etc_diff() to accept Storage and BootedComposefs parameters instead of calling composefs_deployment_status() and opening hardcoded paths. Changes: - composefs_gc() now accepts storage and booted_cfs parameters - composefs_backend_finalize() uses storage.physical_root instead of hardcoded "/sysroot" - get_etc_diff() uses storage.physical_root instead of hardcoded "/sysroot" - Update CLI dispatchers to use BootedStorageKind pattern for finalize and config-diff - Remove unused imports from finalize.rs (open_dir, CWD) This eliminates remaining composefs_deployment_status() calls from gc.rs and finalize.rs, and removes several hardcoded "/sysroot" paths. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

… helper Add a `cwd: Option<&Dir>` parameter to `run_findmnt()` to support running findmnt with a working directory context. This allows inspecting filesystems using a Dir fd rather than requiring absolute paths. Also add `inspect_filesystem_of_dir()` as a convenience wrapper that inspects the filesystem of a Dir by calling findmnt on "." with the directory as cwd. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

Refactor `get_sysroot_parent_dev()` to accept a `physical_root: &Dir` parameter instead of hardcoding access to `/sysroot`. This continues the ongoing work to eliminate global state access in the composefs backend, following the same pattern as the recent BootedComposefs and BootedOstree refactoring. The function now uses `inspect_filesystem_of_dir()` which operates on a Dir fd. All callers updated to pass `&storage.physical_root` or open a Dir for the upgrade code paths. Helper functions like `list_bootloader_entries()` and `delete_depl_boot_entries()` also updated to accept and pass down the physical_root parameter. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

There were a few checks that asserted ostree booted system even for systems booted via composefs. Move the checks in `Storage::new` Signed-off-by: Pragyan Poudyal <[email protected]> Signed-off-by: Colin Walters <[email protected]>

Check for verity inside the json returned by `bootc status --json` and compare it with the compsefs digest from kernel cmdline Signed-off-by: Pragyan Poudyal <[email protected]> Signed-off-by: Colin Walters <[email protected]>

For ostree systems, before making any changes to the repo/boot entries we call `prepare_for_write`. Before this we were calling it unconditionally even if the command was `bootc status`, which was causing test failures. We pass in a flag now to `BootedStorage::new` which will conditionally call `prepare_for_write`. We also need this to make sure we keep the API more or less similar to what we had before Signed-off-by: Pragyan Poudyal <[email protected]> Signed-off-by: Colin Walters <[email protected]>

cgwalters · 2025-11-05T16:54:24Z

OK i tried to clean this up even more. There's a few prep commits here and then I added a final one which uses Result<Option<>>.

Change `BootedStorage::new()` to return `Result<Option<Self>>` instead of taking a `prep_for_write` boolean parameter. This makes the API more idiomatic and clearer about when the system is not booted via bootc. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

Johan-Liebert1 · 2025-11-06T06:35:22Z

LGTM

PR bootc-dev#1718 introduced a regression where /sysroot was left writable after running `bootc status`. This occurred because BootedStorage::new() unconditionally calls set_mount_namespace_in_use(), which tells ostree it can safely remount /sysroot as writable. When sysroot.load() is called without actually being in a mount namespace, it leaves the global /sysroot writable. Fix by introducing an Environment enum that detects the runtime environment (ostree, composefs, container, or other) early in the execution flow. Callers now detect the environment and call prepare_for_write() if needed before constructing BootedStorage. This ensures a single call to prepare_for_write() per execution path and eliminates the previous layering violation where storage code called into CLI code. The Environment abstraction also makes it clearer when mount namespace setup is required and provides a foundation for future environment-specific behavior. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

PR #1718 introduced a regression where /sysroot was left writable after running `bootc status`. This occurred because BootedStorage::new() unconditionally calls set_mount_namespace_in_use(), which tells ostree it can safely remount /sysroot as writable. When sysroot.load() is called without actually being in a mount namespace, it leaves the global /sysroot writable. Fix by introducing an Environment enum that detects the runtime environment (ostree, composefs, container, or other) early in the execution flow. Callers now detect the environment and call prepare_for_write() if needed before constructing BootedStorage. This ensures a single call to prepare_for_write() per execution path and eliminates the previous layering violation where storage code called into CLI code. The Environment abstraction also makes it clearer when mount namespace setup is required and provides a foundation for future environment-specific behavior. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

github-actions bot added the area/install Issues related to `bootc install` label Oct 31, 2025

bootc-bot bot requested a review from gursewak1997 October 31, 2025 13:06

gemini-code-assist bot reviewed Oct 31, 2025

View reviewed changes

crates/lib/src/cli.rs Show resolved Hide resolved

cgwalters force-pushed the composefs-improvements branch from 441ccf9 to bfff92d Compare October 31, 2025 14:39

github-actions bot added the area/system-reinstall-bootc Issues related to system-reinstall-botoc label Oct 31, 2025

cgwalters enabled auto-merge (rebase) October 31, 2025 17:34

cgwalters requested review from Johan-Liebert1 and jeckersb October 31, 2025 20:44

Johan-Liebert1 requested changes Nov 3, 2025

View reviewed changes

cgwalters assigned Johan-Liebert1 Nov 3, 2025

Johan-Liebert1 disabled auto-merge November 5, 2025 07:41

Johan-Liebert1 approved these changes Nov 5, 2025

View reviewed changes

cgwalters and others added 12 commits November 5, 2025 11:53

cli: Only detect containers, not ostree containers

acfe29e

Distinguishing isn't useful and we want to get away from having ostree repos in images anyways. Signed-off-by: Colin Walters <[email protected]>

cli: Remove ostree detection in prepare_for_write

4658f84

We detect things later regardless, and this is prep for composefs. Signed-off-by: Colin Walters <[email protected]>

style: Remove unnecessary explicit return statements

7ebd101

Use implicit return expressions instead of explicit `return` statements for the final expression in functions, following Rust conventions. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <[email protected]>

storage: Fix premature ostree booted check

efa4bef

There were a few checks that asserted ostree booted system even for systems booted via composefs. Move the checks in `Storage::new` Signed-off-by: Pragyan Poudyal <[email protected]> Signed-off-by: Colin Walters <[email protected]>

tests/integration: Update composefs booted status test

7c61342

Check for verity inside the json returned by `bootc status --json` and compare it with the compsefs digest from kernel cmdline Signed-off-by: Pragyan Poudyal <[email protected]> Signed-off-by: Colin Walters <[email protected]>

cgwalters force-pushed the composefs-improvements branch from 54d5d30 to 361795b Compare November 5, 2025 16:53

cgwalters force-pushed the composefs-improvements branch from 361795b to 4e07f74 Compare November 5, 2025 17:34

cgwalters merged commit e5249d1 into bootc-dev:main Nov 6, 2025
37 checks passed

cgwalters mentioned this pull request Nov 11, 2025

store: Preserve /sysroot readonly for read-only operations #1753

Merged

composefs: Refactoring to use storage #1718

composefs: Refactoring to use storage #1718

Uh oh!

Conversation

cgwalters commented Oct 31, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Johan-Liebert1 Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Johan-Liebert1 commented Nov 4, 2025

Uh oh!

Johan-Liebert1 commented Nov 4, 2025

Uh oh!

Johan-Liebert1 commented Nov 5, 2025

Uh oh!

cgwalters commented Nov 5, 2025

Uh oh!

cgwalters commented Nov 5, 2025

Uh oh!

cgwalters commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Johan-Liebert1 commented Nov 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cgwalters commented Nov 5, 2025 •

edited

Loading