Fail Kubelet at startup if swap is configured with cgroup v1 #119327

iholder101 · 2023-07-14T12:05:31Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Follow-up to #118764 and #119726.

Since swap is no longer supported for cgroup v1, fail kubelet if it starts with swap and cgroup v1. This way, in future versions of k8s, we could turn NodeSwap on by default.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fail Kubelet at startup if swap is configured with cgroup v1

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

- KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/2400-node-swap/README.md

k8s-ci-robot · 2023-07-14T12:05:40Z

Hi @iholder101. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2023-07-14T12:05:41Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

ffromani · 2023-07-14T13:31:00Z

/ok-to-test

SergeyKanzhelev · 2023-07-17T20:09:13Z

pkg/kubelet/cm/container_manager_linux.go

@@ -225,6 +225,10 @@ func NewContainerManager(mountUtil mount.Interface, cadvisorInterface cadvisor.I
 				return nil, fmt.Errorf("running with swap on is not supported, please disable swap! or set --fail-swap-on flag to false. /proc/swaps contained: %v", swapLines)
 			}
 		}
+	} else {
+		if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.NodeSwap) && !cgroups.IsCgroup2UnifiedMode() {
+			return nil, fmt.Errorf("running swap with cgroups v1 is not supported. please either disable %s feature gate", kubefeatures.NodeSwap)


Suggested change

return nil, fmt.Errorf("running swap with cgroups v1 is not supported. please either disable %s feature gate", kubefeatures.NodeSwap)

return nil, fmt.Errorf("running swap with cgroups v1 is not supported. Please either disable %s feature gate or start using cgroup v2.", kubefeatures.NodeSwap)

Thanks :) Probably my mind had a context switch and didn't switch back..

This is not addressed yet but outdated for rebasing.

SergeyKanzhelev · 2023-07-17T20:12:20Z

pkg/kubelet/cm/container_manager_linux.go

@@ -225,6 +225,10 @@ func NewContainerManager(mountUtil mount.Interface, cadvisorInterface cadvisor.I
 				return nil, fmt.Errorf("running with swap on is not supported, please disable swap! or set --fail-swap-on flag to false. /proc/swaps contained: %v", swapLines)
 			}
 		}
+	} else {
+		if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.NodeSwap) && !cgroups.IsCgroup2UnifiedMode() {


I wasn't thinking about it before, but now I realize that the problem with this is that all tests running today with --fail-on-swap=false will stop working as the feature gate will be enabled by default.

Even though it is a desirable behavior to fail fast long term, breaking many scenarios is not ideal.

One option is to change the default value for the feature gate.

@deads2k (PRR reviewer for the SWAP KEP) for the opinion

I wasn't thinking about it before, but now I realize that the problem with this is that all tests running today with --fail-on-swap=false will stop working as the feature gate will be enabled by default.

Yes this is exactly right, and was one of the reasons I asked to do so in a follow-up :)

I think you're right, and that the right approach moving forward is:

Introduce a PR that would adapt the tests.

Merge this PR after making sure it won't break any lanes.

One option is to change the default value for the feature gate.

What do you mean by that? Isn't the default behavior for the feature gate is to be disabled?
IIUC all we need to do is remove all jobs that are defined with cgroup v1 + NodeSwap enabled. We should keep (and maybe add more? but outside of this PR's scope) the jobs with swap + v2.

WDYT?

iholder101 · 2023-07-27T10:47:57Z

/test all

Let's see exactly which tests fail to discuss our next steps.

iholder101 · 2023-07-30T09:30:37Z

/test pull-kubernetes-cos-cgroupv1-containerd-node-e2e
/test pull-kubernetes-cos-cgroupv1-containerd-node-e2e-features
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-features
/test pull-kubernetes-node-crio-cgrpv2-e2e
/test pull-kubernetes-node-crio-e2e
/test pull-kubernetes-node-kubelet-serial-crio-cgroupv1
/test pull-kubernetes-node-kubelet-serial-crio-cgroupv2
/test pull-kubernetes-node-swap-fedora
/test pull-kubernetes-node-swap-fedora-serial

iholder101 · 2023-07-30T15:36:53Z

/test pull-kubernetes-node-swap-ubuntu-serial

iholder101 · 2023-07-30T15:45:54Z

/test pull-kubernetes-node-kubelet-serial-crio-cgroupv2
/test pull-kubernetes-node-kubelet-serial-crio-cgroupv1

iholder101 · 2023-08-08T15:19:35Z

/test pull-kubernetes-e2e-kind-ipv6

iholder101 · 2023-08-08T15:29:05Z

@SergeyKanzhelev Can you PTAL?

I've also changed the way the functional test checks whether a feature gate is on. It seem to work nice, as the tests pass for several times now.

note: pull-kubernetes-node-swap-ubuntu-serial is failing waiting for Kubelet to be ready. I'm not sure it's related to recent changes and suspect it's just flakie. WDYT?

iholder101 · 2023-08-09T07:51:34Z

/retest

iholder101 · 2023-08-23T14:43:17Z

@SergeyKanzhelev I've moved the commits that were dealing with the feature gate check into another PR: #120139. This should be simpler to review now. PTAL.

iholder101 · 2023-08-23T15:13:08Z

/test pull-kubernetes-node-kubelet-serial-crio-cgroupv2
/test pull-kubernetes-node-kubelet-serial-crio-cgroupv1

/test pull-kubernetes-node-swap-ubuntu-serial

/test pull-kubernetes-node-swap-fedora
/test pull-kubernetes-node-swap-fedora-serial

k8s-ci-robot · 2023-08-23T16:59:48Z

@iholder101: The following tests 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-node-swap-ubuntu-serial	dd3a8259bf894fb87fb5071d431a047a3468ab89	link	false	`/test pull-kubernetes-node-swap-ubuntu-serial`
pull-kubernetes-node-swap-fedora	dd3a8259bf894fb87fb5071d431a047a3468ab89	link	false	`/test pull-kubernetes-node-swap-fedora`
pull-kubernetes-node-swap-fedora-serial	dd3a8259bf894fb87fb5071d431a047a3468ab89	link	false	`/test pull-kubernetes-node-swap-fedora-serial`
pull-kubernetes-node-kubelet-serial-crio-cgroupv2	dd3a8259bf894fb87fb5071d431a047a3468ab89	link	false	`/test pull-kubernetes-node-kubelet-serial-crio-cgroupv2`
pull-kubernetes-node-kubelet-serial-crio-cgroupv1	dd3a8259bf894fb87fb5071d431a047a3468ab89	link	false	`/test pull-kubernetes-node-kubelet-serial-crio-cgroupv1`

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.

Signed-off-by: Itamar Holder <[email protected]>

iholder101 · 2023-09-20T14:44:03Z

@SergeyKanzhelev ping
would you mind having another look?

kannon92 · 2023-10-26T21:50:35Z

/cc

kannon92 · 2023-10-26T21:53:52Z

pkg/kubelet/cm/container_manager_linux.go

@@ -224,6 +224,8 @@ func NewContainerManager(mountUtil mount.Interface, cadvisorInterface cadvisor.I
 				return nil, fmt.Errorf("running with swap on is not supported, please disable swap! or set --fail-swap-on flag to false. /proc/swaps contained: %v", swapLines)
 			}
 		}
+	} else if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.NodeSwap) && !cgroups.IsCgroup2UnifiedMode() {
+		return nil, fmt.Errorf("running swap with cgroups v1 is not supported. please either disable %s feature gate", kubefeatures.NodeSwap)


Suggested change

return nil, fmt.Errorf("running swap with cgroups v1 is not supported. please either disable %s feature gate", kubefeatures.NodeSwap)

return nil, fmt.Errorf("running swap with cgroups v1 is not supported. please disable %s feature gate", kubefeatures.NodeSwap)

kannon92 · 2023-12-08T20:35:39Z

@iholder101 thank you for this change. I moved the code to #122241 so we can continue this work.

/close

k8s-ci-robot · 2023-12-08T20:35:46Z

@kannon92: Closed this PR.

In response to this:

@iholder101 thank you for this change. I moved the code to #122241 so we can continue this work.

/close

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.

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 14, 2023

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 14, 2023

k8s-ci-robot requested review from derekwaynecarr and matthyx July 14, 2023 12:06

iholder101 mentioned this pull request Jul 14, 2023

[KEP-2400] Add full cgroup v2 swap support with automatically calculated swap limit for LimitedSwap and Burstable QoS Pods #118764

Merged

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 14, 2023

SergeyKanzhelev reviewed Jul 17, 2023

View reviewed changes

iholder101 mentioned this pull request Jul 19, 2023

[KEP-2400] Node memory swap support (replacement for inactive issue #2400) kubernetes/enhancements#4128

Closed

35 tasks

This was referenced Jul 30, 2023

[crio_swap1g.ign]: Use cgroup v2 instead of v1 kubernetes/test-infra#30242

Merged

Adapt swap jobs to run with cgroup v2 #119726

Closed

pull-kubernetes-node-swap-ubuntu-serial to run with cgroup v2 kubernetes/test-infra#30297

Merged

k8s-ci-robot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Aug 8, 2023

iholder101 marked this pull request as draft August 23, 2023 14:08

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2023

iholder101 force-pushed the swap/fail-kubelet-with-cgroupv1 branch from 8c5e84f to afb6bab Compare August 23, 2023 14:41

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2023

iholder101 marked this pull request as ready for review August 23, 2023 14:41

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2023

k8s-ci-robot requested review from bart0sh and krmayankk August 23, 2023 14:42

iholder101 force-pushed the swap/fail-kubelet-with-cgroupv1 branch from afb6bab to 2fa0a64 Compare August 23, 2023 14:42

iholder101 force-pushed the swap/fail-kubelet-with-cgroupv1 branch from 2fa0a64 to dd3a825 Compare August 23, 2023 15:11

pacoxu mentioned this pull request Sep 14, 2023

Node memory swap support kubernetes/enhancements#2400

Open

69 tasks

iholder101 added 2 commits September 20, 2023 17:43

Fail Kubelet at startup if swap is configured with cgroup v1

467a8de

Signed-off-by: Itamar Holder <[email protected]>

swap functest: log isLimitedSwap

59fd923

Signed-off-by: Itamar Holder <[email protected]>

iholder101 force-pushed the swap/fail-kubelet-with-cgroupv1 branch from dd3a825 to 59fd923 Compare September 20, 2023 14:43

k8s-ci-robot requested a review from kannon92 October 26, 2023 21:50

kannon92 reviewed Oct 26, 2023

View reviewed changes

kannon92 mentioned this pull request Dec 8, 2023

Fail Kubelet at startup if swap is configured with cgroup v1 #122241

Closed

k8s-ci-robot closed this Dec 8, 2023

	return nil, fmt.Errorf("running swap with cgroups v1 is not supported. please either disable %s feature gate", kubefeatures.NodeSwap)
	return nil, fmt.Errorf("running swap with cgroups v1 is not supported. Please either disable %s feature gate or start using cgroup v2.", kubefeatures.NodeSwap)

Fail Kubelet at startup if swap is configured with cgroup v1 #119327

Fail Kubelet at startup if swap is configured with cgroup v1 #119327

Uh oh!

Conversation

iholder101 commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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.:

Uh oh!

k8s-ci-robot commented Jul 14, 2023

Uh oh!

k8s-ci-robot commented Jul 14, 2023

Uh oh!

ffromani commented Jul 14, 2023

Uh oh!

SergeyKanzhelev Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

iholder101 Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

pacoxu Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

iholder101 Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

iholder101 commented Jul 27, 2023

Uh oh!

iholder101 commented Jul 30, 2023

Uh oh!

iholder101 commented Jul 30, 2023

Uh oh!

iholder101 commented Jul 30, 2023

Uh oh!

iholder101 commented Aug 8, 2023

Uh oh!

iholder101 commented Aug 8, 2023

Uh oh!

iholder101 commented Aug 9, 2023

Uh oh!

iholder101 commented Aug 23, 2023

Uh oh!

iholder101 commented Aug 23, 2023

Uh oh!

k8s-ci-robot commented Aug 23, 2023

Uh oh!

iholder101 commented Sep 20, 2023

Uh oh!

kannon92 commented Oct 26, 2023

Uh oh!

kannon92 Oct 26, 2023

Choose a reason for hiding this comment

Uh oh!

kannon92 commented Dec 8, 2023

Uh oh!

k8s-ci-robot commented Dec 8, 2023

Uh oh!

Uh oh!

iholder101 commented Jul 14, 2023 •

edited

Loading