add integration test for pod with pvc has node-affinity to non-existent/illegal nodes #124666

chengjoey · 2024-05-02T09:32:46Z

What type of PR is this?

/kind cleanup
/sig scheduling

What this PR does / why we need it:

an integration test for pod with pvc has node-affinity to non-existent/illegal nodes could be schduable
related pr #124559
related issue #123465

Special notes for your reviewer: @Huang-Wei @alculquicondor

Does this PR introduce a user-facing change?

None

k8s-ci-robot · 2024-05-02T09:32:53Z

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.

k8s-ci-robot · 2024-05-02T09:32:54Z

Hi @chengjoey. 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.

alculquicondor · 2024-05-02T12:07:57Z

/ok-to-test

AxeZhan · 2024-05-02T14:38:45Z

test/integration/scheduler/filters/filters_test.go

+					AccessModes([]v1.PersistentVolumeAccessMode{v1.ReadWriteOncePod}).
+					Capacity(storage.Requests).
+					HostPathVolumeSource(&v1.HostPathVolumeSource{Path: "/tmp", Type: &volType}).
+					NodeAffinityIn("kubernetes.io/hostname", []string{"node-available", string(uuid.NewUUID()), string(uuid.NewUUID())}). // one node exist, two don't


Do we actually need to use UUID here? Won't a simple name like non-existing work as expected?

AxeZhan · 2024-05-02T14:48:58Z

test/integration/scheduler/filters/filters_test.go

@@ -2036,6 +2037,53 @@ func TestUnschedulablePodBecomesSchedulable(t *testing.T) {
 				return deletePod(cs, "pod-to-be-deleted", ns)
 			},
 		},
+		{
+			name: "pod with pvc has node-affinity to non-existent/illegal nodes",


Can we also add a case which pod got scheduled after node updated its label?

Maybe sth like this:

Pod with pv's affinity with foo in bar created

Node with label foo = foo created

Verify that the pod is not scheduled

Update the node with label foo = bar

Verify that the pod is scheduled successfully.

alculquicondor · 2024-05-02T16:06:48Z

/approve
I'll leave LGTM to @AxeZhan

k8s-ci-robot · 2024-05-02T16:07:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, chengjoey

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/scheduler/OWNERS~~ [alculquicondor]
~~test/integration/scheduler/OWNERS~~ [alculquicondor]

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

AxeZhan

Thanks, looks good! Only some nits about comments.

AxeZhan · 2024-05-03T10:15:48Z

pkg/scheduler/testing/wrappers.go

@@ -882,6 +882,20 @@ func (p *PersistentVolumeWrapper) HostPathVolumeSource(src *v1.HostPathVolumeSou
 	return p
 }

+// NodeAffinityIn creates a HARD node affinity (with the operator In)
+// // and injects into the pv.


Suggested change

// // and injects into the pv.

// and injects into the pv.

AxeZhan · 2024-05-03T10:16:16Z

test/integration/scheduler/filters/filters_test.go

+					AccessModes([]v1.PersistentVolumeAccessMode{v1.ReadWriteOncePod}).
+					Capacity(storage.Requests).
+					HostPathVolumeSource(&v1.HostPathVolumeSource{Path: "/tmp", Type: &volType}).
+					NodeAffinityIn("kubernetes.io/hostname", []string{"node-available", "non-existing"}). // one node exist, two don't


Suggested change

NodeAffinityIn("kubernetes.io/hostname", []string{"node-available", "non-existing"}). // one node exist, two don't

NodeAffinityIn("kubernetes.io/hostname", []string{"node-available", "non-existing"}). // one node exists, one doesn't

AxeZhan · 2024-05-03T10:17:20Z

test/integration/scheduler/filters/filters_test.go

+					AccessModes([]v1.PersistentVolumeAccessMode{v1.ReadWriteOncePod}).
+					Capacity(storage.Requests).
+					HostPathVolumeSource(&v1.HostPathVolumeSource{Path: "/tmp", Type: &volType}).
+					NodeAffinityIn("foo", []string{"bar"}). // one node exist, two don't


Suggested change

NodeAffinityIn("foo", []string{"bar"}). // one node exist, two don't

NodeAffinityIn("foo", []string{"bar"}).

Think we can just delete this comment here.

thanks @AxeZhan , done.

…nt/existent nodes Signed-off-by: joey <zchengjoey@gmail.com>

AxeZhan · 2024-05-03T12:24:05Z

/lgtm

k8s-ci-robot · 2024-05-03T12:24:12Z

LGTM label has been added.

Git tree hash: 2a3cf94ca45feab6d83c4d511167e34c612a84c6

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 2, 2024

k8s-ci-robot requested review from damemi and sanposhiho May 2, 2024 09:33

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 May 2, 2024

chengjoey force-pushed the ut-for-123465 branch from be295f1 to 25ffa37 Compare May 2, 2024 13:25

AxeZhan reviewed May 2, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2024

chengjoey force-pushed the ut-for-123465 branch from 25ffa37 to f4597ea Compare May 3, 2024 09:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 3, 2024

chengjoey force-pushed the ut-for-123465 branch from f4597ea to 7841788 Compare May 3, 2024 09:49

AxeZhan reviewed May 3, 2024

View reviewed changes

add integration test for pod with pvc has node-affinity to non-existe…

a56cc6b

…nt/existent nodes Signed-off-by: joey <zchengjoey@gmail.com>

chengjoey force-pushed the ut-for-123465 branch from 7841788 to a56cc6b Compare May 3, 2024 11:45

k8s-ci-robot assigned AxeZhan May 3, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2024

k8s-ci-robot merged commit f97ac22 into kubernetes:master May 3, 2024
15 checks passed

k8s-ci-robot added this to the v1.31 milestone May 3, 2024

chengjoey mentioned this pull request May 3, 2024

hotfix when a plugin (in-tree or out-of-tree) return non-existent/illegal nodes, the pod scheduling flow will abort immediately. #124559

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add integration test for pod with pvc has node-affinity to non-existent/illegal nodes #124666

add integration test for pod with pvc has node-affinity to non-existent/illegal nodes #124666

chengjoey commented May 2, 2024

k8s-ci-robot commented May 2, 2024

k8s-ci-robot commented May 2, 2024

alculquicondor commented May 2, 2024

AxeZhan May 2, 2024

chengjoey May 3, 2024

AxeZhan May 2, 2024

chengjoey May 3, 2024

alculquicondor commented May 2, 2024

k8s-ci-robot commented May 2, 2024

AxeZhan left a comment

AxeZhan May 3, 2024

AxeZhan May 3, 2024

AxeZhan May 3, 2024

chengjoey May 3, 2024

AxeZhan commented May 3, 2024

k8s-ci-robot commented May 3, 2024

	NodeAffinityIn("kubernetes.io/hostname", []string{"node-available", "non-existing"}). // one node exist, two don't
	NodeAffinityIn("kubernetes.io/hostname", []string{"node-available", "non-existing"}). // one node exists, one doesn't

	NodeAffinityIn("foo", []string{"bar"}). // one node exist, two don't
	NodeAffinityIn("foo", []string{"bar"}).

add integration test for pod with pvc has node-affinity to non-existent/illegal nodes #124666

add integration test for pod with pvc has node-affinity to non-existent/illegal nodes #124666

Conversation

chengjoey commented May 2, 2024

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer: @Huang-Wei @alculquicondor

Does this PR introduce a user-facing change?

k8s-ci-robot commented May 2, 2024

k8s-ci-robot commented May 2, 2024

alculquicondor commented May 2, 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

alculquicondor commented May 2, 2024

k8s-ci-robot commented May 2, 2024

AxeZhan 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

AxeZhan commented May 3, 2024

k8s-ci-robot commented May 3, 2024