Skip to content

Conversation

tommyp1ckles
Copy link
Contributor

@tommyp1ckles tommyp1ckles commented Feb 12, 2025

The datapath/sockets code evolved from the use-case of serviceLB socket termination. This makes the code very specific to that code. Instead this generalizes the underlying code to expose more general functions for iterating and closing sockets via netlink.

Make datapath sockets code more general to promote code reuse.

@maintainer-s-little-helper
Copy link

Commit cb34a1d does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 12, 2025
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from 1ca65cb to e7d05a8 Compare March 4, 2025 20:54
@maintainer-s-little-helper
Copy link

Commit e8628a0 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from e7d05a8 to 93501e9 Compare March 4, 2025 21:38
@maintainer-s-little-helper
Copy link

Commit e8628a0 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from 93501e9 to 2756851 Compare March 4, 2025 22:34
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 4, 2025
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch 8 times, most recently from e9f363f to d4f3fc5 Compare March 6, 2025 03:18
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from d4f3fc5 to 725cac7 Compare March 13, 2025 04:50
@tommyp1ckles tommyp1ckles added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels Mar 13, 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 Mar 13, 2025
@tommyp1ckles tommyp1ckles marked this pull request as ready for review March 13, 2025 04:50
@tommyp1ckles tommyp1ckles requested review from a team as code owners March 13, 2025 04:50
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from 0e5cf58 to 030ffd1 Compare April 1, 2025 17:47
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from 030ffd1 to c555be9 Compare April 1, 2025 19:22
@tommyp1ckles tommyp1ckles requested a review from a team as a code owner April 1, 2025 19:22
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from c555be9 to ad080c0 Compare April 1, 2025 20:20
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

Solid step in generalizing the sockets termination logic. 🚀

It'll be good to fix the inconsistencies in the unit tests.

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from ad080c0 to 66bc5c3 Compare April 1, 2025 23:03
@tommyp1ckles
Copy link
Contributor Author

/test

This will be used in subsequent commits for building tests for service
UDP socket termination functionality.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
This provides an easy iterator type for iterating netns in
the default pinned directory.
This being a simple function that returns the iterator will
allow for easy test mocking in subsequent commands where we
want to create real network-namespaces, but don't necessarily
need to pin them to the filesystem.

Errors are aggregated in a buffered channel so they can be
accessed during iteration.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
This abstracts concern related to iterating netns and
uses the new netns.All() function to replace the current
default functionality parity.

This will allow for providing an iterator that doesn't
rely on pinned netns (i.e. just ones we create at runtime
for the test process) which will make it easier to test.

As well, switch global use of *option.Config to a field
now provided via hive. This will avoiding global options
mutation for tests.

Finally, we now return errors from the terminate function,
although we do not handle it as errors are already logged.
The purpose is to return errors while testing in following
commits.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
This provides integration testing to test service manager
TerminateUDPConnectionsToBackend functionality together
with sockets.Destroy implementation.

Note: This is committed prior to further changes to
pkg/datapath/sockets code to validate that tests succeed
before and after the change.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
Instead of having a custom function for iterating UDP connections
rename and change this function to: iterateNetlinkSockets.

iterateNetlinkSockets will be able iterate any type of socket via
the netlink api.

In subsequent commits, this will be used to make the datapath/sockets
library a general purpose sockets library that allows for separately
iterating and destroying sockets via netlink.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
This is used to validate that subsequent changes do
not introduce regressions to functionality relying on
sockets.Destroy (i.e. svc-LB UDP sock destroy).

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
Note: Up to this change was benchmarks to validate no memory
usage regressions are introduced following previous optimizations
in previous commits:

-------------
New:
-------------

goos: linux
goarch: arm64
pkg: github.com/cilium/cilium/pkg/datapath/sockets
BenchmarkSocketReqSerialize-8           69173708               174.3 ns/op           128 B/op          2 allocs/op
BenchmarkSocketDeserialize-8            159207418               75.43 ns/op           64 B/op          4 allocs/op
PASS
ok      github.com/cilium/cilium/pkg/datapath/sockets   31.865s

-------------
Old:
-------------

goos: linux
goarch: arm64
pkg: github.com/cilium/cilium/pkg/datapath/sockets
BenchmarkSocketReqSerialize-8           69530899               172.2 ns/op           128 B/op          2 allocs/op
BenchmarkSocketDeserialize-8            163702172               73.25 ns/op           64 B/op          4 allocs/op
PASS
ok      github.com/cilium/cilium/pkg/datapath/sockets   31.551s

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
The only consumer of this code is (currently) service
manager so we should reflect that in reviews.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/general-sockets-lib branch from 66bc5c3 to 9ca58db Compare April 3, 2025 00:33
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles
Copy link
Contributor Author

Rebasing/re-testing: #38566 fixes test failure

@tommyp1ckles tommyp1ckles enabled auto-merge April 3, 2025 00:33
@tommyp1ckles tommyp1ckles added this pull request to the merge queue Apr 3, 2025
Merged via the queue into cilium:main with commit dc068ea Apr 3, 2025
67 checks passed
@tommyp1ckles tommyp1ckles deleted the pr/tp/general-sockets-lib branch April 3, 2025 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants