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

install: Automatically set up bind mounts if not provided #919

Merged
merged 6 commits into from
Dec 4, 2024

Conversation

cgwalters
Copy link
Collaborator

@cgwalters cgwalters commented Nov 25, 2024

install: Automatically set up /dev and /var/lib/containers

We're looking again at the ergonomics of bootc install to-existing-root.
This uses the "mounting into mount namespaces" from the new
mount API to automatically set up /dev and /var/lib/containers
if they weren't provided to podman run, which shrinks what's
needed a bit.

Closes: #826

Signed-off-by: Colin Walters [email protected]


install: Drop skopeo-in-host fallback code

Now that we unconditionally mount /var/lib/containers, drop
the hacky fork-skopeo-in-host-mountns code.

Signed-off-by: Colin Walters [email protected]


Drop test references to /dev and /var/lib/containers mounts

Keep the bind mounts in the docs though for now because many
people will be using the current docs with older bootc.

Signed-off-by: Colin Walters [email protected]


install: Drop need for -v /:/target

Build on our new logic for bind mounting from the host mountns
to also drop the need for the -v /:/target in the alongside
install code.

Signed-off-by: Colin Walters [email protected]


install: Centralize PID1 definition

Hooray for const.

Signed-off-by: Colin Walters [email protected]


tests: Drop more bind mount instances

These should be unnecessary.

Signed-off-by: Colin Walters [email protected]


@github-actions github-actions bot added the area/install Issues related to `bootc install` label Nov 25, 2024
@cgwalters
Copy link
Collaborator Author

Tested with this patch to podman-bootc:

diff --git a/pkg/bootc/bootc_disk.go b/pkg/bootc/bootc_disk.go
index ed07c5b..6bfcace 100644
--- a/pkg/bootc/bootc_disk.go
+++ b/pkg/bootc/bootc_disk.go
@@ -332,7 +332,7 @@ func (p *BootcDisk) createInstallContainer(config DiskImageConfig, tempLosetup s
 	// Basic config:
 	// - force on --remote because we depend on podman machine.
 	// - add privileged, pid=host, SELinux config and bind mounts per https://containers.github.io/bootc/bootc-install.html
-	podmanArgs := []string{"--remote", "run", "--rm", "-i", "--pid=host", "--privileged", "--security-opt=label=type:unconfined_t", "--volume=/dev:/dev", "--volume=/var/lib/containers:/var/lib/containers"}
+	podmanArgs := []string{"--remote", "run", "--rm", "-i", "--pid=host", "--privileged", "--security-opt=label=type:unconfined_t"}
 	// Custom bind mounts
 	podmanArgs = append(podmanArgs, fmt.Sprintf("--volume=%s:/output", p.Directory), fmt.Sprintf("--volume=%s:/usr/local/sbin/losetup:ro", tempLosetup))
 	if term.IsTerminal(int(os.Stdin.Fd())) {

@cgwalters
Copy link
Collaborator Author

One thing I will say I'm pretty proud of is this code almost worked the first try, including the whole fd passing over the socketpair. The famous "in Rust if it compiles, it works". The only thing I missed was that for /var/lib/containers because it's not present in our base image, podman run ... -v /var/lib/containers:/var/lib/containers will automatically create the directory. So we just needed a std::fs::create_dir_all() in our mount reconciliation path.

@cgwalters
Copy link
Collaborator Author

cgwalters commented Nov 25, 2024

  • TODO: also drop the need for -v /:/target in the alongside path.

tests/e2e/playbooks/install.yaml Outdated Show resolved Hide resolved
hack/lldb/deploy.sh Outdated Show resolved Hide resolved
ostree-ext/.github/workflows/bootc.yml Outdated Show resolved Hide resolved
@omertuc
Copy link
Contributor

omertuc commented Nov 27, 2024

Title is outdated

@cgwalters cgwalters changed the title install: Automatically set up /dev and /var/lib/containers install: Automatically set up bind mounts if not provided Nov 27, 2024
@cgwalters
Copy link
Collaborator Author

The PR title? Updated.

@cgwalters
Copy link
Collaborator Author

cgwalters commented Dec 2, 2024

Hmm, so one job failed like this:

Initializing ostree layout
ERROR Installing to disk: Creating ostree deployment: Pulling: Creating importer: failed to invoke method OpenImage: failed to invoke method OpenImage: reference "[overlay@/var/lib/containers/storage+/run/containers/storage:overlay.mountopt=nodev,metacopy=on]192.168.100.1:5000/bootc-workflow-test:95ad@sha256:71116de023138a0960d152377f1c2a353a8c0139ab6adfbd8bf45399364c5fa6" does not resolve to an image ID: identifier is not an image

Which looks like it could be caused by this PR, but I don't have any idea why it would just silently not work there...what I was thinking could fail is if we were running from an older OS without the new mount API but I think all operating system versions we care about right now have it. I have no idea how to rerun testing farm jobs to see if this was a flake...

@omertuc
Copy link
Contributor

omertuc commented Dec 2, 2024

Amend force to rerun all?

We're looking again at the ergonomics of `bootc install to-existing-root`.
This uses the "mounting into mount namespaces" from the new
mount API to automatically set up `/dev` and `/var/lib/containers`
if they weren't provided to `podman run`, which shrinks what's
needed a bit.

Closes: containers#826

Signed-off-by: Colin Walters <[email protected]>
Now that we unconditionally mount /var/lib/containers, drop
the hacky fork-skopeo-in-host-mountns code.

Signed-off-by: Colin Walters <[email protected]>
Keep the bind mounts in the docs though for now because many
people will be using the current docs with older bootc.

Signed-off-by: Colin Walters <[email protected]>
Build on our new logic for bind mounting from the host mountns
to also drop the need for the `-v /:/target` in the alongside
install code.

Signed-off-by: Colin Walters <[email protected]>
Hooray for `const`.

Signed-off-by: Colin Walters <[email protected]>
These should be unnecessary.

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters merged commit 8d4bf5c into containers:main Dec 4, 2024
26 of 32 checks passed
@omertuc
Copy link
Contributor

omertuc commented Dec 4, 2024

Sorry, didn't realize auto-merge was on, don't know if you meant it to be merged given the CI issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/install Issues related to `bootc install`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

install: switch to doing dynamic mounts
3 participants