Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use PatchStatus to update node status in kubelet. #37871

Merged
merged 1 commit into from
Dec 9, 2016
Merged

Use PatchStatus to update node status in kubelet. #37871

merged 1 commit into from
Dec 9, 2016
+196 −118

Conversation

Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Dec 1, 2016 •
edited by k8s-oncall
Loading

Fixes #37771.

This PR changes kubelet to update node status with PatchStatus.

@caesarxuchao @ymqytw told me that there is a limitation of current CreateTwoWayMergePatch, it doesn't support primitive type slice which uses strategic merge.

@yujuhong @dchen1107
/cc @kubernetes/sig-node

This change is Reviewable

@Random-Liu Random-Liu added area/kubelet release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Dec 1, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 1, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 1, 2016
@Random-Liu Random-Liu force-pushed the use-patch-in-kubelet branch from c82f2fa to 6a8624c Compare December 2, 2016 00:40
@mengqiy
Copy link
Member

mengqiy commented Dec 2, 2016

@Random-Liu PATCH doesn't work for MERGING list of primitives, such as finalizers. Merging list should specify patchStrategy:"merge". Other list of primitives use replace patchStrategy for default.

ContainerImage.Names
VolumesInUse

The 2 you mentioned above are using replace patchStrategy. I believe PATCH should work for them by replacing the whole list.

@Random-Liu
Copy link
Member Author

@ymqytw Thanks for confirmation! :)

@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit 6a8624c714090df7dd17c1d348a77e7cc17a2026. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@Random-Liu
Copy link
Member Author

Will update the PR to fix the unit test.

@Random-Liu Random-Liu force-pushed the use-patch-in-kubelet branch from 6a8624c to 6e53fff Compare December 6, 2016 08:57
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 6e53fffb69c30750d1579800a54c55cd2c4cc5f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 6e53fffb69c30750d1579800a54c55cd2c4cc5f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit 6e53fffb69c30750d1579800a54c55cd2c4cc5f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 6e53fffb69c30750d1579800a54c55cd2c4cc5f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 6e53fffb69c30750d1579800a54c55cd2c4cc5f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 6e53fffb69c30750d1579800a54c55cd2c4cc5f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 6e53fffb69c30750d1579800a54c55cd2c4cc5f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 6, 2016
@Random-Liu Random-Liu force-pushed the use-patch-in-kubelet branch from 6e53fff to 999af9e Compare December 6, 2016 09:03
@Random-Liu
Copy link
Member Author

@yujuhong Fixed the unit test. Please take a look. :)

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 999af9eea87fcd87e82f477c791e2fd22eea782b. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@Random-Liu Random-Liu force-pushed the use-patch-in-kubelet branch from 999af9e to f518abb Compare December 6, 2016 18:37
@rmmh
Copy link
Contributor

rmmh commented Dec 6, 2016

@k8s-bot gci gce e2e test this (job was accidentally deleted)

@yujuhong
Copy link
Contributor

yujuhong commented Dec 8, 2016

@caesarxuchao, could you review the status update operation? Thanks!

caesarxuchao
caesarxuchao reviewed Dec 8, 2016
View reviewed changes
pkg/util/node/node.go
@@ -153,3 +154,27 @@ func SetNodeCondition(c clientset.Interface, node types.NodeName, condition v1.N
_, err = c.Core().Nodes().PatchStatus(string(node), patch)
return err
}

// UpdateNodeStatus updates node status with PatchStatus.
func UpdateNodeStatus(c clientset.Interface, nodeName types.NodeName, oldNode *v1.Node, newNode *v1.Node) (*v1.Node, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you reset the NodeSpec and ObjectMeta before generating the patch? I have two concerns:

  1. objectMeta is updatable via the /status subresource. I don't know if Node has alpha status fields recorded in the annotation, if not, we'd better prevent updating ObjectMeta in this function
  2. Generating patch for unnecessary fields might lead to unnecessary conflicts, though the chance is low.

Copy link
Member Author

@Random-Liu Random-Liu Dec 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by reset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, I see. Just set the old one to new one. I see.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

caesarxuchao
caesarxuchao reviewed Dec 8, 2016
View reviewed changes
pkg/util/node/node.go
return nil, fmt.Errorf("failed to create patch for node %q: %v", nodeName, err)
}

updatedNode, err := c.Core().Nodes().PatchStatus(string(nodeName), patchBytes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you switch to use the regular Patch() method? It supports specifying subresources now. We should deprecate the special PatchStatus() method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now Patch can update status, too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you need to set the subreources.

Patch(name string, pt api.PatchType, data []byte, subresources ...string) (result *v1.Node, err error)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Random-Liu Random-Liu requested a review from yujuhong December 8, 2016 01:51
@Random-Liu Random-Liu force-pushed the use-patch-in-kubelet branch from f518abb to 05d0bfe Compare December 8, 2016 02:50
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 05d0bfe6754bcb0b0bd6fbe61dee3033628b0dab. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@Random-Liu
Copy link
Member Author

The unit test TestTryRegisterWithApiServer failed because we are actually using UpdateStatus to update ObjectMeta(Annotation). (See https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_node_status.go#L143-L171)

We could:

  1. Explicitly change the function from UpdateNodeStatus to UpdateNode.
  2. Still name the function as UpdateNodeStatus and do not reset ObjectMeta inside the container.

@caesarxuchao Which do you prefer?

@Random-Liu
Copy link
Member Author

Offline discussed with @caesarxuchao, only reset Node.Spec in UpdateNodeStatus, because:

  1. Apiserver allows updating objectmeta when updating node status.
  2. Some component does use this to update node annotations.

@Random-Liu Random-Liu force-pushed the use-patch-in-kubelet branch from 05d0bfe to 309b66f Compare December 8, 2016 19:00
@Random-Liu
Copy link
Member Author

@caesarxuchao @yujuhong Addressed comments. Let's merge this PR after #38375 to avoid conflict during cherry-pick.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 309b66f16d107894680b7b7013481cb0d10bb1d7. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@Random-Liu
Copy link
Member Author

@k8s-bot gce etcd3 e2e test this

@Random-Liu Random-Liu force-pushed the use-patch-in-kubelet branch from 309b66f to beba1eb Compare December 9, 2016 01:14
@Random-Liu
Copy link
Member Author

@caesarxuchao PR ready to go.

@caesarxuchao
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 9, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 36692, 37871)

@k8s-github-robot k8s-github-robot merged commit 43233ca into kubernetes:master Dec 9, 2016
@Random-Liu Random-Liu deleted the use-patch-in-kubelet branch December 9, 2016 19:38
@jingxu97 jingxu97 mentioned this pull request Dec 14, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caesarxuchao caesarxuchao caesarxuchao left review comments

@yujuhong yujuhong Awaiting requested review from yujuhong

Assignees

@caesarxuchao caesarxuchao

@yujuhong yujuhong

Labels
area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Random-Liu @mengqiy @k8s-ci-robot @rmmh @yujuhong @caesarxuchao @k8s-github-robot