c8d/list: Implement `reference` by vvoland · Pull Request #45269 · moby/moby

vvoland · 2023-04-04T16:01:08Z

Related to: containerd integration: implement image filters with containerd's filter package #43845
Closes: containerd integration: implement "reference" filter for images #43847

- What I did
Implemented reference filter.
Note: The fork implementation used fuzzy match for reference filter, but this isn't consistent with the graphdriver Docker.

- How I did it

- How to verify it

$ docker images
REPOSITORY   TAG       IMAGE ID       CREATED              SIZE
alpine       latest    124c7d270790   About a minute ago   3.27MB
busybox      latest    b5d6fe071263   2 seconds ago        2.01MB
$ docker images --filter 'reference=busybox'
REPOSITORY   TAG       IMAGE ID       CREATED         SIZE
busybox      latest    b5d6fe071263   8 seconds ago   2.01MB
$ docker images busybox
REPOSITORY   TAG       IMAGE ID       CREATED          SIZE
busybox      latest    b5d6fe071263   10 seconds ago   2.01MB
$ docker images alpine
REPOSITORY   TAG       IMAGE ID       CREATED              SIZE
alpine       latest    124c7d270790   About a minute ago   3.27MB

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah

LGTM (left some suggestions, but no blockers)

thaJeztah · 2023-04-04T16:04:03Z

daemon/containerd/image_list.go

@@ -254,8 +255,9 @@ type imageFilterFunc func(image images.Image) bool
 // setupFilters constructs an imageFilterFunc from the given imageFilters.
 //
 // TODO(thaJeztah): reimplement filters using containerd filters: see https://github.com/moby/moby/issues/43845


Are there (more) filters we can delegate to containerd? (Otherwise we could remove this comment) - not a blocker though we can always remove in a follow-up

Hm, dangling can be done with c8d too.
Not sure about time and how c8d would compare them. Will check.

Let's keep the ticket open then (feel free to leave a comment there on other filters we could still implement using containerd to improve things)

Yeah currently we can't do any more (except dangling). Posted a code that specified available properties in the issue.

Crap, dangling is fun too. There's no fuzzy negation, so we'd need something like this: https://github.com/moby/moby/blob/master/.github/workflows/buildkit.yml#L123 😅

😬 yeah, let's not do that for now 😂

thaJeztah · 2023-04-04T16:05:28Z

daemon/containerd/image_list.go

 //
 // TODO(thaJeztah): reimplement filters using containerd filters: see https://github.com/moby/moby/issues/43845
-func (i *ImageService) setupFilters(ctx context.Context, imageFilters filters.Args) (imageFilterFunc, error) {
+func (i *ImageService) setupFilters(ctx context.Context, imageFilters filters.Args) ([]string, imageFilterFunc, error) {


Not a blocker (as it's not exported), but we could consider to use either named return variables (to "document" what they are), or update the GoDoc of the function to describe what it returns.

Changed parameters to be named and described them in Godoc.

thaJeztah

LGTM

daemon/containerd/image_list.go

rumpl

LGTM after the fix for the equals operator

vvoland · 2023-04-05T10:11:51Z

Hmm wait, isn't the fork behavior for label wrong?
It checks the labels on containerd Image object, not Labels from the image Config.

vvoland · 2023-04-05T10:25:51Z

For example, Ubuntu:

$ docker inspect ubuntu -f '{{.Config.Labels}}'
map[org.opencontainers.image.ref.name:ubuntu org.opencontainers.image.version:22.04]

This is on "non-c8d Docker":

$ docker images --filter label=org.opencontainers.image.ref.name
REPOSITORY   TAG       IMAGE ID       CREATED       SIZE
ubuntu       latest    bab8ce5c00ca   4 weeks ago   69.2MB

But it won't have the same result with this implementation because ubuntu image doesn't have these labels attached to the containerd image object (labels are empty):

$ ctr image ls
REF                              TYPE                                                      DIGEST                                                                  SIZE     PLATFORMS                                                                                                                          LABELS   
docker.io/library/ubuntu:latest  application/vnd.oci.image.index.v1+json                   sha256:67211c14fa74f070d27cc59d69a7fa9aeff8e28ea118ef3babc295a0428a6d21 26.1 MiB linux/amd64,linux/arm/v7,linux/arm64/v8,linux/ppc64le,linux/s390x                                                                  -

So, unless we want to populate containerd Image object's Labels with the labels from Config, we can't use containerd filter for this one.

daemon/containerd/image_list.go

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland · 2023-04-06T11:47:35Z

Moved the label filter out of this PR as it's not a simple upstream PR as it initially was supposed to be.

Code changed from last LGTM

thaJeztah

LGTM, thanks!

vvoland added status/2-code-review area/images Image Distribution containerd-integration Issues and PRs related to containerd integration labels Apr 4, 2023

vvoland added this to the 24.0.0 milestone Apr 4, 2023

thaJeztah approved these changes Apr 4, 2023

View reviewed changes

vvoland force-pushed the c8d-reference-filter branch from 61d4383 to 8ab18ae Compare April 4, 2023 16:49

vvoland mentioned this pull request Apr 4, 2023

c8d: Implement prune #45270

Merged

thaJeztah previously approved these changes Apr 4, 2023

View reviewed changes

rumpl reviewed Apr 5, 2023

View reviewed changes

daemon/containerd/image_list.go Outdated Show resolved Hide resolved

rumpl approved these changes Apr 5, 2023

View reviewed changes

vvoland force-pushed the c8d-reference-filter branch from 8ab18ae to d24bc42 Compare April 5, 2023 10:09

vvoland requested review from rumpl and thaJeztah April 5, 2023 10:26

thaJeztah reviewed Apr 5, 2023

View reviewed changes

daemon/containerd/image_list.go Outdated Show resolved Hide resolved

vvoland force-pushed the c8d-reference-filter branch from d24bc42 to 9b6249a Compare April 5, 2023 15:17

c8d/list: Implement reference filter

ba47cdc

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the c8d-reference-filter branch from 9b6249a to ba47cdc Compare April 6, 2023 11:45

vvoland changed the title ~~c8d/list: Implement reference and label! filter~~ c8d/list: Implement reference Apr 6, 2023

rumpl approved these changes Apr 6, 2023

View reviewed changes

vvoland requested a review from thaJeztah April 6, 2023 15:31

vvoland mentioned this pull request Apr 6, 2023

c8d/list: Fix label (and add label!) filter #45289

Merged

thaJeztah approved these changes Apr 6, 2023

View reviewed changes

thaJeztah merged commit 79bf167 into moby:master Apr 6, 2023

Conversation

vvoland commented Apr 4, 2023 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rumpl left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland commented Apr 5, 2023

Uh oh!

vvoland commented Apr 5, 2023

Uh oh!

Uh oh!

vvoland commented Apr 6, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vvoland commented Apr 4, 2023 •

edited by thaJeztah

Loading