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

Bound install time #712

Merged
merged 5 commits into from
Jul 19, 2024
Merged

Bound install time #712

merged 5 commits into from
Jul 19, 2024

Conversation

cgwalters
Copy link
Collaborator

utils: Drop unnecessary lint allow

This function doesn't use unsafe anymore.

Signed-off-by: Colin Walters walters@verbum.org


boundimage: Use high level deployment_fd helper

This keeps things even simpler, it's the same thing we're doing
in kargs.rs.

Signed-off-by: Colin Walters walters@verbum.org


boundimage: More struct definition to top

I think it's generally best to have type definitions come before
the code that references them.

Signed-off-by: Colin Walters walters@verbum.org


boundimage: Drop const parameter

We were always passing this single constant value to the
function; let's just reference it inside the function
and simplify callers.

Signed-off-by: Colin Walters walters@verbum.org


boundimage: Rename helper function

I think this name is a bit clearer.

Signed-off-by: Colin Walters walters@verbum.org


boundimage: Make helpers pub(crate)

Prep for using them in the install path.

Signed-off-by: Colin Walters walters@verbum.org


install: Pull bound images by default

Unless overridden by a CLI option, in which case they will
likely be pulled on firstboot.

Signed-off-by: Colin Walters walters@verbum.org


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

cgwalters commented Jul 18, 2024

OK, this works for me to pull bound images at install time by default (and is a somewhat improved version of what was in my original PR).

However:

  • I'm not happy with the /var situation (xref https://gitlab.com/fedora/bootc/base-images/-/issues/18 too); this also argues somewhat that bound images should be owned in the "bootc storage" as an additional store (also xref tracker: logically bound app images #128 (comment) )
  • I think what we're very likely to need in the future is to support a model where we also look for bound images in /var/lib/containers in the installation environment, and pull from there if they exist. This would dramatically simplify both local development and support hermetic-style builds better
  • Needs tests (but so does bound images in general)

Copy link
Contributor

@ckyrouac ckyrouac left a comment

Choose a reason for hiding this comment

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

lgtm! Validated it works locally via podman-bootc.

@cgwalters cgwalters marked this pull request as ready for review July 19, 2024 14:17
I think it's generally best to have type definitions come before
the code that references them.

Signed-off-by: Colin Walters <walters@verbum.org>
We were always passing this single constant value to the
function; let's just reference it inside the function
and simplify callers.

Signed-off-by: Colin Walters <walters@verbum.org>
I think this name is a bit clearer.

Signed-off-by: Colin Walters <walters@verbum.org>
Prep for using them in the install path.

Signed-off-by: Colin Walters <walters@verbum.org>
Unless overridden by a CLI option, in which case they will
likely be pulled on firstboot.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
Copy link
Collaborator Author

OK, rebased 🏄 on git main and lifting draft. I also added a hide = true to the install option as bound images are still classified as experimental.

@cgwalters cgwalters merged commit affe394 into containers:main Jul 19, 2024
25 of 28 checks passed
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.

2 participants