trace/tcpretrans: Add support to tcploss events #1987

eiffel-fl · 2023-08-31T07:26:37Z

Hi.

In this PR, I added probing of tcp_send_loss_probe:

RUNTIME.CONTAINERNAME       PID        COMM            IP SRC                                  DST                                 STATE        TCPFLAGS       TYPE        
netem                       7711       wget            4  10.10.0.2:48490                      1.1.1.1:80                          SYN_SENT     SYN            retranmissi…
netem                       7721       wget            4  10.10.0.2:43628                      1.1.1.1:80                          ESTABLISHED                 loose       
netem                       7721       wget            4  10.10.0.2:43628                      1.1.1.1:80                          ESTABLISHED  PSH|ACK        retranmissi…
netem                       7721       wget            4  10.10.0.2:43628                      1.1.1.1:80                          ESTABLISHED  PSH|ACK        retranmissi…
netem                       7721       wget            4  10.10.0.2:33040                      1.1.1.1:443                         SYN_SENT     SYN            retranmissi…
netem                       7721       wget            4  10.10.0.2:43628                      1.1.1.1:80                          FIN_WAIT1                   loose       
netem                       7721       wget            4  10.10.0.2:43628                      1.1.1.1:80                          FIN_WAIT1    FIN|ACK        retranmissi…
netem                       7721       wget            4  10.10.0.2:33040                      1.1.1.1:443                         FIN_WAIT1                   loose       
netem                       7721       wget            4  10.10.0.2:33040                      1.1.1.1:443                         FIN_WAIT1    FIN|ACK        retranmissi…
netem                       7721       wget            4  10.10.0.2:33040                      1.1.1.1:443                         FIN_WAIT1    FIN|ACK        retranmissi…

Best regards.

eiffel-fl · 2023-08-31T08:15:35Z

I need to modify the documentation too but I would like to get opinions regarding the output before update the docs.

alban · 2023-09-04T10:32:37Z

Fixes #1585

alban

I am in favour of adding this feature: that will fix #1585.

Note: luckily, tcp_send_loss_probe was added a long time ago in Linux v3.10 (torvalds/linux@6ba8a3b) so we don't need to update docs/requirements.md.

alban · 2023-09-04T10:40:31Z

pkg/gadgets/trace/tcpretrans/tracer/tracer.go

+	case 1:
+		return "retranmission"
+	case 2:
+		return "loose"


Suggested change

return "loose"

// Tail Loss Probe (TLP)

// https://datatracker.ietf.org/doc/html/draft-dukkipati-tcpm-tcp-loss-probe-01

return "LTP"

pkg/gadgets/trace/tcpretrans/tracer/tracer.go

alban · 2023-09-04T10:49:29Z

pkg/gadgets/trace/tcpretrans/types/types.go

@@ -37,6 +37,8 @@ type Event struct {

 	SrcEndpoint eventtypes.L4Endpoint `json:"src,omitempty" column:"src"`
 	DstEndpoint eventtypes.L4Endpoint `json:"dst,omitempty" column:"dst"`
+
+	Type string `json:"type,omitempty" column:"type,minWidth:9,maxWidth:12,order:9000"`


Suggested change

Type string `json:"type,omitempty" column:"type,minWidth:9,maxWidth:12,order:9000"`

Type string `json:"type,omitempty" column:"type,minWidth:9,maxWidth:10,order:9000"`

Could it be named differently than Type to avoid conflict with Event->Type?

I modified everything to rather print it the event was lost or not.

pkg/gadgets/trace/tcpretrans/tracer/bpf/tcpretrans.h

github-actions · 2024-03-07T01:40:44Z

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

eiffel-fl · 2024-03-11T10:31:37Z

I rebased and tested it again and everythings works fine:

RUNTIME.CONTAINERNAME  PID        COMM         IP SRC                           DST                          STATE       TCPFLAGS    LOST
netem                  53934      wget         4  10.10.0.3:55190               1.1.1.1:443                  SYN_SENT    SYN         N   
netem                  53944      wget         4  10.10.0.3:55206               1.1.1.1:443                  ESTABLISHED             Y   
netem                  53944      wget         4  10.10.0.3:55206               1.1.1.1:443                  ESTABLISHED PSH|ACK     N   
netem                  53944      wget         4  10.10.0.3:55206               1.1.1.1:443                  ESTABLISHED PSH|ACK     N   
netem                  53944      wget         4  10.10.0.3:55206               1.1.1.1:443                  ESTABLISHED PSH|ACK     N   
netem                  53944      wget         4  10.10.0.3:55206               1.1.1.1:443                  FIN_WAIT1               Y

@alban @mauriciovasquezbernal reviews would be welcomed as it closes #1585.

alban

Thanks for adding this.

Some small things to change.

docs/builtin-gadgets/trace/tcpretrans.md

pkg/gadgets/trace/tcpretrans/types/types.go

pkg/gadgets/trace/tcpretrans/tracer/bpf/tcpretrans.bpf.c

alban · 2024-03-11T10:44:47Z

Could you make the same addition in the image-based gadget?

eiffel-fl · 2024-03-11T11:23:57Z

Could you make the same addition in the image-based gadget?

Let's first agree on what we to have for built-in gadgets and I will port the code to image based one.

mauriciovasquezbernal

I don't have a lot of experience with this gadget. I'm slightly leaned towards having a "TYPE" column with two possible values (for now) "RETRANS", and "TLP", it seems to me it's more clear. But I'll leave it to @alban as he implemented this gadget,

pkg/gadgets/trace/tcpretrans/tracer/bpf/tcpretrans.h

pkg/gadgets/trace/tcpretrans/types/types.go

eiffel-fl · 2024-03-11T16:14:35Z

I don't have a lot of experience with this gadget. I'm slightly leaned towards having a "TYPE" column with two possible values (for now) "RETRANS", and "TLP", it seems to me it's more clear.

Why not, but this would not be possible for the image based flavor.

alban · 2024-03-11T16:25:29Z

I don't have a lot of experience with this gadget. I'm slightly leaned towards having a "TYPE" column with two possible values (for now) "RETRANS", and "TLP", it seems to me it's more clear.

Why not, but this would not be possible for the image based flavor.

Couldn't it work with an enum with the 2 possible values RETRANS and TLP?

mauriciovasquezbernal · 2024-03-11T16:35:12Z

Why not, but this would not be possible for the image based flavor.

We're doing exactly that for the trace mount gadget:

inspektor-gadget/gadgets/trace_mount/program.bpf.c

Lines 17 to 20 in 1ea3f17

    
           enum op { 
        
           	MOUNT, 
        
           	UMOUNT, 
        
           };

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

eiffel-fl · 2024-03-11T18:22:24Z

Why not, but this would not be possible for the image based flavor.

We're doing exactly that for the trace mount gadget:

inspektor-gadget/gadgets/trace_mount/program.bpf.c

Lines 17 to 20 in 1ea3f17

enum op {

MOUNT,

UMOUNT,

};

Indeed, I switched to use an enum:

RUNTIME.CONTAINERNAME PID        COMM         IP SRC                          DST                         STATE       TCPFLAGS    TYPE   
netem                 195631     wget         4  10.10.0.3:50748              1.1.1.1:443                 ESTABLISHED             loss   
netem                 195631     wget         4  10.10.0.3:50748              1.1.1.1:443                 ESTABLISHED PSH|ACK     retrans

mauriciovasquezbernal

I'm still not sure about loss vs tlp, but that's not a big deal for me. LGTM once the documentation is updated.

pkg/gadgets/trace/tcpretrans/tracer/tracer.go

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

eiffel-fl · 2024-03-12T09:42:50Z

Could you make the same addition in the image-based gadget?

Done:

$ sudo -E ./ig run retrans                                     francis/tcploss % u+3-2
INFO[0000] Experimental features enabled                
RUNTIME.CONTAIN… SRC                      DST                      TASK      PID       UID       GID       TCPFLAGS REASON   TY… STATE   
netem            10.10.0.3:35828          1.1.1.1:443              wget      72868     0         0         2        0        RE… 2       
netem            10.10.0.3:35828          1.1.1.1:443              wget      72868     0         0         2        0        RE… 2       
netem            10.10.0.3:51932          1.1.1.1:443              wget      73041     0         0         2        0        RE… 2       
netem            10.10.0.3:37652          1.1.1.1:80               wget      73041     0         0         0        0        LO… 4       
netem            10.10.0.3:37652          1.1.1.1:80               wget      73041     0         0         17       0        RE… 4       
netem            10.10.0.3:37652          1.1.1.1:80               wget      73041     0         0         17       0        RE… 4

mauriciovasquezbernal

Changes to the image-based gadget LGTM as well.

eiffel-fl · 2024-03-13T08:14:18Z

Thank you for the review!

Modified and approved

eiffel-fl requested review from alban and mauriciovasquezbernal as code owners August 31, 2023 07:26

eiffel-fl force-pushed the francis/tcploss branch from 9a4bcd3 to fed5321 Compare August 31, 2023 08:15

eiffel-fl force-pushed the francis/tcploss branch 2 times, most recently from f2dd92d to d9e95d0 Compare September 1, 2023 07:45

alban reviewed Sep 4, 2023

View reviewed changes

eiffel-fl force-pushed the francis/tcploss branch 3 times, most recently from 21639da to 3097b04 Compare September 5, 2023 14:24

eiffel-fl force-pushed the francis/tcploss branch from 3097b04 to addc70a Compare December 18, 2023 14:46

github-actions bot added the lifecycle/stale Marked to be closed in next 14 days because of inactivity. label Mar 7, 2024

eiffel-fl force-pushed the francis/tcploss branch from addc70a to 5430bdc Compare March 11, 2024 09:48

eiffel-fl removed the lifecycle/stale Marked to be closed in next 14 days because of inactivity. label Mar 11, 2024

alban previously requested changes Mar 11, 2024

View reviewed changes

docs/builtin-gadgets/trace/tcpretrans.md Outdated Show resolved Hide resolved

pkg/gadgets/trace/tcpretrans/types/types.go Outdated Show resolved Hide resolved

pkg/gadgets/trace/tcpretrans/tracer/bpf/tcpretrans.bpf.c Outdated Show resolved Hide resolved

eiffel-fl force-pushed the francis/tcploss branch from 5430bdc to f2f0a76 Compare March 11, 2024 11:23

mauriciovasquezbernal reviewed Mar 11, 2024

View reviewed changes

pkg/gadgets/trace/tcpretrans/tracer/bpf/tcpretrans.h Outdated Show resolved Hide resolved

pkg/gadgets/trace/tcpretrans/types/types.go Outdated Show resolved Hide resolved

eiffel-fl force-pushed the francis/tcploss branch from f2f0a76 to 4951d31 Compare March 11, 2024 16:13

trace/tcpretrans: Add support to tcploss in eBPF code.

3b1fa7b

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

eiffel-fl force-pushed the francis/tcploss branch from 4951d31 to e9dbe8d Compare March 11, 2024 18:21

mauriciovasquezbernal approved these changes Mar 11, 2024

View reviewed changes

mauriciovasquezbernal reviewed Mar 11, 2024

View reviewed changes

pkg/gadgets/trace/tcpretrans/tracer/tracer.go Outdated Show resolved Hide resolved

eiffel-fl added 3 commits March 12, 2024 10:22

trace/tcpretrans: Modify tracer to support tcploss.

cabd6ec

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

gadgets: Add tcploss support to trace_tcpretrans.

0ecbac7

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

docs: Update documentation for trace tcpretrans

d47d8dd

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

eiffel-fl force-pushed the francis/tcploss branch from e9dbe8d to d47d8dd Compare March 12, 2024 09:42

eiffel-fl requested a review from alban March 12, 2024 09:42

mauriciovasquezbernal approved these changes Mar 12, 2024

View reviewed changes

eiffel-fl merged commit 1bf66e3 into main Mar 13, 2024
59 checks passed

eiffel-fl deleted the francis/tcploss branch March 13, 2024 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trace/tcpretrans: Add support to tcploss events #1987

trace/tcpretrans: Add support to tcploss events #1987

eiffel-fl commented Aug 31, 2023

eiffel-fl commented Aug 31, 2023

alban commented Sep 4, 2023

alban left a comment

alban Sep 4, 2023

alban Sep 4, 2023

eiffel-fl Sep 4, 2023

github-actions bot commented Mar 7, 2024

eiffel-fl commented Mar 11, 2024

alban left a comment

alban commented Mar 11, 2024

eiffel-fl commented Mar 11, 2024

mauriciovasquezbernal left a comment

eiffel-fl commented Mar 11, 2024

alban commented Mar 11, 2024

mauriciovasquezbernal commented Mar 11, 2024

eiffel-fl commented Mar 11, 2024

mauriciovasquezbernal left a comment

eiffel-fl commented Mar 12, 2024

mauriciovasquezbernal left a comment

eiffel-fl commented Mar 13, 2024

-		return "loose"
+		// Tail Loss Probe (TLP)
+		// https://datatracker.ietf.org/doc/html/draft-dukkipati-tcpm-tcp-loss-probe-01
+		return "LTP"

	Type string `json:"type,omitempty" column:"type,minWidth:9,maxWidth:12,order:9000"`
	Type string `json:"type,omitempty" column:"type,minWidth:9,maxWidth:10,order:9000"`

trace/tcpretrans: Add support to tcploss events #1987

trace/tcpretrans: Add support to tcploss events #1987

Conversation

eiffel-fl commented Aug 31, 2023

eiffel-fl commented Aug 31, 2023

alban commented Sep 4, 2023

alban left a comment

Choose a reason for hiding this comment

alban Sep 4, 2023

Choose a reason for hiding this comment

alban Sep 4, 2023

Choose a reason for hiding this comment

eiffel-fl Sep 4, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 7, 2024

eiffel-fl commented Mar 11, 2024

alban left a comment

Choose a reason for hiding this comment

alban commented Mar 11, 2024

eiffel-fl commented Mar 11, 2024

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

eiffel-fl commented Mar 11, 2024

alban commented Mar 11, 2024

mauriciovasquezbernal commented Mar 11, 2024

eiffel-fl commented Mar 11, 2024

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

eiffel-fl commented Mar 12, 2024

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

eiffel-fl commented Mar 13, 2024