Skip to content

cilium, socklb: Terminate both UDP and TCP sockets #40304

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 8, 2025
Merged

cilium, socklb: Terminate both UDP and TCP sockets #40304

merged 3 commits into from
Jul 8, 2025

Conversation

borkmann
Copy link
Member

@borkmann borkmann commented Jul 1, 2025

(see commits)

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 1, 2025
@borkmann borkmann added the release-note/misc This PR makes changes that have no direct user impact. label Jul 1, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 1, 2025
@borkmann borkmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 1, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 1, 2025
@borkmann borkmann added the feature/socket-lb Impacts the Socket-LB part of Cilium's kube-proxy replacement. label Jul 1, 2025
@borkmann
Copy link
Member Author

borkmann commented Jul 2, 2025

/test

@borkmann borkmann marked this pull request as ready for review July 2, 2025 08:57
@borkmann borkmann requested review from a team as code owners July 2, 2025 08:57
@borkmann borkmann requested a review from ysksuzuki July 2, 2025 08:57
ysksuzuki
ysksuzuki reviewed Jul 2, 2025
View reviewed changes
borkmann added 2 commits July 4, 2025 08:53
@borkmann
cilium, socklb: Terminate both UDP and TCP sockets
4cb8e24 
Extend the socket termination logic and also terminate ongoing TCP
connections when a backend goes down which allows for short feedback
time since it does not require traffic to be radiated from clients
to receive a RST from the remote.

Note in terminateConnectionsToBackend we also remove the lb.ANY as
they are removed by the reconciler in the new control plane once the
maps have been populated with the new entries.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: https://github.com/cilium/cilium/pull/39800/files#r2122023563
@borkmann
cilium, bpf: Add comment on revnat map wrt L4 protos
8cdac65 
Add a small comment explaining why we don't have the L4 proto as part
of the lookup key.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
Copy link
Member Author

borkmann commented Jul 4, 2025

/test

ysksuzuki
ysksuzuki reviewed Jul 7, 2025
View reviewed changes
@borkmann borkmann force-pushed the pr/tcp-sk-dest branch 2 times, most recently from 9ee885f to d397b9e Compare July 7, 2025 11:48
@borkmann
cilium, socklb: Add states filter for the termination iteration
a02f477 
Add a state filter to the iterator and skip TCP sockets which are in
closing or time wait state. There is no need to spend time to iterate
these. Technically, there is no harm since when the client app closes
the socket and it goes into time wait state, then upon close the socket
LB removes the socket from the revnat map in cil_sock_release.. but
then again, no need to iterate through these.

Suggested-by: Yusuke Suzuki <yusuke.suzuki@isovalent.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann
Copy link
Member Author

borkmann commented Jul 7, 2025

/test

ysksuzuki
ysksuzuki approved these changes Jul 8, 2025
View reviewed changes
Copy link
Member

@ysksuzuki ysksuzuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 8, 2025
@borkmann borkmann merged commit 16e9d5d into main Jul 8, 2025
429 of 435 checks passed
@borkmann borkmann deleted the pr/tcp-sk-dest branch July 8, 2025 06:10
@borkmann borkmann mentioned this pull request Jul 10, 2025
Closed
@marseel marseel mentioned this pull request Jul 10, 2025
Closed
borkmann added a commit that referenced this pull request Jul 11, 2025
@borkmann
cilium, socklb: Add a flag for opting into terminating all protos
2079b06 
Marcel reported that #40304 introduced a significant regression in the
servicechurn scale test, causing almost 6x more CPU usage due to the
cost of iterating sockets via netlink. Disable this by default for now
until we find optimizations to make this less costly, for example,
in-kernel socket iterators.

Closes: #40463
Reported-by: Marcel Zieba <marcel.zieba@isovalent.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
borkmann added a commit that referenced this pull request Jul 14, 2025
@borkmann
cilium, socklb: Add a flag for opting into terminating all protos
87cafd8 
Marcel reported that #40304 introduced a significant regression in the
servicechurn scale test, causing almost 6x more CPU usage due to the
cost of iterating sockets via netlink. Disable this by default for now
until we find optimizations to make this less costly, for example,
in-kernel socket iterators.

Closes: #40463
Reported-by: Marcel Zieba <marcel.zieba@isovalent.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
borkmann added a commit that referenced this pull request Jul 14, 2025
@borkmann
cilium, socklb: Add a flag for opting into terminating all protos
5cb7676 
Marcel reported that #40304 introduced a significant regression in the
servicechurn scale test, causing almost 6x more CPU usage due to the
cost of iterating sockets via netlink. Disable this by default for now
until we find optimizations to make this less costly, for example,
in-kernel socket iterators.

Closes: #40463
Reported-by: Marcel Zieba <marcel.zieba@isovalent.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
borkmann added a commit that referenced this pull request Jul 14, 2025
@borkmann
cilium, socklb: Add a flag for opting into terminating all protos
f542672 
Marcel reported that #40304 introduced a significant regression in the
servicechurn scale test, causing almost 6x more CPU usage due to the
cost of iterating sockets via netlink. Disable this by default for now
until we find optimizations to make this less costly, for example,
in-kernel socket iterators.

Closes: #40463
Reported-by: Marcel Zieba <marcel.zieba@isovalent.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
rabelmervin pushed a commit to rabelmervin/cilium that referenced this pull request Aug 18, 2025
@borkmann @rabelmervin
cilium, socklb: Add a flag for opting into terminating all protos
73b4ccc 
Marcel reported that cilium#40304 introduced a significant regression in the
servicechurn scale test, causing almost 6x more CPU usage due to the
cost of iterating sockets via netlink. Disable this by default for now
until we find optimizations to make this less costly, for example,
in-kernel socket iterators.

Closes: cilium#40463
Reported-by: Marcel Zieba <marcel.zieba@isovalent.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ysksuzuki ysksuzuki ysksuzuki approved these changes

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. feature/socket-lb Impacts the Socket-LB part of Cilium's kube-proxy replacement. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@borkmann @ysksuzuki