Changes for typecasting node in drain #48082

ravisantoshgudimetla · 2017-06-26T17:59:04Z

What this PR does / why we need it:

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

Special notes for your reviewer:
Precursor to #44944

Release note:

kubectl drain now uses PATCH instead of PUT to update the node. The node object is now of type v1 instead of using internal api.

k8s-ci-robot · 2017-06-26T17:59:11Z

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 /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.

I understand the commands that are listed here.

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-06-26T18:00:23Z

/cc @mml
/sig cli

shiywang · 2017-06-26T23:39:03Z

pkg/kubectl/cmd/drain.go

+		oldData, err := json.Marshal(obj)
+		node, ok := obj.(*v1.Node)
+		if !ok {
+			fmt.Println("Here Type%T", obj)


Is this print for debug?

Yes it is, thanks for pointing. I will remove this.

shiywang · 2017-06-27T00:18:36Z

/ok-to-test

ravisantoshgudimetla · 2017-06-27T00:26:57Z

@shiywang Thanks for reviewing, I wanted to check if the change is ok. If it is, I will proceed with build generation after which the tests should be passing.

ravisantoshgudimetla · 2017-06-27T20:45:50Z

@shiywang This is ready for another round of review. Can you please let me know, if you have any other comments?

mengqiy · 2017-07-11T06:17:54Z

pkg/kubectl/cmd/drain.go

@@ -43,7 +47,7 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	"k8s.io/kubernetes/pkg/kubectl/resource"
-	"k8s.io/kubernetes/pkg/kubelet/types"
+	ktypes "k8s.io/kubernetes/pkg/kubelet/types"


This dependency should be gone after rebase.

ravisantoshgudimetla · 2017-07-12T18:45:58Z

@mengqiy I have rebased the file. Any other changes needed?

mengqiy · 2017-07-12T22:40:49Z

pkg/kubectl/cmd/drain.go

+			if createdPatch {
+				_, err = helper.Patch(cmdNamespace, o.nodeInfo.Name, types.StrategicMergePatchType, patchBytes)
+			} else {
+				_, err := helper.Replace(cmdNamespace, o.nodeInfo.Name, false, obj)


Don't shadow this error. Or I will return the wrong error.

mengqiy · 2017-07-12T22:44:32Z

pkg/kubectl/cmd/drain.go

@@ -17,11 +17,11 @@ limitations under the License.
 package cmd

 import (
+	"encoding/json"


Please use "k8s.io/apimachinery/pkg/util/json", even though it doesn't have any difference when you only do marshal.

mengqiy · 2017-07-12T22:47:13Z

pkg/kubectl/cmd/drain_test.go

@@ -32,12 +32,13 @@ import (
 	"time"

 	"github.com/spf13/cobra"


Add a new line here.

mengqiy · 2017-07-12T22:52:00Z

pkg/kubectl/cmd/drain.go

+			if createdPatch {
+				_, err = helper.Patch(cmdNamespace, o.nodeInfo.Name, types.StrategicMergePatchType, patchBytes)
+			} else {
+				_, err := helper.Replace(cmdNamespace, o.nodeInfo.Name, false, obj)


We don't want to overwrite any more?
It used to be Replace(cmdNamespace, o.nodeInfo.Name, true, o.nodeInfo.Object)

Why cannot we switch to PATCH completely? It should not have any backward compatibility issue. Any other concerns?

So, I had this impression of Replace being an alternate to Patch in case it fails but didn't want it to overwrite and I wasn't sure if it is completely backward compatible but I will remove it completely now.

mengqiy · 2017-07-12T23:39:46Z

pkg/kubectl/cmd/drain.go

-				// It's a race, no need to sleep
+			newData, err := json.Marshal(obj)
+			patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, obj)
+			createdPatch := err == nil


Error handling can be simplified here. It seems we don't need createdPatch any more.

Thanks for correction. I just copied it from taints code. Deleted that now.

mengqiy

One last nit

mengqiy · 2017-07-13T00:20:21Z

pkg/kubectl/cmd/drain.go

+			patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, obj)
+			if err != nil {
+				return err
+			} else {


This else can be dropped as well.

patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, obj) if err != nil { return err } _, err = helper.Patch(cmdNamespace, o.nodeInfo.Name, types.StrategicMergePatchType, patchBytes) if err != nil { return err }

Done. Thanks again :)

ravisantoshgudimetla · 2017-07-13T02:16:57Z

/test pull-kubernetes-node-e2e

mengqiy · 2017-07-13T02:54:39Z

I think we need a release note saying that kubectl drain now uses PATCH instead of PUT to update the node.

mengqiy · 2017-07-13T02:54:54Z

/lgtm

fabianofranz · 2017-07-13T18:29:19Z

/approve

k8s-github-robot · 2017-07-13T18:29:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianofranz, mengqiy, ravisantoshgudimetla

Associated issue: 48059

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/kubectl/OWNERS~~ [fabianofranz]

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

ravisantoshgudimetla · 2017-07-13T19:18:52Z

/test pull-kubernetes-federation-e2e-gce

ravisantoshgudimetla · 2017-07-13T20:25:47Z

/retest

ravisantoshgudimetla · 2017-07-13T23:53:34Z

/retest

k8s-github-robot · 2017-07-14T05:43:54Z

Automatic merge from submit-queue (batch tested with PRs 48082, 48815, 48901, 48824)

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 Jun 26, 2017

k8s-github-robot assigned rootfs and ghodss Jun 26, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 26, 2017

k8s-ci-robot requested a review from mml June 26, 2017 18:00

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jun 26, 2017

ravisantoshgudimetla force-pushed the kubectl_drain_node_conversion branch 3 times, most recently from 205e77c to 16970e4 Compare June 26, 2017 18:15

shiywang reviewed Jun 26, 2017

View reviewed changes

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 Jun 27, 2017

ravisantoshgudimetla force-pushed the kubectl_drain_node_conversion branch from 16970e4 to b30eff5 Compare June 27, 2017 18:32

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

ravisantoshgudimetla force-pushed the kubectl_drain_node_conversion branch 3 times, most recently from 45d9d43 to 92d621a Compare June 27, 2017 20:42

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

ravisantoshgudimetla force-pushed the kubectl_drain_node_conversion branch from 92d621a to 55350e0 Compare June 27, 2017 22:16

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

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

mengqiy reviewed Jul 11, 2017

View reviewed changes

ravisantoshgudimetla force-pushed the kubectl_drain_node_conversion branch 2 times, most recently from 56e644e to a147060 Compare July 12, 2017 16:48

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

ravisantoshgudimetla force-pushed the kubectl_drain_node_conversion branch 2 times, most recently from 3ed161f to c3137ee Compare July 12, 2017 17:41

mengqiy reviewed Jul 12, 2017

View reviewed changes

ravisantoshgudimetla force-pushed the kubectl_drain_node_conversion branch from c3137ee to df3cf1b Compare July 12, 2017 23:19

mengqiy reviewed Jul 12, 2017

View reviewed changes

mengqiy reviewed Jul 13, 2017

View reviewed changes

Changes for converting node to v1 in drain

db120eb

ravisantoshgudimetla force-pushed the kubectl_drain_node_conversion branch from ad5fc9c to db120eb Compare July 13, 2017 01:36

k8s-ci-robot assigned mengqiy Jul 13, 2017

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

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

k8s-github-robot merged commit d58d29d into kubernetes:master Jul 14, 2017

ravisantoshgudimetla deleted the kubectl_drain_node_conversion branch July 14, 2017 17:15

shiywang mentioned this pull request Jul 26, 2017

Add shiywang to sig-cli help out review code #47587

Merged

soltysh mentioned this pull request Sep 28, 2017

UPSTREAM: 52440: add --dry-run option -> oadm <drain,cordon,uncordon> openshift/origin#16333

Merged

		@@ -32,12 +32,13 @@ import (
		"time"

		"github.com/spf13/cobra"

Changes for typecasting node in drain #48082

Changes for typecasting node in drain #48082

Uh oh!

Conversation

ravisantoshgudimetla commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 26, 2017

Uh oh!

ravisantoshgudimetla commented Jun 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shiywang commented Jun 27, 2017

Uh oh!

ravisantoshgudimetla commented Jun 27, 2017

Uh oh!

ravisantoshgudimetla commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravisantoshgudimetla commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengqiy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravisantoshgudimetla commented Jul 13, 2017

Uh oh!

mengqiy commented Jul 13, 2017

Uh oh!

mengqiy commented Jul 13, 2017

Uh oh!

fabianofranz commented Jul 13, 2017

Uh oh!

k8s-github-robot commented Jul 13, 2017

Uh oh!

ravisantoshgudimetla commented Jul 13, 2017

Uh oh!

ravisantoshgudimetla commented Jul 13, 2017

Uh oh!

ravisantoshgudimetla commented Jul 13, 2017

Uh oh!

k8s-github-robot commented Jul 14, 2017

Uh oh!

Uh oh!

ravisantoshgudimetla commented Jun 26, 2017 •

edited

Loading