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

remove LegacyNodeRoleBehavior and mv ServiceNodeExclusion to GA #97543

Merged
merged 1 commit into from Jan 5, 2021
Merged

remove LegacyNodeRoleBehavior and mv ServiceNodeExclusion to GA #97543

merged 1 commit into from Jan 5, 2021

Conversation

pacoxu
Copy link
Member

@pacoxu pacoxu commented Dec 28, 2020 •
edited

What type of PR is this?
/kind bug
/kind cleanup
/sig network
/area kubelet

What this PR does / why we need it:
According to KEP from https://github.com/kubernetes/enhancements/blob/54b409decd8d2e7f8a631fb845db58c810b4254c/keps/sig-architecture/2019-07-16-node-role-label-use.md
Master link:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-architecture/1143-node-role-labels

Which issue(s) this PR fixes:
Fixes #97542

Part of #94495
@thockin @smarterclayton @rikatz

Special notes for your reviewer:

During investigating the 97542 in my kubelet log, I read the KEP and #94495 comments. It needs to be done in 1.21.

The part related to #97542 is the function of node checking.
https://github.com/kubernetes/kubernetes/pull/97543/files#diff-62d886bf35c4945f56ff8cfd3d60e9f47a0deb8f6b3d09d89f48aacf3ba7b70eL972-L977

Does this PR introduce a user-facing change?:

[ACTION REQUIRED] `ServiceNodeExclusion`, `NodeDisruptionExclusion` and `LegacyNodeRoleBehavior`(locked to false) features have been promoted to GA. 
To prevent control plane nodes from being added to load balancers automatically, upgrade users need to add "node.kubernetes.io/exclude-from-external-load-balancers" label to control plane nodes.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-architecture/1143-node-role-labels

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 28, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/cloudprovider sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 28, 2020
@pacoxu
Copy link
Member Author

pacoxu commented Dec 28, 2020

/test pull-kubernetes-integration

@rikatz
Copy link
Contributor

rikatz commented Dec 28, 2020

/assign
/remove-kind bug

@k8s-ci-robot k8s-ci-robot removed the kind/bug Categorizes issue or PR as related to a bug. label Dec 28, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 5, 2021
thockin
thockin reviewed Jan 5, 2021
View reviewed changes
staging/src/k8s.io/cloud-provider/controllers/service/controller.go
// labelNodeRoleExcludeBalancer specifies that the node should not be considered as a target
// for external load-balancers which use nodes as a second hop (e.g. many cloud LBs which only
// understand nodes). For services that use externalTrafficPolicy=Local, this may mean that
// any backends on excluded nodes are not reachable by those external load-balancers.
// Implementations of this exclusion may vary based on provider. This label is honored starting
// in 1.16 when the ServiceNodeExclusion gate is on.
// Implementations of this exclusion may vary based on provider.
labelNodeRoleExcludeBalancer = "node.kubernetes.io/exclude-from-external-load-balancers"
Copy link
Member

Choose a reason for hiding this comment

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

followup nit - we should rename this to "labelNodeExcludeBalancers" or something - "role" has no place any more :)

Copy link
Contributor

Choose a reason for hiding this comment

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

want me to open a new issue and mark as a good first issue? I'm on it :D

Copy link
Contributor

Choose a reason for hiding this comment

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

#97734 :)

thockin
thockin reviewed Jan 5, 2021
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pacoxu, thockin

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2021
@rikatz rikatz mentioned this pull request Jan 5, 2021
Closed
@k8s-ci-robot k8s-ci-robot merged commit 0300aa7 into kubernetes:master Jan 5, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Jan 5, 2021
@rikatz rikatz mentioned this pull request Jan 5, 2021
Closed
@neolit123
Copy link
Member

the KEP link is now broken because we moved some files in k/e.
also it should be under:

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@neolit123
Copy link
Member

neolit123 commented Jan 6, 2021 •
edited

does sig-network recommend that kubeadm in 1.21 starts applying the node.kubernetes.io/exclude-from-external-load-balancers label on its CP nodes?

@neolit123 neolit123 mentioned this pull request Jan 6, 2021
Closed
2 tasks
@pacoxu
Copy link
Member Author

pacoxu commented Jan 7, 2021

KEP link updated @neolit123

@wilsonehusin
Copy link
Contributor

@pacoxu hello from 1.21 release notes team! The release notes annotated in bold caught my attention:

To prevent control plane nodes being added to load balancers automatically, upgrade users need to add "node.kubernetes.io/exclude-from-external-load-balancers" label to control plane nodes.

Can you clarify that this requires users / operators to take action / warrants a spot in "Urgent upgrade notes"? If so, please prepend the release note with [ACTION REQUIRED], otherwise confirm that users / operators do not need to take action when upgrading from 1.20 to 1.21.

Thanks!

@andrewsykim
Copy link
Member

This definitely warrants an ACTION REQUIRED item in the release notes IMO.

@wilsonehusin
Copy link
Contributor

This definitely warrants an ACTION REQUIRED item in the release notes IMO.

Thanks @andrewsykim! I have marked this to be in ACTION REQUIRED section.

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 20, 2021
@pacoxu
Copy link
Member Author

pacoxu commented Jan 20, 2021

@wilsonehusin thanks.
I edited like
image

Am I right?

@wilsonehusin
Copy link
Contributor

yup that works, thanks @pacoxu

@rikatz rikatz mentioned this pull request Jan 26, 2021
Merged
@pacoxu pacoxu mentioned this pull request Feb 26, 2021
Closed
@olemarkus olemarkus mentioned this pull request Feb 27, 2021
Merged
@neolit123 neolit123 mentioned this pull request Mar 2, 2021
Merged
This was referenced Apr 2, 2021
Merged
Merged
@pacoxu pacoxu deleted the fix/94495-ga branch June 23, 2021 05:35
@neolit123 neolit123 mentioned this pull request Sep 2, 2021
Closed
14 tasks
@chrigl chrigl mentioned this pull request May 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

@rikatz rikatz rikatz left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@aveshagarwal aveshagarwal Awaiting requested review from aveshagarwal

@lavalamp lavalamp Awaiting requested review from lavalamp

Assignees

@smarterclayton smarterclayton

@thockin thockin

@rikatz rikatz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

kubelet log continuously prints "Initializing eviction metric for zone"
8 participants
@pacoxu @rikatz @k8s-ci-robot @neolit123 @wilsonehusin @andrewsykim @thockin @smarterclayton