Test pod becoming schedulable when another pod is added or updated #92074

nodo · 2020-06-12T15:36:04Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
It adds two integration tests.

The first one is for a pod creation event. The pod we are testing is scheduled because a new pod with affinity his scheduled.

The second one is for a pod update event. The pod we are testing is scheduled because a previously scheduled pod is updated with a new label (which correspond to the affinity of the pod we are testing).

Which issue(s) this PR fixes:
Fixes of #91111

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2020-06-12T15:36:12Z

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

nodo · 2020-06-12T15:38:05Z

/cc @alculquicondor

Huang-Wei

I'd prefer to add other tests for pod-antiaffinity like this:

init: create 2 nodes and 2 pods with label "foo", with pod-antiaffinity to not co-exist with pod with label "foo" - so they don't get scheduled to the same node
pod: create a regular pod with label "foo"
update: delete one of the 2 pods we created in init()

And we can come up with another test:

init: create 2 nodes and 2 pods with label "foo", with nodeAffinity (or nodeSelector) to land on node1/node2 accordingly
pod: create a pod with label "foo", and with pod-antiaffinity to not co-exist with pod with label "foo"
update: delete one of the 2 pods we created in init()

Huang-Wei · 2020-06-12T19:29:37Z

test/integration/scheduler/predicates_test.go

@@ -1106,7 +1106,49 @@ func TestUnschedulablePodBecomesSchedulable(t *testing.T) {
 				return nil
 			},
 		},
-		// TODO(#91111): Add more test cases.
+		{
+			name: "other pod gets added",


Suggested change

name: "other pod gets added",

name: "pod with pod-affinity gets added",

Please, correct me if I am wrong, but I thought the purpose of #91111 is to add coverage to the events that triggers the scheduler. The tests you suggest seems to trigger the scheduler when a "pod gets deleted" which is covered already in #91734.

What makes you prefer these tests?

cc @alculquicondor

Ok, let's back up a bit.

The idea of these tests is to cover the different events that change the state of the cluster. Ideally, we cover all variations of events that we could think of. But since these are integrations tests, and they are expensive, we should be a bit more conservative.

Then, we can cover the cases where we now a different path is taken. That is not the case for delete: all pods are moved back to the active queue. So I'm happy to just keep the test we have that uses the Fit plugin. The rest of the affinity workflow is already covered with unit tests.

wdyt @Huang-Wei

@nodo @alculquicondor In terms of using integration tests to only cover different paths, I'm fine with holding off the extra deletion tests.

However, I think we missed the Update path, any thoughts on adding a test like this:

init: create a node and a pod (without any label). The pod is expected to be scheduled.

pod: create a pod without any lane, and with pod-affinity to co-exist with label "foo". The pod is expected to be pending - just like the current test.

update: apply label "foo" to the scheduled pod. Then it's expected to trigger cache#UpdatePod(), and the pending pod is expected to be scheduled.

Is that a valid case? Can you change labels in running Pods?
If so, I'm ok with having that test. Otherwise it's unnecessary.

I tried both plain pod and podTemplate, they're both mutable.

cool, it makes sense to me. I will add an additional test in this PR.

Don't forget to update the name of the test to be more descriptive :)

pod with pod-affinity gets added

aha! good point, thanks! :)

Huang-Wei · 2020-06-12T19:33:03Z

test/integration/scheduler/predicates_test.go

+				if err := utils.AddLabelsToNode(cs, node.Name, map[string]string{"region": "test"}); err != nil {
+					return fmt.Errorf("cannot add labels to node: %v", err)
+				}


Cannot we just initialize the node with the labels?

I was replicating the same logic in this other test: https://github.com/kubernetes/kubernetes/blob/master/test/integration/scheduler/predicates_test.go#L47-L58.

I could not find a test utility to create nodes with labels, do you know if there is any? If not I am not sure it's worth to add, but what do you think?

Sigh... we should really be cleaning existing mistakes, otherwise, other developers will copy them over and over.

For now, please leave a TODO here, as well as https://github.com/kubernetes/kubernetes/blob/master/test/integration/scheduler/predicates_test.go#L47-L58. A good and consistent way to craft nodes/pods is to use pkg/scheduler/testing/wrapper.go - but it's a big item.

Will do, I am happy to work on the cleanup after this PR.

alculquicondor · 2020-06-12T20:49:48Z

Let's just get all test cases described by @Huang-Wei in this PR

Huang-Wei · 2020-06-15T19:46:19Z

/ok-to-test

test/integration/scheduler/predicates_test.go

Huang-Wei · 2020-06-16T17:05:53Z

/retest

test/integration/scheduler/predicates_test.go

alculquicondor · 2020-06-16T17:38:22Z

test/integration/scheduler/predicates_test.go

@@ -1106,7 +1106,49 @@ func TestUnschedulablePodBecomesSchedulable(t *testing.T) {
 				return nil
 			},
 		},
-		// TODO(#91111): Add more test cases.
+		{
+			name: "other pod gets added",


Don't forget to update the name of the test to be more descriptive :)

pod with pod-affinity gets added

test/integration/scheduler/predicates_test.go

alculquicondor · 2020-06-16T19:36:02Z

/approve

will leave lgtm to @Huang-Wei

k8s-ci-robot · 2020-06-16T19:36:38Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/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

Huang-Wei · 2020-06-16T23:01:40Z

/lgtm

Thanks @nodo.

Huang-Wei · 2020-06-16T23:01:44Z

/retest

Huang-Wei · 2020-06-17T00:20:55Z

/retest

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 12, 2020

k8s-ci-robot requested review from Huang-Wei and wgliang June 12, 2020 15:37

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 12, 2020

k8s-ci-robot requested a review from alculquicondor June 12, 2020 15:38

Huang-Wei reviewed Jun 12, 2020

View reviewed changes

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 15, 2020

nodo commented Jun 16, 2020

View reviewed changes

test/integration/scheduler/predicates_test.go Show resolved Hide resolved

nodo changed the title ~~Add test case for a pod becoming schedulable when another pod is added~~ Test pod becoming schedulable when another pod is added or updated Jun 16, 2020

alculquicondor reviewed Jun 16, 2020

View reviewed changes

test/integration/scheduler/predicates_test.go Outdated Show resolved Hide resolved

alculquicondor reviewed Jun 16, 2020

View reviewed changes

Add test case for a pod becoming schedulable when another pod is added

ad0bee4

nodo force-pushed the pod-added-unschedulable-pod-integration-tests branch 2 times, most recently from 139a553 to 1e6027f Compare June 16, 2020 18:38

Test a pod becoming schedulable when a scheduled pod is updated

0c23caf

nodo force-pushed the pod-added-unschedulable-pod-integration-tests branch from 1e6027f to 0c23caf Compare June 16, 2020 18:40

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2020

k8s-ci-robot assigned Huang-Wei Jun 16, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2020

k8s-ci-robot merged commit e760ecd into kubernetes:master Jun 17, 2020

k8s-ci-robot added this to the v1.19 milestone Jun 17, 2020

nodo deleted the pod-added-unschedulable-pod-integration-tests branch June 17, 2020 05:49

nodo mentioned this pull request Jun 25, 2020

Use NodeWrapper to directly initialize nodes with labels #92514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test pod becoming schedulable when another pod is added or updated #92074

Test pod becoming schedulable when another pod is added or updated #92074

nodo commented Jun 12, 2020 •

edited

k8s-ci-robot commented Jun 12, 2020

nodo commented Jun 12, 2020

Huang-Wei left a comment

Huang-Wei Jun 12, 2020

nodo Jun 13, 2020

alculquicondor Jun 15, 2020

Huang-Wei Jun 15, 2020

alculquicondor Jun 15, 2020

Huang-Wei Jun 15, 2020

nodo Jun 16, 2020

alculquicondor Jun 16, 2020

nodo Jun 16, 2020

Huang-Wei Jun 12, 2020

nodo Jun 13, 2020

Huang-Wei Jun 15, 2020

nodo Jun 16, 2020

alculquicondor commented Jun 12, 2020

Huang-Wei commented Jun 15, 2020

Huang-Wei commented Jun 16, 2020

alculquicondor Jun 16, 2020

alculquicondor commented Jun 16, 2020

k8s-ci-robot commented Jun 16, 2020

Huang-Wei commented Jun 16, 2020

Huang-Wei commented Jun 16, 2020

Huang-Wei commented Jun 17, 2020

	name: "other pod gets added",
	name: "pod with pod-affinity gets added",

Test pod becoming schedulable when another pod is added or updated #92074

Test pod becoming schedulable when another pod is added or updated #92074

Conversation

nodo commented Jun 12, 2020 • edited

k8s-ci-robot commented Jun 12, 2020

nodo commented Jun 12, 2020

Huang-Wei 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

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

alculquicondor commented Jun 12, 2020

Huang-Wei commented Jun 15, 2020

Huang-Wei commented Jun 16, 2020

Choose a reason for hiding this comment

alculquicondor commented Jun 16, 2020

k8s-ci-robot commented Jun 16, 2020

Huang-Wei commented Jun 16, 2020

Huang-Wei commented Jun 16, 2020

Huang-Wei commented Jun 17, 2020

nodo commented Jun 12, 2020 •

edited