-
Notifications
You must be signed in to change notification settings - Fork 31
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
NETOBSERV-1112: Add TCP RTT calculation to ebpf-agent and userspace. #117
Conversation
handshake SYN and ACK packets. Introduces a new ebpf map for tracking incoming and outgoing packets for RTT. Can be extended to any other way of TCP sequence tracking like TCPSecr/TCPval, or continous TCP latency tracking and other protocols like ICMP etc. Signed-off-by: Dushyant Behl <dushyantbehl@in.ibm.com>
Signed-off-by: Dushyant Behl <dushyantbehl@in.ibm.com>
Signed-off-by: Dushyant Behl <dushyantbehl@in.ibm.com>
Signed-off-by: Dushyant Behl <dushyantbehl@in.ibm.com>
15e5896
to
9517a51
Compare
Small changes to bpf rtt calculations. Signed-off-by: Dushyant Behl <dushyantbehl@in.ibm.com>
Tested on my end to be working end to end for calculating RTT for TCP handshake. |
@dushyantbehl, thanks for working on this PR ( and also, thanks, @praveingk ). @dushyantbehl Can you add some text that explains the logic of the calculation? I suggest using |
@jotak @jpinsonneau |
@dushyantbehl I ran some high-level test, and the results are ok, but not exactly what I expected:: (1) I ran in shell#1 (2) I ran in shell#2 (3) I ran in shell#3 The output I get is::
When I run
Now, the rtt 88ms ( from ping) is similar to the reported RTT we get from the eBPF agent :-) This looks good. The thing that is a bit challenging is the direction of the reports and the zeros. I see that we get RTT just in the return path (which makes sense), but it is a bit strange. We need to fix that in the UI or maybe in the user-space agent somehow. Can you add to the dump also the flags so we will know in which flows we get "syn-acks" and ion which reports we do not? |
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.
It works :-)
From the conversation ID we can link back to the first flow and show any missing info. I would be more in favor of that than duplicating data; even if it would require an extra query in some cases. FYI there is a doc for conversation tracking followup. Feel free to add any thoughts 😸 |
Codecov Report
@@ Coverage Diff @@
## main #117 +/- ##
==========================================
- Coverage 40.11% 39.64% -0.47%
==========================================
Files 31 31
Lines 2124 2154 +30
==========================================
+ Hits 852 854 +2
- Misses 1227 1254 +27
- Partials 45 46 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Thanks @dushyantbehl !
Help me understand.. the RTT is the time elapsed between the SYN emission and the ACK reception - so when when you say future flows in the conversation do not get the info, do you mean there could be more SYN/ACK for the same n-tuples? Or do you see other cases of potential loss of information? Also .. I have a concern, is it safe with sampling? ( @dushyantbehl @eranra @praveingk ) |
@jpinsonneau I am not able to access the doc could you please share access with my redhat account, username is |
Correct, RTT here is for initial handshake right now, the differnece between the time when SYN packet gets sent and when a corresponding ACK packet is received. Ideally we should do calculations for all packets because latency can vary in the network at any time which is something we can add on top of this patch.
Its safe to the level that this won't cause any wrong RTT values to be shown but if a SYN or its corresponding ACK packet gets sampled we won't see RTT measurement for that flow handshake because any ACK that will come later will have a different sequence id and not the id of first one. The only problem I foresee is if first ACKs are re-transmitted due to some reason the RTT calculation might not reflect real world round trip latency but in that case I'm not sure how best to mitigate this issue other than perform RTT measurement for all packets in the flow and then take a running average or something on top. |
Sure thing, I'm assuming by the dump you mean flowlogs-dump-collector.go it already has the flags just that they are not printed as SYN/ACK/FIN etc but just dumped in binary form. I can change that if that's what you meant. |
New image: quay.io/netobserv/netobserv-ebpf-agent:6ae2ab2. It will expire after two weeks. |
/ok-to-test |
New image: quay.io/netobserv/netobserv-ebpf-agent:b4e571b. It will expire after two weeks. |
@dushyantbehl: This pull request references NETOBSERV-1112 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Dushyant Behl <dushyantbehl@users.noreply.github.com>
Thanks @dushyantbehl !! |
/ok-to-test |
New image: quay.io/netobserv/netobserv-ebpf-agent:c1d9ac0. It will expire after two weeks. |
@dushyantbehl: This pull request references NETOBSERV-1112 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Related PR - netobserv/netobserv-ebpf-agent#117 Signed-off-by: Dushyant Behl <dushyantbehl@users.noreply.github.com>
Related PR - netobserv/netobserv-ebpf-agent#117 Signed-off-by: Dushyant Behl <dushyantbehl@users.noreply.github.com>
/lgtm @dushyantbehl thanks a lot and sorry for the long time to wait! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Add support for RoundTripTime in FLP. Related PR - netobserv/netobserv-ebpf-agent#117 Signed-off-by: Dushyant Behl <dushyantbehl@users.noreply.github.com> * Update the ebpf-agent go dependency. Signed-off-by: Dushyant Behl <dushyantbehl@users.noreply.github.com> --------- Signed-off-by: Dushyant Behl <dushyantbehl@users.noreply.github.com>
Fixes #102
Feature is disabled by default and can be enable with an environment variable flag.
Here are the CPU and Memory Numbers before and after the PR