Support antctl command for packetcapture #6884

hangyan · 2024-12-24T03:25:06Z

TODO items:

wait for Support collect logs for failed agents and controller for supportbundle #3659 merged and reuse it's uncompress function

after #3659 , the CodeQL alert should be fixed and UT coverage will also be improved.

pkg/antctl/raw/packetcapture/cp.go

hangyan · 2025-02-06T10:19:53Z

cc @antoninbas Can you help review this? also maybe merge #3659 first

cc @luolanzone

Signed-off-by: Hang Yan <[email protected]>

pkg/antctl/raw/packetcapture/command.go

antoninbas · 2025-02-07T18:16:15Z

pkg/antctl/raw/packetcapture/cp.go

+	restInterface rest.Interface
+}
+
+func (p *podFile) CopyFromPod(ctx context.Context, namespace, name, containerName, srcPath, dstDir string) error {


We already have ExecInPod defined in pkg/antctl/raw/check/. Can we unify a bit?

i will try.

I think it would make more sense to move the common code to pkg/antctl/raw/helper.go

moved. I only verified this with single file in container, not directory.

pkg/antctl/raw/packetcapture/command.go

antoninbas · 2025-02-07T18:22:27Z

pkg/antctl/raw/packetcapture/command.go

+	return fmt.Sprintf("%s-%s", prefix, rand.String(8))
+}
+
+func parseFlow() (*v1alpha1.Packet, error) {


could you group all input parsing functions together?

getFlowFields is not grouped with the other ones

pkg/antctl/raw/packetcapture/command.go

pkg/antctl/raw/packetcapture/command_test.go

luolanzone · 2025-02-08T08:36:14Z

Please fix the antctl e2e test failures: https://github.com/antrea-io/antrea/actions/runs/13198383877?pr=6884

Co-authored-by: Lan <[email protected]> Co-authored-by: Antonin Bas <[email protected]>

luolanzone · 2025-02-11T07:33:36Z

@hangyan there are lots of failures in github checks, please check and fix them.

hangyan · 2025-02-12T02:57:39Z

Please fix the antctl e2e test failures: https://github.com/antrea-io/antrea/actions/runs/13198383877?pr=6884

I disable the flag to run packetcapture sub-command in agentMode but controllerMode will still fail. One must be chosen, or this sub-command wont be registered to the rootCmd. However, seems packetcapture doens't make much sense in agentMode or controllerMode(controllerMode also contains outside of cluster case I think, but the e2e will check the command in controller pod).

The tests expect run antctl packetcapture to be successful. the first error comes from packet number should be larger than 0, that's the default value and it will cause error. Even we set with a valid default value, no src/dest endpoint （ i think Traceflow support this?), or packetcapture is not enabled , the command will still fail.

Can we make an exception in the test-case or we need to provide some sane defaults for it to working in the controllerMode?

cc @luolanzone @antoninbas

Signed-off-by: Hang Yan <[email protected]>

pkg/antctl/raw/packetcapture/command.go

antoninbas · 2025-02-12T19:43:03Z

pkg/antctl/raw/packetcapture/command.go

+		return false, nil
+	})
+
+	if wait.Interrupted(err) {


my concern is that when the timeout is hit, these 2 events will happen at roughly the same time:

the PacketCapture expires and the agent writes the partial pcap to disk

the wait.PollUntilContextTimeout is interrupted and the antctl command exits right away, without retrieving the capture results

Do you think this can happen?
Maybe after the wait.PollUntilContextTimeout expires, we could wait for the results for an extra (at most) 5 seconds. What do you think?

the concern make sense.updated with increased timeout for PollUntilContextTimeout. If the PC timeout, and we have a pacap file path, it will be copied

lgtm, can you add a comment before the wait.PollUntilContextTimeout line to explain why we are doing this

antoninbas · 2025-02-12T19:44:03Z

pkg/antctl/raw/packetcapture/cp.go

+	restInterface rest.Interface
+}
+
+func (p *podFile) CopyFromPod(ctx context.Context, namespace, name, containerName, srcPath, dstDir string) error {


I think it would make more sense to move the common code to pkg/antctl/raw/helper.go

Signed-off-by: Hang Yan <[email protected]>

antoninbas · 2025-02-28T00:55:53Z

docs/antctl.md

+### PacketCapture
+
+`antctl packetcapture` (or  `antctl pc`) command is used to start a `PacketCapture`
+and retrieve the captured result. After the result packet file is copied out,


After the result packet file (in pcapng format)

antoninbas · 2025-02-28T00:58:46Z

docs/antctl.md

+
+The `--flow` (or `-f`) argument can be used to specify the PacketCapture packet
+headers with the [ovs-ofctl](http://www.openvswitch.org//support/dist-docs/ovs-ofctl.8.txt)
+flow syntax(This argument works similar as Traceflow). The supported flow fields


Suggested change

flow syntax(This argument works similar as Traceflow). The supported flow fields

flow syntax. This argument works the same way as the one for `antctl traceflow`. The supported flow fields

antoninbas · 2025-02-28T00:59:03Z

docs/antctl.md

+(`tcp_src`, `tcp_dst`, `udp_src`, `udp_dst`).
+
+By default, the command will wait for the PacketCapture to succeed or fail, or
+timeout. The default timeout is 10 seconds, but can be changed with the


or to timeout

antoninbas · 2025-02-28T00:59:41Z

docs/antctl.md

+(`tcp_src`, `tcp_dst`, `udp_src`, `udp_dst`).
+
+By default, the command will wait for the PacketCapture to succeed or fail, or
+timeout. The default timeout is 10 seconds, but can be changed with the


wrong default timeout value, it is 60s

antoninbas · 2025-02-28T01:00:22Z

docs/antctl.md

+More examples of `antctl packetcapture`:
+
+```bash
+Start capturing packets from pod1 to pod2, both Pods are in Namespace default


if you use the bash syntax highlighting, add the comment sign # before each description sentence

antoninbas · 2025-02-28T01:13:39Z

pkg/antctl/raw/packetcapture/command.go

+		return false, nil
+	})
+
+	if wait.Interrupted(err) {


lgtm, can you add a comment before the wait.PollUntilContextTimeout line to explain why we are doing this

antoninbas · 2025-02-28T01:14:32Z

pkg/antctl/raw/packetcapture/command.go

+	splits := strings.Split(latestPC.Status.FilePath, ":")
+	if len(splits) < 2 {
+		return errors.New("no packets file generated, this maybe caused by timeout")
+	}


do you think we should check the Status field first?

antoninbas · 2025-02-28T01:14:52Z

pkg/antctl/raw/packetcapture/command.go

+	if len(splits) < 2 {
+		return errors.New("no packets file generated, this maybe caused by timeout")
+	}
+	fileName := filepath.Base(splits[1])


same comment as earlier, path would make more sense

antoninbas · 2025-02-28T01:16:55Z

pkg/antctl/raw/packetcapture/command.go

+	defer func() {
+		if !option.nowait {
+			if err = antreaClient.CrdV1alpha1().PacketCaptures().Delete(context.TODO(), pc.Name, metav1.DeleteOptions{}); err != nil {
+				fmt.Fprintf(cmd.OutOrStdout(), "error when deleting PacketCapture: %s", err.Error())


nit: maybe define out := cmd.OutOrStdout() instead of repeating cmd.OutOrStdout() multiple times (whenever you call fmt.Printf)

antoninbas · 2025-02-28T01:17:48Z

pkg/antctl/raw/packetcapture/command.go

+	return fmt.Sprintf("%s-%s", prefix, rand.String(8))
+}
+
+func parseFlow() (*v1alpha1.Packet, error) {


getFlowFields is not grouped with the other ones

github-advanced-security bot found potential problems Dec 24, 2024

View reviewed changes

pkg/antctl/raw/packetcapture/cp.go Fixed Show resolved Hide resolved

hangyan force-pushed the topic/yhang/packetcapture-antctl branch from 4a616d6 to bf4cb68 Compare January 14, 2025 09:03

github-advanced-security bot found potential problems Jan 14, 2025

View reviewed changes

pkg/antctl/raw/packetcapture/cp.go Fixed Show fixed Hide fixed

hangyan changed the title ~~[WIP] Support antctl command for packetcapture~~ Support antctl command for packetcapture Jan 14, 2025

hangyan requested review from antoninbas, tnqn and luolanzone January 14, 2025 09:56

hangyan force-pushed the topic/yhang/packetcapture-antctl branch from 676981f to b27b72b Compare February 6, 2025 10:21

hangyan added 2 commits February 7, 2025 17:20

Support antctl command for packetcapture

12bc0c3

Signed-off-by: Hang Yan <[email protected]>

reuse decompress func

Loading
Loading status checks…

43a9451

Signed-off-by: Hang Yan <[email protected]>

hangyan force-pushed the topic/yhang/packetcapture-antctl branch from b27b72b to 43a9451 Compare February 7, 2025 10:54

antoninbas reviewed Feb 7, 2025

View reviewed changes

luolanzone mentioned this pull request Feb 8, 2025

antctl support for PacketCapture #6860

Open

luolanzone reviewed Feb 8, 2025

View reviewed changes

luolanzone added this to the Antrea v2.4 release milestone Feb 8, 2025

hangyan force-pushed the topic/yhang/packetcapture-antctl branch from e1ba1b2 to 3436bfe Compare February 11, 2025 10:19

update

Loading
Loading status checks…

e44de62

Signed-off-by: Hang Yan <[email protected]>

hangyan force-pushed the topic/yhang/packetcapture-antctl branch from 5ac92d9 to e44de62 Compare February 12, 2025 07:15

antoninbas reviewed Feb 12, 2025

View reviewed changes

hangyan added 2 commits February 13, 2025 18:29

update

Loading
Loading status checks…

d47637b

Signed-off-by: Hang Yan <[email protected]>

update doc

Loading
Loading status checks…

1459f0f

Signed-off-by: Hang Yan <[email protected]>

antoninbas reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support antctl command for packetcapture #6884

Support antctl command for packetcapture #6884

hangyan commented Dec 24, 2024 •

edited

Loading

hangyan commented Feb 6, 2025

antoninbas Feb 7, 2025

hangyan Feb 11, 2025

hangyan Feb 12, 2025

antoninbas Feb 12, 2025

hangyan Feb 13, 2025

antoninbas Feb 7, 2025

antoninbas Feb 28, 2025

luolanzone commented Feb 8, 2025

luolanzone commented Feb 11, 2025

hangyan commented Feb 12, 2025 •

edited

Loading

antoninbas Feb 12, 2025

hangyan Feb 13, 2025

antoninbas Feb 28, 2025

antoninbas Feb 12, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

antoninbas Feb 28, 2025

	flow syntax(This argument works similar as Traceflow). The supported flow fields
	flow syntax. This argument works the same way as the one for `antctl traceflow`. The supported flow fields

Support antctl command for packetcapture #6884

Are you sure you want to change the base?

Support antctl command for packetcapture #6884

Conversation

hangyan commented Dec 24, 2024 • edited Loading

hangyan commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Feb 8, 2025

luolanzone commented Feb 11, 2025

hangyan commented Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangyan commented Dec 24, 2024 •

edited

Loading

hangyan commented Feb 12, 2025 •

edited

Loading