Add image_id to CRI Container message

[saschagrunert]

What type of PR is this?

/kind bug

What this PR does / why we need it:

This new field allows fixing the kubelet image garbage collection in container runtimes. The image_ref has been historically used by container runtimes to reference images by digest.

Which issue(s) this PR fixes:

Allows to fix cri-o/cri-o#7143

Special notes for your reviewer:

cc @mrunalp @haircommander @kubernetes/sig-node-api-reviews

Does this PR introduce a user-facing change?

Allowing container runtimes to fix an image garbage collection bug by adding an `image_id` field to the CRI Container message.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

[saschagrunert]

/retest

[k8s-triage-robot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[saschagrunert]

/test pull-kubernetes-unit

[haircommander]

thanks for picking this up! I believe for backwards compatibility with the kube pod API we need to change https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_pods.go#L1903 to be ImageID: ImageRef (if imageRef is != "", otherwise ImageID) or else we'll do the same thing as if CRI reported ImageID as ImageRef (new, broken cri-o behavior)

[saschagrunert]

thanks for picking this up! I believe for backwards compatibility with the kube pod API we need to change https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_pods.go#L1903 to be ImageID: ImageRef (if imageRef is != "", otherwise ImageID) or else we'll do the same thing as if CRI reported ImageID as ImageRef (new, broken cri-o behavior)

@haircommander the ContainerStatus has not changed in this PR, means we have no field ImageRef at all. Therefore the behavior will also not change. We only changed the Container message, which is the one used for the image GC.

[sftim]

Does this PR make any change that a Kubernetes cluster operator could observe (even indirectly, but without using a debugger or trace logging)?

[saschagrunert]

Does this PR make any change that a Kubernetes cluster operator could observe (even indirectly, but without using a debugger or trace logging)?

No, runtimes do not support that field yet so there will be no change. Only if runtimes set the value of the field, then it will influence how the image GC works.

[sftim]

runtimes do not support that field yet so there will be no change

If I used the release of Kubernetes that included this change, and a container runtime with support, would I notice the behavior? If so, we should change log it so people know it's available.

[mrunalp]

@mikebrow @SergeyKanzhelev ptal

[mikebrow]

see suggested comment doc edits

[mikebrow]

thanks for picking this up! I believe for backwards compatibility with the kube pod API we need to change https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_pods.go#L1903 to be ImageID: ImageRef (if imageRef is != "", otherwise ImageID) or else we'll do the same thing as if CRI reported ImageID as ImageRef (new, broken cri-o behavior)

@haircommander the ContainerStatus has not changed in this PR, means we have no field ImageRef at all. Therefore the behavior will also not change. We only changed the Container message, which is the one used for the image GC.

fair to say we should also fix the status response in a similar fashion.. perhaps in another pr?

[haircommander]

can someone help find where we propegate kuberuntime.ContainerStatus to the kube API? From my perspective, we are translating kuberuntime.ImageID -> ContainerStatus.ImageID somewhere but I am struggling to find where. If that's the case, then it feels like this PR would change it so that we're reporting ImageID (which is coming from the CRI as an ImageID, not ImageRef) and then we'd change the ContainerStatus to have the hash and not repo + digest. It sounds like this isn't what is happening though

[saschagrunert]

runtimes do not support that field yet so there will be no change

If I used the release of Kubernetes that included this change, and a container runtime with support, would I notice the behavior? If so, we should change log it so people know it's available.

Yes, changed the release note for that.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikebrow, mrunalp, 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:

• pkg/kubelet/OWNERS [mrunalp]
• staging/src/k8s.io/cri-api/OWNERS [mikebrow,mrunalp,saschagrunert]

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

[saschagrunert]

There is a image ID test available: https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/image_id_test.go

CRI-O still returns the image ID if the image is already present on disk, that's why the test allows both:

kubernetes/test/e2e_node/image_id_test.go

Lines 67 to 68 in 411c29c

	gomega.Equal(busyBoxImage),
	gomega.MatchRegexp(`[[:xdigit:]]{64}`),

[saschagrunert]

But there is a corner case when developers use ConvertPodStatusToRunningPod and override the field. Closing this gap in #123583