Skip to content
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

prepare-root: Move sysroot.tmp creation earlier #2864

Merged
merged 1 commit into from
May 30, 2023

Conversation

cgwalters
Copy link
Member

Main motivation is prep for composefs in
#2640
In the interest of that, we add a bool using_composefs but it's currently always false.

err (EXIT_FAILURE, "failed to MS_MOVE %s to %s", deploy_path, root_mountpoint);
err (EXIT_FAILURE, "failed to MS_MOVE %s to %s", TMP_SYSROOT, root_mountpoint);

if (chdir ("/sysroot") < 0)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be using root_mountpoint, not "/sysroot". They will mostly be the same, but all the other places use root_mountpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. That said, I had to re-read and re-re-read this code and think through things because I realized this change overall was sufficiently large that it had removed my confidence in knowing how things work.

I added some more comments, and updated.

@alexlarsson
Copy link
Member

Minor comment, but otherwise LGTM

@cgwalters cgwalters force-pushed the prepare-root-prepare-composefs branch from 8907f42 to 1fab85e Compare May 24, 2023 14:36
@cgwalters
Copy link
Member Author

I find myself wondering though if we shouldn't instead create ostree-prepare-composefs.c that is dedicated to just that. Among other things, there we can not care about the "no-initramfs" path, we can hardcode the sysroot_readonly flag, etc.

But bigger picture also, I think with composefs we should stop doing the hardlinked checkouts at all. And that also leads into the discussions around having ostree=1|2 and not checksums, etc., which also drops out the need to call this resolve_deploy_path() etc.

Also related to this, I'm thinking that instead of parsing options from the target repo, we should inject a config file into the initramfs (e.g. /usr/lib/ostree/initramfs/config). In fact I'm pretty sure that's what we must do because it chains into the SB story. And if we do that it's again different from what ostree-prepare-root.c is doing today and an argument to make a new file.

@alexlarsson
Copy link
Member

For the config, the current PR uses a ot-composefs kernel cmdline options for similar reasons, i.e. in a secureboot system it will be part of a trusted set of info. But yes, the initrd is also a trusted place for such info.

As for ostree-prepare-composefs. What would decide to run this instead of ostree-prepare-root?

@alexlarsson
Copy link
Member

The current PR setup is kinda incrementally adding composefs. It deploys both styles, and the code picks up whichever works. I agree that a more opinionated version may be better where we just do the composefs during the deploy if that is configured.

Don't we still need a partial checkout for the etc merging to work though?

@alexlarsson
Copy link
Member

Hmm, also it feels like we shouldn't embedd details about how the image is deployed in the initrd, because you may want to have a commit that works in both kinds of deployment.

Main motivation is prep for composefs in
ostreedev#2640
In the interest of that, we add a `bool using_composefs` but
it's currently always `false`.

Co-authored-by: Alexander Larsson <alexl@redhat.com>
@cgwalters cgwalters force-pushed the prepare-root-prepare-composefs branch from 1fab85e to c22576c Compare May 24, 2023 19:50
@cgwalters
Copy link
Member Author

As for ostree-prepare-composefs. What would decide to run this instead of ostree-prepare-root?

With systemd in the initramfs, there's an immense amount of flexibilty. In the general case we can install a generator which parses an injected config file and chooses whether to activate ostree-prepare-root.service or ostree-prepare-composefs.service. Or we can just have a single binary that parses the config itself and dispatches.

I think what seems simplest though is having one binary with two source files, and very early on in ostree-prepare-root.c we detect the composefs case and just dispatch to that - explicitly erroring out if we're pid 1.

@cgwalters
Copy link
Member Author

@alexlarsson mind adding an official "approve" on this PR? Branch protection here requires it.

@alexlarsson alexlarsson self-requested a review May 30, 2023 13:13
Copy link
Member

@alexlarsson alexlarsson left a comment

Choose a reason for hiding this comment

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

ack

@cgwalters cgwalters merged commit 0dd2788 into ostreedev:main May 30, 2023
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.

None yet

2 participants