feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list #125270

nikzayn · 2024-06-01T17:15:41Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds the net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list because to handle thousands of concurrent connections used by Cassandra, DataStax recommends these settings to optimize the Linux network stack.

Which issue(s) this PR fixes:

#125234

Fixes #125234

Special notes for your reviewer:

I have updated the test cases and it's running fine. Meanwhile, you can check and comment on this PR to suggest some other changes which I might need to update. One callout, I think that I also need to update the CheckSysctls function to support checks for the allowed sysctl list.

Does this PR introduce a user-facing change?

NONE

…list kubernetes#125234 Signed-off-by: nikzayn <nikhilvaidyar1997@gmail.com>

k8s-ci-robot · 2024-06-01T17:15:44Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

k8s-ci-robot · 2024-06-01T17:15:51Z

Hi @nikzayn. 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-sigs/prow repository.

mauri870 · 2024-06-01T17:51:01Z

pkg/kubelet/sysctl/safe_sysctls.go

@@ -60,6 +60,12 @@ var safeSysctls = []sysctl{
 		name:   "net.ipv4.tcp_keepalive_probes",
 		kernel: utilkernel.TCPKeepAliveProbesNamespacedKernelVersion,
 	},
+	{
+		name: "net.ipv4.tcp_rmem",


I believe these sysctls are supported starting from kernel 4.15. Can you add a constant to utilkernel and set the kernel property here, like the other entries above? Thanks.

utilkernel will look something like this

// TCPReceiveMemoryNamespacedKernelVersion is the kernel version in which net.ipv4.tcp_rmem was namespaced(netns). // (ref: https://github.com/torvalds/linux/commit/356d1833b638bd465672aefeb71def3ab93fc17d) const TCPReceiveMemoryNamespacedKernelVersion = "4.15" // TCPTransmitMemoryNamespacedKernelVersion is the kernel version in which net.ipv4.tcp_wmem was namespaced(netns). // (ref: https://github.com/torvalds/linux/commit/356d1833b638bd465672aefeb71def3ab93fc17d) const TCPTransmitMemoryNamespacedKernelVersion = "4.15"

Sure, I will add it. I know that we need to add it just wanted to be confirmed. Thanks!

mauri870 · 2024-06-01T17:53:40Z

Perhaps this also requires changes in staging/src/k8s.io/pod-security-admission/policy/check_sysctls.go?

nikzayn · 2024-06-01T18:29:48Z

Perhaps this also requires changes in staging/src/k8s.io/pod-security-admission/policy/check_sysctls.go?

Thanks, I will take a look and possibly make the changes accordingly. Thanks for reviewing.

…ernetes#125234 Signed-off-by: nikzayn <nikhilvaidyar1997@gmail.com>

k8s-ci-robot · 2024-06-01T18:49:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nikzayn
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr, liggitt for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS
pkg/util/kernel/OWNERS
staging/src/k8s.io/pod-security-admission/OWNERS

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

k8s-ci-robot · 2024-06-01T18:50:55Z

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found 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-sigs/prow repository.

mauri870 · 2024-06-01T18:57:03Z

staging/src/k8s.io/pod-security-admission/policy/check_sysctls.go

@@ -118,6 +124,10 @@ func sysctlsV1Dot29(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) Che
 	return sysctls(podMetadata, podSpec, sysctlsAllowedV1Dot29)
 }

+func sysctlsV1Dot30(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) CheckResult {


I'm not 100% sure this is correct/needed, I'd like a more experienced reviewer to confirm it.

Yeah, let's wait for an experienced member. Even I also have doubt for the same.

mauri870 · 2024-06-01T20:49:23Z

staging/src/k8s.io/pod-security-admission/test/fixtures_sysctls.go

+		},
+	}
+	registerFixtureGenerator(
+		fixtureKey{level: api.LevelBaseline, version: api.MajorMinorVersion(1, 29), check: "sysctls"},


Given that you are using the V1Dot30 suffix this should be 1, 30

Oh okay, thanks my bad!!. Fixing it right away

Signed-off-by: nikzayn <nikhilvaidyar1997@gmail.com>

SataQiu · 2024-06-17T14:40:29Z

/ok-to-test

ritazh · 2024-06-17T16:28:21Z

/assign @enj

ritazh · 2024-06-17T16:29:54Z

/triage accepted

ritazh · 2024-06-17T16:30:55Z

/sig network
/cc @thockin

thockin · 2024-06-17T17:57:55Z

This seems fine to me, but I don't know this code path, so I really want someone on sig-node to own the approval.

@pacoxu ?

pacoxu

This is target to v1.31 and like #118846 the test data needs some update.

pacoxu · 2024-06-19T08:03:19Z

staging/src/k8s.io/pod-security-admission/policy/check_sysctls.go

@@ -104,6 +106,10 @@ var (
 		"net.ipv4.tcp_keepalive_intvl",
 		"net.ipv4.tcp_keepalive_probes",
 	)
+	sysctlsAllowedV1Dot30 = sets.NewString(


Suggested change

sysctlsAllowedV1Dot30 = sets.NewString(

sysctlsAllowedV1Dot31 = sets.NewString(

pacoxu · 2024-06-19T08:03:43Z

staging/src/k8s.io/pod-security-admission/policy/check_sysctls.go

+func sysctlsV1Dot30(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) CheckResult {
+	return sysctls(podMetadata, podSpec, sysctlsAllowedV1Dot30)


Suggested change

func sysctlsV1Dot30(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) CheckResult {

return sysctls(podMetadata, podSpec, sysctlsAllowedV1Dot30)

func sysctlsV1Dot31(podMetadata *metav1.ObjectMeta, podSpec *corev1.PodSpec) CheckResult {

return sysctls(podMetadata, podSpec, sysctlsAllowedV1Dot31)

pacoxu · 2024-06-19T08:04:23Z

staging/src/k8s.io/pod-security-admission/policy/check_sysctls_test.go

+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			result := sysctlsV1Dot30(&tc.pod.ObjectMeta, &tc.pod.Spec)


Suggested change

result := sysctlsV1Dot30(&tc.pod.ObjectMeta, &tc.pod.Spec)

result := sysctlsV1Dot31(&tc.pod.ObjectMeta, &tc.pod.Spec)

mauri870 · 2024-06-19T14:10:43Z

staging/src/k8s.io/pod-security-admission/test/fixtures_sysctls.go

@@ -156,4 +156,39 @@ func init() {
 		fixtureKey{level: api.LevelBaseline, version: api.MajorMinorVersion(1, 29), check: "sysctls"},
 		fixtureDataV1Dot29,
 	)
+
+	fixtureDataV1Dot30 := fixtureGenerator{


Suggested change

fixtureDataV1Dot30 := fixtureGenerator{

fixtureDataV1Dot31 := fixtureGenerator{

Update other references accordingly.

mauri870 · 2024-06-19T14:11:05Z

staging/src/k8s.io/pod-security-admission/test/fixtures_sysctls.go

+		},
+	}
+	registerFixtureGenerator(
+		fixtureKey{level: api.LevelBaseline, version: api.MajorMinorVersion(1, 30), check: "sysctls"},


Suggested change

fixtureKey{level: api.LevelBaseline, version: api.MajorMinorVersion(1, 30), check: "sysctls"},

fixtureKey{level: api.LevelBaseline, version: api.MajorMinorVersion(1, 31), check: "sysctls"},

mauri870 · 2024-06-19T14:11:33Z

Thanks for working on this!

k8s-triage-robot · 2024-09-17T14:28:18Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

thockin · 2024-09-18T19:35:08Z

Is this PR abandoned? Does it need someone to take it over?

k8s-ci-robot · 2024-10-12T08:26:30Z

PR needs rebase.

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-sigs/prow repository.

pacoxu · 2024-10-12T08:40:13Z

#127489 is based on @nikzayn's commits here and was merged today.

(I co-authored you in the commits https://github.com/kubernetes/kubernetes/pull/127489/commits.)

/close

k8s-ci-robot · 2024-10-12T08:40:18Z

@pacoxu: Closed this PR.

In response to this:

#127489 is based on @nikzayn's commits here and was merged today.

/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-sigs/prow repository.

feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl …

b0cb9cc

…list kubernetes#125234 Signed-off-by: nikzayn <nikhilvaidyar1997@gmail.com>

k8s-ci-robot requested review from derekwaynecarr and krmayankk June 1, 2024 17:15

mauri870 reviewed Jun 1, 2024

View reviewed changes

feat: support changes for net.ipv4.tcp_rmem and net.ipv4.tcp_wmem kub…

219d5d3

…ernetes#125234 Signed-off-by: nikzayn <nikhilvaidyar1997@gmail.com>

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 1, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 1, 2024

Merge branch 'master' into feature/125234

18f0967

k8s-ci-robot added do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 1, 2024

mauri870 reviewed Jun 1, 2024

View reviewed changes

feat: versioning --update kubernetes#125234

b08ded2

Signed-off-by: nikzayn <nikhilvaidyar1997@gmail.com>

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 Jun 17, 2024

k8s-ci-robot assigned enj Jun 17, 2024

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 17, 2024

k8s-ci-robot requested a review from thockin June 17, 2024 16:30

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jun 17, 2024

pacoxu suggested changes Jun 19, 2024

View reviewed changes

mauri870 reviewed Jun 19, 2024

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 17, 2024

pacoxu mentioned this pull request Sep 20, 2024

feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list #127489

Merged

1 task

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2024

k8s-ci-robot closed this Oct 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list #125270

feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list #125270

nikzayn commented Jun 1, 2024

k8s-ci-robot commented Jun 1, 2024

k8s-ci-robot commented Jun 1, 2024

mauri870 Jun 1, 2024

mauri870 Jun 1, 2024

nikzayn Jun 1, 2024

mauri870 commented Jun 1, 2024

nikzayn commented Jun 1, 2024

k8s-ci-robot commented Jun 1, 2024

k8s-ci-robot commented Jun 1, 2024

mauri870 Jun 1, 2024

nikzayn Jun 2, 2024

mauri870 Jun 1, 2024

nikzayn Jun 2, 2024

SataQiu commented Jun 17, 2024

ritazh commented Jun 17, 2024

ritazh commented Jun 17, 2024

ritazh commented Jun 17, 2024

thockin commented Jun 17, 2024

pacoxu left a comment

pacoxu Jun 19, 2024

pacoxu Jun 19, 2024

pacoxu Jun 19, 2024

mauri870 Jun 19, 2024

mauri870 Jun 19, 2024

mauri870 commented Jun 19, 2024

k8s-triage-robot commented Sep 17, 2024

thockin commented Sep 18, 2024

k8s-ci-robot commented Oct 12, 2024

pacoxu commented Oct 12, 2024 •

edited

Loading

k8s-ci-robot commented Oct 12, 2024

	sysctlsAllowedV1Dot30 = sets.NewString(
	sysctlsAllowedV1Dot31 = sets.NewString(

		func sysctlsV1Dot30(podMetadata metav1.ObjectMeta, podSpec corev1.PodSpec) CheckResult {
		return sysctls(podMetadata, podSpec, sysctlsAllowedV1Dot30)

	result := sysctlsV1Dot30(&tc.pod.ObjectMeta, &tc.pod.Spec)
	result := sysctlsV1Dot31(&tc.pod.ObjectMeta, &tc.pod.Spec)

	fixtureDataV1Dot30 := fixtureGenerator{
	fixtureDataV1Dot31 := fixtureGenerator{

	fixtureKey{level: api.LevelBaseline, version: api.MajorMinorVersion(1, 30), check: "sysctls"},
	fixtureKey{level: api.LevelBaseline, version: api.MajorMinorVersion(1, 31), check: "sysctls"},

feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list #125270

feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list #125270

Conversation

nikzayn commented Jun 1, 2024

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?

k8s-ci-robot commented Jun 1, 2024

k8s-ci-robot commented Jun 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauri870 commented Jun 1, 2024

nikzayn commented Jun 1, 2024

k8s-ci-robot commented Jun 1, 2024

k8s-ci-robot commented Jun 1, 2024

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

SataQiu commented Jun 17, 2024

ritazh commented Jun 17, 2024

ritazh commented Jun 17, 2024

ritazh commented Jun 17, 2024

thockin commented Jun 17, 2024

pacoxu left a 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

mauri870 commented Jun 19, 2024

k8s-triage-robot commented Sep 17, 2024

thockin commented Sep 18, 2024

k8s-ci-robot commented Oct 12, 2024

pacoxu commented Oct 12, 2024 • edited Loading

k8s-ci-robot commented Oct 12, 2024

pacoxu commented Oct 12, 2024 •

edited

Loading