nodeipam: align eTP=Cluster to kubernetes cloud-providers service lb #31406
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
MrFreezeex
force-pushed
the
nodeipam-align-ccm
branch
from
fd66434
to
6722a57
Compare
gandro
approved these changes
Mar 18, 2024
gandro
added
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
qmonnet
requested changes
Mar 18, 2024
MrFreezeex
force-pushed
the
nodeipam-align-ccm
branch
from
6722a57
to
57c45de
Compare
qmonnet
requested changes
Mar 18, 2024
MrFreezeex
force-pushed
the
nodeipam-align-ccm
branch
4 times, most recently
from
0adfa10
to
ef44b60
Compare
qmonnet
requested changes
Mar 19, 2024
MrFreezeex
force-pushed
the
nodeipam-align-ccm
branch
from
ef44b60
to
d0dfdbb
Compare
|
/test |
youngnick
approved these changes
Mar 27, 2024
MrFreezeex
force-pushed
the
nodeipam-align-ccm
branch
from
d0dfdbb
to
4d2994d
Compare
|
I rebased to fix the conflict on the helm values and related generated files ~ |
|
/test |
nodeipam was always looking at the related EndpointSlices of the Service LoadBalancer to decide which nodes should be "advertised". This is a problem when Service LoadBalancers are created with dummy endpoints which is the case for Cilium Ingress/GatewayAPI for instance. This commit attempts to replicate a bit more the behavior that a CCM would do to select nodes when eTP=Cluster. In that case we select all Nodes we consider all nodes as potential candidate instead of checking where the pods are scheduled via their EndpointSlices. In the case of eTP=Local, we fallback to the previous behavior of checking the EndpointSlice to know which Nodes are backing your corresponding Service. This is not the behavior done in classic CCM as eTP local seems to be typically implemented by Cloud providers via an health check mechanism that we currently don't have in nodeipam. And at this very moment is not planned to be implemented because of the extra complexity. If this gets implemented at some point nodeipam could also align with CCM on the eTP=Local case though. Also in both eTP=Cluster/Local we will respect KEP-3458 that is becoming stable in Kubernetes 1.30 and dictate how CCM does their first node filtering. The Predicates were extracted as is from kubernetes/cloud-provider repo where this is normally implemented for CCM. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
Now that nodeipam consider all nodes as potential candidate in the eTP=Cluster case, a way for user to filter nodes become way more critical and thus this commit is implementing this. Co-authored-by: Brendan Dalpe <bdalpe@gmail.com> Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
auto-merge was automatically disabled
April 2, 2024 11:28
Head branch was pushed to by a user without write access
MrFreezeex
force-pushed
the
nodeipam-align-ccm
branch
from
4d2994d
to
e3b13d0
Compare
|
Rebased again because the ipsec upgrade job was failing because of this: https://cilium.slack.com/archives/C7PE7V806/p1711615975498649?thread_ts=1711592330.743979&cid=C7PE7V806 |
|
/test |
maintainer-s-little-helper
bot
added
the
ready-to-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.
This aligns Node IPAM eTP=Cluster with how kubernetes CCM handle service lb node selection meaning that it will consider all nodes instead of only the node where your selected pods are scheduled. As a result of this change nodeipam will be compatible to Cilium Ingress/GatewayAPI by default which use a dummy endpoint which would fail with the previous behavior. See the first commit description for more details about this.
It also add
nodeipam.cilium.io/match-node-labelsannotation now that by default select all nodes and that nodeipam might be less usable if this wasn't added on anything that is not a really small cluster.Fixes: #31356