containerd integration: add support for since,before,labels images filters#43816
containerd integration: add support for since,before,labels images filters#43816thaJeztah merged 2 commits intomoby:masterfrom
Conversation
c0e6854 to
49e8b7d
Compare
|
Rebased, because #43815 was merged |
tianon
left a comment
There was a problem hiding this comment.
I was looking at https://github.com/moby/moby/blob/2fbc30739b437d91f7a444f49143305e5f0a303e/daemon/images/image_list.go hoping we could have some overlapping implementation here but saw quickly that the filtering is integral to the list generation itself over there, so I guess not so much. 😅
I do wonder (as a user of many systems with an extremely prolific number of images on them) how well this implementation will scale -- I guess the other way to put that is that this is probably somewhere the containerd API perhaps needs some changes for batching/streaming/"iterator" of the image list instead of returning the full list of objects in a single call. 😇 🙈
Yes, I think we should look if this can be implemented in upstream containerd somehow; I agree that the current approach is not ideal (sending a lot of info over the wire, and filtering after the fact) |
daemon/containerd/image_list.go
Outdated
|
|
||
| // Images returns a filtered list of images. | ||
| func (i *ImageService) Images(ctx context.Context, opts types.ImageListOptions) ([]*types.ImageSummary, error) { | ||
| imgs, err := i.client.ListImages(ctx) |
There was a problem hiding this comment.
You can probably do the filters as fieldpaths in this call.
There was a problem hiding this comment.
https://github.com/containerd/containerd/blob/main/filters/filter.go
Doing the date comparison is not possible today, but most things should be.
There was a problem hiding this comment.
Oh, that looks interesting indeed. Will have to dig into that a bit to see how to integrate it (I'm mostly upstreaming patches here 😂)
what's your preference? trying to make that work before we merge this, or improve in a follow-up?
There was a problem hiding this comment.
I'm fine with it as is, let's maybe make sure to note though that the implementation needs work.
There was a problem hiding this comment.
I added some todo's and tracking issues;
- containerd integration: implement image filters with containerd's filter package #43845
- containerd integration: implement "dangling" filter for images #43846
- containerd integration: implement "reference" filter for images #43847
- containerd integration: make sure images are sorted, and optimize before/since filters #43848
daemon/containerd/image_list.go
Outdated
| return ret, nil | ||
| } | ||
|
|
||
| func (i *ImageService) setupFilters(ctx context.Context, opts types.ImageListOptions) ([]imageFilterFunc, error) { |
There was a problem hiding this comment.
| func (i *ImageService) setupFilters(ctx context.Context, opts types.ImageListOptions) ([]imageFilterFunc, error) { | |
| func (i *ImageService) setupFilters(ctx context.Context, opts types.ImageListOptions) (imageFilterFunc, error) { |
return func(image containerd.Image) bool {
for _, filter := range filters {
if !filter(image) {
return false
}
}
return true
}, nilThere is only one filter. It may have more than one clause, but they're all logically parts of one filter. The caller of setupFilters shouldn't need to care how the filter is implemented; returning a slice of filter clauses just opens up opportunities for the calling code to evaluate the filter incorrectly (e.g. by ORing the clauses when they should be ANDed).
There was a problem hiding this comment.
Good suggestion; I was also looking at that.
- I updated the function as suggested
- Changed the signature to accept
filters.Argsinstead of the wholeImageListOptions - I moved the
i.setupFilters()invocation to the start of the function, as parsing the filters should be less work than first fetching all images (only to return an error if there's an issue with constructing the filters) - I added a second commit to validate the filters (as not all filters are implemented yet)
- Created some tracking issues for that; see containerd integration: add support for since,before,labels images filters #43816 (comment)
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Not all filters are implemented yet, so make sure an error is returned if a not-yet implemented filter is used. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
Interesting failure on Windows (could be just a hiccup) https://github.com/moby/moby/runs/7446210210?check_suite_focus=true |
|
I'm gonna ignore the |
containerd integration: add support for since,before,labels images filters Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)