-
Notifications
You must be signed in to change notification settings - Fork 80
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
Introduce bootc-owned container store, use for bound images #724
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we globally configure the additional image store via containers-storage.conf? That would quite improve the UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's a possibility. That said, changing the storage configuration without having containers/storage#1885 is...a bit ugly.
There are a few other things that have me slightly wary:
podman
invocation would see it...it'd be empty, but it would be a global behavioral changeYeah ok, I think this a near showstopper for globally (i.e. in
storage.conf
)setting additionalimagestores (hereafter: AIS) as currently defined today.
For the current bootc logically bound images (hereafter: LBI) design, we only opt in individual podman invocations to use the store, and every image there is readonly. It cannot change or break any non-LB images.
But when globally configuring an AIS, there's two actors:
That operate independently of each other, but every podman invocation will see the bootc AIS.
Take the situation where there are two different "FROM " images, one is an LBI and another is free-floating (could be managed by kube/ansible/whatever). If the bootc AIS happens to have the layer(s) for
<somebase>
, thepodman pull <derived2>
for the non-LBI (floating) image will reuse the common base. Which...makes sense!Except today the bootc bound images logic will prune images that are no longer referenced.
And yes, I just tested this, and things explode (as you'd expect) in the scenario where a lower read-only layer disappears for the case of an image/container that lives in
/var/lib/containers
.(Note we don't have the inverse problem, because when bootc operates on its storage it uses
--root
and should not be looking at or touching/var/lib/containers
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess of course, bootc's GC logic could learn to take into account the
/var/lib/containers
store state...but we're starting to "cross the worlds" with stuff like that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, one might be thinking...isn't the real fix for this to have one storage by default, i.e. on bootc systems podman and bootc default to having the same storage for images (underneath
/sysroot
, though containers would probably still make sense under `/var).And yes maybe, but the thing that terrified me about going down that path is that typing "podman ..." has the potential to break the operating system. There's lots of docs talking about
podman system reset
and the like.Could we make it reliable? Yes, clearly. But it'd require I think a lot of careful thought, code auditing, cooperation and integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if podman had a way to recover from it. IE Pull down the missing image but I am not sure the missing image/layer would still exist or have a way to discover where it exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had agreed to create a pinning image mechanism for you that was going to avoid the reset problem no? i thought that is the way we were going?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware of any commitment from anyone to work on image pinning on the podman side.
That said again, I should be very clear that my thoughts on this design changed as we got into the implementation. My original thought was basically "bootc is just running podman pull, how hard can it be Michael? 1 week?"
But yeah...having bootc touch
/var
(/var/lib/containers
) just blows up our story around state, messes with transactional updates, partitioning, etc. So that led me rapidly down this path of having a bootc-owned storage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also don't we want the ability to roll back versions of images?
But bottom line we have the same issue if anyone sets up additional stores. If an image is used from an additional store and gets removed, then the container will no longer work. In RHIVOS we thought about this and came to the conclusion that we would know which images were being used so we would know what images to remove. Also containers are ephemeral, they did not survive reboot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bootc retains all LBIs that are referenced from any deployment, i.e. when you upgrade but then roll back, you are guaranteed that the LBIs that are referenced via the
.image/.container
files are still there.(However, there's ill-defined semantics currently around how LBIs work with floating tags, but basically "don't do that")