c8d: Various images/json API fixes #46034

rumpl · 2023-07-20T08:57:27Z

- What I did

Added more tests for the image list filters
Added the image labels to the image list
Sort the images by creation date
Populate RepoTags with all the tags pointing to an image instead of having multiple objects of the same image in the array.

Fixes #43848
Fixes #43852
Also partially fix*s #43861, I didn't touch RepoDigests

- How I did it

...

- How to verify it

Run

make DOCKER_GRAPHDRIVER=overlayfs TEST_FILTER=TestAPIImagesFilter TEST_INTEGRATION_USE_SNAPSHOTTER=1 test-integration
make DOCKER_GRAPHDRIVER=overlayfs TEST_FILTER=TestImagesFilterMultiReference TEST_INTEGRATION_USE_SNAPSHOTTER=1 test-integration

- Description for the changelog

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

integration-cli/docker_api_images_test.go

daemon/containerd/image_list.go

thaJeztah · 2023-07-20T10:05:59Z

daemon/containerd/image_list.go

+// byCreated is a temporary type used to sort a list of images by creation
+// time.
+type byCreated []*types.ImageSummary
+
+func (r byCreated) Len() int           { return len(r) }
+func (r byCreated) Swap(i, j int)      { r[i], r[j] = r[j], r[i] }
+func (r byCreated) Less(i, j int) bool { return r[i].Created < r[j].Created }


Not a blocker, but starting to wonder if we need to move this somewhere central (I was planning to move the image types from types to types/image, so perhaps that's a good opportunity).

Also considering that sorting may at some point be something we want on the client side as well

thaJeztah · 2023-07-20T10:10:32Z

daemon/containerd/image_list.go

+		return nil, nil, err
+	}
+	var cfg configLabels
+	if err := readConfig(ctx, contentStore, cfgDesc, &cfg); err != nil {


LOL, very unrelated, but I was curious if we make use of the Data field anywhere in our code; https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/specs-go/v1/descriptor.go#L38-L41

// Data is an embedding of the targeted content. This is encoded as a base64 // string when marshalled to JSON (automatically, by encoding/json). If // present, Data can be used directly to avoid fetching the targeted content. Data []byte `json:"data,omitempty"`

I haven't seen any code using that yet TBH. Let me check, if it can be used that would be great actually

It doesn't seem to be set by containerd, we might want to take a note of this and see if it can be used really

Yup! I could see something along the lines of

if desc.Data != nil { // fast path }

That said; I wonder if the Spec is clear enough on verifying Data matches the referenced data (would be fun if there's no such check and Data would return something very different from the referenced file 😂)

Related fetch: Use data from descriptor when available. containerd/containerd#8076

Nice, so yes, looks like we can start looking into using that

thaJeztah · 2023-07-20T11:17:03Z

Some failures on Windows, which look related;

=== Failed
=== FAIL: github.com/docker/docker/integration-cli TestDockerAPISuite/TestAPIImagesFilter/all_images (0.01s)
    docker_api_images_test.go:103: assertion failed: 2 (int) != 7 (testcase.expectedImages int)

=== FAIL: github.com/docker/docker/integration-cli TestDockerAPISuite/TestAPIImagesFilter/dangling_images (0.00s)
    docker_api_images_test.go:103: assertion failed: 0 (int) != 1 (testcase.expectedImages int)

=== FAIL: github.com/docker/docker/integration-cli TestDockerAPISuite/TestAPIImagesFilter (0.24s)

=== FAIL: github.com/docker/docker/integration-cli TestDockerAPISuite (1.39s)
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0

goroutine 78 [running]:
testing.tRunner.func1.2({0x13a12c0, 0xc00090ea38})
	C:/hostedtoolcache/windows/go/1.20.6/x64/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	C:/hostedtoolcache/windows/go/1.20.6/x64/src/testing/testing.go:1529 +0x39f
panic({0x13a12c0, 0xc00090ea38})
	C:/hostedtoolcache/windows/go/1.20.6/x64/src/runtime/panic.go:884 +0x213
github.com/docker/docker/integration-cli.(*DockerAPISuite).TestAPIImagesFilter.func1(0x0?)
	D:/a/moby/moby/go/src/github.com/docker/docker/integration-cli/docker_api_images_test.go:105 +0x2c5
testing.tRunner(0xc000846ea0, 0xc000909b78)
	C:/hostedtoolcache/windows/go/1.20.6/x64/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	C:/hostedtoolcache/windows/go/1.20.6/x64/src/testing/testing.go:1629 +0x3ea

DONE 16 tests, 2 skipped, 4 failures in 99.924s

integration/image/list_test.go

daemon/containerd/image_list.go

integration/image/list_test.go

vvoland

Overall LGTM, just have one performance nit.

daemon/containerd/image_list.go

laurazard

LGTM

thaJeztah

LGTM

left some nits (you know me 😂), but not blockers, so up to you if you want to address them ❤️

daemon/containerd/image_list.go

integration/image/list_test.go

thaJeztah · 2023-07-26T12:22:24Z

daemon/containerd/image_list.go

+		return nil, nil, err
+	}
+	var cfg configLabels
+	if err := readConfig(ctx, contentStore, cfgDesc, &cfg); err != nil {


Nice, so yes, looks like we can start looking into using that

- use assert.Check to continue the test even if a check fails - assert the total number of images returned, not only their RepoTags - use subtests Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

Aggregate same images into one object and add the list of tags pointing to it to the RepoTags array Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

thaJeztah

LGTM!

thaJeztah · 2023-07-26T14:50:23Z

Aaaaaaaand.... all green

rumpl added area/api status/2-code-review area/images containerd-integration Issues and PRs related to containerd integration labels Jul 20, 2023

rumpl changed the title ~~integration-cli: Add more checks to images filter~~ c8d: Various images/json API fixes Jul 20, 2023

rumpl force-pushed the c8d-image-list branch from 7bcb150 to 4ff6fe8 Compare July 20, 2023 09:35

thaJeztah reviewed Jul 20, 2023

View reviewed changes

rumpl force-pushed the c8d-image-list branch 2 times, most recently from b74fff4 to 5308d40 Compare July 20, 2023 13:29

rumpl commented Jul 20, 2023

View reviewed changes

integration/image/list_test.go Outdated Show resolved Hide resolved

rumpl force-pushed the c8d-image-list branch from 5308d40 to e008874 Compare July 20, 2023 14:11

rumpl requested review from thaJeztah, vvoland and laurazard July 21, 2023 08:42

rumpl force-pushed the c8d-image-list branch 2 times, most recently from 440a1f7 to 8c70f65 Compare July 21, 2023 11:54

vvoland requested changes Jul 24, 2023

View reviewed changes

rumpl force-pushed the c8d-image-list branch 4 times, most recently from b0576bc to bd86b37 Compare July 25, 2023 08:40

rumpl requested a review from vvoland July 25, 2023 08:49

rumpl force-pushed the c8d-image-list branch from bd86b37 to 6037149 Compare July 25, 2023 08:51

vvoland reviewed Jul 25, 2023

View reviewed changes

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

rumpl force-pushed the c8d-image-list branch from 6037149 to 1cf092c Compare July 26, 2023 09:01

vvoland approved these changes Jul 26, 2023

View reviewed changes

laurazard approved these changes Jul 26, 2023

View reviewed changes

rumpl added this to the 25.0.0 milestone Jul 26, 2023

thaJeztah approved these changes Jul 26, 2023

View reviewed changes

rumpl added 4 commits July 26, 2023 14:33

integration: Move image filter tests to integration

ad2760e

- use assert.Check to continue the test even if a check fails - assert the total number of images returned, not only their RepoTags - use subtests Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

c8d: Populate RepoTags instead of adding a new image to the list

a0bba26

Aggregate same images into one object and add the list of tags pointing to it to the RepoTags array Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

c8d: images/json, add image labels to the response

1d8108b

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

c8d: Fix TestImagesFilterMultiReference

e5cffc0

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

rumpl force-pushed the c8d-image-list branch from 1cf092c to e5cffc0 Compare July 26, 2023 12:34

thaJeztah approved these changes Jul 26, 2023

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Jul 26, 2023

thaJeztah merged commit 7a44e4c into moby:master Jul 26, 2023
103 checks passed

thaJeztah deleted the c8d-image-list branch July 26, 2023 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c8d: Various images/json API fixes #46034

c8d: Various images/json API fixes #46034

rumpl commented Jul 20, 2023 •

edited

thaJeztah Jul 20, 2023

thaJeztah Jul 20, 2023

rumpl Jul 20, 2023

rumpl Jul 20, 2023

thaJeztah Jul 20, 2023

rumpl Jul 26, 2023 •

edited by thaJeztah

thaJeztah Jul 26, 2023

thaJeztah commented Jul 20, 2023

vvoland left a comment

laurazard left a comment

thaJeztah left a comment

thaJeztah Jul 26, 2023

thaJeztah left a comment

thaJeztah commented Jul 26, 2023

c8d: Various images/json API fixes #46034

c8d: Various images/json API fixes #46034

Conversation

rumpl commented Jul 20, 2023 • edited

thaJeztah Jul 20, 2023

Choose a reason for hiding this comment

thaJeztah Jul 20, 2023

Choose a reason for hiding this comment

rumpl Jul 20, 2023

Choose a reason for hiding this comment

rumpl Jul 20, 2023

Choose a reason for hiding this comment

thaJeztah Jul 20, 2023

Choose a reason for hiding this comment

rumpl Jul 26, 2023 • edited by thaJeztah

Choose a reason for hiding this comment

thaJeztah Jul 26, 2023

Choose a reason for hiding this comment

thaJeztah commented Jul 20, 2023

vvoland left a comment

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Jul 26, 2023

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Jul 26, 2023

rumpl commented Jul 20, 2023 •

edited

rumpl Jul 26, 2023 •

edited by thaJeztah