Rewrite livefs (in Rust)

[cgwalters]

The old livefs code was broken, it's time to rewrite it, and that's a good excuse to use Rust.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [cgwalters]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cgwalters]

There are 4 major things that make this safer than the previous version:

1. We always operate on an overlayfs that will be thrown away
2. And correlated with that, we only operate on /usr
3. It's structured as performing a diff+update between two commits; we drop the crazy "just replace all of /usr" thing
4. It's written in Rust

Now after working on this a bit I realized that /etc is probably going to need to be handled and we also will likely want the systemd-tmpfiles bits for /var etc. And we'll likely need to grow at least integration with transfiletriggers so we do things like run systemctl daemon-reload if a unit file changed.

[cgwalters]

Need to rewrite the tests, and also figure out an elegant way to "escape" our mount namespace to create the "transient" unlock in the real ns. I originally tried using systemd-run .. ostree admin unlock --transient but that got blocked on waiting for the sysroot lock, which the livefs txn is holding. Probably the best fix here is to make the sysroot locking in a txn optional - conceptually livefs shouldn't change the real sysroot except for that unlock.

[cgwalters]

layering-basic-2: + fatal 'File '''out.txt''' matches regexp '''Checking out tree ''''

Cool, test case successfully found a read of uninitialized variable.

[lucab]

When I was touching this data last time, I was unsure about the actual semantics of this, so let me ask it here. Are commit and inprogress_commit actually related in some way?
That is, are they maybe in an XOR state (and thus an enum here) or perhaps in a linked state (and thus a Option<LiveFsState> with non-null fields)?

Either way, it would be good to document what these two fields hold and what is the scenario they represent, to ease the life of casual readers.

[cgwalters]

It's

diff --git a/rust/src/livefs.rs b/rust/src/livefs.rs
index 2dc2bef8..03f9f1a4 100644
--- a/rust/src/livefs.rs
+++ b/rust/src/livefs.rs
@@ -19,7 +19,12 @@ const LIVEFS_STATE_NAME: &str = "rpmostree-livefs-state";
 
 #[derive(Debug, Default, Clone, Serialize, Deserialize)]
 struct LiveFsState {
+    /// The OSTree commit that the running root filesystem is using,
+    /// as distinct from the one it was booted with.
     commit: Option<String>,
+    /// Set when a livefs operation is in progress; if the process
+    /// is interrupted, some files from this commit may exist
+    /// on disk but in an incomplete state.
     inprogress_commit: Option<String>,
 }
 

(But not pushing yet since I don't want to break the green CI)

[cgwalters]

Definitely not a XOR - we support going from "base commit" ➡️ livefs(A) ➡️ livefs(B) and if the A ➡️ B transition is interrupted, then we have

state {
  commit: Some(A),
  inprogress_commit: Some(B)
}

Whereas if B completes then we have

state {
  commit: Some(B)
  inprogress_commit: None
}

right?

[lucab]

Thanks for the insight! No need to amend the PR right now, it's already good to have these details noted down here.
Aside, in the end it looks like:

1. this models a two-states InProgress/Active FSM
2. in both states the commit field is present (is there an actual case where it's None?)

EDIT: blurred into this there is a state-machine, which is more complex than my couple of lines above.

[cgwalters]

Hooray, this one is passing CI now.

So...debate as to merging as is and doing more as followup, or waiting? I lean a bit to the former but could be convinced that e.g. we should handle /etc and /var or we want more tests.

[lucab]

So...debate as to merging as is and doing more as followup, or waiting?

This is an ex subcommand, I'd be in favour of landing these changes now and gradually get to the remaining tasks. However if we use this as a checkpoint I'd like to see:

• TODO items to record the high-level logic items that are missing
• better modeling of the livefs-state as a state-machine, because right now the states and transitions are blurred into the rest of the logic, and hard to verify

[cgwalters]

Eh I'll do a bit more on this.

[jlebon]

The general approach here makes sense. In #639, we were discussing possible ways to do this by taking the target commit's /usr, but I don't think we landed on a design that was clean enough mount-wise. The payoff though would be a much more straightforward implementation.

[jlebon]

Feels like we could reduce those two APIs (ror_livefs_get_state and ror_livefs_query) to just one?

[cgwalters]

Mmm...probably it should be a helper on the C side? There are a lot of callers that just want a boolean, so we'd really end up inlining it anyways.

[cgwalters]

OK updated 🆕 and now fully supports changes to /etc and we run systemd-tmpfiles so we now fully support the targeted use case of:

$ rpm-ostree install httpd
$ rpm-ostree ex livefs
$ systemctl start httpd

again!

[cgwalters]

I think we can also merge #2060 after this, since the solution of "escaping" via systemd-run also fixes that issue.

[jlebon]

Nice work! I think let's get this in and we can do any tweaks and fixes as follow-ups.

/lgtm

[cgwalters]

/refresh