[RuntimeClassScheduling] test(runtimeclass): add e2e tests for runtimeclass scheduling - Part3 #81915

draveness · 2019-08-26T00:10:44Z

/kind cleanup
/sig node
/priority important-soon
/assign @tallclair @yastij

What this PR does / why we need it:

Add e2e tests for runtimeclass scheduling

Which issue(s) this PR 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.:

draveness · 2019-08-26T01:00:14Z

/retest

tallclair · 2019-08-26T18:11:17Z

Can you cherry-pick #81016 into this PR so that the tests can be run?

draveness · 2019-08-26T18:25:19Z

Do you mean #81862? Regards, Draven

…

On Aug 27, 2019, 2:14 AM +0800, Tim Allclair (St. Clair) ***@***.***>, wrote: Can you cherry-pick #81016 into this PR so that the tests can be run? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

tallclair

Thanks!

tallclair · 2019-08-26T18:14:05Z

test/e2e/common/runtimeclass.go

@@ -97,21 +98,100 @@ var _ = ginkgo.Describe("[sig-node] RuntimeClass", func() {
 		pod := createRuntimeClassPod(f, rcName)
 		expectSandboxFailureEvent(f, pod, fmt.Sprintf("\"%s\" not found", rcName))
 	})
+
+	ginkgo.It("should reject a Pod requesting a RuntimeClass with conflict node selector", func() {


needs [NodeFeature:RuntimeHandler]

Actually, since we expect this to fail in scheduling, the handler doesn't matter, so you can leave it off.

tallclair · 2019-08-26T18:14:22Z

test/e2e/common/runtimeclass.go

+		handler := PreconfiguredRuntimeHandler
+		if framework.TestContext.ContainerRuntime == "docker" {
+			handler = DockerRuntimeHandler
+		}


extract this to a helper function in the framework directory, so it can be shared with the non-common tests.

tallclair · 2019-08-26T18:18:31Z

test/e2e/common/runtimeclass.go

+			"foo": "bar",
+		}
+		f.PodClient().Create(pod)
+		expectSandboxFailureEvent(f, pod, "conflict: runtimeClass.scheduling.nodeSelector[foo] = bar; pod.spec.nodeselector[foo] = conflict")


This should be an admission failure, not a sandbox failure. I'd expect the Create call to fail. Since the PodClient always asserts success, you'll need to call the client directly:

_, err := f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(...)

tallclair · 2019-08-26T18:19:15Z

test/e2e/common/runtimeclass.go

+		expectSandboxFailureEvent(f, pod, "conflict: runtimeClass.scheduling.nodeSelector[foo] = bar; pod.spec.nodeselector[foo] = conflict")
+	})
+
+	ginkgo.It("should run a Pod requesting a RuntimeClass with scheduling", func() {


Here you need the NodeFeature

tallclair · 2019-08-26T18:24:11Z

test/e2e/common/runtimeclass.go

+		f.PodClient().Create(pod)
+
+		expectPodSuccess(f, pod)
+		pod, err = f.PodClient().Get(pod.Name, metav1.GetOptions{})


nit: just use the pod returned by PodClient().Create

tallclair · 2019-08-26T18:29:02Z

test/e2e/common/runtimeclass.go

+		}
+		f.PodClient().Create(pod)
+
+		expectPodSuccess(f, pod)


I don't think you ever labeled the nodes? In that case the pod won't actually be scheduled. See https://github.com/kubernetes/kubernetes/blob/master/test/e2e/scheduling/predicates.go#L372
Also, taint the node.

Once the node is labeled, also verify the pod is assigned to the correct node.

why are we tainting the node? this is disruptive and seems unnecessary?

tainting might break parallel tests - so either Disruptive or Serial seems appropriate.

I see now that runtimeclass scheduling includes taints.

filed #83647 for tagging the test, looking for feedback...

tallclair · 2019-08-26T18:32:47Z

test/e2e/common/runtimeclass.go

@@ -97,21 +98,100 @@ var _ = ginkgo.Describe("[sig-node] RuntimeClass", func() {
 		pod := createRuntimeClassPod(f, rcName)
 		expectSandboxFailureEvent(f, pod, fmt.Sprintf("\"%s\" not found", rcName))
 	})
+
+	ginkgo.It("should reject a Pod requesting a RuntimeClass with conflict node selector", func() {


The test/e2e/common directory is for tests that should be run as part of the regular cluster E2E suite, and also the node E2E suite. We don't actually run admission controllers or a scheduler in the node E2E suite, so these tests shouldn't be part of that suite.

Move these tests to test/e2e/node/runtimeclass.go, and add a comment explaining why these are note in common

draveness · 2019-08-30T03:06:24Z

/assign @oomichi @neolit123

Could you help to take a look at the change in the test/e2e/framework package?

neolit123 · 2019-08-30T10:59:12Z

test/e2e/framework/framework.go

+)
+
+// PreconfiguredRuntimeClassHandler returns configured runtime handler.
+func PreconfiguredRuntimeClassHandler() string {


i think we need a better location for this function instead of framework.go.
possibly /test/e2e/framework/node/runtime.go, @oomichi WDYT?

/hold

Either of test/e2e/framework/{node,pod}/runtime.go works for me.

it would also be really nice if we documented this somewhere, other than kube-up, clusters don't tend to have "test-handler" registered, so they're only passing whenw e run docker and ~ skip them ... (?)

I understand why it's like this, but it's a bit annoying to track this down and modify cluster deployments to support the test. I imagine most just don't have this setup and aren't aware of it.

I understand why it's like this, but it's a bit annoying to track this down and modify cluster deployments to support the test. I imagine most just don't have this setup and aren't aware of it.

Moving it to the test/e2e/framework/node/runtime.go would make the e2e node package depends on the framework. I checked out several other e2e packages which do not have this dependency.

I mean documenting somewhere that the cluster must have this magical test-handler configured

Makes sense. Since this PR is just moving the code that's already there & needs to get in to 1.16, can we follow up on that?

I think the better approach would be to make it a variable that get's filled in by the test setup, rather than branching on the runtime, just make the handler parameter explicit (and skip the test if it's missing). But again, I'd like to get this in to 1.16 as is, and follow up with that change.

I created an issue to track this #82213

draveness · 2019-08-30T18:40:18Z

/retest Regards, Draven

…

On Aug 30, 2019, 7:01 PM +0800, Lubomir I. Ivanov ***@***.***>, wrote: @neolit123 commented on this pull request. In test/e2e/framework/framework.go: > @@ -884,3 +884,18 @@ func GetLogToFileFunc(file *os.File) func(format string, args ...interface{}) { writer.Flush() } } + +const ( + // preconfiguredRuntimeHandler is the name of the runtime handler that is expected to be + // preconfigured in the test environment. + preconfiguredRuntimeHandler = "test-handler" +) + +// PreconfiguredRuntimeClassHandler returns configured runtime handler. +func PreconfiguredRuntimeClassHandler() string { i think we need a better location for this function instead of framework.go. possibly /test/e2e/framework/node/runtime.go, @oomichi WDYT? /hold — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

yastij · 2019-08-30T20:37:37Z

test/e2e/node/runtimeclass.go

+
+		framework.ExpectNoError(e2epod.WaitForPodSuccessInNamespace(f.ClientSet, pod.Name, f.Namespace.Name))
+		framework.ExpectEqual(nodeSelector, pod.Spec.NodeSelector)
+		framework.ExpectEqual(tolerations, pod.Spec.Tolerations)


tests fail due to this

Try:

gomega.Expect(pod.Spec.Tolerations).To(gomega.ContainElement(tolerations[0]))

yastij

/lgtm

cc @neolit123

neolit123

thanks for the update.
/lgtm
/approve

k8s-ci-robot · 2019-09-02T11:29:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: draveness, neolit123, tallclair

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/common/OWNERS~~ [tallclair]
~~test/e2e/framework/OWNERS~~ [neolit123]
~~test/e2e/node/OWNERS~~ [tallclair]

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

draveness · 2019-09-02T11:36:03Z

/hold cancel

k8s-ci-robot assigned tallclair and yastij Aug 26, 2019

draveness mentioned this pull request Aug 26, 2019

Umbrella issue for RuntimeClass Scheduling #81016

Closed

7 tasks

k8s-ci-robot requested review from dchen1107 and yujuhong August 26, 2019 00:12

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch from bee1483 to e6da6bc Compare August 26, 2019 00:23

draveness changed the title ~~test(runtimeclass): add e2e tests for runtimeclass scheduling~~ [RuntimeClassScheduling] test(runtimeclass): add e2e tests for runtimeclass scheduling - Part3 Aug 26, 2019

tallclair reviewed Aug 26, 2019

View reviewed changes

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch from e6da6bc to 5d3a8e2 Compare August 27, 2019 06:32

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch from 5d3a8e2 to 8f93edb Compare August 27, 2019 06:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2019

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch from 8f93edb to 78da0f2 Compare August 27, 2019 06:35

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Aug 27, 2019

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch from e56161a to 170a227 Compare August 27, 2019 06:58

k8s-ci-robot assigned lavalamp Aug 30, 2019

k8s-ci-robot assigned neolit123 and oomichi Aug 30, 2019

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch from 7ba7421 to 6132bef Compare August 30, 2019 03:09

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 30, 2019

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch 4 times, most recently from cada957 to 39ef57f Compare August 30, 2019 03:51

neolit123 reviewed Aug 30, 2019

View reviewed changes

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

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch from 39ef57f to d2cb6a7 Compare August 30, 2019 18:20

yastij reviewed Aug 30, 2019

View reviewed changes

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch 2 times, most recently from a296ed3 to ddc2e29 Compare September 1, 2019 06:54

draveness mentioned this pull request Sep 1, 2019

Test cluster must configure test-handler #82213

Closed

test(runtimeclass): add e2e tests for runtimeclass scheduling

702646d

draveness force-pushed the feature/add-e2e-tests-for-runtime-class-scheduling branch from ddc2e29 to 702646d Compare September 1, 2019 08:42

yastij reviewed Sep 2, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2019

neolit123 reviewed Sep 2, 2019

View reviewed changes

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

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 2, 2019

k8s-ci-robot merged commit d114f33 into kubernetes:master Sep 2, 2019

BenTheElder mentioned this pull request Oct 9, 2019

use 3 worker nodes when testing kubernetes-sigs/kind#937

Closed

bobbypage mentioned this pull request Sep 6, 2021

Skip runtimeclass test unless provider GCE #104803

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RuntimeClassScheduling] test(runtimeclass): add e2e tests for runtimeclass scheduling - Part3 #81915

[RuntimeClassScheduling] test(runtimeclass): add e2e tests for runtimeclass scheduling - Part3 #81915

draveness commented Aug 26, 2019

draveness commented Aug 26, 2019

tallclair commented Aug 26, 2019

draveness commented Aug 26, 2019 via email

tallclair left a comment

tallclair Aug 26, 2019

tallclair Aug 26, 2019

tallclair Aug 26, 2019

tallclair Aug 26, 2019

tallclair Aug 26, 2019

tallclair Aug 26, 2019

tallclair Aug 26, 2019

BenTheElder Oct 9, 2019

neolit123 Oct 9, 2019

BenTheElder Oct 9, 2019

tallclair Aug 26, 2019

draveness commented Aug 30, 2019

neolit123 Aug 30, 2019

tallclair Aug 30, 2019

BenTheElder Aug 30, 2019

BenTheElder Aug 30, 2019

draveness Aug 31, 2019

BenTheElder Aug 31, 2019

tallclair Aug 31, 2019

BenTheElder Aug 31, 2019

draveness Sep 1, 2019

draveness commented Aug 30, 2019 via email

yastij Aug 30, 2019

tallclair Aug 30, 2019

yastij left a comment

neolit123 left a comment

k8s-ci-robot commented Sep 2, 2019

draveness commented Sep 2, 2019

[RuntimeClassScheduling] test(runtimeclass): add e2e tests for runtimeclass scheduling - Part3 #81915

[RuntimeClassScheduling] test(runtimeclass): add e2e tests for runtimeclass scheduling - Part3 #81915

Conversation

draveness commented Aug 26, 2019

draveness commented Aug 26, 2019

tallclair commented Aug 26, 2019

draveness commented Aug 26, 2019 via email

tallclair 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

draveness commented Aug 30, 2019

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

draveness commented Aug 30, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yastij left a comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 2, 2019

draveness commented Sep 2, 2019