-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for --replace-mode=alongside
for ostree target
#137
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
86f6038
to
22671b5
Compare
22671b5
to
e4f00af
Compare
Sorry @cgwalters , accidentally pushed the rebase to your fork instead of mine
EDIT2: Continuing here |
e4f00af
to
22671b5
Compare
I think you can just take over this PR too if you want, or open a new PR from your fork - either way. |
22671b5
to
e4f00af
Compare
Rebased. Without any changes, I'm facing an issue where in an ostree system, the mounted I'll see how I can tweak it so that it finds the right device |
With additional |
@cgwalters thoughts on the above mounts? Do we want to require them for install on ostree targets, or should I figure out a way to make this work without them, using just the already-documented install mounts (i.e. |
We should learn how to peel that. This is really the same thing as https://bugzilla.redhat.com/show_bug.cgi?id=2308594 and ostreedev/ostree#3198 and containers/composefs#280 Short term the simplest is the same logic as the grub patch - detect overlayfs for |
e4f00af
to
923b0ed
Compare
OK. Changed it so that when the target rootfs is an overlay, we'll implicitly try targetting It wasn't working at first and was a bit of a headache for me to debug because apparently if you mount Current code might need a bit of touch-ups, but do you think the direction of the code in its current state is good? Should I clean it up and undraft? |
I think that's possibly because it's bootc that's special casing mounting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up!!
3561a64
to
ed94f1e
Compare
d392280
to
eed91ff
Compare
Yeah I was downloading Fedora CoreOS (as opposed to Silverblue) today to see if we hit similar issues, I'll report with what I find |
Prep for containers#137 Signed-off-by: Colin Walters <[email protected]>
OK yeah there's been a ton of changes in the main branch since this PR was last rebased, it's a conflict-fest. I took an attempt at it and pushed my update to https://github.com/cgwalters/bootc/tree/install-existing-ostree2 |
One big conflict was around how the install phase got split up, and threading through I did #872 instead which is just dead code to start, but moves the "we detected an extant repo" into the "big bag of state" we already had in |
stop stop stop 😆 you're working on your old stale branch, I've already solved all these conflicts a while back |
30b3e31
to
dd7b49f
Compare
Forced pushed now to solve some new tiny conflict on import lines, other than that there's no conflicts vs main |
dd7b49f
to
18e244d
Compare
Forced push again because of duplicate import |
I just tried Fedora CoreOS and I can confirm the same issue happens even with xfs, so it's not a btrfs issue |
Oops! Sorry |
Got around to taking a serious look at this read-only behavior on FCOS/Silverblue, it seems to simply be due to an immutable attribute on the ostree deployment, which gets preserved when the ostree deployment directory is simply mounted directly only On bootc systems I assume this attribute gets lost due to usage of composefs/overlay instead of a direct mount |
Oh duh of course.
Kind of, it's not lost so much as shadowed. But basically with composefs we don't need the immutable bit anymore, it was always a hack. Although wait, there's two immutable bits potentially - one on the deployment root, and one on the physical Basically let's change our code to run |
18e244d
to
7bd228b
Compare
Yep, force pushed with rebase and this change. However we're now hitting another problem:
Looking into what's behind this one
Is that right? They seem to be tied from what I can see:
Removing the bit from one removes it from the other |
This is confusing for sure! In ostree there's the term "physical root" vs "booted root". When we're in the booted system, then But mounting the filesystem from outside, I am pretty sure what we're hitting here at first is the physical root, not the deployment root.
Well that's busted, we should be getting stderr from that...is there nothing there? |
Looks like inside the container, Looking into this |
When remounting both, installation seems to proceed smoothly, so this seems to be the final hurdle |
7386012
to
e75b8b1
Compare
Ahh I think I understand the root of confusion here. I think early on we need to detect "is this an ostree system" and if so, then we need to actually look at But for non-ostree systems then The install code may need some cleanups around this. In the non-ostree case we can assume that the physical root |
So basically what you're suggesting is to change your original suggestion here from "detect overlayfs" to "detect ostree system" ? |
Either way is OK by me honestly. For grub, it does feel a bit cleaner not to do anything specific to ostree but to just check for But for bootc we have other code specific to ostree anyways that aren't going to go away anytime soon so "detect ostree" is something we'll need to do regardless, whether it's early on or a bit later in the flow. |
lib/src/install.rs
Outdated
let path = "sysroot".into(); | ||
let _ = crate::utils::open_dir_remount_rw(rootfs_dir, path) | ||
.context("remounting target sysroot as read-write")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one won't exist in the non-ostree case, that's why the install test GHA is failing (those tests install overtop the GHA runner).
This gets to what I was saying in that I think we'll need to detect this early on dispatch on it.
I think concretely I'd say we check for ostree/repo
and there should be a bool existing_ostree
in the State
structure or so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see I'll rework that tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one won't exist in the non-ostree case
This code is already inside a code branch that's ostree only. I'd argue the test failures are due to a lacking cleanup operation in fn reset_root
- it only deletes the ostree deployments directory, not the ostree repo which is what we used to detect whether there's ostree on the system
72159e6
to
cb97d18
Compare
Ironically our support for `--replace-mode=alongside` breaks when we're targeting an already extant ostree host, because when we first blow away the `/boot` directory, this means the ostree stack loses its knowledge that we're in a booted deployment, and will attempt to GC it... ostreedev/ostree-rs-ext@8fa019b is a key part of the fix for that. However, a notable improvement we can do here is to grow this whole thing into a real "factory reset" mode, and this will be a compelling answer to coreos/fedora-coreos-tracker#399 To implement this though we need to support configuring the stateroot and not just hardcode `default`. Signed-off-by: Omer Tuchfeld <[email protected]>
cb97d18
to
b32fdf5
Compare
Ironically our support for
--replace-mode=alongside
breaks when we're targeting an already extant ostree host, because when we first blow away the/boot
directory, this means the ostree stack loses its knowledge that we're in a booted deployment, and will attempt to GC it...ostreedev/ostree-rs-ext@8fa019b is a key part of the fix for that.
However, a notable improvement we can do here is to grow this whole thing into a real "factory reset" mode, and this will be a compelling answer to
coreos/fedora-coreos-tracker#399
To implement this though we need to support configuring the stateroot and not just hardcode
default
.