RFC: Turn ListImages(filter) into ImageStatus #7336

mtrmac · 2023-09-26T17:59:43Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Turn ListImages(filter) into ImageStatus. That "filter" was never really much of a filter, we always only got one image.

The current logic is, effectively, the only user which accepts that specific form of heuristically-interpreted syntax (because all other non-PauseImage users of internal/storage use ResolveNames), so removing that syntax would consolidate and clarify the semantics of internal/storage names.

Kubelet never calls ListImages with a filter AFAIK, so this should make no difference in practice - and it means we get to remove a chunk of code and tests.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Do note that this changes how ListImagesRequest.Filter.Image.Image is interpreted (to be the same as the same field in of the same struct in ImageStatusRequest.Image.Image), and that might be an incompatible change.

It’s quite possible that there are other non-Kubelet users of this API I’m unaware of.

I don’t really need this change to happen for signing — it clarifies how CRI-O interprets names on input, and I think that’s a bit valuable in itself, as well as the code simplification it would eventually allow (ultimately removing getRef entirely). But this PR is mostly a side-effect of me clarifying my thinking about which values matter and how they are parsed

Does this PR introduce a user-facing change?

If a filter is specified in the `ListImages` CRI method, it is now interpreted the same way image names in the `ImageStatus` methods are.

mtrmac · 2023-09-26T18:03:45Z

Cc: @saschagrunert

server/image_status.go

mtrmac · 2023-09-26T18:35:06Z

(Unit tests fail, I’ll return to this next week.)

haircommander · 2023-09-26T18:38:13Z

server/image_list.go

+			if status == nil {
+				return &types.ListImagesResponse{
+					Images: []*types.Image{},
+				}, nil
+			}
+			return &types.ListImagesResponse{
+				Images: []*types.Image{
+					ConvertImage(status),
+				},
+			}, nil


we could update ConvertImage() to return an empty list instead of a single element, and then compress all of this to

return &types.ListImagesResponse{ Images: ConvertImage(status), }, nil

ConvertImage is also used below, in the primary ListImages cause, where there are almost always multiple images to collect in Images.

It might be cleaner to move all of the filterImage.Image != "" code out of the main body, as a rare case into a separate function out of sight. OTOH “out of sight” might be a bad idea for a corner case — entirely up to CRI-O maintainers.

Or would you prefer

resp := &types.ListImagesResponse{} if status != nil { resp.Images = append(resp.Images, ConvertImage(status)) } return resp, nil

here? I don’t have a preference either way.

as a rare case into a separate function out of sight.

I am open to this. Part of me feels like the code smells a bit that filters never really worked, and we probably should have them working in case kubelet uses them.

ConvertImage is also used below, in the primary ListImages cause, where there are almost always multiple images to collect in Images.

Another option is ConvertImages, which could take the filtered status here and then it could be called below. I am not feeling very convinced in any direction her--all of these are suggestions for consideration

I've thought of it, I think I would prefer:

resp := &types.ListImagesResponse{} if status != nil { resp.Images = append(resp.Images, ConvertImage(status)) } return resp, nil

haircommander · 2023-09-26T18:45:01Z

internal/storage/image.go

-					if !donePulling {
-						continue
-					}
+				if os.IsNotExist(err) && imageIsBeingPulled(image) { // skip reporting errors if the images haven't finished pulling


nit: can all functions be defined underneath the functions that call them so we can read top to bottom?

codecov · 2023-10-03T20:13:20Z

Codecov Report

Merging #7336 (f7d3228) into main (83eb88a) will decrease coverage by 0.06%.
The diff coverage is 88.31%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7336      +/-   ##
==========================================
- Coverage   49.23%   49.18%   -0.06%     
==========================================
  Files         136      136              
  Lines       15693    15672      -21     
==========================================
- Hits         7727     7708      -19     
+ Misses       7046     7044       -2     
  Partials      920      920

mtrmac · 2023-10-09T19:52:21Z

/retest

mtrmac · 2023-10-09T19:55:39Z

test/image.bats

@@ -110,9 +110,6 @@ function teardown() {
 	imageid=$(crictl images --quiet "$IMAGE:go")
 	[ "$imageid" != "" ]

-	output=$(crictl images --quiet @"$imageid")


Note all these changes: the @id syntax is no longer recognized for a “filter”, with the minimal version of this PR.

Alternatively, the new code could explicitly recognize and support the @id syntax.

(Or, going the other extreme, all of these test commands could use crictl inspecti instead.)

... so that we can reuse the "look up a name and get storage data" code in other places, and and eventually so that we can restructure the name lookup logic in some more distant future. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

We will also call the "resolve and get status" code from ListImages. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... now that the more complex control flow is in a helper. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Now that it is a separate function, we can just return early on a match. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

@id

That "filter" was never really much of a filter, we always only got one image. The current logic is, effectively, the only user which accepts that specific form of heuristically-interpreted syntax (because all other non-PauseImage users of internal/storage use ResolveNames), so removing that syntax would consolidate and clarify the semantics of internal/storage names. Kubelet never calls ListImages with a filter AFAIK, so this should make no difference in practice - and it means we get to remove a chunk of code and tests. We do need to update the tests for the @id filter syntax which is no longer recognized, though. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

We have removed the only caller of that code, so start removing it. Should not change behavior (any more). Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... now that we are always building a new cache. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Have ListImages itself concern itself with newImageCache and results, instead of doing that inside a helper. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

It's a nice little self-contained predicate, so move it out of line to make the caller a bit more readable. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

This makes the handling of svc.imageCache a bit more transparent. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

We know we have a full image ID, so avoid the parsing complexity and c/storage lookup. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

openshift-ci · 2023-10-10T07:22:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, saschagrunert

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:

~~OWNERS~~ [saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

saschagrunert · 2023-10-10T07:22:53Z

/override ci/prow/ci-e2e
/override ci/prow/ci-cgroupv2-e2e-crun

openshift-ci · 2023-10-10T07:23:01Z

@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/ci-cgroupv2-e2e-crun, ci/prow/ci-e2e

In response to this:

/override ci/prow/ci-e2e
/override ci/prow/ci-cgroupv2-e2e-crun

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mtrmac requested a review from mrunalp as a code owner September 26, 2023 17:59

openshift-ci bot requested review from klihub and QiWang19 September 26, 2023 18:00

mtrmac force-pushed the list-filter-nonsense branch from 64a8f8a to def6ea9 Compare September 26, 2023 18:03

haircommander reviewed Sep 26, 2023

View reviewed changes

server/image_status.go Outdated Show resolved Hide resolved

haircommander reviewed Sep 26, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2023

mtrmac force-pushed the list-filter-nonsense branch from 9036e95 to b076b23 Compare October 3, 2023 19:42

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2023

mtrmac force-pushed the list-filter-nonsense branch from b076b23 to 0702839 Compare October 3, 2023 20:03

mtrmac force-pushed the list-filter-nonsense branch 3 times, most recently from e5aac64 to 47a5bf4 Compare October 3, 2023 22:06

mtrmac commented Oct 9, 2023

View reviewed changes

mtrmac added 8 commits October 9, 2023 23:03

Simplify error handling in ImageStatus

ad8be44

Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Split storageImageStatus from ImageStatus

1b0e82a

We will also call the "resolve and get status" code from ListImages. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Simplify ImageStatus a bit

2cb1665

... now that the more complex control flow is in a helper. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Simplify storageImageStatus a bit

50ce23e

Now that it is a separate function, we can just return early on a match. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Remove the "filter" parameter to ListImages

ae597ba

We have removed the only caller of that code, so start removing it. Should not change behavior (any more). Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Simplify appendCachedResult

1cfe3c8

... now that we are always building a new cache. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added 4 commits October 9, 2023 23:04

Simplify ListImages more

c718851

Have ListImages itself concern itself with newImageCache and results, instead of doing that inside a helper. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Split imageIsBeingPulled from ListImages

e4f2b88

It's a nice little self-contained predicate, so move it out of line to make the caller a bit more readable. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Inline getImageCacheItem into ListImages

96e6aa9

This makes the handling of svc.imageCache a bit more transparent. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Avoid an unnecessary c/storage lookup in ListImages

f7d3228

We know we have a full image ID, so avoid the parsing complexity and c/storage lookup. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the list-filter-nonsense branch from 47a5bf4 to f7d3228 Compare October 9, 2023 21:05

saschagrunert approved these changes Oct 10, 2023

View reviewed changes

openshift-ci bot assigned saschagrunert Oct 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2023

cri-o deleted a comment from openshift-ci bot Oct 10, 2023

openshift-ci bot merged commit dda013c into cri-o:main Oct 10, 2023
60 of 64 checks passed

mtrmac deleted the list-filter-nonsense branch October 10, 2023 18:33

mtrmac mentioned this pull request Oct 10, 2023

RFC: Explicitly define a much stricter format for pause_image #7358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Turn ListImages(filter) into ImageStatus #7336

RFC: Turn ListImages(filter) into ImageStatus #7336

mtrmac commented Sep 26, 2023

mtrmac commented Sep 26, 2023

mtrmac commented Sep 26, 2023

haircommander Sep 26, 2023

mtrmac Sep 26, 2023

haircommander Sep 26, 2023

haircommander Sep 26, 2023

mtrmac Oct 9, 2023

haircommander Sep 26, 2023

mtrmac Oct 9, 2023

codecov bot commented Oct 3, 2023 •

edited

mtrmac commented Oct 9, 2023

mtrmac Oct 9, 2023

openshift-ci bot commented Oct 10, 2023

saschagrunert commented Oct 10, 2023

openshift-ci bot commented Oct 10, 2023

RFC: Turn ListImages(filter) into ImageStatus #7336

RFC: Turn ListImages(filter) into ImageStatus #7336

Conversation

mtrmac commented Sep 26, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

mtrmac commented Sep 26, 2023

mtrmac commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 3, 2023 • edited

Codecov Report

mtrmac commented Oct 9, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Oct 10, 2023

saschagrunert commented Oct 10, 2023

openshift-ci bot commented Oct 10, 2023

codecov bot commented Oct 3, 2023 •

edited