Refactoring reorganize taints function in kubectl to expose operations #43171

ravisantoshgudimetla · 2017-03-15T22:01:59Z

What this PR does / why we need it:
This adds some UX functionality when specifying taints using kubectl.
For example:

./kubectl.sh taint nodes XYZ dedicated1=abca2:NoSchedule 
node "XYZ" tainted
 ./kubectl.sh taint nodes XYZ dedicated1=abca1:NoSchedule --overwrite=True
node "XYZ overwritten
./kubectl.sh taint nodes XYZ dedicated1-
node "XYZ" untainted
./kubectl.sh taint nodes XYZ dedicated=abca1:NoSchedule dedicated1-
node "XYZ" modified

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

Release note:

Fixed the output of kubectl taint node command with minor improvements.

k8s-ci-robot · 2017-03-15T22:02:06Z

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.

ravisantoshgudimetla · 2017-03-15T22:03:14Z

/cc @jayunit100.

k8s-reviewable · 2017-03-15T22:03:17Z

This change is

ravisantoshgudimetla · 2017-03-15T22:20:04Z

@smarterclayton PTAL.

ethernetdan · 2017-03-15T22:20:15Z

@k8s-bot ok to test

ravisantoshgudimetla · 2017-03-16T12:23:53Z

@fabianofranz - As I not able to mention sig-cli and sig-cli-pr-reviewers.

fabianofranz · 2017-03-16T18:57:06Z

@kubernetes/sig-cli-pr-reviews

ravisantoshgudimetla · 2017-03-24T16:47:53Z

@fabianofranz I just wanted to make sure if this PR can be reviewed as 1.6 code freeze has officially ended.
P.S:, Should I be a member to mention sig-cli-pr reviewers?

jayunit100 · 2017-03-24T18:40:15Z

pkg/kubectl/cmd/taint.go

@@ -366,23 +382,20 @@ func validateNoTaintOverwrites(obj runtime.Object, taints []v1.Taint) error {
 }

 // updateTaints updates taints of obj


can we improve this comment as part of this PR? the fact that its validating, and reorganizing, ... not sure what 'update' really is doing

I am just reorganizing in this PR. I will create one more with some other modifications.

Please update the comment here. There's no point of having separate PR for that - we want to keep review load as low as possible.

Makes sense...

I still don't see fixed comment...

@ravisantoshgudimetla updateTaints applies taints to a node in a clusters and returns a human understandable description of the resulting operation.

jayunit100 · 2017-03-24T18:41:07Z

pkg/kubectl/cmd/taint.go

+	return allErrs, taintsBefore != len(*newTaints)
+}
+
+// AddTaints adds the newTaints list to existing ones and updates the newTaints List.


s/AddTaints/addTaints for godoc . surprised this isnt caught by golint actually...

jayunit100 · 2017-03-24T18:47:24Z

pkg/kubectl/cmd/taint.go

 	allErrs := []error{}
+	taintsBefore := len(*newTaints)
 	for _, taintToRemove := range taintsToRemove {
 		removed := false
 		if len(taintToRemove.Effect) > 0 {


Do you think we can make this taintToRemove.Effect != "" ? Otherwise seems to imply that there can be multiple effects....

When can do that but the problem is Effect here is a type again. So this is the convention that others have been using in other locations as well.

can you clarify Effect here is type again ? If != works it seems like a better comparison.

jayunit100 · 2017-03-24T18:50:04Z

pkg/kubectl/cmd/taint.go

 		}
 		if !removed {
 			allErrs = append(allErrs, fmt.Errorf("taint %q not found", taintToRemove.ToString()))
 		}
 	}
-	return newTaints, utilerrors.NewAggregate(allErrs)
+	return allErrs, taintsBefore != len(*newTaints)


is this redundant to saying return allErrs, len(allErrs) > 0? if so i'd say just return []error as the function header , rather then []error, bool.

So, here the errors are different and taints are different, we are returning errors as well as checking if the original length of taints list is equal to old length of taints to check if some of them got deleted.

Isn't the latter equivalent to removed, i.e. it's always true if we reached this part of the code?

I guess not, if we do not enter the loop in first place, we cannot return true and 'removed' variable is declared inside loop. So, if we have removed outside loop, then I guess we can.

Fair point. I think such change would make the code cleaner.

jayunit100 · 2017-03-30T16:23:10Z

lgtm but im not an owner for this section

aveshagarwal · 2017-03-30T17:04:36Z

This adds some UX functionality when specifying taints using kubectl.
For example:
./kubectl.sh taint nodes XYZ dedicated1=abca2:NoSchedule
node "XYZ" tainted
./kubectl.sh taint nodes XYZ dedicated1=abca1:NoSchedule --overwrite=True
node "XYZ overwritten
./kubectl.sh taint nodes XYZ dedicated1-
node "XYZ" untainted

The above one seem a bit confusing as there might still be other taints left.

./kubectl.sh taint nodes XYZ dedicated=abca1:NoSchedule dedicated1-
node "XYZ" modified

In general, I am wondering how these UI changes are going to be helpful? May be I am missing something?

Also there are several possibilities I am not sure these UI changes cover all of them:

On one or multiple nodes:

Addition of new taints (one or multiple)
Deletion of existing taints (one or multiple)
Modification/Updates/Overwritten of existing taints (one or multiple)
And a combination of above operations.

ravisantoshgudimetla · 2017-03-30T17:36:25Z

So the composite operations(which are complementary like adding taints and deleting them) in a single command will always result in "modified" output, if the composite operations are non-complementary(in this case addition of multiple taints or removal of them) will always result in either "tainted" or "untainted".

If you have atleast one overwritten operation, the output will be "overwritten".

This is something that I came up with but if you have other suggestions, I am more than happy to include.

jayunit100 · 2017-03-30T18:11:58Z

@aveshagarwal do you agree that we should provide more feedback ? or you think just saying 'tainted' is enough.

I see your point that some of this is not super informative,

But

i think 'tainted','modified','untainted', etc... is better then just returning tainted every time.

So

If the goal is to just return tainted every time , then i guess we can close this PR.
If the goal is to return more information, lets decide in the parent issue what the expected output should be kubectl reporting wrong for taints. #43167 , and then ravi can reimplement it here.

davidopp · 2017-03-30T21:36:56Z

@kubernetes/sig-scheduling-pr-reviews

timothysc · 2017-04-03T13:25:09Z

pkg/kubectl/cmd/taint.go

@@ -40,6 +40,13 @@ import (
 	utiltaints "k8s.io/kubernetes/pkg/util/taints"
 )

+const (
+	MODIFIED    = "modified"


These should not be here.
They should either be in a scheduling library or near the api itself.

To decide where the right place is, we should decide: what is this describing .... I guess maybe we could satisfy tim's suggestion by making this more localized.

localize the definitions If this is describing a kubelet result, then I guess putting them here and making them more descriptive is ok, i.e

MODIFICATION="taint was modified" UNTAINTED="taint was removed"

... and so on, we can isolate the semantics unambiguously to kubectl so that this doesn't blur the lines between API constants and UX constants.

In addition, we should unexport these (i.e. s/MODIFIED/modified).

I think (1) + (2) might make this less 'sprawly'

@timothysc These are the constants that are being used in output of kubectl, doesn't it make sense to have them here instead of API?
@jayunit100 This gets appended to 'node 'XYZ' from printSuccess at https://github.com/kubernetes/kubernetes/pull/43171/files#diff-92dd749c11309adffd4b2b2112b36303R364. I guess a better way would be to remove 'node 'XYZ' ' part when printing and tell if the taint was added or deleted. I guess that will be a different PR but if you want it, I can include it here.

I think we can all agree these constants shouldnt be exported , if they are going to be specific to kubectl result processing, then i think thats ok. as of now they are higher level then that , which is why tim has mentioned they shouldn't be here.

Wherever we decide to put it, I think we should just use standard verbs: Added, Removed, Modified.

ravisantoshgudimetla · 2017-04-17T03:26:18Z

@k82cn Apologies for the confusion. I was trying to access files from another machine and messed up my git history. There is no generated code in my case as I am touching a single file :).

k8s-ci-robot · 2017-04-17T13:31:23Z

@ravisantoshgudimetla: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-cross	e8966112ab50976952a08761b90ec71815cd2119	link	`@k8s-bot pull-kubernetes-cross test this`

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.

jayunit100 · 2017-04-17T16:01:29Z

pkg/kubectl/cmd/taint.go

@@ -366,24 +379,21 @@ func validateNoTaintOverwrites(obj runtime.Object, taints []v1.Taint) error {
 	return utilerrors.NewAggregate(allErrs)
 }

-// updateTaints updates taints of obj
-func (o TaintOptions) updateTaints(obj runtime.Object) error {
+// updateTaints applies a taint option(o) to a node in cluster after validating conditions like overwriting of option and returns a human understandable description of the said option result.


+// updateTaints applies a taint option (o) to a node in the cluster, and after computing the net effect of the operation (i.e. does it result in an overwrite?), it reports the end result back in a way that a user can easily interpret.

jayunit100 · 2017-04-17T18:30:11Z

@gmarek @timothysc @adohe PTAL , LGTM on my end.

jayunit100 · 2017-04-19T18:41:20Z

bump :)

jayunit100 · 2017-04-20T10:30:53Z

/lgtm

k8s-github-robot · 2017-04-20T10:30:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jayunit100, ravisantoshgudimetla
We suggest the following additional approver: @fabianofranz

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

pkg/kubectl/OWNERS

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

jayunit100 · 2017-04-20T10:32:06Z

Ping @sig-cli-maintainers this one is ready to merge now, reached consensus.

k8s-github-robot · 2017-04-20T11:17:17Z

Automatic merge from submit-queue

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

k8s-github-robot assigned 0xmichalis Mar 15, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Mar 15, 2017

ravisantoshgudimetla changed the title ~~Refactoring reorganize taints function to expose operations~~ Refactoring reorganize taints function in kubectl to expose operations Mar 15, 2017

0xmichalis assigned fabianofranz and unassigned 0xmichalis Mar 16, 2017

jayunit100 reviewed Mar 24, 2017

View reviewed changes

aveshagarwal mentioned this pull request Mar 30, 2017

kubectl reporting wrong for taints. #43167

Closed

timothysc added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Apr 3, 2017

timothysc added this to the v1.7 milestone Apr 3, 2017

timothysc added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 3, 2017

timothysc reviewed Apr 3, 2017

View reviewed changes

ravisantoshgudimetla force-pushed the kubectl_taints_ux branch from e896611 to cd4dba8 Compare April 17, 2017 02:41

ravisantoshgudimetla force-pushed the kubectl_taints_ux branch from 56f7c17 to 2aeaaaa Compare April 17, 2017 03:20

ravisantoshgudimetla force-pushed the kubectl_taints_ux branch from 2aeaaaa to 0a4bc34 Compare April 17, 2017 13:01

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

ravisantoshgudimetla force-pushed the kubectl_taints_ux branch from 0a4bc34 to 941a778 Compare April 17, 2017 13:20

ravisantoshgudimetla force-pushed the kubectl_taints_ux branch from 941a778 to 148f109 Compare April 17, 2017 13:57

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

jayunit100 reviewed Apr 17, 2017

View reviewed changes

Small changes suggested by reviewers for comments and rebase

5bdf79b

ravisantoshgudimetla force-pushed the kubectl_taints_ux branch from 148f109 to 5bdf79b Compare April 17, 2017 16:57

gmarek approved these changes Apr 20, 2017

View reviewed changes

k8s-ci-robot assigned jayunit100 Apr 20, 2017

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

k8s-github-robot merged commit 2c6fbc9 into kubernetes:master Apr 20, 2017

ravisantoshgudimetla mentioned this pull request Apr 20, 2017

Kubectl taint node based on label selector #44740

Merged

ravisantoshgudimetla mentioned this pull request Apr 28, 2017

Refactoring taints logic to reduce sprawl across components. #45060

Closed

ravisantoshgudimetla deleted the kubectl_taints_ux branch May 23, 2017 20:47

		@@ -366,23 +382,20 @@ func validateNoTaintOverwrites(obj runtime.Object, taints []v1.Taint) error {
		}

		// updateTaints updates taints of obj

Refactoring reorganize taints function in kubectl to expose operations #43171

Refactoring reorganize taints function in kubectl to expose operations #43171

Conversation

ravisantoshgudimetla commented Mar 15, 2017 • edited Loading

k8s-ci-robot commented Mar 15, 2017

ravisantoshgudimetla commented Mar 15, 2017

k8s-reviewable commented Mar 15, 2017

ravisantoshgudimetla commented Mar 15, 2017

ethernetdan commented Mar 15, 2017

ravisantoshgudimetla commented Mar 16, 2017

fabianofranz commented Mar 16, 2017

ravisantoshgudimetla commented Mar 24, 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

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

jayunit100 Mar 24, 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

jayunit100 commented Mar 30, 2017

aveshagarwal commented Mar 30, 2017

ravisantoshgudimetla commented Mar 30, 2017

jayunit100 commented Mar 30, 2017 • edited Loading

davidopp commented Mar 30, 2017

Choose a reason for hiding this comment

jayunit100 Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayunit100 Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla commented Apr 17, 2017

k8s-ci-robot commented Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

jayunit100 commented Apr 17, 2017

jayunit100 commented Apr 19, 2017

jayunit100 commented Apr 20, 2017

k8s-github-robot commented Apr 20, 2017

jayunit100 commented Apr 20, 2017

k8s-github-robot commented Apr 20, 2017

ravisantoshgudimetla commented Mar 15, 2017 •

edited

Loading

jayunit100 Mar 24, 2017 •

edited

Loading

jayunit100 commented Mar 30, 2017 •

edited

Loading

jayunit100 Apr 3, 2017 •

edited

Loading

jayunit100 Apr 3, 2017 •

edited

Loading

k8s-ci-robot commented Apr 17, 2017 •

edited

Loading