-
Notifications
You must be signed in to change notification settings - Fork 225
libimage.runtime.layerTree: Use consistent layers and images #2125
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
Conversation
c1113a8
to
599eb71
Compare
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.
Note particularly the comment about racy lookups; I don’t immediately know what to do there.
Maybe we can change the callers instead — I see only Podman’s Mount
and Unmount
, maybe those could get a ListImagesByNames
with one set of features, and everyone else could get an ListAllImages
with the names
parameter removed entirely.
We need to resolve this somehow, I didn’t analyze the callers enough to have an opinion yet.
As a general principle, a commit should go from a buildable (ideally, working) codebase to a buildable codebase; don’t split a change of a function parameter from updates of callers into separate commits. (The reason is to make git bisect
work well, and to generally make it possible to do backports without having to think too much about the commit sequence.)
fc5b51a
to
cc7ded3
Compare
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.
.
@mtrmac Did you add a bogus comment? |
Yes, I’m sorry — I think all the pending review comments got published in the meantime. |
5d8807d
to
e3ae831
Compare
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.
Great, that solves that problem.
Now, can we fix the other one as well, and do the filtering based on the same layerTree
? I think this should work:
- Have
compileImageFilters
also return a “needs a layer tree” boolean - Add a
*layerTree
parameter tofilterFunc
; move tree creation fromcompileImageFilters
tofilterImages
(based on the boolean from the previous step) - Then move
compileImageFilters
directly intoListImages
, and call it before theMultiList
MultiList
will now know whether alayerTree
is necessary, and always create only one.
This is getting a bit large — can this be split into at least 3 commits?
- Add
ListImagesByNames
, change theListImages
API, without changing the implementation - Refactor
filterImages
+filterFunc
to create the tree infilterFunc
- Use
MultiList
, and all the other stuff.
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
5c42955
to
0177418
Compare
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.
Thanks! ACK overall, just some nits to help future maintainers.
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
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.
LGTM.
Great work!
@vrothberg PTAL |
No time to review, sorry. I trust the PR has been thoroughly reviewed :) |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR changes
runtime.layerTree
method to use consistent values for the creation of layerTree. Before images and layers could vary between calls that obtain images and layers now usingstorage.MultiList
to obtain consistent images and layers. Also, this PR adapts callers ofruntime.layerTree
to use consistent images and layers and vendor pre-release ofc/storage
withstorage.MultiList
method.Fixes: containers/podman#23331
Depends on: #2131