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 [email protected]


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 [email protected]


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 [email protected]


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 [email protected]


boundimage: Rename helper function

I think this name is a bit clearer.

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


boundimage: Make helpers pub(crate)

Prep for using them in the install path.

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


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 [email protected]


@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 <[email protected]>
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 <[email protected]>
I think this name is a bit clearer.

Signed-off-by: Colin Walters <[email protected]>
Prep for using them in the install path.

Signed-off-by: Colin Walters <[email protected]>
Unless overridden by a CLI option, in which case they will
likely be pulled on firstboot.

Signed-off-by: Colin Walters <[email protected]>
@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