@jdumars: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

/release-note-none

/assign @mikedanese @brendandburns

/retest

/retest
/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, jdumars

Associated issue: 53146

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

/test pull-kubernetes-e2e-kubeadm-gce

I still want a network api reviewer to sign off.

/hold

@mikedanese can you help facilitate that? This is critical for inclusion in 1.8.1.

cc: @kubernetes/sig-network-api-reviews

I'm wondering why this is going straight to GA rather than alpha or beta - can't seem to find the history where that call was made. Was that an explicit decision and are we confident in that?

I also seem to have lost my comment on the original PR somehow regarding the name of the label, specifically the fact that it's not really a "role" as it is currently worded - e.g. load-balancer-target or similar is a role, whereas exclude-balancer is an action to take on the node. Not too fussed about that really but thought it would be worth raising, especially if going straight to GA.

This is an api change that went in a month after code freeze without review. It's going straight to GA. There was no input from stakeholders. I'm concerned this didn't follow due process for API changes.

@mikedanese I don't think of this as an API change, since this is a node-level annotation.

It doesn't break anyone who doesn't set it, and anyone who does set it has explicitly opted in.

If we'd prefer that it was alpha.node-label.... to indicate that it is not supported by deprecation policy I'm fine with that.

Thanks

Also @mikedanese

There are plenty of examples of functionality going straight in via annotations with little/no API review, eg.

• AWS: SSL support for ELB listeners through annotations #23495
• AWS: More ELB attributes via service annotations #30695

And more.

@thockin thanks, please note I just sent in:

#53678

Which adds the alpha label to the annotation.

We'll add that to this cherry-pick.

/test pull-kubernetes-e2e-kubeadm-gce

Copied from #53146

Hey, all.

This should not have been merged without an issue, and probably should not have merged without someone from sig-network OWNERS looking at it.

In truth, the whole notion of using nodes as LB targets should be folded down into per-cloud-provider logic. Service controller should not be doing this predicate check or passing nodes around.

Secondly, the node-role concept is highly contentious, so we should not overload it.

Thirdly, "don't do X" is not a role.

Given that the nodes ARE being passed by service controller, fine. I don't think it should be about "role" though. Probably it should be named "node" rather than "node-role", or even something new like "service-controller.kubernetes.io/exclude-lb-node" ?

Let's revisit this one after 1.8.1.

This PR is not for the master branch but does not have the cherrypick-approved label. Adding the do-not-merge/cherry-pick-not-approved label.

@thockin how do you feel about alpha.service-controller.kubernetes.io/exclude-lb-node

But yes, I agree that cloud provider-specific is probably a more natural place for this eventually, but clearly that's not the world we live in right now.

If I modify the label to alpha.service-controller.kubernetes.io/exclude-lb-node are you ok with a cherry-pick/release?

If it is Alpha, it needs to exist behind an alpha feature-gate, defaulted to off.

Given that we pass a slice of Nodes to cloud-provider, could you just filter yourself there, for which I probably would not be as cautious?

Why is this in cmd/kubeadm? It really shouldn't be. kubeadm doesn't use it. kubeadm is not responsible for various labels cloud providers might or might not use. Please move this somewhere else.

cc @liggitt who has/have had opinions on the node-role labels.

@luxas it was there because that's where the other node-role related label was added. See @mikedanese 's comment above. I'm fine to move it, but someone needs to tell me where the "right" place is...

@thockin given that someone has to pro-actively add this annotation to their Service object, do we really need a flag gate too? That seems like over-kill. This is an opt-in feature.

I can place it in the cloud provider too, but if you look at the original PR, it looks like this feature would be useful to others outside of Azure (OpenStack folks seemed interested)

Following up here...

I'm fine to move it, but someone needs to tell me where the "right" place is...

#39112, sigh
Basically needs a steering committee decision. kubeadm, bootkube, kops, kubespray, etc. and other deployment solutions use this label since v1.6, but have the value in their own source code duplicated around. We are in need of a centralized, "right" place for this, and it requires a steering committee decision as lazy consensus failed.

FWIW, I'm happy with the implementation in #54644 (thanks @dims for helping move the constants as well!)

Then, going forward, we definitely need someone to write up a proper proposal for possibly graduating this from alpha.

Removing 1.8 milestone as this is superseded by #54955