UPSTREAM: 52917: add --selector support to drain,cordon #16445

juanvallejo · 2017-09-19T22:49:30Z

Related Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1466390
Related Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1466382

Adds --selector option to oc adm drain,cordon,uncordon, performing
each action on a list of nodes matching the given label.

If at least one node fails to be cordoned/uncordoned, the command will
continue to operate on any remaining nodes, uninterrupted, reporting
any errors it encounters along the way.

If at least one node fails to be drained, the command will halt, printing
the immediate error encountered, and a list of nodes yet to be drained
(including the node that just failed to be drained).

Work in progress.
TODO

add tests

cc @openshift/cli-review @soltysh

soltysh · 2017-09-21T09:15:12Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/drain.go

-		Do()
+		SelectorParam(o.Selector)
+
+	if len(args) > 0 {


Move that if before you create the builder and you can then squash the whole builder invocation into one.

soltysh · 2017-09-21T09:25:51Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/drain.go

+			return err
+		}
+		if len(infos) == 0 {
+			return fmt.Errorf("error: no nodes matched selector %q", o.Selector)


Do you think we need that message? I haven't seen this being error-ed out anywhere else when applying label selector.

Thanks, will remove

juanvallejo · 2017-09-21T14:37:32Z

@soltysh thanks for the review, will open upstream PR

soltysh

One more comment.

soltysh · 2017-09-22T10:15:44Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/drain.go

 	var err error
-	if len(args) != 1 {


With this new if you're swallowing the case where nothing was passed and you should return error. I think the best option would be to:

check args == 0 -> error out

check if args and selector is passed -> error out

soltysh · 2017-09-22T10:20:01Z

test/cmd/admin.sh

@@ -84,6 +85,18 @@ echo "manage-node: ok"
 os::test::junit::declare_suite_end

 os::test::junit::declare_suite_start "cmd/admin/certs"
+# check node-management cmds: drain,cordon,uncordon
+# ensure drain fails when both a nodeName and --selector are given
+os::cmd::expect_failure_and_text 'oc adm drain fake-node --selector fake=label' 'cannot specify both a node name and a \-\-selector option'


Can you also add the other failure, when no parameters are passed?

soltysh · 2017-09-22T10:20:54Z

I'm tagging this as do-not-merge, until an actual upstream commit is present. I don't want to end up with this being merged before upstream will be created and approved.

juanvallejo · 2017-09-22T21:15:18Z

/test cmd

juanvallejo · 2017-09-25T14:34:45Z

/test cmd

soltysh · 2017-09-28T10:52:30Z

Still waiting for upstream approval.

juanvallejo · 2017-10-05T13:56:30Z

@soltysh thanks, upstream PR has merged

liggitt · 2017-10-05T13:57:45Z

hold upstreams for rebase

liggitt · 2017-10-05T13:57:45Z

hold upstreams for rebase

juanvallejo · 2017-10-11T21:31:19Z

@liggitt @soltysh is this good to go?

liggitt · 2017-10-12T04:37:21Z

vendor/k8s.io/kubernetes/pkg/kubectl/cmd/drain.go

 	r := o.Factory.NewBuilder(true).
 		NamespaceParam(cmdNamespace).DefaultNamespace().
-		ResourceNames("node", args[0]).
+		SelectorParam(o.Selector).


shoudn't this be ResourceNames("nodes", args...)?

the nameArgs := []string{"nodes"} and if strings.Contains(args[0], "/") block above means this command parses resource args differently than other commands...

I'd expect ResourceNames("nodes", args...).SingleResourceType() and a check on the resource type of selected resources to make sure they are nodes

link to the upstream PR that resolved this? also, did 52917 go into 1.8?

Upstream patch: kubernetes/kubernetes#54083
52917 is not in 1.8: https://github.com/kubernetes/kubernetes/blob/release-1.8/pkg/kubectl/cmd/drain.go

openshift-merge-robot · 2017-10-21T01:20:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, liggitt, soltysh

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

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

~~OWNERS~~ [liggitt]

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

juanvallejo · 2017-11-09T22:36:31Z

/test extended_conformance_gce

openshift-bot · 2017-11-14T13:03:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

juanvallejo · 2017-11-14T15:49:36Z

/test extended_conformance_install
/test extended_conformance_install_update

juanvallejo · 2017-11-14T18:07:58Z

/test extended_conformance_install_update

openshift-bot · 2017-11-15T03:10:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-11-15T11:14:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

juanvallejo · 2017-11-15T15:04:57Z

/test extended_conformance_install_update

openshift-bot · 2017-11-15T19:18:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-11-16T01:21:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-11-16T13:27:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

juanvallejo · 2017-11-16T15:44:31Z

/test extended_conformance_install_update

openshift-merge-robot · 2017-11-16T15:47:49Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @fabianofranz @juanvallejo @liggitt @soltysh

openshift-ci-robot · 2017-11-20T22:52:40Z

@juanvallejo: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_update	`68c59ce`	link	`/test extended_conformance_install_update`
ci/openshift-jenkins/cmd	`5c02473`	link	`/test cmd`
ci/openshift-jenkins/extended_conformance_gce	`5c02473`	link	`/test extended_conformance_gce`

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.

openshift-merge-robot · 2017-11-20T22:52:50Z

@juanvallejo PR needs rebase

juanvallejo · 2017-12-06T22:55:23Z

Closing as the current rebase will be introducing these commits to origin

openshift-merge-robot assigned soltysh and liggitt Sep 19, 2017

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2017

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch 5 times, most recently from 234ac34 to 6d9241a Compare September 20, 2017 16:13

soltysh suggested changes Sep 21, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from 6d9241a to 445618c Compare September 21, 2017 14:35

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from fb85bc7 to 1e6231d Compare September 21, 2017 19:04

soltysh suggested changes Sep 22, 2017

View reviewed changes

soltysh reviewed Sep 22, 2017

View reviewed changes

soltysh added the do-not-merge label Sep 22, 2017

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from 1e6231d to ccac83e Compare September 22, 2017 16:05

juanvallejo changed the title ~~UPSTREAM: 0000: add --selector support to drain,cordon~~ UPSTREAM: 52917: add --selector support to drain,cordon Sep 22, 2017

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from ccac83e to b3a05ef Compare September 22, 2017 18:25

juanvallejo removed the do-not-merge label Oct 5, 2017

liggitt added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2017

liggitt reviewed Oct 12, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from b3a05ef to 25351c1 Compare October 12, 2017 19:31

liggitt added the kind/enhancement label Oct 21, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2017

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from 68c59ce to 16fd41a Compare November 16, 2017 15:47

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Nov 16, 2017

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from 16fd41a to 0192ecc Compare November 20, 2017 18:33

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2017

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from 0192ecc to 4dfcc84 Compare November 20, 2017 18:34

juanvallejo added 3 commits November 20, 2017 15:26

UPSTREAM: 52917: add --selector support

9a2936a

add tests

416b696

update completions

5c02473

juanvallejo force-pushed the jvallejo/add-support-multiple-node-selection branch from ebec124 to 5c02473 Compare November 20, 2017 20:26

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2017

juanvallejo closed this Dec 6, 2017

UPSTREAM: 52917: add --selector support to drain,cordon #16445

UPSTREAM: 52917: add --selector support to drain,cordon #16445

Conversation

juanvallejo commented Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Sep 21, 2017

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Sep 22, 2017

juanvallejo commented Sep 22, 2017

juanvallejo commented Sep 25, 2017

soltysh commented Sep 28, 2017

juanvallejo commented Oct 5, 2017

liggitt commented Oct 5, 2017

liggitt commented Oct 5, 2017

juanvallejo commented Oct 11, 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

openshift-merge-robot commented Oct 21, 2017

juanvallejo commented Nov 9, 2017

openshift-bot commented Nov 14, 2017

juanvallejo commented Nov 14, 2017

juanvallejo commented Nov 14, 2017

openshift-bot commented Nov 15, 2017

openshift-bot commented Nov 15, 2017

juanvallejo commented Nov 15, 2017

openshift-bot commented Nov 15, 2017

openshift-bot commented Nov 16, 2017

openshift-bot commented Nov 16, 2017

juanvallejo commented Nov 16, 2017

openshift-merge-robot commented Nov 16, 2017

openshift-ci-robot commented Nov 20, 2017

openshift-merge-robot commented Nov 20, 2017

juanvallejo commented Dec 6, 2017

juanvallejo commented Sep 19, 2017 •

edited

Loading