connectivity: rework sniffer to execute tcpdump in background #40487

smagnani96 · 2025-07-11T14:47:47Z

Please do refer to commit messages for a detailed explanation.
I'd love as many feedback as possible, given this ended up being a not so easy change 😅

1st and 2nd commit are the major updates. Sometimes while re-reading them they do feel quite hacky, but haven't found a better way to implement that.
3rd commit connectivity: limit tcpdump capture also for Assert mode is a Why not?, definitively can go in a different PR (or even cancel it if I missed the reason why we shouldn't do that).

Fixes: #38643

smagnani96 · 2025-07-25T00:11:38Z

/ci-e2e-upgrade

giorio94

Nice! A few drive-by comments.

cilium-cli/connectivity/sniff/sniffer.go

gentoo-root · 2025-07-28T10:18:47Z

ci-e2e-upgrade didn't fail with the tcpdump error in 22 attempts. Some tests are unsuccessful because of other flakes and quay outage.

I'd say it's stable: the debug pull request #40692 can reproduce it after 3–4 retries, while this one survived 22 in a row.

gentoo-root · 2025-07-28T10:25:43Z

/test

gentoo-root

Simone is on vacation, I'm taking it over for the while.

cilium-cli/connectivity/sniff/sniffer.go

gentoo-root · 2025-07-28T12:46:29Z

/test

Prior to this commit, we were running remote sniffers via ExecInPodWithWriters and streaming stdout/stderr through the connection. We observed race conditions with underlying libraries (both WebSocket and SPDY) in case one of the stream is not available yet/anymore. In that case, the remote tcpdump command that is printing packet/logs in the streams would fail with error code 14 (broken pipe). Such flake was reproducible only in CI. The test consisted of the following instructions: 1. running an ExecInPodWithWriters connection with stdout/stderr stream for tcpdump 2. running an ExecInPod at the end of the test to retrieve packets count 3. running an ExecInPod at the end of the test to retrieve and print packets With this commit, we run the remote sniffer in background, without keeping open connections for streaming data. Tcpdump now logs both stdout/stderr and potential captured packets to local files in the pod, which are then retrieved/read by us at the end of each test. The difference is in the way we start/stop such command: given we do not have anymore an open connection to close, we need to carefully make sure that we call stop in all the possible circumstances (e.g., context canceled, timeout hit, errors in between two `Sniff()`, etc.). To do that, we introduce a close/finalizer function that is being returned in the costructor, so that the caller can defer its execution. The `Validate()` is also able to stop the sniffer, as well as the constructor `Sniff()` itself in case of errors (e.g., context canceled or timeout hit). We do now perform the following operations: i. run an ExecInPod to create the remote tcpdump process in background ii. run an ExecInPod to stop the remote process at the end of the test iii. running an ExecInPod at the end of the test to retrieve packets count iv. running an ExecInPod at the end of the test to retrieve and print packets While it is true that it might overload more the API server given we do need to ExecInPod once more, there's no relevant stdout/stderr in (i) and (ii) anymore, hopefully mitigating the tcpdump issue we were facing in CI flakes. Signed-off-by: Simone Magnani <simone.magnani@isovalent.com> Co-developed-by: Maxim Mikityanskiy <maxim@isovalent.com> Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

Prior to this patch series of 3 commits, we were: 1. running an ExecInPodWithWriters connection with stdout/stderr stream for tcpdump 2. running an ExecInPod at the end of the test to retrieve packets count 3. running an ExecInPod at the end of the test to retrieve and print packets In previous commits, we reworked the way we create and stop such sniffers: the tcpdump command is now left in background in the pod, and no active stream connections are present during the related tests. The total amount of remote commands executions were as follows: i. run an ExecInPod to create the remote tcpdump process in background ii. run an ExecInPod to stop the remote process at the end of the test iii. running an ExecInPod at the end of the test to retrieve packets count iv. running an ExecInPod at the end of the test to retrieve and print packets This is clearly inefficient, given we moved to background tcpdump in pods to avoid error 14 (broken pipe) due to race conditions while setting up the stream (the flake happened with both WebSockets and SPDY connections), observed especially under heavy load of the API server. This commit optimizes such behavior, unifying (ii) - (iii) - (iv). Upon successfully stopping the remote sniffer, the stop command executed in (ii) will additionally print to stdout (iii) and (iv) without having to re-execute ExecInPod. If the sniffer has been successfully stopped, this command will not fail, even if facing errors while printing out the packet count (iii) or the packet themselves (iv). This is aligned to what we're already doing right now, but with less connections. Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>

When running in Sanity mode, we do limit the number of captured packets to one, just to make sure that we see traffic in the interface where the sniffer is listening to. In Assert mode, we capture all the packets matching the filter, and we signal the leak at the end of the test by printing all of them. I'd suggest we limit also in Assert mode the amount of packets captured. Historically, I've seen captured packets to always repeat themselves, and I'm not sure it could be very helpful to have, for instance, more than 1000 leaked packets being logged. In addition, limiting the amount of captured packets would also help with keeping low memory footprint, given we do not remove the dump files yet from the pods. Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>

Avoid reverse DNS resolution of IP addresses. Suggested-by: Marco Iorio <marco.iorio@isovalent.com> Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

gentoo-root · 2025-07-28T14:45:57Z

/test

pchaigno

LGTM! Nice commit history! 🙏

pchaigno · 2025-07-28T15:59:04Z

cilium-cli/connectivity/sniff/sniffer.go

+	// (tcpdump: Unable to write output: Broken pipe)
+	//
+	// Runs silently: succeeds if tcpdump is active ("listening on <iface>"),
+	// else fails with its error code. Includes `timeout 60` to prevent orphaned


Nit in case of repush:

Suggested change

// else fails with its error code. Includes `timeout 60` to prevent orphaned

// else fails with its error code. Includes `timeout` to prevent orphaned

given the timeout is not hardcoded here.

pchaigno · 2025-07-28T16:06:07Z

cilium-cli/connectivity/sniff/sniffer.go

+	case ModeSanity:
+		count = 1
+	case ModeAssert:
+		count = 1000


IMO, even 100 would be largely enough. I wouldn't delay the PR on this though.

smagnani96 · 2025-07-29T12:33:58Z

cilium-cli/connectivity/sniff/sniffer.go

+	# Return an error if tcpdump timed out before capturing any packets.
+	if kill -0 $pid 2>/dev/null; then
+		kill -2 $pid 2>/dev/null
+	elif [ "$(tcpdump -r {{ .DumpPath }} --count 2>/dev/null)" = "0 packets" ]; then


@gentoo-root I believe this is incorrect for the ModeAssert where having 0 packets is good.
Isn't this checked later in Go anyway?

The point is to error out if tcpdump exited prematurely. If it didn't collect any packets and exited, it means it may have missed some packets, so we better fail the test.

In ModeAssert, however, we expect that tcpdump survives until it's killed on line 99. We don't hit the check on line 100 if everything works properly. Only if tcpdump was running during the entire test, we can be sure that 0 packets is indeed 0 packets, and we return 0 on line 111 in this case. So it's all good.

Nope, given we now have also -c 1000 for ModeAssert.
Tcpdump might have already recorded 1000 packets and exited before our check here.

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 11, 2025

github-actions bot added cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary labels Jul 11, 2025

smagnani96 mentioned this pull request Jul 11, 2025

CI: Cilium E2E Upgrade (ci-e2e-upgrade) - node-to-node-encryption: "Failed to execute tcpdump [...] command terminated with exit code 14" #38643

Closed

smagnani96 force-pushed the pr/smagnani96/fix-tcpdump-error-14-v2 branch from 8fd6428 to 5e83713 Compare July 18, 2025 17:58

joestringer added the release-note/ci This PR makes changes to the CI. label Jul 18, 2025

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 18, 2025

smagnani96 force-pushed the pr/smagnani96/fix-tcpdump-error-14-v2 branch from 5e83713 to 61df29e Compare July 25, 2025 00:03

smagnani96 self-assigned this Jul 25, 2025

smagnani96 changed the title ~~connectivity: remote tcpdump commands dump stream only to local file~~ connectivity: rework sniffer to execute tcpdump in background Jul 25, 2025

giorio94 reviewed Jul 25, 2025

View reviewed changes

gentoo-root reviewed Jul 28, 2025

View reviewed changes

cilium-cli/connectivity/sniff/sniffer.go Show resolved Hide resolved

gentoo-root force-pushed the pr/smagnani96/fix-tcpdump-error-14-v2 branch from 61df29e to 2e3e11a Compare July 28, 2025 12:34

gentoo-root force-pushed the pr/smagnani96/fix-tcpdump-error-14-v2 branch 2 times, most recently from ae8206e to 041bfb8 Compare July 28, 2025 14:27

smagnani96 added 3 commits July 28, 2025 17:31

gentoo-root force-pushed the pr/smagnani96/fix-tcpdump-error-14-v2 branch from 041bfb8 to a47aa75 Compare July 28, 2025 14:32

connectivity: Pass -n to tcpdump

d5d7c91

Avoid reverse DNS resolution of IP addresses. Suggested-by: Marco Iorio <marco.iorio@isovalent.com> Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>

gentoo-root force-pushed the pr/smagnani96/fix-tcpdump-error-14-v2 branch from a47aa75 to d5d7c91 Compare July 28, 2025 14:35

gentoo-root marked this pull request as ready for review July 28, 2025 15:50

gentoo-root requested review from a team as code owners July 28, 2025 15:50

gentoo-root requested review from rgo3 and christarazi July 28, 2025 15:50

pchaigno requested review from pchaigno and removed request for christarazi and rgo3 July 28, 2025 15:53

pchaigno approved these changes Jul 28, 2025

View reviewed changes

maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jul 28, 2025

pchaigno enabled auto-merge July 28, 2025 16:39

pchaigno added this pull request to the merge queue Jul 28, 2025

Merged via the queue into main with commit 1c2807c Jul 28, 2025
238 checks passed

pchaigno deleted the pr/smagnani96/fix-tcpdump-error-14-v2 branch July 28, 2025 16:50

smagnani96 commented Jul 29, 2025

View reviewed changes

	// else fails with its error code. Includes `timeout 60` to prevent orphaned
	// else fails with its error code. Includes `timeout` to prevent orphaned

connectivity: rework sniffer to execute tcpdump in background #40487

connectivity: rework sniffer to execute tcpdump in background #40487

Uh oh!

Conversation

smagnani96 commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smagnani96 commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giorio94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gentoo-root commented Jul 28, 2025

Uh oh!

gentoo-root commented Jul 28, 2025

Uh oh!

gentoo-root left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gentoo-root commented Jul 28, 2025

Uh oh!

gentoo-root commented Jul 28, 2025

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

pchaigno Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

pchaigno Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smagnani96 Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

gentoo-root Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

smagnani96 Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smagnani96 commented Jul 11, 2025 •

edited

Loading

smagnani96 commented Jul 25, 2025 •

edited

Loading