service: NodePort frontend reconcilation #30374
Merged
Conversation
This file contains hidden or 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
Useful in combination with Collect/CollectSet/Map:
var iter statedb.Iterator[MyObject]
iter = statedb.Filter(iter, func(o MyObject) bool { return o.interesting })
iter = statedb.Map(iter, func(o MyObject) Attr { return o.Attr })
objs := statedb.Collect(iter)
Signed-off-by: Jussi Maki <jussi@isovalent.com>
When node addresses change we need to synchronize the NodePort frontends. This commit removes the earlier version in device-reloader.go and adds a reconciler to the service package. It also addresses the race with ParseService() by periodically checking that there are no unexpected service frontends. Signed-off-by: Jussi Maki <jussi@isovalent.com>
As NodePort services by definition are serviced only from the node's IP addresses we cannot use them in the nat46x64 test with an IP address not assigned to the node, but must instead use LoadBalancer service. This fixes the test failure that was caused by the newly added reconciliation of the NodePort frontends. Signed-off-by: Jussi Maki <jussi@isovalent.com>
|
There's two big changes this introduces that we should review here:
The automatic reaction to IP changes is something that we're trying to push for v1.16, essentially making |
28 tasks
christarazi
approved these changes
Jan 23, 2024
There was a problem hiding this comment.
LGTM, very clean implementation. Happy to see health check reporting being used more as well.
I've added the upgrade-impact label to signal if we need to take any action aside from warning users in the upgrade guide. It's not clear to me if there's a need for that.
dylandreimerink
approved these changes
Jan 24, 2024
oblazek
added a commit
to oblazek/cilium
that referenced
this pull request
Feb 23, 2024
With the recent changes in NodePort reconciliation (see cilium#30374) it is needed to switch service type from --k8s-node-port to --k8s-load-balancer as the VIP is not assigned to the node. Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 23, 2024
With the recent changes in NodePort reconciliation (see #30374) it is needed to switch service type from --k8s-node-port to --k8s-load-balancer as the VIP is not assigned to the node. Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
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.
The runtime device detection feature requires refreshing the NodePort services if the frontend addresses change (e.g. the IP addresses assigned to the node). This was implemented under
daemon/cmdby watching device changes (DeviceManager.Listen), but now the node addresses are available asTable[NodeAddress]and can be directly watched from pkg/service.This PR implements a reconciliation loop in pkg/service that watches the NodeAddress table and refreshes the services when addresses change. It also mitigates the race condition where ParseService might use stale addresses by periodically checking all services for missing NodePort frontends.
The changes this PR makes are also tested by the ginko test
Checks device reconfigurationintest/k8s/services.gowhich creates a new vxlan device connected to a non-cilium node and tests that a NodePort service can be accessed via the new IP assigned to it.Side note: the race condition with ParseService should really be addressed by moving the handling of the actual NodePort frontend IPs way down the stack, either to the datapath layer ("lbmap") or potentially even to BPF code. That's a larger refactoring though that also has implications to the
/serviceREST API, so not feasible to tackle it currently.Side note 2: this new code does not check for
--runtime-device-detection. We're going to make that a no-op and eventually remove the flag.This PR is also part of the general effort to get rid of
daemon/cmd/device-reloader.goand instead have each component deal with reconciling it's state when devices or addresses change.