-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Active connection tracking in ebpf #32584
Active connection tracking in ebpf #32584
Conversation
e8f0244
to
beecf87
Compare
This comment was marked as resolved.
This comment was marked as resolved.
c7a9677
to
58f239e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
58f239e
to
6bd2077
Compare
6bd2077
to
66e1d80
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loader changes are fine. As a rando reviewer, it's not at all clear to me what lrs
stands for however.
@julianwiedmann would you be able to review this for @cilium/sig-datapath, I'm assigned sig-cli but my review would also count towards datapath - and I'm not familiar enough with this code to provide a thorough review. |
76f4e16
to
27af564
Compare
LRS stands for Load Reporting Service from Envoy. Please see: https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/load_stats/v3/lrs.proto. I am bad at naming stuff, so I am open to suggestions |
@AwesomePatrol Does the TODO list in the second commits message represent work that's still to be done? |
Based on the TODOs and the missing description in the second patch, this looks like it still needs some more work. Flipping to draft for now to get it off my queue, please flip back when you're ready! 🙏 |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 thank you! I'd want to address the integration into the BPF codepath below.
Besides that, could you please squash the last two patches in the series into the initial series? I don't think we need to merge code that immediately gets refactored again.
Head branch was pushed to by a user without write access
8aa2cc8
to
3cc35fe
Compare
Note that you'll probably need a rebase to pick up the changes from #32653. |
3cc35fe
to
3bc306b
Compare
3bc306b
to
b1f5513
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Add new map - LB_ACT_MAP - behind ENABLE_ACTIVE_CONNECTION_TRACKING flag with counters of opened and closed connections. Behavior of eBPF remains completely unchanged when ENABLE_ACTIVE_CONNECTION_TRACKING flag is not set. When an entry, to conntrack table is created, an entry in LB_ACT_MAP.opened is incremented by one. When connection is closed, the related LB_ACT_MAP.closed is incremented by one. This works only for traffic originating from the local pods. LB_ACT_MAP is keyed by svc_id (also known as rev_nat_index) and zone, which is obtained from backend entry. Zone field in backend is populated only when EndpointSlice contains a reference to zone in FixedZoneMapping (so it is possible to convert between uint8 ID and string). Signed-off-by: Aleksander Mistewicz <amistewicz@google.com>
b1f5513
to
999920e
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, the conntrack integration feels much simpler now. Thank you!
Relating to
This sounds to me like a low-level detail that operators might consult during debugging, rather than unlocking for instance better load balancing across the cluster. What's the user-facing impact of this change? Could it be broader and we can describe it better in the release note, or is this pretty accurate, in which case do we think this has a major impact on users? |
I'm the one who added this release note. I added it because my understanding is that this is a new feature being exposed to advanced users (who would poll the new map). In the past, I've used |
I agree. I work on exposing these as prometheus metrics, so I believe this PR is an implementation detail and not really a new feature. Hopefully I will publish new PR this week. It could be marked
There should be no impact. Even with flags set (both
I don't think that map polling is very user friendly, but are there many examples of usage of cilium maps outside of cilium binary? |
As described in cilium/design-cfps#31, this is part of #31752
Example configuration (in
cilium-config
) for KIND cluster:where topology labels were set on the nodes:
Zone mapping can be verified with
cilium map get cilium_lb4_backends_v3
:LRS accounting can be verified with
cilium map get cilium_lb_act
: