KEP 4216: Add changes for alpha version under RuntimeClassInImageCriApi feature gate #121456

kiashok · 2023-10-23T18:30:03Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add changes for alpha release of Image pull per runtime class feature described in KEP 4216: kubernetes/enhancements#4216

Adds feature gate "RuntimeClassInImageCriApi" and necessary related alpha changes

Which issue(s) this PR fixes:

Add changes for alpha release of Image pull per runtime class feature described in KEP 4216: kubernetes/enhancements#4216

Adds feature gate "RuntimeClassInImageCriApi" and necessary related alpha changes

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Added new feature gate called "RuntimeClassInImageCriApi" to address kubelet changes needed for KEP 4216.
Noteable changes:
1. Populate new RuntimeHandler field in CRI's ImageSpec struct during image pulls from container runtimes.
2. Pass runtimeHandler field in RemoveImage() call to container runtime in kubelet's image garbage collection

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


- [KEP]: https://github.com/kubernetes/enhancements/commit/5d2744011e205f6d759bac6ab316a085bec2a605

kiashok · 2023-10-23T18:30:38Z

cc @jsturtevant @marosset @aravindhp @mrunalp @haircommander @mikebrow

pkg/features/kube_features.go

jsturtevant · 2023-10-23T18:48:15Z

/priority important-soon
/sig windows
/sig node
/milestone v1.29

kiashok · 2023-10-24T03:12:48Z

/retest

haircommander · 2023-10-24T15:15:04Z

pkg/kubelet/kuberuntime/kuberuntime_image.go

+ // Therefore, validate that image returned from ListImages() call above has a valid
+ // runtimehandler field set; that is, it should not be ""
+ if utilfeature.DefaultFeatureGate.Enabled(features.RuntimeClassInImageCriAPI) {
+ if img.Spec != nil && img.Spec.RuntimeHandler == "" {


how would the CRI say the image was pulled in the default handler?

Not sure I understand the question -
If no runtimeHandler was specified for the pod in deployment yaml, then kubelet sets the pod's runtimehandler as "" and container runtime recognizes it and uses the default runtime handler. When the feature flag is set, we expect runtime handler to set the newly added runtime handler field of ImageSpec in CRI to fill in the runtime handler that was used - this field will be filled always (for default runtime used or non-default runtime handler used)
RuntimeHandler field added to CRI in #121121

This check here is making sure that the field is set by container runtimes when the feature gate is enabled and fails if it doesn't (this will indicate a bug on the container runtime side).

you should be able to show that in the mock test linked above ^

@mikebrow added test. Please let me know if this looks good. Thanks.

I am concerned about version skew policy if you require this field always be supported. Is there a reason kubelet can't consider default runtime handler the empty string "" and not have these error conditions if the CRI doesn't return a runtime handler? For instance, CRI-O may not want to support this feature and it'd be easier to leave it empty (though trivially easy to add support if there's a design reason to not allow the CRI to leave it empty that I've missed)

The admin decides if runtime class definition was changed and flushes/remove images from containerd and restarts. For kubelet a simple service restart should suffice right as it doesn't see to persist the tables that GC looks at. Not sure if node should be cordoned. Should they?

It was not immediately obvious to me that admin intervention was required. Not sure if this is a great UX but I will defer to SIG-node here.

Not sure that there is a better way to do this. If not, the existing images can be allowed to be loaded and used by containerd and eventually starting containers could fail and then user would have to flush containerd of the older images. The latter might be a painful debugging experience. There it might be better to flush containerd and restart just like in toggling the feature gate for this KEP would need manual intervention.
Alternatively, we could add an option/annotation/config on containerd side which when set in the toml causes containerd to purge and delete the images using the edited runtime class rather than loading them. Any thoughts on this last option @mikebrow @dmcgowan?

we had to make a similar concession in the cgroup driver reported by runtime KEP. Basically, it's up to the container runtime/admin to verify this information isn't changing while there are containers/images that already exist. Kubelet has no way to verify it mid run (or it would be inefficent to do so). Explicitly declaring this is a requirement of the CRI is good here, and having a mechanism in the CRI to report errors to the admin would be good to (but out of scope here)

this invariant https://github.com/kubernetes/kubernetes/pull/118770/files#diff-7a3c2723dedc6b44cf183fc52f84cc4015ffbd65a998a65c3959a953cb63c945R139-R141 it's declared in the CRI definition

this invariant https://github.com/kubernetes/kubernetes/pull/118770/files#diff-7a3c2723dedc6b44cf183fc52f84cc4015ffbd65a998a65c3959a953cb63c945R139-R141 it's declared in the CRI definition

This is great!
"The Kubelet will not re-request the RuntimeConfiguration after startup, and CRI implementations should avoid updating them without a full node reboot."
-- Node reboot might still not fix containerd loading previous images in containerd's store. I discussed this in the containerd community meeting this morning and we are fine with the existing invariant which is to ask admins to purge if runtimeclass name is the same but some of its fields like platform versioning and snapshotters have changed.

cc @mikebrow @dmcgowan

@haircommander @jsturtevant @aravindhp @mikebrow pushed new changes based on the discussion. Updated the tests as necessary.
Please let me know if it looks ok :) Thanks a lot!

haircommander · 2023-10-24T16:13:20Z

personally, I am more of a fan of writing the base struct and overriding with an if statement, rather than defining the struct in two places:


			im.imageRecords[image.ID] = &imageRecord{
				firstDetected: detectTime,
			}
			if utilfeature.DefaultFeatureGate.Enabled(features.RuntimeClassInImageCriAPI) {
				im.imageRecords[image.ID].runtimeHandlerUsedToPullImage = image.Spec.RuntimeHandler
			}

and so fourth. It's a stylistic thing, and if you prefer the way it is, keep it. but if it's no different to you, I'd prefer the other way :)

kiashok · 2023-10-24T16:42:23Z

The tests are failing with an unrelated error. Should this error be fixed in a separate PR?

cc @haircommander

kiashok · 2023-10-24T16:44:46Z

personally, I am more of a fan of writing the base struct and overriding with an if statement, rather than defining the struct in two places:
			im.imageRecords[image.ID] = &imageRecord{
				firstDetected: detectTime,
			}
			if utilfeature.DefaultFeatureGate.Enabled(features.RuntimeClassInImageCriAPI) {
				im.imageRecords[image.ID].runtimeHandlerUsedToPullImage = image.Spec.RuntimeHandler
			}
and so fourth. It's a stylistic thing, and if you prefer the way it is, keep it. but if it's no different to you, I'd prefer the other way :)

I'll make this change where applicable and push new changes.

aravindhp · 2023-10-31T21:44:28Z

This comment has already been fixed in the PR but it still complains about it. Does anyone know why this could be?
cc @kannon92

I notice this in my PR also. Seems that there was an update of the linter.
I think its saying that the first item for any exported function should be the variable first.
ie RuntimeClassInImageCRIAPI

@kannon92 , the latest changes in the PR has already addressed this. The comment starts with the name of the featuregate. But still the linter is complaining about the same

Looks like the linter is complaining about the comment starting with // owner: @kiashok which is strange given that is pattern elsewhere. As an experiment, I suggest starting the comment with the description and moving the rest below to see if the linter reacts differently. But this is going against the pattern in the file, so I am not sure how you should proceed. PS: you can try different combinations locally and figure out what works.

@kiashok Oh wait, this is with the pull-kubernetes-linter-hints job which is optional. I think you can ignore that and stick with pattern existing in the file.

mikebrow

/lgtm

k8s-ci-robot · 2023-10-31T21:55:21Z

LGTM label has been added.

Git tree hash: e0580f737b6574fa8c202a32f6967b9c9abb41d1

k8s-ci-robot · 2023-10-31T21:55:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, kiashok, mikebrow

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/features/OWNERS~~ [derekwaynecarr]
~~pkg/kubelet/OWNERS~~ [derekwaynecarr]

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

dims · 2023-10-31T22:31:55Z

golangci-lint-pr-hints has caught something, please fix.

kiashok · 2023-10-31T22:44:33Z

golangci-lint-pr-hints has caught something, please fix.

@dims This seems like a new linter check and not sure what its complaining about:

Any combination that I try is failing this linter. Other PRs that went in few hours back are starting their comments with // Owners just like the rest of this file. Do you happen to know what this linter check is about?

pkg/features/kube_features.go

dims · 2023-10-31T22:51:09Z

@kiashok if it doesn't make sense, you can use // nolint:revive to shut it off :) ( see https://github.com/search?type=code&q=%2Fnolint.*revive%2F+language%3AGo and https://golangci-lint.run/usage/false-positives/ )

This commit does the following: 1. Add RuntimeClassInImageCriApi feature gate 2. Extend pkg/kubelet/container Image struct 3. Adds runtimeHandler string in the following CRI calls i. ImageStatus ii. PullImageRequest iii. RemoveImage Signed-off-by: kiashok <[email protected]>

jsturtevant · 2023-10-31T22:56:09Z

/lgtm

k8s-ci-robot · 2023-10-31T22:56:16Z

LGTM label has been added.

Git tree hash: ede380cc2bb7f41c78f93324e4fb5688e8911a10

k8s-ci-robot · 2023-10-31T23:20:34Z

@kiashok: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-linter-hints	`252e1d2`	link	false	`/test pull-kubernetes-linter-hints`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

kannon92 · 2023-10-31T23:25:29Z

golangci-lint-pr-hints has caught something, please fix.

@dims This seems like a new linter check and not sure what its complaining about:

Any combination that I try is failing this linter. Other PRs that went in few hours back are starting their comments with // Owners just like the rest of this file. Do you happen to know what this linter check is about?

	// KubeletSeparateDiskGC enables Kubelet to garbage collection images/containers on different filesystems
	// owner: @kannon92
	// kep: https://kep.k8s.io/4191
	// alpha: v1.29
	KubeletSeparateDiskGC featuregate.Feature = "KubeletSeparateDiskGC"

Trying this in my PR. I don't love this change as it goes against all the other kubelet features. But it makes sense for exported functions.

kiashok · 2023-10-31T23:43:06Z

golangci-lint-pr-hints has caught something, please fix.

@dims This seems like a new linter check and not sure what its complaining about:

Any combination that I try is failing this linter. Other PRs that went in few hours back are starting their comments with // Owners just like the rest of this file. Do you happen to know what this linter check is about?
	// KubeletSeparateDiskGC enables Kubelet to garbage collection images/containers on different filesystems
	// owner: @kannon92
	// kep: https://kep.k8s.io/4191
	// alpha: v1.29
	KubeletSeparateDiskGC featuregate.Feature = "KubeletSeparateDiskGC"
Trying this in my PR. I don't love this change as it goes against all the other kubelet features. But it makes sense for exported functions.

yeah! The other PRs that went in for kubelet features, seem to have ignored this and kept to the format of the rest of the file which is // Owner followed by the rest

jsturtevant · 2023-10-31T23:44:43Z

I am in favor of matching the rest of the feature gates comments as it is now and letting the PR merge as is (since this isn't blocking).

We should create a follow up issue to sort his out. I don't think adding the nolint:revive makes sense here as it matches the rest of the comments in this file /cc @dims

kiashok · 2023-11-01T00:09:14Z

golangci-lint-pr-hints

Opened a new github issue to fix this #121660

sftim · 2023-11-22T14:44:15Z

Changelog suggestion

Added new feature gate `RuntimeClassInImageCriApi` to support per-RuntimeClass container image pulls

Notable changes:
1. Populate new `RuntimeHandler` field in CRI's `ImageSpec` struct during image pulls from container runtimes.
2. Pass `runtimeHandler` field in CRI `RemoveImage()` call to container runtime in kubelet's image garbage collection.

k8s-ci-robot requested review from liggitt and smarterclayton October 23, 2023 18:30

aravindhp reviewed Oct 23, 2023

View reviewed changes

pkg/features/kube_features.go Outdated Show resolved Hide resolved

k8s-ci-robot added this to the v1.29 milestone Oct 23, 2023

kiashok force-pushed the addRuntimeClassInCriFeatureGate branch from 3539df2 to 81ddedd Compare October 23, 2023 23:28

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/kubelet and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 23, 2023

kiashok changed the title ~~Add RuntimeClassInImageCriApi feature gate~~ KEP 4216: Add changes for alpha version under RuntimeClassInImageCriApi feature gate Oct 23, 2023

kiashok mentioned this pull request Oct 23, 2023

Pass runtime handler to image puller #118907

Closed

kiashok force-pushed the addRuntimeClassInCriFeatureGate branch from 81ddedd to 6cd700a Compare October 24, 2023 01:08

haircommander reviewed Oct 24, 2023

View reviewed changes

mikebrow approved these changes Oct 31, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023

kiashok mentioned this pull request Oct 31, 2023

KEP 4216: Follow up on ImageRuntimeHandler field in kuberuntime.Container #121658

Open

jsturtevant reviewed Oct 31, 2023

View reviewed changes

pkg/features/kube_features.go Outdated Show resolved Hide resolved

kiashok force-pushed the addRuntimeClassInCriFeatureGate branch from 481a472 to 252e1d2 Compare October 31, 2023 22:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023

k8s-ci-robot requested review from jsturtevant and mikebrow October 31, 2023 22:53

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023

kiashok mentioned this pull request Nov 1, 2023

Fix golangci-lint-pr-hints test #121660

Closed

k8s-ci-robot merged commit a8b7e19 into kubernetes:master Nov 1, 2023
14 of 15 checks passed

SIG Node PR Triage automation moved this from Needs Approver to Done Nov 1, 2023

pohly mentioned this pull request Nov 1, 2023

golangci-lint: tone down comment checking #121672

Merged

ChengyuZhu6 mentioned this pull request Nov 16, 2023

CC: Nydus-snapshotter trigger enablement cached? kata-containers/kata-containers#8337

Open

ChengyuZhu6 mentioned this pull request Apr 11, 2024

WIP: CC Adding image pull on host with snapshotter kata-containers/kata-containers#8186

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP 4216: Add changes for alpha version under RuntimeClassInImageCriApi feature gate #121456

KEP 4216: Add changes for alpha version under RuntimeClassInImageCriApi feature gate #121456

kiashok commented Oct 23, 2023 •

edited

kiashok commented Oct 23, 2023

jsturtevant commented Oct 23, 2023

kiashok commented Oct 24, 2023

haircommander Oct 24, 2023

kiashok Oct 24, 2023

mikebrow Oct 24, 2023

kiashok Oct 24, 2023

haircommander Oct 25, 2023

kiashok Oct 25, 2023

haircommander Oct 26, 2023

haircommander Oct 26, 2023

kiashok Oct 26, 2023

kiashok Oct 26, 2023

haircommander commented Oct 24, 2023

kiashok commented Oct 24, 2023

kiashok commented Oct 24, 2023

aravindhp commented Oct 31, 2023

mikebrow left a comment

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

dims commented Oct 31, 2023

kiashok commented Oct 31, 2023

dims commented Oct 31, 2023

jsturtevant commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

kannon92 commented Oct 31, 2023

kiashok commented Oct 31, 2023

jsturtevant commented Oct 31, 2023

kiashok commented Nov 1, 2023

sftim commented Nov 22, 2023

KEP 4216: Add changes for alpha version under RuntimeClassInImageCriApi feature gate #121456

KEP 4216: Add changes for alpha version under RuntimeClassInImageCriApi feature gate #121456

Conversation

kiashok commented Oct 23, 2023 • edited

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?

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

kiashok commented Oct 23, 2023

jsturtevant commented Oct 23, 2023

kiashok commented Oct 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Oct 24, 2023

kiashok commented Oct 24, 2023

kiashok commented Oct 24, 2023

aravindhp commented Oct 31, 2023

mikebrow left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

dims commented Oct 31, 2023

kiashok commented Oct 31, 2023

dims commented Oct 31, 2023

jsturtevant commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

kannon92 commented Oct 31, 2023

kiashok commented Oct 31, 2023

jsturtevant commented Oct 31, 2023

kiashok commented Nov 1, 2023

sftim commented Nov 22, 2023

kiashok commented Oct 23, 2023 •

edited