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

Always check if default labels on node need to be updated in kubelet #47044

Merged
merged 1 commit into from
Aug 28, 2017
Merged

Always check if default labels on node need to be updated in kubelet #47044

merged 1 commit into from
Aug 28, 2017

Conversation

mrIncompetent
Copy link
Contributor

What this PR does / why we need it:
Nodes join again but maybe OS/Arch/Instance-Type has changed in the meantime.
In this case the kubelet needs to check if the default labels are still correct and if not it needs to update them.

Kubelet updates default labels if those are deprecated

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 6, 2017
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 6, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @mrIncompetent. 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.

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.

@k8s-github-robot k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 6, 2017
@k8s-ci-robot 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 6, 2017
@feiskyer
Copy link
Member

feiskyer commented Jun 6, 2017

@mrIncompetent is this targeted for v1.7?

@mrIncompetent
Copy link
Contributor Author

@feiskyer Yes!

@dims
Copy link
Member

dims commented Jun 6, 2017

@k8s-bot ok to test

@k8s-ci-robot 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 6, 2017
gyliu513
gyliu513 reviewed Jun 6, 2017
View reviewed changes
pkg/kubelet/kubelet_node_status_test.go Outdated
@@ -43,6 +43,7 @@ import (
core "k8s.io/client-go/testing"
"k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/client/clientset_generated/clientset/fake"
kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis"
Copy link
Contributor

Choose a reason for hiding this comment

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

The BUILD file also needs some update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done with 5a1587f

pkg/kubelet/kubelet_node_status.go Outdated
@@ -158,6 +159,38 @@ func (kl *Kubelet) tryRegisterWithApiServer(node *v1.Node) bool {
return false
}

//updateDefaultLabels will set the default labels on the node
Copy link
Contributor

Choose a reason for hiding this comment

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

// updateDefaultLabels

Add a blank space after //

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done with 5a1587f

pkg/kubelet/kubelet_node_status.go Outdated
kubeletapis.LabelArch,
}

if existingNode.Labels == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add some comments when this will happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this only happened during unit tests where other tests had node objects without labels.
I updated the tests and removed this block in 5a1587f

@feiskyer
Copy link
Member

feiskyer commented Jun 7, 2017

cc/ @kubernetes/sig-node-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jun 7, 2017
gyliu513
gyliu513 reviewed Jun 8, 2017
View reviewed changes
Copy link
Contributor

@gyliu513 gyliu513 left a comment

Choose a reason for hiding this comment

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

/lgtm thanks @mrIncompetent for the update

@mrIncompetent
Copy link
Contributor Author

@feiskyer @derekwaynecarr could you review?

@mrIncompetent
Copy link
Contributor Author

@k8s-bot pull-kubernetes-kubemark-e2e-gce test this

@mrIncompetent
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@feiskyer
Copy link
Member

LGTM. Seems this is not a blocker for v1.7, target it to 1.8?

@mrIncompetent
Copy link
Contributor Author

@feiskyer Yep, 1.8 is fine!

@feiskyer
Copy link
Member

/lgtm

Need @derekwaynecarr for approval.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2017
@mrIncompetent
Copy link
Contributor Author

/assign @derekwaynecarr

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2017
@mrIncompetent
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-etcd3
/test pull-kubernetes-unit

@derekwaynecarr
Copy link
Member

thanks, this is /lgtm

@derekwaynecarr
Copy link
Member

/approve

@derekwaynecarr
Copy link
Member

/approve no-issue

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2017
@mrIncompetent
Copy link
Contributor Author

@k8s-bot retest

@mrIncompetent
Copy link
Contributor Author

@gyliu513 Please check again. Lost lgtm after rebase+squash

@sttts
Copy link
Contributor

sttts commented Aug 28, 2017

/retest

gyliu513
gyliu513 reviewed Aug 28, 2017
View reviewed changes
Copy link
Contributor

@gyliu513 gyliu513 left a comment

Choose a reason for hiding this comment

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

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, feiskyer, gyliu513, mrIncompetent

Associated issue requirement bypassed by: derekwaynecarr

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit c17d70c into kubernetes:master Aug 28, 2017
@mrIncompetent mrIncompetent deleted the kubelet-update-default-labels branch August 28, 2017 15:32
@xilabao xilabao mentioned this pull request Aug 29, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gyliu513 gyliu513 gyliu513 left review comments

Assignees

@feiskyer feiskyer

@gyliu513 gyliu513

@derekwaynecarr derekwaynecarr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Denotes a PR that will be considered when it comes time to generate release notes. 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.
8 participants
@mrIncompetent @k8s-ci-robot @feiskyer @dims @derekwaynecarr @sttts @k8s-github-robot @gyliu513