Handled taints on node in batch. #49524

k82cn · 2017-07-25T00:13:18Z

What this PR does / why we need it:
Enhanced helpers to handled taints on node in batch.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #49522

Release note:

None

k82cn · 2017-07-25T07:18:48Z

/retest

gmarek · 2017-07-25T07:43:48Z

pkg/controller/controller_utils.go

+			if err != nil {
+				return fmt.Errorf("Failed to update taint of node!")
+			}
+			if ok {


I miss languages where you can write updated |= ok :)

gmarek · 2017-07-25T07:47:39Z

pkg/controller/controller_utils.go

@@ -916,20 +934,11 @@ func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taint *v1.Ta
 // won't fail if target taint doesn't exist or has been removed.
 // If passed a node it'll check if there's anything to be done, if taint is not present it won't issue
 // any API calls.
-func RemoveTaintOffNode(c clientset.Interface, nodeName string, taint *v1.Taint, node *v1.Node) error {
-	// Short circuit for limiting amount of API calls.
-	if node != nil {


Can we keep this logic? It was added to prevent calling API server if there are no matching Taints.

k82cn · 2017-07-25T16:06:33Z

/retest

k82cn · 2017-07-26T01:39:56Z

/assign @gmarek

gmarek · 2017-07-31T08:44:13Z

/lgtm

k82cn · 2017-07-31T09:03:54Z

/assign @janetkuo

Would you help to review it?

k82cn · 2017-07-31T09:24:37Z

/test pull-kubernetes-bazel

gmarek · 2017-07-31T09:40:23Z

/test pull-kubernetes-e2e-gce-etcd3

k82cn · 2017-07-31T23:44:39Z

/retest

gmarek · 2017-08-01T07:54:53Z

/test pull-kubernetes-e2e-kops-aws

gmarek · 2017-08-03T15:33:53Z

/lgtm

gmarek · 2017-08-03T16:45:28Z

/retest

k82cn · 2017-08-04T01:16:35Z

ping for approve :). @kubernetes/sig-apps-pr-reviews

gmarek · 2017-08-04T06:59:29Z

pkg/controller/controller_utils.go

 	// Short circuit for limiting amount of API calls.
 	if node != nil {
 		match := false
+	MATCHED:


I don't like loop labels. Can you instead wrap for _, taint... into a annonymous function that returns 'match' and break if it returns true?

Also we should have some simple unit test for those functions...

hm... try to add unit test when creating this PR; it will import node/testutil for FakeNodeHandler, although it did not introduce Circular Dependency. Do you think we should move it node/testutil to controller/testutil which is also used by cloud controller, or just import it for test.

moving node/testutil to controller/testutil SG

handled moving in #50179 .

k82cn · 2017-08-05T06:11:45Z

pkg/controller/controller_utils_test.go

+				{Key: "key1", Value: "value1", Effect: "NoSchedule"},
+				{Key: "key2", Value: "value2", Effect: "NoExecute"},
+			},
+			requestCount: 3,


@gmarek , do you think we should also pass *v1.Node to reduce api call?

k82cn · 2017-08-07T03:21:38Z

/retest

gmarek · 2017-08-07T06:35:02Z

@k82cn - please fix gofmt.

k82cn · 2017-08-07T06:47:16Z

k82cn@ - please fix gofmt.

Done. Proxy was down those day; can not open the detail of failed test :(.

gmarek · 2017-08-07T09:57:25Z

/retest

gmarek · 2017-08-07T09:57:49Z

Please squash commits.

deads2k · 2017-08-07T14:10:47Z

/approve

gmarek · 2017-08-07T19:10:46Z

/lgtm

k8s-github-robot · 2017-08-07T19:11:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, gmarek, k82cn

Associated issue: 49522

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/controller/OWNERS~~ [deads2k]
~~test/OWNERS~~ [deads2k,gmarek]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-07T20:51:54Z

Automatic merge from submit-queue (batch tested with PRs 49524, 46760, 50206, 50166, 49603)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 25, 2017

k8s-github-robot assigned deads2k and aveshagarwal Jul 25, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 25, 2017

k82cn force-pushed the k8s_49522 branch from 050c03c to 62b235c Compare July 25, 2017 00:18

k82cn changed the title ~~[WIP] Handled taints on node in batch.~~ Handled taints on node in batch. Jul 25, 2017

k82cn force-pushed the k8s_49522 branch 2 times, most recently from 6fb233e to 2fa5df7 Compare July 25, 2017 05:08

k82cn mentioned this pull request Jul 25, 2017

Task 1: Tainted node by condition. #49257

Merged

gmarek reviewed Jul 25, 2017

View reviewed changes

k82cn force-pushed the k8s_49522 branch 2 times, most recently from 778c373 to 7ed2f29 Compare July 25, 2017 14:36

deads2k removed their assignment Jul 25, 2017

k8s-ci-robot assigned gmarek Jul 26, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2017

k8s-ci-robot assigned janetkuo Jul 31, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2017

k82cn force-pushed the k8s_49522 branch from 7ed2f29 to ad3d79d Compare August 3, 2017 15:27

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2017

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

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

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Aug 4, 2017

gmarek reviewed Aug 4, 2017

View reviewed changes

k82cn force-pushed the k8s_49522 branch from ad3d79d to abb1174 Compare August 5, 2017 06:05

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 5, 2017

k82cn commented Aug 5, 2017

View reviewed changes

Handled taints on node in batch.

18ae1ba

k82cn force-pushed the k8s_49522 branch from 407b712 to 18ae1ba Compare August 7, 2017 11:29

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2017

k8s-github-robot merged commit 35eb03e into kubernetes:master Aug 7, 2017

davidopp mentioned this pull request Sep 22, 2017

delete pods API call latencies shot up on large cluster tests #51899

Closed

k82cn deleted the k8s_49522 branch May 16, 2022 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handled taints on node in batch. #49524

Handled taints on node in batch. #49524

k82cn commented Jul 25, 2017

k82cn commented Jul 25, 2017

gmarek Jul 25, 2017

k82cn Jul 25, 2017

gmarek Jul 25, 2017

k82cn Jul 25, 2017

k82cn commented Jul 25, 2017

k82cn commented Jul 26, 2017

gmarek commented Jul 31, 2017

k82cn commented Jul 31, 2017

k82cn commented Jul 31, 2017

gmarek commented Jul 31, 2017

k82cn commented Jul 31, 2017

gmarek commented Aug 1, 2017

gmarek commented Aug 3, 2017

gmarek commented Aug 3, 2017

k82cn commented Aug 4, 2017

gmarek Aug 4, 2017

k82cn Aug 4, 2017

gmarek Aug 4, 2017

k82cn Aug 4, 2017

gmarek Aug 4, 2017

k82cn Aug 5, 2017 •

edited

k82cn Aug 5, 2017

k82cn commented Aug 7, 2017

gmarek commented Aug 7, 2017

k82cn commented Aug 7, 2017

gmarek commented Aug 7, 2017

gmarek commented Aug 7, 2017

deads2k commented Aug 7, 2017

gmarek commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

Handled taints on node in batch. #49524

Handled taints on node in batch. #49524

Conversation

k82cn commented Jul 25, 2017

k82cn commented Jul 25, 2017

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

k82cn commented Jul 25, 2017

k82cn commented Jul 26, 2017

gmarek commented Jul 31, 2017

k82cn commented Jul 31, 2017

k82cn commented Jul 31, 2017

gmarek commented Jul 31, 2017

k82cn commented Jul 31, 2017

gmarek commented Aug 1, 2017

gmarek commented Aug 3, 2017

gmarek commented Aug 3, 2017

k82cn commented Aug 4, 2017

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

k82cn Aug 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Aug 7, 2017

gmarek commented Aug 7, 2017

k82cn commented Aug 7, 2017

gmarek commented Aug 7, 2017

gmarek commented Aug 7, 2017

deads2k commented Aug 7, 2017

gmarek commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k82cn Aug 5, 2017 •

edited