Use NodeWrapper to directly initialize nodes with labels #92514

nodo · 2020-06-25T15:44:24Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
In #92074 (comment), we discussed the possibility to use NodeWrapper in the scheduler integration tests so that we have more flexibility when creating the nodes. For instance we can create nodes directly with a label.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2020-06-25T15:44:32Z

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-25T15:48:20Z

@alculquicondor @Huang-Wei I have opened this PR as WIP to get your feedback about the cleanup.

In particular:

What do you think about keeping initNode and use it in the integration tests, so that we can easily initialize nodes with certain default?
I deliberately created createNodeFromWrapper to keep this PR small in scope, however, do you think we should change createNode to use NodeWrapper as well?
Also, if you agree I think we should also change createNodes to use createNodeFromWrapper, but I wanted to hear from you first.

Huang-Wei · 2020-06-25T18:03:52Z

What do you think about keeping initNode and use it in the integration tests, so that we can easily initialize nodes with certain default?

There are just 2 callers using initNode():

⇒  ag initNode test/integration/scheduler
test/integration/scheduler/util.go
151:// initNode returns a node with the given resource list and images. If 'res' is nil, a predefined amount of
153:func initNode(name string, res *v1.ResourceList, images []v1.ContainerImage) *v1.Node {
174:	return cs.CoreV1().Nodes().Create(context.TODO(), initNode(name, res, nil), metav1.CreateOptions{})
179:	return cs.CoreV1().Nodes().Create(context.TODO(), initNode(name, res, images), metav1.CreateOptions{})

I think it makes more sense to eliminate it by migrating to NodeWrapper.

I deliberately created createNodeFromWrapper to keep this PR small in scope, however, do you think we should change createNode to use NodeWrapper as well?

Also, if you agree I think we should also change createNodes to use createNodeFromWrapper, but I wanted to hear from you first.

My 2 cents is to use NodeWrapper inside createNode() & createNodes() to craft the Node(s) object first, and then call API server to create them.

Huang-Wei · 2020-06-25T18:13:34Z

pkg/scheduler/testing/wrappers.go

+	return n
+}
+
+func (n *NodeWrapper) Capacity(capacity v1.ResourceList) *NodeWrapper {


I'd suggest using paired key-value string (or just values) instead of v1.ResourceList here. v1.ResourceList is too low-level API, which is friendly for new-users to build, and if you look at the usage of createNode(), there is only one caller specifying a non-nil resourceList.

Option 1:

func (n *NodeWrapper) Capacity(pairs ...string) *NodeWrapper { res := v1.ResourceList{} for i := 0; i < len(pairs); i += 2 { k, v := pairs[i], pairs[i+1] res[v1.ResourceName(k)] = resource.MustParse(v) } n.Status.Capacity, n.Status.Allocatable = res, res return n }

Option 2:

func (n *NodeWrapper) Capacity(cpu, mem string, pods int64) *NodeWrapper { res := v1.ResourceList{ v1.ResourcePods: *resource.NewQuantity(pods, resource.DecimalSI), } if len(cpu) != 0 { res[v1.ResourceCPU] = resource.MustParse(cpu) } if len(mem) != 0 { res[v1.ResourceMemory] = resource.MustParse(mem) } n.Status.Capacity, n.Status.Allocatable = res, res return n }

I would prefer Option 1, but accepting map[string]string, instead of the pairs of lists

yup, option 1 is also my preference.

nit: map[v1.ResourceName]string should be less error-prone.

Good idea, I will make the change!

Huang-Wei · 2020-06-25T18:14:41Z

pkg/scheduler/testing/wrappers.go

+	return n
+}
+
+func (n *NodeWrapper) Images(images []v1.ContainerImage) *NodeWrapper {


Similar as above, this function should take the burden of building []v1.ContainerImage inside, so users just pass in image names, etc.

Huang-Wei · 2020-06-25T18:40:44Z

/ok-to-test

test/integration/scheduler/util.go

test/integration/scheduler/predicates_test.go

alculquicondor · 2020-06-25T19:22:22Z

pkg/scheduler/testing/wrappers.go

+	return n
+}
+
+func (n *NodeWrapper) Images(images []v1.ContainerImage) *NodeWrapper {


alculquicondor · 2020-06-25T19:23:11Z

pkg/scheduler/testing/wrappers.go

+	return n
+}
+
+func (n *NodeWrapper) Capacity(capacity v1.ResourceList) *NodeWrapper {


I would prefer Option 1, but accepting map[string]string, instead of the pairs of lists

nodo · 2020-06-29T15:01:31Z

@alculquicondor @Huang-Wei Thanks a lot for the feedback. I have done some changes and cleaned up the rest of the integration tests. Feel free to review it when you have time.

alculquicondor · 2020-06-29T15:06:42Z

pkg/scheduler/testing/wrappers.go

+// Capacity sets the capacity and the allocatable resources of the inner node.
+// Each entry in `resources` corresponds to a resource name and its quantity.
+func (n *NodeWrapper) Capacity(resources map[v1.ResourceName]string) *NodeWrapper {
+	res := v1.ResourceList{}


nit: res := make(v1.ResourceList)

alculquicondor · 2020-06-29T15:17:08Z

test/integration/scheduler/util.go

@@ -194,6 +167,10 @@ func createNodes(cs clientset.Interface, prefix string, res *v1.ResourceList, nu
 	return nodes[:], nil
 }

+func defaultNodeWrapper() *st.NodeWrapper {


I prefer we don't hide the details in this function.
Each caller should define the resources they need in their tests.
I know this goes against having repeated code, but this is actually a good practice when writing tests.

IMO v1.ResourcePods is a bit special, unlike zero cpu/mem, a zero Pods number would simply fail a NodeFit plugin in any case. Which is usually not what we want. So having a default v1.ResourcePods looks good to me. However, I'd prefer to set the default value on NodesWrapper side - which would also benefit UT (I did that way in another PR: https://github.com/kubernetes/kubernetes/pull/92571/files?file-filters%5B%5D=.go#diff-1cea28cd0be3cdbab57f5dc287dc98c0R427-R438)

If you do so, this function is not needed anymore - you can just replace callers of defaultNodeWrapper() directly to st.MakeNode().

Sg, we can just do res[pods] = 32 at the beginning of the function, and the loop would override it.

Yup, that would save one if block.

Sounds good, I have made the change.

Huang-Wei

Just some nits.

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

pkg/scheduler/testing/wrappers.go

+	for name, value := range resources {
+		res[name] = resource.MustParse(value)
+	}
+	n.Status.Capacity, n.Status.Allocatable = res, res


would be good to ensure v1.ResourcePods is always present, otherwise some pod scheduling may fail due to "no Pods quota".

FYI: I put it in https://github.com/kubernetes/kubernetes/pull/92571/files?file-filters%5B%5D=.go#diff-1cea28cd0be3cdbab57f5dc287dc98c0R427-R438

Huang-Wei · 2020-06-29T19:17:15Z

pkg/scheduler/testing/wrappers.go

+// Images sets the images of the inner node. Each entry in `images` corresponds
+// to an image name and its size in bytes.
+func (n *NodeWrapper) Images(images map[string]int64) *NodeWrapper {
+	containerImages := []v1.ContainerImage{}


nit: var containerImages []v1.ContainerImage.

Huang-Wei · 2020-06-29T19:30:31Z

test/integration/scheduler/util.go

@@ -194,6 +167,10 @@ func createNodes(cs clientset.Interface, prefix string, res *v1.ResourceList, nu
 	return nodes[:], nil
 }

+func defaultNodeWrapper() *st.NodeWrapper {


IMO v1.ResourcePods is a bit special, unlike zero cpu/mem, a zero Pods number would simply fail a NodeFit plugin in any case. Which is usually not what we want. So having a default v1.ResourcePods looks good to me. However, I'd prefer to set the default value on NodesWrapper side - which would also benefit UT (I did that way in another PR: https://github.com/kubernetes/kubernetes/pull/92571/files?file-filters%5B%5D=.go#diff-1cea28cd0be3cdbab57f5dc287dc98c0R427-R438)

If you do so, this function is not needed anymore - you can just replace callers of defaultNodeWrapper() directly to st.MakeNode().

alculquicondor · 2020-06-30T15:02:52Z

pkg/scheduler/testing/wrappers.go

@@ -390,3 +391,27 @@ func (n *NodeWrapper) Label(k, v string) *NodeWrapper {
 	n.Labels[k] = v
 	return n
 }
+
+// Capacity sets the capacity and the allocatable resources of the inner node.
+// Each entry in `resources` corresponds to a resource name and its quantity.


Indicate that sets 32 pods limit by default

alculquicondor · 2020-06-30T15:05:29Z

test/integration/scheduler/framework_test.go

@@ -1841,7 +1842,7 @@ func initTestSchedulerForFrameworkTest(t *testing.T, testCtx *testutils.TestCont
 	go testCtx.Scheduler.Run(testCtx.Ctx)

 	if nodeCount > 0 {
-		_, err := createNodes(testCtx.ClientSet, "test-node", nil, nodeCount)
+		_, err := createNodes(testCtx.ClientSet, "test-node", st.MakeNode(), nodeCount)


Shouldn't MakeNode call Capacity(nil) or something like that?

Totally! My bad, let me fix it.

Using NodeWrapper in the integration tests gives more flexibility when creating nodes. For instance, tests can create nodes with labels or with a specific sets of resources. Also, NodeWrapper initialises a node with a capacity of 32 pods, which can be overridden by the caller. This makes sure that a node is usable as soon as it is created.

alculquicondor

/lgtm

Thanks for the cleanup!

k8s-ci-robot · 2020-06-30T16:12:25Z

@nodo: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kind-ipv6	`2e1042f`	link	`/test pull-kubernetes-e2e-kind-ipv6`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Huang-Wei · 2020-06-30T16:51:50Z

/approve
/retest

Thanks @nodo .

k8s-ci-robot · 2020-06-30T16:52:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, 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:

~~pkg/scheduler/OWNERS~~ [Huang-Wei]
~~test/integration/scheduler/OWNERS~~ [Huang-Wei]

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 added area/test sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. 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 25, 2020

k8s-ci-robot requested review from liu-cong and resouer June 25, 2020 15:47

Huang-Wei reviewed Jun 25, 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 25, 2020

alculquicondor reviewed Jun 25, 2020

View reviewed changes

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 Jun 28, 2020

nodo changed the title ~~WIP: Use NodeWrapper to directly initialize node with labels~~ Use NodeWrapper to directly initialize nodes with labels Jun 29, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2020

alculquicondor reviewed Jun 29, 2020

View reviewed changes

Huang-Wei reviewed Jun 29, 2020

View reviewed changes

nodo force-pushed the scheduler-integration-test-cleanup branch from df00fed to b9a36c3 Compare June 30, 2020 14:56

alculquicondor reviewed Jun 30, 2020

View reviewed changes

nodo force-pushed the scheduler-integration-test-cleanup branch from b9a36c3 to 2e1042f Compare June 30, 2020 15:18

alculquicondor reviewed Jun 30, 2020

View reviewed changes

k8s-ci-robot assigned alculquicondor Jun 30, 2020

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

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

k8s-ci-robot merged commit ff1d6b3 into kubernetes:master Jun 30, 2020

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

nodo deleted the scheduler-integration-test-cleanup branch July 1, 2020 06:43

nodo mentioned this pull request Jul 1, 2020

Create nodes with labels instead of adding them later #92706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use NodeWrapper to directly initialize nodes with labels #92514

Use NodeWrapper to directly initialize nodes with labels #92514

nodo commented Jun 25, 2020

k8s-ci-robot commented Jun 25, 2020

nodo commented Jun 25, 2020 •

edited

Huang-Wei commented Jun 25, 2020

Huang-Wei Jun 25, 2020

alculquicondor Jun 25, 2020

Huang-Wei Jun 25, 2020

Huang-Wei Jun 25, 2020

nodo Jun 28, 2020

Huang-Wei Jun 25, 2020

alculquicondor Jun 25, 2020

Huang-Wei commented Jun 25, 2020

alculquicondor Jun 25, 2020

alculquicondor Jun 25, 2020

nodo commented Jun 29, 2020

alculquicondor Jun 29, 2020

alculquicondor Jun 29, 2020

Huang-Wei Jun 29, 2020 •

edited

alculquicondor Jun 29, 2020

Huang-Wei Jun 29, 2020

nodo Jun 30, 2020

Huang-Wei left a comment

Huang-Wei Jun 29, 2020 •

edited

Huang-Wei Jun 29, 2020

Huang-Wei Jun 29, 2020 •

edited

alculquicondor Jun 30, 2020

alculquicondor Jun 30, 2020

nodo Jun 30, 2020

alculquicondor left a comment

k8s-ci-robot commented Jun 30, 2020 •

edited

Huang-Wei commented Jun 30, 2020

k8s-ci-robot commented Jun 30, 2020

Use NodeWrapper to directly initialize nodes with labels #92514

Use NodeWrapper to directly initialize nodes with labels #92514

Conversation

nodo commented Jun 25, 2020

k8s-ci-robot commented Jun 25, 2020

nodo commented Jun 25, 2020 • edited

Huang-Wei commented Jun 25, 2020

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

Huang-Wei commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodo commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei Jun 29, 2020 • edited

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

Huang-Wei left a comment

Choose a reason for hiding this comment

Huang-Wei Jun 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei Jun 29, 2020 • edited

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

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 30, 2020 • edited

Huang-Wei commented Jun 30, 2020

k8s-ci-robot commented Jun 30, 2020

nodo commented Jun 25, 2020 •

edited

Huang-Wei Jun 29, 2020 •

edited

Huang-Wei Jun 29, 2020 •

edited

Huang-Wei Jun 29, 2020 •

edited

k8s-ci-robot commented Jun 30, 2020 •

edited