Kubectl taint node based on label selector #44740

ravisantoshgudimetla · 2017-04-20T20:33:56Z

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

Taints the node based on label selector

k8s-reviewable · 2017-04-20T20:34:02Z

This change is

k8s-ci-robot · 2017-04-20T20:34:04Z

Hi @ravisantoshgudimetla. 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 @k8s-bot 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.

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.

jayunit100 · 2017-04-20T20:57:35Z

Thanks @ravisantoshgudimetla this works for me :)

cluster/kubectl.sh label nodes --all "myLabel=x" ; cluster/kubectl.sh taint nodes --selector='myLabel==x' dedicated=foo:PreferNoSchedule ;
node "127.0.0.1" labeled
node "127.0.0.1" tainted

Do we need the unit tests in order to merge this ? I'd prefer to push it forward since the unit tests (we think) don't seem to support combining kubectl labelling with tainting (although maybe there is way to do so, its just not obvious).

ravisantoshgudimetla · 2017-04-20T21:05:33Z

/cc @jayunit100.

What is lacking?
Writing a uni test seems problematic, as the fake client is not able to build the information from labels.
What should we do?
So, the ideal way would be to have arguments to RunTaint(factory etc), fill in the values in test suite and fill in TaintOptions{} struct and build the information in test case. As this whole file needs some re-ordering(as discussed in Refactoring reorganize taints function in kubectl to expose operations #43171) the test-cases should be based on https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/taint.go#L362, if the node is tainted or not. The current approach seems to be checking from API object of node.(which should be in other file like TaintController etc).

ravisantoshgudimetla · 2017-04-20T21:39:55Z

cc @aveshagarwal

jayunit100 · 2017-04-20T21:56:19Z

@k8s-bot test this please

jayunit100 · 2017-04-20T22:52:20Z

cc @nimeshksingh

jayunit100 · 2017-04-20T22:54:48Z

pkg/kubectl/cmd/taint.go

 	if o.all {
 		o.builder = o.builder.SelectAllParam(o.all).ResourceTypes("node")
 	} else {
-		if len(o.resources) < 2 {


@aveshagarwal ok to get rid of this check?

It seems No. if both "selector" and "all" are NOT provided, we should do this check to verify the sequence of <resource> <name>.

@aveshagarwal https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/taint.go#L236 isn't sufficient ?

@aveshagarwal So, in the below command, I have a resource node without selector and all options. Its throwing a valid error.
cluster/kubectl.sh taint node 127.0.0.1
error: at least one taint update is required

I meant this errneous case:
kubectl taint node key=value:Noschedule

IOW, if you dont specify node name, but just the resource type node.

Ok, thanks for the information @aveshagarwal. So, we already have a check at API server to handle it. There is no need to validate it here again.

cluster/kubectl.sh taint node dedicated=foo:PreferNoSchedule
error: You must provide one or more resources by argument or filename.
Example resource specifications include:
'-f rsrc.yaml'
'--filename=rsrc.json'
'pods my-pod'
'services'

This is part of command line validation, and should be done in kubectl too.

So, if that is the case, it should be something like if(o.selector)!=="" and o.all=false then we should throw this error. I can make that change.

Sound good. And as you are at this, make sure one of selector and all is present not both simultaneously if not already.

supereagle · 2017-04-21T13:08:42Z

pkg/kubectl/cmd/taint.go

+		kubectl taint nodes foo dedicated-
+
+		# Add from node 'foo' all the taints with key 'dedicated'
+		kubectl taint nodes foo dedicated-


The commands for Add and Remove examples are the same. Any error?

It seems incorrect, the format with hyphen in the end ("dedicated-") is only for deletion. It does not make sense with addition.

Right. Its a mistake, Thanks for catching it @supereagle & @aveshagarwal , I will change this.

0xmichalis · 2017-04-21T14:25:14Z

@kubernetes/sig-cli-pr-reviews ptal

ravisantoshgudimetla · 2017-04-21T19:16:41Z

@aveshagarwal PTAL.

aveshagarwal · 2017-04-21T19:23:31Z

@ravisantoshgudimetla lgtm, now need to figure out the issue with your unit test.

jayunit100 · 2017-04-24T15:49:33Z

@kubernetes/sig-cli-pr-reviews Can we merge this, it fixes a pretty important label + kubectl bug ? @ravisantoshgudimetla can you create a follow on issue for adding a unit test.

derekwaynecarr · 2017-04-24T22:34:09Z

the code looks fine to me, for a unit test, we appear to have testing for label selectors in delete_test.go that may have a convention you could follow in TestDeleteMultipleSelector. if there is no urgency, would prefer a test case with the code in one pr.

ravisantoshgudimetla · 2017-04-25T13:31:33Z

@derekwaynecarr Thanks for the pointer regarding test case. I will check that and get back in case I am stuck.

ravisantoshgudimetla · 2017-04-25T16:23:22Z

@derekwaynecarr I tried running the cmd using the way in deleted_test.go and it is causing the same error. The test case that is failing is https://gist.github.com/ravisantoshgudimetla/d66fb36b1279f118395e540cc8c61505#file-taint_test-go-L106.

As updated earlier, It seems the issue is with object being built using builder. The ones coming from unit tests are not having name created at https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/taint.go#L320. So, wondering if there is a way to work around that to write a unit test. Initially I was thinking of mopdifying RunTaint() to take some arguments(especially for builders). Please lmk, if you have other thoughts or if I missed anything.

jayunit100 · 2017-04-25T20:43:50Z

@derekwaynecarr @ravisantoshgudimetla ok so sounds like to move forward, we have to do two things...

Lets create a follow on issue delineating the lack of the builder mocks needed for better higher level tests, and aim to improve the general situation for kubectl tests in general, so that in the future we can more declaratively test the subtleties of taints w/ different builder+label+... situations and so on.
per @derekwaynecarr suggestion , we can make a unit test for this patch by making a separate function for object creation.

(derek let us know if this sounds good to you as well, i think that will solve everything brought up in this issue, including getting this much needed fix merged :))

jayunit100 · 2017-04-27T20:15:30Z

pkg/kubectl/cmd/taint_test.go

@@ -332,3 +334,50 @@ func TestTaint(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateFlags(t *testing.T) {


yeah this is way cleaner, thanks ravi ,test looks good modulo description fixes

jayunit100 · 2017-04-27T20:17:38Z

pkg/kubectl/cmd/taint_test.go

+		},
+		{
+			taintOpts:   TaintOptions{selector: "", all: false, resources: []string{"node"}},
+			description: "With both selector and all flags and if node name is not provided",


jayunit100 · 2017-04-27T20:21:14Z

the fix seems to work
now I think the test is sufficient to prevent regressions now

so i guess lets move this forward and follow up on #44953 to mock these out properly for better kubectl tests........

PTAL @derekwaynecarr @aveshagarwal

aveshagarwal · 2017-04-27T20:40:02Z

lgtm

ravisantoshgudimetla · 2017-04-28T01:02:55Z

@jayunit100 - PTAL, I have addressed your comments on descriptions.

jayunit100 · 2017-04-28T13:52:22Z

/lgtm

ravisantoshgudimetla · 2017-04-28T14:29:37Z

@jayunit100 I did a gofmt again. Can you please ask the bot test?

ravisantoshgudimetla · 2017-04-28T14:55:54Z

and the unit tests seem to be a flake..

jayunit100 · 2017-05-02T14:49:36Z

@k8s-bot ok to test

ravisantoshgudimetla · 2017-05-02T16:13:04Z

#45170 - Flake..

jayunit100 · 2017-05-03T16:18:16Z

@k8s-bot unit test this please

jayunit100 · 2017-05-03T16:19:17Z

/lgtm

jayunit100 · 2017-05-05T12:42:34Z

@kubernetes/sig-cli-maintainers can we get an approve on this ? Thanks :)

pwittrock · 2017-05-05T15:37:12Z

/approve

k8s-github-robot · 2017-05-05T15:37:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jayunit100, pwittrock, ravisantoshgudimetla

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

~~pkg/kubectl/OWNERS~~ [pwittrock]

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-05-05T16:25:01Z

Automatic merge from submit-queue

k8s-github-robot · 2017-05-05T16:25:07Z

@ravisantoshgudimetla PR needs rebase

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

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 Apr 20, 2017

k8s-github-robot assigned derekwaynecarr Apr 20, 2017

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 20, 2017

ravisantoshgudimetla force-pushed the kubectl_taints_label_selector#44522 branch from 75a5941 to 3a60861 Compare April 20, 2017 21:27

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 20, 2017

jayunit100 reviewed Apr 20, 2017

View reviewed changes

supereagle reviewed Apr 21, 2017

View reviewed changes

ravisantoshgudimetla force-pushed the kubectl_taints_label_selector#44522 branch 3 times, most recently from ec3ce3c to 4017ccc Compare April 21, 2017 19:16

jayunit100 reviewed Apr 27, 2017

View reviewed changes

ravisantoshgudimetla force-pushed the kubectl_taints_label_selector#44522 branch from 44b22fb to c52b4b0 Compare April 28, 2017 01:02

k8s-ci-robot assigned jayunit100 Apr 28, 2017

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

Changes to kubectl taint to respect selector flag

081ba02

ravisantoshgudimetla force-pushed the kubectl_taints_label_selector#44522 branch from c52b4b0 to 081ba02 Compare April 28, 2017 14:28

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

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 2, 2017

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

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

k8s-github-robot merged commit cc1f9f7 into kubernetes:master May 5, 2017

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

ravisantoshgudimetla deleted the kubectl_taints_label_selector#44522 branch June 19, 2017 15:22

ravisantoshgudimetla mentioned this pull request Jul 2, 2017

Support node selectors for node, drain, taint and apply to new nodes #46231

Closed

Kubectl taint node based on label selector #44740

Kubectl taint node based on label selector #44740

Conversation

ravisantoshgudimetla commented Apr 20, 2017 • edited Loading

k8s-reviewable commented Apr 20, 2017

k8s-ci-robot commented Apr 20, 2017

jayunit100 commented Apr 20, 2017

ravisantoshgudimetla commented Apr 20, 2017

ravisantoshgudimetla commented Apr 20, 2017

jayunit100 commented Apr 20, 2017

jayunit100 commented Apr 20, 2017

Choose a reason for hiding this comment

aveshagarwal Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aveshagarwal Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla Apr 21, 2017 • edited Loading

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

0xmichalis commented Apr 21, 2017

ravisantoshgudimetla commented Apr 21, 2017

aveshagarwal commented Apr 21, 2017

jayunit100 commented Apr 24, 2017

derekwaynecarr commented Apr 24, 2017

ravisantoshgudimetla commented Apr 25, 2017

ravisantoshgudimetla commented Apr 25, 2017

jayunit100 commented Apr 25, 2017

jayunit100 Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayunit100 commented Apr 27, 2017 • edited Loading

aveshagarwal commented Apr 27, 2017

ravisantoshgudimetla commented Apr 28, 2017

jayunit100 commented Apr 28, 2017

ravisantoshgudimetla commented Apr 28, 2017

ravisantoshgudimetla commented Apr 28, 2017

jayunit100 commented May 2, 2017

ravisantoshgudimetla commented May 2, 2017

jayunit100 commented May 3, 2017

jayunit100 commented May 3, 2017

jayunit100 commented May 5, 2017

pwittrock commented May 5, 2017

k8s-github-robot commented May 5, 2017

k8s-github-robot commented May 5, 2017

k8s-github-robot commented May 5, 2017

ravisantoshgudimetla commented Apr 20, 2017 •

edited

Loading

aveshagarwal Apr 21, 2017 •

edited

Loading

aveshagarwal Apr 21, 2017 •

edited

Loading

ravisantoshgudimetla Apr 21, 2017 •

edited

Loading

jayunit100 Apr 27, 2017 •

edited

Loading

jayunit100 commented Apr 27, 2017 •

edited

Loading