Refactor scheduler factory test #69412

tossmilestone · 2018-10-04T03:34:55Z

What this PR does / why we need it:
Use k8s.io/client-go/kubernetes/fake.Clientset as the fake k8s client in pkg/scheduler/factory/factory_test.go.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #68963

Special notes for your reviewer:
@misterikkit

Release note:

Refactor factory_test.go to use a fake k8s client.

tossmilestone · 2018-10-04T03:38:31Z

/assign @misterikkit @bsalamat @liggitt

liggitt · 2018-10-04T15:37:52Z

staging/src/k8s.io/client-go/kubernetes/typed/core/v1/fake/fake_pod_expansion.go

@@ -26,6 +26,7 @@ import (
 func (c *FakePods) Bind(binding *v1.Binding) error {
 	action := core.CreateActionImpl{}
 	action.Verb = "create"
+	action.Namespace = binding.Namespace


is name needed as well?

@liggitt The action is create, so the name is not required in action and already in the binding object.

misterikkit

The overall change is fewer lines than I was anticipating! 😄

Anyway, I'd like us to avoid reactors when possible, as that is a pretty detailed interface. It lets us fake some pretty sophisticated interactions, but my hope is that most tests don't need that level of sophistication.

misterikkit · 2018-10-05T01:07:34Z

pkg/scheduler/factory/factory_test.go

-	mux := http.NewServeMux()
+	client := fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testPod}})
+	requestReceived := false
+	client.PrependReactor("get", string(v1.ResourcePods),


This seems like new behavior in the test. What problem does it solve, and is this test flaky? (I notice the 10ms sleep that's probably there for this reason.)

If we're asserting very specific interactions with the k8s API, then using reactors might be the way to go. I've found that it's often sufficient to inject objects and check state through the Clientset API itself. For example, https://github.com/kubernetes/client-go/tree/master/examples/fake-client

@misterikkit It's not a new behaviour, I just want to use reactor to validate that if the request is received or the request is expected, like what the original test does using ValidateRequest. I found no other method to do the validation in a simple way.

Ah, I see that now. What do you think about calling client.Actions() and doing checks on that slice, rather than using the reactor interface? I think that code would be a little simpler for others to understand.

@misterikkit client.Actions() would be a better way, thanks.

misterikkit · 2018-10-05T01:16:12Z

pkg/scheduler/factory/factory_test.go

+	client := fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testPod}})
+	expectedBody := runtime.EncodeOrDie(schedulertesting.Test.Codec(), binding)
+	requestReceived := false
+	client.PrependReactor("create", string(v1.ResourcePods), func(action clienttesting.Action) (bool, runtime.Object, error) {


Do you know what the default reactor does with bindings? Does it do anything that can be observed by going in through the Clientset interface?

@misterikkit The default reactor is to react to the action using a object tracker, and will store the object states. You're right, we could check the object using the Clientset interface here instead of the reactor.

@misterikkit Updated, now it's simpler, PTAL.

misterikkit · 2018-10-05T18:24:51Z

pkg/scheduler/factory/factory_test.go

+
+	bind, err := pod.GetBinding("foo")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)


Errorf + return could be replaced with Fatalf, but since it matches existing code, I don't care if it gets cleaned up in this PR.

Agree, will update soon.

misterikkit · 2018-10-05T18:27:01Z

pkg/scheduler/factory/factory_test.go

+
+	pod := client.CoreV1().Pods(metav1.NamespaceDefault).(*fakeV1.FakePods)
+
+	bind, err := pod.GetBinding("foo")


Just some side commentary, but the original design of this helper is assuming that the passed in binding object is on a pod named "foo". It makes the test fragile and harder to read. Not something to fix in this PR - I just had to point it out.

Emm... it's really easy to make some misunderstanding.

tossmilestone · 2018-10-06T02:23:44Z

@misterikkit Updated, PTAL.

k82cn · 2018-10-07T01:36:32Z

/ok-to-test
/kind cleanup

tossmilestone · 2018-10-07T06:58:04Z

/retest

misterikkit

/lgtm

misterikkit · 2018-10-08T23:03:12Z

pkg/scheduler/factory/factory_test.go

-		handler.ValidateRequest(t, schedulertesting.Test.ResourcePath(string(v1.ResourcePods), "bar", "foo"), "GET", nil)
+		requestReceived := false
+		actions := client.Actions()
+		for _, a := range actions {


Thanks for updating this. It is easier to read (to me) than the reactor stuff. I almost wonder if this test should assert len(actions)==1 since the test fails otherwise.

caesarxuchao · 2018-10-08T23:42:33Z

The client-go changes lgtm.

/approve

bsalamat

/lgtm

Thanks, @tossmilestone!

bsalamat · 2018-10-09T20:46:58Z

/approve

misterikkit · 2018-10-11T17:57:12Z

Looks like you need to do bazel update again.

apelisse · 2018-10-11T23:51:30Z

From a kubectl perspective:
/approve

k8s-ci-robot · 2018-10-11T23:51:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, bsalamat, caesarxuchao, tossmilestone

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/kubectl/OWNERS~~ [apelisse]
~~pkg/scheduler/OWNERS~~ [bsalamat]
~~staging/src/k8s.io/client-go/OWNERS~~ [caesarxuchao]

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

Use `k8s.io/client-go/kubernetes/fake.Clientset` as the fake k8s client. Signed-off-by: He Xiaoxi <xxhe@alauda.io>

misterikkit

/lgtm

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 4, 2018

k8s-ci-robot requested review from aveshagarwal and deads2k October 4, 2018 03:35

tossmilestone mentioned this pull request Oct 4, 2018

[scheduler cleanup phase 1]: Refactor factory_test.go to use a fake k8s client #68963

Closed

k8s-ci-robot assigned bsalamat, liggitt and misterikkit Oct 4, 2018

liggitt reviewed Oct 4, 2018

View reviewed changes

misterikkit reviewed Oct 5, 2018

View reviewed changes

tossmilestone force-pushed the scheduler-factory-test branch from 722ff92 to 68df88a Compare October 5, 2018 02:42

misterikkit reviewed Oct 5, 2018

View reviewed changes

tossmilestone force-pushed the scheduler-factory-test branch 3 times, most recently from 38aa570 to 26ecb74 Compare October 6, 2018 02:22

tossmilestone force-pushed the scheduler-factory-test branch 2 times, most recently from 789b518 to 5172ff6 Compare October 7, 2018 02:16

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Oct 7, 2018

misterikkit approved these changes Oct 8, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2018

bsalamat approved these changes Oct 9, 2018

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2018

Refactor scheduler factory test

a96a390

Use `k8s.io/client-go/kubernetes/fake.Clientset` as the fake k8s client. Signed-off-by: He Xiaoxi <xxhe@alauda.io>

tossmilestone force-pushed the scheduler-factory-test branch from e7edc3a to a96a390 Compare October 12, 2018 06:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2018

misterikkit approved these changes Oct 15, 2018

View reviewed changes

k8s-ci-robot merged commit 060218a into kubernetes:master Oct 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor scheduler factory test #69412

Refactor scheduler factory test #69412

tossmilestone commented Oct 4, 2018

tossmilestone commented Oct 4, 2018

liggitt Oct 4, 2018

tossmilestone Oct 4, 2018

misterikkit left a comment

misterikkit Oct 5, 2018

tossmilestone Oct 5, 2018 •

edited

misterikkit Oct 5, 2018

tossmilestone Oct 6, 2018

misterikkit Oct 5, 2018

tossmilestone Oct 5, 2018

tossmilestone Oct 5, 2018

misterikkit Oct 5, 2018

tossmilestone Oct 6, 2018

misterikkit Oct 5, 2018

tossmilestone Oct 6, 2018

tossmilestone commented Oct 6, 2018

k82cn commented Oct 7, 2018

tossmilestone commented Oct 7, 2018

misterikkit left a comment

misterikkit Oct 8, 2018

caesarxuchao commented Oct 8, 2018

bsalamat left a comment

bsalamat commented Oct 9, 2018

misterikkit commented Oct 11, 2018

apelisse commented Oct 11, 2018

k8s-ci-robot commented Oct 11, 2018

misterikkit left a comment


		pod := client.CoreV1().Pods(metav1.NamespaceDefault).(*fakeV1.FakePods)

		bind, err := pod.GetBinding("foo")

Refactor scheduler factory test #69412

Refactor scheduler factory test #69412

Conversation

tossmilestone commented Oct 4, 2018

tossmilestone commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterikkit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tossmilestone Oct 5, 2018 • 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

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

tossmilestone commented Oct 6, 2018

k82cn commented Oct 7, 2018

tossmilestone commented Oct 7, 2018

misterikkit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Oct 8, 2018

bsalamat left a comment

Choose a reason for hiding this comment

bsalamat commented Oct 9, 2018

misterikkit commented Oct 11, 2018

apelisse commented Oct 11, 2018

k8s-ci-robot commented Oct 11, 2018

misterikkit left a comment

Choose a reason for hiding this comment

tossmilestone Oct 5, 2018 •

edited