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.

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

Clarify use of node-role labels within Kubernetes #80238

Merged
merged 4 commits into from Aug 28, 2019

Conversation

@smarterclayton
Copy link
Contributor

commented Jul 17, 2019

kubernetes/enhancements#1143

Node role labels were not intended for use internal to Kubernetes components because they are not required for conformant clusters. Replace uses with implementations that match the linked KEP.

/kind feature

Introduce `node.kubernetes.io/exclude-balancer` and `node.kubernetes.io/exclude-disruption` labels in alpha to prevent cluster deployers from being dependent on the optional `node-role` labels which not all clusters may provide.
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

/assign @liggitt

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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

@smarterclayton smarterclayton force-pushed the smarterclayton:disable_node_role branch from ad55ee9 to 46e444f Jul 17, 2019
@smarterclayton smarterclayton force-pushed the smarterclayton:disable_node_role branch from 46e444f to c8d4794 Jul 17, 2019
pkg/controller/service/service_controller.go Outdated Show resolved Hide resolved
pkg/controller/service/service_controller.go Outdated Show resolved Hide resolved
pkg/features/kube_features.go Outdated Show resolved Hide resolved
test/e2e/framework/metrics_util.go Outdated Show resolved Hide resolved
test/e2e/system/system_utils.go Outdated Show resolved Hide resolved
@smarterclayton smarterclayton force-pushed the smarterclayton:disable_node_role branch from c8d4794 to df8c16d Jul 17, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/XXL labels Jul 17, 2019
@liggitt liggitt added this to Assigned in API Reviews Jul 17, 2019
@@ -804,6 +805,42 @@ func (nc *Controller) monitorNodeHealth() error {
return nil
}

// labelNodeDisruptionExclusion is a label on nodes that controls whether they are
// excluded from being considered for disruption checks by the node controller.
const labelNodeDisruptionExclusion = "node.kubernetes.io/exclude-disruption"

This comment has been minimized.

Copy link
@timothysc

timothysc Jul 17, 2019

Member

Shouldn't this be in a publicly exported place?

This comment has been minimized.

Copy link
@smarterclayton

smarterclayton Jul 17, 2019

Author Contributor

Until it hits beta, I would say no. Referencing constants across multiple concrete domains (controller manager / rest of kube) would be a bug.

@smarterclayton smarterclayton force-pushed the smarterclayton:disable_node_role branch 2 times, most recently from 290146b to 16c987d Aug 19, 2019
@smarterclayton smarterclayton force-pushed the smarterclayton:disable_node_role branch from 16c987d to 66a92b1 Aug 19, 2019
@oomichi

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

/cc @oomichi

@k8s-ci-robot k8s-ci-robot requested a review from oomichi Aug 19, 2019
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

Updated with the new name.

Copy link
Member

left a comment

/lgtm

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 28, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

commented Aug 28, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

This gate will default to on in 1.16 to cover the behavior of the
existing system, and then in the future default to off and then be
removed once all consumers have migrated.
The service load balancer controller should honor the
LegacyNodeRoleBehavior feature gate for checks that use node-roles,
switch to using a non alpha annotation behind the gate, and prepare
to graduate to beta.
The current mechanism for excluding "master" nodes based on node names
is fragile and should be fixed by using a label exclusion similar to
service load balancers. The legacy code path is preserved behind a
defaulted-on gate and will be removed in the future.
@smarterclayton smarterclayton force-pushed the smarterclayton:disable_node_role branch from 66a92b1 to c57ebb9 Aug 28, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 28, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

New changes are detected. LGTM label has been removed.

@smarterclayton smarterclayton force-pushed the smarterclayton:disable_node_role branch from c57ebb9 to 507fbb1 Aug 28, 2019
A future change will stop using this signal and instead use a
label selector passed on creation.
@smarterclayton smarterclayton force-pushed the smarterclayton:disable_node_role branch from 507fbb1 to a49a554 Aug 28, 2019
@smarterclayton smarterclayton added the lgtm label Aug 28, 2019
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

Trivial rebase

@k8s-ci-robot k8s-ci-robot merged commit 92a320a into kubernetes:master Aug 28, 2019
24 checks passed
24 checks passed
cla/linuxfoundation smarterclayton authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 28, 2019
@liggitt liggitt moved this from In progress to API review completed, 1.16 in API Reviews Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.