Describe appropriate use of node role labels and fixes that should be made #1144

smarterclayton · 2019-07-17T01:39:29Z

node-roles were not intended for internal use and this KEP clarifies both
their use and describes the process for resolving their internal use.

smarterclayton · 2019-07-17T01:43:23Z

@liggitt

keps/sig-architecture/2019-07-16-node-role-label-use.md

justinsb · 2019-07-17T20:49:12Z

keps/sig-architecture/2019-07-16-node-role-label-use.md

+
+* Kubernetes components MUST NOT set or alter behavior on any label within the `node-role.kubernetes.io/*` namespace.
+* Kubernetes components (such as `kubectl`) MAY simplify the display of `node-role.kubernetes.io/*` labels to convey the node roles of a node
+* Kubernetes examples and documentation MUST NOT leverage the node-role labels for node placement


This is the big one that has historically been the problem: how can a workload be targeted only to the control plane nodes?

i guess apart from a node-role label, a CP node would also need a regular label, that a nodeSelector can then target?

That is not specified and is implementation specific.

There is no such thing as a "control plane node" defined in Kube, and I think targeting user workloads to those nodes is completely outside the scope of Kube. Anyone who wants to do that needs to accept a label selector at a generic level.

I'll add that to the KEP, but I don't see this KEP as trying to solve that problem or any appetite to solve it, because control plane is vague and undefined.

My feeling is that we don't have a concept of "control plane nodes". Once you register a node of any sort, you have the "standard" mechanisms for workload inclusion/exclusion.

My question is where the line falls. Is it OK for a tool (think kubeadm) to have a "mode" which dedicates some nodes as control-plane? Should it accept an arbitrary label name to use instead of assuming this magic label? Is the label actually a sufficient primitive?

keps/sig-architecture/2019-07-16-node-role-label-use.md

justinsb · 2019-07-17T20:58:46Z

keps/sig-architecture/2019-07-16-node-role-label-use.md

+
+#### Node controller excludes master nodes from consideration for eviction
+
+The `k8s.io/kubernetes/pkg/util/system/IsMasterNode(nodeName)` function is used by the NodeLifecycleController to exclude nodes with a node name that ends in `master` or starts with `master-` when considering whether to mark nodes as disrupted. A recent PR attempted to change this to use node-roles and was blocked. Instead, the controller should be updated to use a label `node.kubernetes.io/exclude-disruption` to decide whether to exclude nodes from being considered for disruption handling.


Sounds good. Can we enable this one right away? Do we need to feature-flag it?

Everything new needs to go in as alpha gated.

Describing the intent rather than implying an intent from a role is vastly preferred.

fyi @dchen1107 as this is node controller.

keps/sig-architecture/2019-07-16-node-role-label-use.md

justinsb · 2019-07-17T21:14:30Z

keps/sig-architecture/2019-07-16-node-role-label-use.md

+
+#### Kubernetes e2e tests
+
+The e2e tests use a number of heuristics including the `IsMasterNode(nodeName)` function and the node-roles labels to select nodes. In order for conformant Kubernetes clusters to run the tests, the e2e suite must change to use individual user-provided label selectors to identify nodes to test, nodes that have special rules for testing unusual cases, and for other selection behaviors. The label selectors may be defaulted by the test code to their current values, as long as a conformant cluster operator can execute the e2e suite against an arbitrary cluster.


We should clearly differentiate between e2e tests that are part of the conformance suite, and the idea that a conformant cluster should be able to pass the whole e2e suite. It is absolutely not the case that a conformant cluster should be able to pass the whole suite (sadly), otherwise why mark tests for conformance. We have coverage of some pretty specific scenarios in our e2e suite, and it's going to be tough to replace this.

Unless what you're saying is we replace

const MasterNodeLabel = "node-role.kubernetes.io/master"

with

var MasterNodeLabel = flag.String("master-node-label", "node-role.kubernetes.io/master", "the label which identifies master nodes")

That seems reasonable, as does the idea that we should avoid tests requiring that the flag be a certain value, but I do think it's reasonable to have a conformance test that e.g. "pods keep running when the control plane never responds to requests", where labeling these nodes is likely the most practical way to implement this. Increasing e2e coverage feels more important than the ideological purity of the non-conformance tests.

The e2e tests need to not encode deployment assumptions. The assumptions that use node label today are almost all "wrong" in the sense that they assume deployment topology. e2e tests MUST NOT assume things like "pods in this role thus have open ports like this". The key distinction here is if an e2e test needs to read a particular port from a particular service, if that port is not part of core Kubernetes the e2e test needs to take it as a parameter. It might have a default for that parameter, but it's not acceptable for an e2e test to only work on one particular type of GCE test (i.e. kind should be able to run e2e tests if the apiserver isn't listening on 6443).

I'd draw the distinction between the e2e tests that are conformance vs non-conformance. Should we have configuration for these things and perfect tests that are ideologically pure, absolutely. But for a non-conformance test, I'd say coverage is more important - sometimes e2e tests need to do bad things.

Ok, I understand that, but I still would rather just fix the e2e tests in general since conformance is an arbitrary subset anyway. This isn't really a huge problem.

justinsb · 2019-07-17T21:17:49Z

We should also address the primary use case for these labels: how can a manifest target a pod to only run on a master node? That's the reason why they are so ubiquitous; their use inside k/k feels almost incidental.

keps/sig-architecture/2019-07-16-node-role-label-use.md

smarterclayton · 2019-07-18T18:57:51Z

how can a manifest target a pod to only run on a master node? That's the reason why they are so ubiquitous; their use inside k/k feels almost incidental.

This cannot be defined by Kubernetes because Kubernetes does not require a particular deployment topology. But certainly a large set of deployers can use node-role.kubernetes.io/master to indicate that this is a "master" - this is a convention in practice today. However, components within Kube may not key behavior or test coverage off that.

justinsb · 2019-07-18T19:31:16Z

I think it's fine to promote the service-exclusion label to GA, and to create another label that controls eviction, etc. Those are wonderful changes that users will appreciate.

But... users today need to steer workloads to run on the master. I would love it if we had a better answer on identity & isolation, but let's wait to tackle "node-role" until we are ready to make that case.

Propose that you rename this KEP to be "promote service-exclusion labels to GA and introduce node.kubernetes.io/exclude-disruption", so that it is technical and not political/philosophical.

smarterclayton · 2019-07-18T20:23:17Z

But... users today need to steer workloads to run on the master

By definition we cannot give all users that guarantee. Can you describe in more detail what your use was is? Anyone who tries to schedule onto any given topology has no guarantee that's even possible within a cluster (like GKE), so is this "I want to schedule on masters if they are available but something else if they aren't?"

justinsb · 2019-07-19T02:04:30Z

The use case seems to be a cluster component that wants either isolation or security. Security can be in case of breakout, or to reuse IAM roles or similar. Before all the installation tooling agreed on these labels, the producers of these manifests had to produce different manifests depending on the installation tooling in use. This is why we standardized it across the OSS kubernetes tooling.

Yes, it doesn't work on GKE / EKS or other kubernetes that host the control plane, but usually these cluster addons require special permissions anyway and thus aren't user installable there anyway.

Let's either expand this KEP so that it addresses the big picture - I'm happy to help you do so - or let's focus it on what it is doing technically, lest it suck the joy out of actually fixing node-roles.

smarterclayton · 2019-07-19T19:18:59Z

Before all the installation tooling agreed on these labels, the producers of these manifests had to produce different manifests depending on the installation tooling in use. This is why we standardized it across the OSS kubernetes tooling.

We did not do this or require this. A number of installers chose to do that. It is not required, nor standard.

I'm really confused what you mean by "fix node-roles" now. Maybe we should have a chat next week and dig in.

neolit123 · 2019-07-19T19:54:26Z

I partialy lack the historic context but if a number of deployers requsted the same label to indicate that a node hosts a control plane in a common, user visible way, i can see how this could have resulted in the unofficial node-role/master label. I think the deployer level standardization can be a good thing, even if the core is agnostic.

…

On Jul 19, 2019 22:19, "Clayton Coleman" ***@***.***> wrote: Before all the installation tooling agreed on these labels, the producers of these manifests had to produce different manifests depending on the installation tooling in use. This is why we standardized it across the OSS kubernetes tooling. We did not do this or require this. A number of installers chose to do that. It is not required, nor standard. I'm really confused what you mean by "fix node-roles" now. Maybe we should have a chat next week and dig in. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1144?email_source=notifications&email_token=AACRATDO54UYJCMLFCIO4QDQAIHSLA5CNFSM4IEJNATKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2MQS4I#issuecomment-513345905>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACRATGJXGCYQ4MOCOB4TZTQAIHSLANCNFSM4IEJNATA> .

derekwaynecarr

I agree with removing the concept of a node role in favor of labels that have more specific intents.

I would like to understand how we can enforce this concept across cloud providers as well given the large amount of innovation in hosting models across the ecosystem which depend on those components that were in-tree and now are out of tree.

derekwaynecarr · 2019-08-02T20:55:35Z

keps/sig-architecture/2019-07-16-node-role-label-use.md

+
+#### Node controller excludes master nodes from consideration for eviction
+
+The `k8s.io/kubernetes/pkg/util/system/IsMasterNode(nodeName)` function is used by the NodeLifecycleController to exclude nodes with a node name that ends in `master` or starts with `master-` when considering whether to mark nodes as disrupted. A recent PR attempted to change this to use node-roles and was blocked. Instead, the controller should be updated to use a label `node.kubernetes.io/exclude-disruption` to decide whether to exclude nodes from being considered for disruption handling.


Describing the intent rather than implying an intent from a role is vastly preferred.

fyi @dchen1107 as this is node controller.

keps/sig-architecture/2019-07-16-node-role-label-use.md

derekwaynecarr · 2019-08-02T21:01:57Z

keps/sig-architecture/2019-07-16-node-role-label-use.md

+QUESTION: Is a single label selector sufficient to identify nodes to test?
+
+
+#### Preventing accidental reintroduction


can we clarify how this should work with cloud provider code? for example, i assume cloud provider (in-tree or out) by convention must not make assumptions on deployment topology? how can we check for that as well?

I added a brief paragraph, but cloud providers are still part of the core K8s rule set because they are owned by a sig and required to maintain conformance on those clouds.

how is this distinct from kubeadm exactly?

owned by a sig

maintaining conformance

derekwaynecarr · 2019-08-02T21:05:18Z

@dchen1107 lets ensure there is no disagreement in next SIG node.

/assign
/assign @dchen1107
/assign @thockin

thockin · 2019-08-08T23:40:44Z

@freehan pointed out that Ingress controllers also use this and we should think of them. He will follow up with thoughts.

liggitt · 2019-08-08T23:58:46Z

@freehan pointed out that Ingress controllers also use this and we should think of them. He will follow up with thoughts

By "this", do you mean role labels or something else?

If you meant role labels, Ingress controllers that rely on role labels today are already dependent on particular deployments, since not all set those labels. Note that this doesn't propose removing anything that is currently applying the role labels, so it wouldn't make ingress controllers more broken than they are today.

thockin · 2019-08-09T00:24:07Z

The eventual removal of node-role labels might break Ingress controllers (or clusters), though, so we should evaluate and do outreach.

…

On Thu, Aug 8, 2019 at 4:58 PM Jordan Liggitt ***@***.***> wrote: @freehan <https://github.com/freehan> pointed out that Ingress controllers also use this and we should think of them. He will follow up with thoughts By "this", do you mean role labels or something else? If you meant role labels, Ingress controllers that rely on role labels today are already dependent on particular deployments, since not all set those labels. Note that this doesn't propose removing anything that is currently applying the role labels, so it wouldn't make ingress controllers more broken than they are today. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1144?email_source=notifications&email_token=ABKWAVA5PLN3LL23B2GTXFLQDSXMBA5CNFSM4IEJNATKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD35G3IQ#issuecomment-519728546>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKWAVHRT6CTYDG3NU2WSF3QDSXMBANCNFSM4IEJNATA> .

freehan · 2019-08-14T23:13:32Z

@liggitt
I believe this is copied from k/k repo https://github.com/kubernetes/ingress-gce/blob/release-1.6/pkg/utils/utils.go#L324

The service controller has this: https://github.com/kubernetes/kubernetes/blob/release-1.15/pkg/controller/service/service_controller.go#L602

sftim · 2019-08-17T21:56:29Z

Is there a role for SIG Docs in this work? (eg: should there be a page that clarifies the current situation vs. historical mixed meanings)

… made node-roles were not intended for internal use and this KEP clarifies both their use and describes the process for resolving their internal use.

liggitt · 2019-08-19T18:02:34Z

/lgtm

thockin · 2019-08-28T06:44:10Z

Thanks!

/lgtm
/approve

neolit123 · 2019-08-28T11:34:53Z

hm, why isn't @thockin on this list?
https://github.com/kubernetes/enhancements/blob/master/OWNERS_ALIASES#L9-L14

/assign @dims @mattfarina
for approval.

dims · 2019-08-28T11:41:30Z

/approve
/lgtm

based on @thockin and @liggitt 's ok above

k8s-ci-robot · 2019-08-28T11:41:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, smarterclayton, 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:

~~keps/sig-architecture/OWNERS~~ [dims]

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

As of 1.16 kubelet refuses to start when setting this label. See: kubernetes/enhancements#1144 for some background. * For now (so the tests pass, I have set an alternate label) * Perhaps we could just remove the concept of a node-role all together. * This means that kubectl get nodes doesn't show `ROLES` any more * If we want this to work then we would need some other process to update the node labels.

BouweCeunen · 2020-08-02T20:20:03Z

keps/sig-architecture/2019-07-16-node-role-label-use.md

+
+#### Service load-balancer
+
+The service load balancer implementation previously implemented a heuristic where `node-role.kubernetes.io/master` is used to exclude masters from the candidate nodes for a service. This is an implementation detail of the cluster and is not allowed. Since there is value in excluding nodes from service load balancer candidacy in some deployments, an alpha feature gated label `alpha.service-controller.kubernetes.io/exclude-balancer` was added in Kubernetes 1.9.


Not sure if this is the right place to comment. As I understand from this, the heuristic based on the label node-role.kubernetes.io/master will be removed? In what version will it be removed? Will the heuristic be done based on the node.kubernetes.io/exclude-from-external-load-balancers label in the future? Will this label be added to each master from a certain release to prevent adding master nodes as candidate nodes?

There should no longer be any issues running router pods on control plane nodes (i.e. kubernetes/kubernetes#65618 which was resolved in kubernetes/enhancements#1144). Remove this limitation from the docs. Signed-off-by: Stephen Finucane <[email protected]>

k8s-ci-robot requested review from derekwaynecarr and jdumars July 17, 2019 01:39

smarterclayton mentioned this pull request Jul 17, 2019

Clarify use of node-role labels within Kubernetes and migrate old components #1143

Closed

smarterclayton force-pushed the node_role branch from 4d63da2 to 06fd04d Compare July 17, 2019 01:43

smarterclayton force-pushed the node_role branch from 06fd04d to 71f34d8 Compare July 17, 2019 02:11

liggitt reviewed Jul 17, 2019

View reviewed changes

smarterclayton force-pushed the node_role branch from 71f34d8 to bf1dd31 Compare July 17, 2019 02:16

liggitt reviewed Jul 17, 2019

View reviewed changes

keps/sig-architecture/2019-07-16-node-role-label-use.md Show resolved Hide resolved

liggitt reviewed Jul 17, 2019

View reviewed changes

keps/sig-architecture/2019-07-16-node-role-label-use.md Show resolved Hide resolved

smarterclayton force-pushed the node_role branch 3 times, most recently from 110d83d to 4252b01 Compare July 17, 2019 03:03

BenTheElder reviewed Jul 17, 2019

View reviewed changes

keps/sig-architecture/2019-07-16-node-role-label-use.md Show resolved Hide resolved

justinsb reviewed Jul 17, 2019

View reviewed changes

keps/sig-architecture/2019-07-16-node-role-label-use.md Outdated Show resolved Hide resolved

fabriziopandini reviewed Jul 18, 2019

View reviewed changes

keps/sig-architecture/2019-07-16-node-role-label-use.md Show resolved Hide resolved

derekwaynecarr requested changes Aug 2, 2019

View reviewed changes

Describe appropriate use of node role labels and fixes that should be…

3ecd464

… made node-roles were not intended for internal use and this KEP clarifies both their use and describes the process for resolving their internal use.

smarterclayton force-pushed the node_role branch from e93419b to 3ecd464 Compare August 19, 2019 14:09

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2019

k8s-ci-robot assigned dims and mattfarina Aug 28, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2019

k8s-ci-robot merged commit 76bc705 into kubernetes:master Aug 28, 2019

k8s-ci-robot added this to the v1.17 milestone Aug 28, 2019

derekwaynecarr mentioned this pull request Sep 5, 2019

store: add kube_node_role metric kubernetes/kube-state-metrics#877

Merged

andrewsykim mentioned this pull request Sep 19, 2019

Defer the removal of labelAlphaNodeRoleExcludeBalancer to 1.18 kubernetes/kubernetes#82855

Merged

jeremyrickard mentioned this pull request Oct 1, 2019

Service Node Exclusion #724

Closed

smarterclayton deleted the node_role branch April 14, 2020 01:02

wking mentioned this pull request May 14, 2020

Masters should not be excluded from service load balancers kubernetes/kubernetes#65618

Closed

BouweCeunen reviewed Aug 2, 2020

View reviewed changes

yuvipanda mentioned this pull request Feb 14, 2021

Evaluate kops vs EKS 2i2c-org/farallon-image#28

Closed

stephenfin mentioned this pull request Jun 13, 2022

Bug 2096376: [openstack] Remove limitation on single node deployments openshift/installer#5997

Closed


		#### Node controller excludes master nodes from consideration for eviction

		The `k8s.io/kubernetes/pkg/util/system/IsMasterNode(nodeName)` function is used by the NodeLifecycleController to exclude nodes with a node name that ends in `master` or starts with `master-` when considering whether to mark nodes as disrupted. A recent PR attempted to change this to use node-roles and was blocked. Instead, the controller should be updated to use a label `node.kubernetes.io/exclude-disruption` to decide whether to exclude nodes from being considered for disruption handling.


		#### Kubernetes e2e tests

		The e2e tests use a number of heuristics including the `IsMasterNode(nodeName)` function and the node-roles labels to select nodes. In order for conformant Kubernetes clusters to run the tests, the e2e suite must change to use individual user-provided label selectors to identify nodes to test, nodes that have special rules for testing unusual cases, and for other selection behaviors. The label selectors may be defaulted by the test code to their current values, as long as a conformant cluster operator can execute the e2e suite against an arbitrary cluster.

		QUESTION: Is a single label selector sufficient to identify nodes to test?


		#### Preventing accidental reintroduction


		#### Service load-balancer

		The service load balancer implementation previously implemented a heuristic where `node-role.kubernetes.io/master` is used to exclude masters from the candidate nodes for a service. This is an implementation detail of the cluster and is not allowed. Since there is value in excluding nodes from service load balancer candidacy in some deployments, an alpha feature gated label `alpha.service-controller.kubernetes.io/exclude-balancer` was added in Kubernetes 1.9.

Describe appropriate use of node role labels and fixes that should be made #1144

Describe appropriate use of node role labels and fixes that should be made #1144

Conversation

smarterclayton commented Jul 17, 2019

smarterclayton commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jul 17, 2019 • edited Loading

smarterclayton commented Jul 18, 2019

justinsb commented Jul 18, 2019

smarterclayton commented Jul 18, 2019

justinsb commented Jul 19, 2019

smarterclayton commented Jul 19, 2019

neolit123 commented Jul 19, 2019 via email

derekwaynecarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Aug 2, 2019

thockin commented Aug 8, 2019

liggitt commented Aug 8, 2019

thockin commented Aug 9, 2019 via email

freehan commented Aug 14, 2019 • edited Loading

sftim commented Aug 17, 2019

liggitt commented Aug 19, 2019

thockin commented Aug 28, 2019

neolit123 commented Aug 28, 2019

dims commented Aug 28, 2019

k8s-ci-robot commented Aug 28, 2019

Choose a reason for hiding this comment

justinsb Jul 17, 2019 •

edited

Loading

justinsb commented Jul 17, 2019 •

edited

Loading

freehan commented Aug 14, 2019 •

edited

Loading