-
Notifications
You must be signed in to change notification settings - Fork 39.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes for typecasting node in drain #48082
Changes for typecasting node in drain #48082
Conversation
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 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. |
/cc @mml |
205e77c
to
16970e4
Compare
pkg/kubectl/cmd/drain.go
Outdated
oldData, err := json.Marshal(obj) | ||
node, ok := obj.(*v1.Node) | ||
if !ok { | ||
fmt.Println("Here Type%T", obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this print for debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is, thanks for pointing. I will remove this.
/ok-to-test |
@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. |
16970e4
to
b30eff5
Compare
45d9d43
to
92d621a
Compare
@shiywang This is ready for another round of review. Can you please let me know, if you have any other comments? |
92d621a
to
55350e0
Compare
pkg/kubectl/cmd/drain.go
Outdated
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency should be gone after rebase.
56e644e
to
a147060
Compare
3ed161f
to
c3137ee
Compare
@mengqiy I have rebased the file. Any other changes needed? |
pkg/kubectl/cmd/drain.go
Outdated
if createdPatch { | ||
_, err = helper.Patch(cmdNamespace, o.nodeInfo.Name, types.StrategicMergePatchType, patchBytes) | ||
} else { | ||
_, err := helper.Replace(cmdNamespace, o.nodeInfo.Name, false, obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't shadow this error. Or I will return the wrong error.
pkg/kubectl/cmd/drain.go
Outdated
@@ -17,11 +17,11 @@ limitations under the License. | |||
package cmd | |||
|
|||
import ( | |||
"encoding/json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use "k8s.io/apimachinery/pkg/util/json"
, even though it doesn't have any difference when you only do marshal.
pkg/kubectl/cmd/drain_test.go
Outdated
@@ -32,12 +32,13 @@ import ( | |||
"time" | |||
|
|||
"github.com/spf13/cobra" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new line here.
pkg/kubectl/cmd/drain.go
Outdated
if createdPatch { | ||
_, err = helper.Patch(cmdNamespace, o.nodeInfo.Name, types.StrategicMergePatchType, patchBytes) | ||
} else { | ||
_, err := helper.Replace(cmdNamespace, o.nodeInfo.Name, false, obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c3137ee
to
df3cf1b
Compare
pkg/kubectl/cmd/drain.go
Outdated
// It's a race, no need to sleep | ||
newData, err := json.Marshal(obj) | ||
patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, obj) | ||
createdPatch := err == nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling can be simplified here. It seems we don't need createdPatch
any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for correction. I just copied it from taints code. Deleted that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit
pkg/kubectl/cmd/drain.go
Outdated
patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, obj) | ||
if err != nil { | ||
return err | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks again :)
ad5fc9c
to
db120eb
Compare
/test pull-kubernetes-node-e2e |
I think we need a release note saying that |
/lgtm |
/approve |
[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:
You can indicate your approval by writing |
/test pull-kubernetes-federation-e2e-gce |
/retest |
1 similar comment
/retest |
Automatic merge from submit-queue (batch tested with PRs 48082, 48815, 48901, 48824) |
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 #48059Special notes for your reviewer:
Precursor to #44944
Release note: