e2e_node: add a test to verify kubelet fails to create pod if userns isn't supported #127484

haircommander · 2024-09-19T20:22:47Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

... and the pod requests a user namespace

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

none

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

haircommander · 2024-09-19T20:22:57Z

/hold

requires kubernetes/test-infra#33511 to actually run

test/e2e_node/user_namespaces_test.go

haircommander · 2024-09-19T20:55:21Z

/test pull-kubernetes-node-crio-cgrpv2-userns-e2e-serial

bart0sh · 2024-09-19T21:04:31Z

/triage accepted
/priority important-soon

kannon92 · 2024-09-19T21:14:39Z

/hold

Let's confirm that the test you added works before merge.

haircommander · 2024-09-20T13:20:20Z

/hold cancel

new test passed https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/127484/pull-kubernetes-node-crio-cgrpv2-userns-e2e-serial/1836871735580823552 some other test that's now in this suite failed, but that shouldn't block IMO

aojea · 2024-09-21T15:48:01Z

test/e2e_node/user_namespaces_test.go

+	admissionapi "k8s.io/pod-security-admission/api"
+)
+
+var _ = SIGDescribe("UserNamespaces", "[LinuxOnly]", nodefeature.UserNamespacesSupport, framework.WithSerial(), func() {


is this test really serial?

Yes, when ever a feature gate is toggled for kubelet, the test has to be serial. Changing kubelet configs triggers a restart of kubelet.

Til, thanks, this explains why some test here are tagged as serial when they are not ..., authors may forget to remove the serial decorator when it is ga .... @pohly does it make sense to automate this behavior for the e2e_node so authors don't need to worry about this behavior?

cc @SergeyKanzhelev

I don't see how. nodefeature.UserNamespacesSupport just says "this test depends on this feature", it doesn't say "this test needs to toogle that feature", so nodefeature.UserNamespacesSupport should not implicitly add the serial tag.

authors may forget to remove the serial decorator when it is ga

Doesn't it say below that the entire test needs to be removed once the feature is GA?

yeah this test is only serial because it changes the kubelet's feature gates, not specifically toggles user namespaces. Once user namespaces are GA'd (and specifically when the feature gate is dropped), we have to remove this test because we can't test with user namespaces disabled anymore

Once user namespaces are GA'd (and specifically when the feature gate is dropped), we have to remove this test because we can't test with user namespaces disabled anymore

Do we need to capture this in userns GA plan not to forget ?

Actually with apiserver emulated versions we'll keep feature gates around after GA, so it will only be removed when it's been GA everywhere for a while. I think this test could also be updated in the future to handle when the node doesn't support userns

aojea · 2024-09-21T15:49:34Z

test/e2e_node/user_namespaces_test.go

+
+			// Pod should stay in pending
+			// Events would be a better way to tell this, as we could actually read the event,
+			// but history proves events aren't reliable enough to base a test on.


v1.Events? those must no be used on tests, are best effort , there is no guarantee there are going to be delivered

A bit confused, how to interpret lines 75 - 76, do you mean events are not a better way ( so we could just delete the two comment lines ) or you mean we should improve reliability of events ?

I mean they'd be a more accurate way if they could be relied on, but since they can't be relied on we can't use them, even though it'd give us the exact reason the pod is still in pending

Thanks for clarifying

esotsal · 2025-03-11T08:57:10Z

/lgtm

esotsal · 2025-03-11T09:03:03Z

/hold

@aojea , @pohly , @bart0sh and @SergeyKanzhelev please review / approve and decide if it is ok to cancel hold, thanks

pohly · 2025-03-12T09:04:03Z

Looks good to me. I think the pending discussions have been resolved.

This is a SIG Node test. It should be approved by someone from SIG Node, not me as SIG Testing TL.

SergeyKanzhelev

/lgtm
/approve

k8s-ci-robot · 2025-03-19T21:45:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, SergeyKanzhelev

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:

~~test/e2e_node/OWNERS~~ [SergeyKanzhelev]

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

esotsal · 2025-03-20T07:12:25Z

/hold cancel

esotsal · 2025-03-20T09:21:11Z

/lgtm cancel

@haircommander this PR cannot be merged because of [KEP-3041] - remove nodefeatures from k/k repo

/cc @kannon92

…isn't supported and the pod requests a user namespace Signed-off-by: Peter Hunt <[email protected]>

haircommander · 2025-03-20T13:35:18Z

oops good point! fixed

haircommander · 2025-03-20T14:26:17Z

/retest

esotsal · 2025-03-20T15:06:04Z

/lgtm

k8s-ci-robot · 2025-03-20T15:06:12Z

LGTM label has been added.

Git tree hash: c9efe769ec6e02c1b0c200a6ebab5e1b5b507470

BenTheElder · 2025-03-20T18:31:59Z

... and the pod requests a user namespace

Phew, I had a huge ???? moment when this PR showed up in the merge queue and the title elided this part 😂

I feel like maybe the super long title would be worth it this time 🙃 🚲 🏠 🎨 🖌️

haircommander · 2025-03-20T18:46:51Z

We have to urge users to switch to userns pods somehow 😈

k8s-ci-robot requested review from mrunalp and odinuge September 19, 2024 20:23

haircommander force-pushed the userns-off-e2e branch from 0b0b4cd to 259f70a Compare September 19, 2024 20:34

kannon92 mentioned this pull request Sep 19, 2024

e2e_node: update userns job to run serially kubernetes/test-infra#33511

Merged

kannon92 reviewed Sep 19, 2024

View reviewed changes

test/e2e_node/user_namespaces_test.go Show resolved Hide resolved

haircommander force-pushed the userns-off-e2e branch from 259f70a to bbcea1c Compare September 19, 2024 20:39

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2024

aojea reviewed Sep 21, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2025

esotsal moved this from PRs Waiting on Author to PRs - Needs Approver in SIG Node CI/Test Board Mar 11, 2025

esotsal moved this from Waiting on Author to Needs Approver in SIG Node: code and documentation PRs Mar 11, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2025

SergeyKanzhelev reviewed Mar 19, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2025

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2025

k8s-ci-robot requested a review from kannon92 March 20, 2025 09:21

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2025

k8s-ci-robot requested review from esotsal and SergeyKanzhelev March 20, 2025 09:21

e2e_node: add a test to verify kubelet fails to create pod if userns …

8b4aab5

…isn't supported and the pod requests a user namespace Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the userns-off-e2e branch from bbcea1c to 8b4aab5 Compare March 20, 2025 13:35

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2025

k8s-ci-robot merged commit 48ba25a into kubernetes:master Mar 20, 2025
14 checks passed

k8s-ci-robot added this to the v1.33 milestone Mar 20, 2025

github-project-automation bot moved this from PRs - Needs Approver to Done in SIG Node CI/Test Board Mar 20, 2025

github-project-automation bot moved this from Needs Approver to Done in SIG Node: code and documentation PRs Mar 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e_node: add a test to verify kubelet fails to create pod if userns isn't supported #127484

e2e_node: add a test to verify kubelet fails to create pod if userns isn't supported #127484

haircommander commented Sep 19, 2024

haircommander commented Sep 19, 2024

haircommander commented Sep 19, 2024

bart0sh commented Sep 19, 2024

kannon92 commented Sep 19, 2024

haircommander commented Sep 20, 2024

aojea Sep 21, 2024

kannon92 Sep 21, 2024

aojea Sep 21, 2024

pohly Sep 23, 2024

haircommander Sep 23, 2024

esotsal Jan 22, 2025 •

edited

Loading

haircommander Mar 7, 2025

aojea Sep 21, 2024

haircommander Sep 23, 2024

esotsal Jan 22, 2025

haircommander Mar 7, 2025

esotsal Mar 10, 2025

esotsal commented Mar 11, 2025

esotsal commented Mar 11, 2025 •

edited

Loading

pohly commented Mar 12, 2025

SergeyKanzhelev left a comment

k8s-ci-robot commented Mar 19, 2025

esotsal commented Mar 20, 2025

esotsal commented Mar 20, 2025

haircommander commented Mar 20, 2025

haircommander commented Mar 20, 2025

esotsal commented Mar 20, 2025

k8s-ci-robot commented Mar 20, 2025

BenTheElder commented Mar 20, 2025

haircommander commented Mar 20, 2025

e2e_node: add a test to verify kubelet fails to create pod if userns isn't supported #127484

e2e_node: add a test to verify kubelet fails to create pod if userns isn't supported #127484

Conversation

haircommander commented Sep 19, 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?

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

haircommander commented Sep 19, 2024

haircommander commented Sep 19, 2024

bart0sh commented Sep 19, 2024

kannon92 commented Sep 19, 2024

haircommander commented Sep 20, 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

Choose a reason for hiding this comment

esotsal Jan 22, 2025 • edited Loading

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

esotsal commented Mar 11, 2025

esotsal commented Mar 11, 2025 • edited Loading

pohly commented Mar 12, 2025

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 19, 2025

esotsal commented Mar 20, 2025

esotsal commented Mar 20, 2025

haircommander commented Mar 20, 2025

haircommander commented Mar 20, 2025

esotsal commented Mar 20, 2025

k8s-ci-robot commented Mar 20, 2025

BenTheElder commented Mar 20, 2025

haircommander commented Mar 20, 2025

esotsal Jan 22, 2025 •

edited

Loading

esotsal commented Mar 11, 2025 •

edited

Loading