Support simple ping mesh in agent. #6120

IRONICBo · 2024-03-19T08:21:03Z

We need to implement a simple ping-mesh monitoring tool to get nodes connectivity latency.

Design doc(TODO): https://docs.google.com/document/d/1EdKJ8iQ3KwVBQAHaPisqHP7cgpq4RW8mD9KPWtYETbE

Issue: #5514

cmd/antrea-agent/agent.go

pkg/agent/monitortool/latency_store.go

go.mod

pkg/agent/monitortool/monitor.go

pkg/agent/monitortool/latency_store.go

pkg/config/agent/config.go

pkg/features/antrea_features.go

pkg/agent/monitortool/monitor.go

go.mod

pkg/agent/monitortool/monitor.go

IRONICBo · 2024-04-08T19:23:02Z

support to get gw0/podCIDR from node spec.
support singleton CRD in nodelatencymonitor.

Maybe I need to consider about the NetworkPolicyOnly mode by using Transport Address, now only support get IP from first CIDR IP.

I0408 19:16:11.801016       1 monitor.go:249] "Connection status" Connection={"FromIP":"10.244.2.1","ToIP":"10.244.0.1","Latency":0,"Status":false,"LastUpdated":"2024-04-08T19:16:11.800804713Z","CreatedAt":"0001-01-01T00:00:00Z"}
I0408 19:16:11.801040       1 monitor.go:249] "Connection status" Connection={"FromIP":"10.244.2.1","ToIP":"10.244.2.1","Latency":97881,"Status":true,"LastUpdated":"2024-04-08T19:16:11.80070513Z","CreatedAt":"0001-01-01T00:00:00Z"}
I0408 19:16:11.801057       1 monitor.go:249] "Connection status" Connection={"FromIP":"10.244.2.1","ToIP":"10.244.1.1","Latency":0,"Status":false,"LastUpdated":"2024-04-08T19:16:11.800928351Z","CreatedAt":"0001-01-01T00:00:00Z"}

build/charts/antrea/crds/nodelatencymonitor.yaml

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/monitor.go

pkg/agent/monitortool/latency_store_test.go

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/monitor.go

cmd/antrea-agent/agent.go

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/latency_store_test.go

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/latency_store_test.go

pkg/agent/monitortool/monitor.go

antoninbas · 2024-04-10T21:33:52Z

pkg/apis/crd/v1alpha2/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+type NodeLatencyMonitorList struct {
+	metav1.TypeMeta `json:",inline"`
+	// +optional
+	metav1.ListMeta `json:"metadata,omitempty"`
+
+	Items []NodeLatencyMonitor `json:"items"`
+}


For that kind of "singleton" CRD, does it make sense to support the List operation? Maybe we could do without?

@antoninbas My previous implementation didn't add a List, but it seems to have failed to generate it in make codegen, suggesting that the type of List needs to be supplied.

You can explicitly tell client-gen to only generate for get/watch etc., or to skip list https://gist.github.com/liangrog/1fc1cfbbfb0ef555729781c73caaef0b#file-k8s-code-gen-tags-L5

If we don't add a list, we can't use the informer interface directly to listen to CRD resources, and if we use watch directly, we will probably handle the events manually in the same way as cmd/antrea-agent-simulator/simulator.go.

It may be necessary to get the crd resource via get for the first time when the node is online, and then listen to the resource changes via watch.

I have added a comment for this NodeLatencyMonitorList struct.

pkg/apis/crd/v1alpha2/types.go

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/monitor.go

pkg/agent/monitortool/latency_store.go

IRONICBo · 2024-04-15T18:09:52Z

I will update my code later.

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/monitor.go

Dyanngg · 2024-04-17T18:54:31Z

pkg/apis/crd/v1alpha2/types.go

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+type NodeLatencyMonitorList struct {
+	metav1.TypeMeta `json:",inline"`
+	// +optional
+	metav1.ListMeta `json:"metadata,omitempty"`
+
+	Items []NodeLatencyMonitor `json:"items"`
+}


You can explicitly tell client-gen to only generate for get/watch etc., or to skip list https://gist.github.com/liangrog/1fc1cfbbfb0ef555729781c73caaef0b#file-k8s-code-gen-tags-L5

pkg/apis/crd/v1alpha2/types.go

pkg/agent/monitortool/monitor.go

pkg/apis/crd/v1alpha2/types.go

IRONICBo · 2024-04-22T18:02:23Z

Latest log:

I0422 18:01:41.173972       1 monitor.go:315] "Recv ICMP message" IP="10.244.0.1" SeqID=6 echo={"ID":1,"Seq":6,"Data":"MjAyNC0wNC0yMlQxODowMTo0MS4xNzM4MDI0MTZa"}
I0422 18:01:41.174009       1 monitor.go:366] "NodeIPLatency status" Key="10.244.0.1" Entry={"SeqID":6,"LastSendTime":"2024-04-22T18:01:41.173802416Z","LastRecvTime":"2024-04-22T18:01:26.173511212Z","LastMeasuredRTT":451565}
I0422 18:01:41.174042       1 monitor.go:366] "NodeIPLatency status" Key="10.244.1.1" Entry={"SeqID":4,"LastSendTime":"2024-04-22T18:01:41.173511596Z","LastRecvTime":"2024-04-22T18:01:26.177130478Z","LastMeasuredRTT":3727957}
I0422 18:01:41.174064       1 monitor.go:366] "NodeIPLatency status" Key="10.244.2.1" Entry={"SeqID":5,"LastSendTime":"2024-04-22T18:01:41.17371097Z","LastRecvTime":"2024-04-22T18:01:26.177815251Z","LastMeasuredRTT":4256801}

Signed-off-by: IRONICBo <boironic@gmail.com>

pkg/apis/controlplane/types.go

pkg/features/antrea_features.go

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/monitor.go

build/charts/antrea/crds/nodelatencymonitor.yaml

heanlan · 2024-04-24T21:48:25Z

Hi @antoninbas , do you know why the codecov bot is not running for this PR? @IRONICBo was asking about the codecov requirements for merging a PR

antoninbas · 2024-04-24T22:23:22Z

Hi @antoninbas , do you know why the codecov bot is not running for this PR? @IRONICBo was asking about the codecov requirements for merging a PR

I don't think there has been a successful CI run yet?
For example currently, even unit tests are failing.

IRONICBo · 2024-04-25T02:44:44Z

pkg/agent/monitortool/monitor.go

+	}
+
+	// Convert to NodeIPLatencyStats
+	nodeIPLatencyStats := &cpv1beta.NodeIPLatencyStat{


One node has dual IPs?

One node only contains a key(name?) to store latencies in map in agg.
map[nodename]NodeIPLatencyEntry

Put a nodename to NodeLatencyEntry struct.

Signed-off-by: IRONICBo <boironic@gmail.com>

pkg/agent/monitortool/monitor.go

antoninbas · 2024-05-01T18:46:47Z

pkg/agent/monitortool/monitor.go

+	// Notify the latency config changed
+	m.latencyConfigChanged <- struct{}{}


this may not be ideal if there is potential for blocking, as event handlers should not block

we can revisit later and potentially use a workqueue, no need to address this for this PR unless someone feels strongly about it

Yes, using the workqueue approach would be better than using the channel directly, I'll discuss and address this in other PRs.

pkg/agent/monitortool/monitor.go

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/monitor.go

antoninbas · 2024-05-01T21:18:45Z

pkg/agent/monitortool/monitor.go

+		case <-stopCh:
+			return
+		default:
+			readBuffer := make([]byte, 1500)


how did you determine the right buffer size here? There should be a comment

Ok, I have added the comments.

The comment is not accurate. The maximum size of an Ethernet frame is not 1500, that depends on the MTU.

We do however only expect small packets, because we only expect ICMP echo replies messages, and the size of the ICMP echo request messages we send is small. I am pretty sure that we don't expect messages larger than 128 bytes, so I would use a buffer with that size. If we get a larger message, the extra data will be discarded, which is fine.

Additionally, in our case it should be possible to reuse the same buffer for each loop iteration, as opposed to allocating a new buffer every time.

pkg/agent/monitortool/monitor.go

Signed-off-by: Asklv <boironic@gmail.com>

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/monitor.go

Signed-off-by: Asklv <boironic@gmail.com>

pkg/agent/monitortool/latency_store.go

pkg/agent/monitortool/monitor.go

IRONICBo · 2024-05-08T02:55:03Z

I have fixed the unit test error in my local machine.

antrea/pkg/agent/monitortool# go test -race ./...
ok      antrea.io/antrea/pkg/agent/monitortool  1.243s

Signed-off-by: Asklv <boironic@gmail.com>

antoninbas

As a general comment, avoid obvious code comments that are just a one sentence description of what the next line of code is doing, when it's already obvious from the code. It just makes everything more verbose and doesn't help visibility.

antoninbas · 2024-05-08T17:40:45Z

pkg/agent/monitortool/monitor.go

+// getICMPSeq returns the next sequence number as uint32,
+// wrapping around to 0 after reaching the maximum value of uint32.
+func getICMPSeq() uint32 {


I still think that this should return a uint16, given that the sequence number is a 2-byte value. You should do an explicit uint16 cast before returning newVal.

OK, I changed the return type.

antoninbas · 2024-05-08T17:41:18Z

pkg/agent/monitortool/monitor.go

+	latencyConfig *LatencyConfig
+	// latencyConfigChanged is the channel to notify the latency config changed.
+	latencyConfigChanged chan struct{}
+	// isIPv4Enabled is the flag to indicate if the IPv4 is enabled.


Suggested change

// isIPv4Enabled is the flag to indicate if the IPv4 is enabled.

// isIPv4Enabled is the flag to indicate whether IPv4 is enabled.

OK, I have fixed it.

antoninbas · 2024-05-08T17:41:30Z

pkg/agent/monitortool/monitor.go

+	latencyConfigChanged chan struct{}
+	// isIPv4Enabled is the flag to indicate if the IPv4 is enabled.
+	isIPv4Enabled bool
+	// isIPv6Enabled is the flag to indicate if the IPv6 is enabled.


Suggested change

// isIPv6Enabled is the flag to indicate if the IPv6 is enabled.

// isIPv6Enabled is the flag to indicate whether IPv6 is enabled.

OK, I have fixed it.

antoninbas · 2024-05-08T17:43:07Z

pkg/agent/monitortool/monitor.go

+	// The informer of Nodes, it will changed by Node watcher
+	nodeInformer coreinformers.NodeInformer
+	// nodeLatencyMonitorInformer is the informer for the NodeLatencyMonitor CRD.


these 2 comments are not really helpful. Unexpected fields don't always benefit from a field comment.

OK, I have fixed it.

antoninbas · 2024-05-08T17:43:58Z

pkg/agent/monitortool/monitor.go

+		nodeLatencyMonitorInformer: nlmInformer,
+	}
+
+	// Get the IPv4/IPv6 enabled status


not a useful comment

OK, I have fixed it.

antoninbas · 2024-05-08T18:08:42Z

pkg/agent/monitortool/latency_store.go

+	getNodeIPs, err := l.getNodeIPs(node)
+	if err != nil {
+		klog.ErrorS(err, "Failed to get IPs for Node", "nodeName", node.Name)
+		return
+	}


It would be much better not to rely on this for deletion. Ideally you should do your cleanup simply based on the Node name and your internal state.

Not addressed, we should not need to call l.getNodeIPs(node) in the deletion handler. We should do a lookup internally based on the Node name, and figure out what we need to clean up.

OK, I use nodeTargetIPsMap to retrieve target IPs and remove them.

antoninbas · 2024-05-08T18:14:25Z

pkg/agent/monitortool/latency_store.go

+	if len(podCIDRStrs) == 0 {
+		// Skip the Node if it does not have a PodCIDR.
+		err := errors.New("node does not have a PodCIDR")
+		klog.ErrorS(err, "Node does not have a PodCIDR", "Node name", node.Name)


As a rule, it is somewhat bad practice to log an error AND also return it, as it easily leads to duplicate error logs.

OK, I have removed all of these logs.

antoninbas · 2024-05-08T18:16:32Z

pkg/agent/monitortool/latency_store.go

+	}
+
+	if node.Spec.PodCIDR == "" {
+		// Does not help to return an error and trigger controller retries.


This comment was copied from node_route_controller.go, but it doesn't make sense in your case because you don't have a workqueue, so there is no retry mechanism. You should remove it.

Thanks, I have removed them.

antoninbas · 2024-05-08T18:17:11Z

pkg/agent/monitortool/latency_store.go

+}
+
+// CleanUp cleans up the latency store.
+func (l *LatencyStore) CleanUp() {


Can you point me to where this is called?

LatencyStore is cleaned up when crd is deleted, I have updated the code.

antoninbas · 2024-05-08T18:17:51Z

cmd/antrea-agent/agent.go

+	// Start the node latency monitor.
+	if features.DefaultFeatureGate.Enabled(features.NodeLatencyMonitor) {
+		nodeLatencyMonitor := monitortool.NewNodeLatencyMonitor(
+			nodeInformer,
+			nodeLatencyMonitorInformer,
+			nodeConfig,
+			networkConfig.TrafficEncapMode,
+		)
+		go nodeLatencyMonitor.Run(stopCh)
+	}


I took some time to think about it, and this should probably be gated by if o.nodeType == config.K8sNode. Check out the rest of this file for reference.

OK, I add the nodeType check.

Signed-off-by: Asklv <boironic@gmail.com>

IRONICBo changed the title ~~feat: support simple ping mesh in agent.~~ Support simple ping mesh in agent. Mar 19, 2024

This comment was marked as outdated.

Sign in to view

antoninbas reviewed Mar 19, 2024

View reviewed changes

IRONICBo commented Mar 21, 2024

View reviewed changes

pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved

antoninbas reviewed Mar 28, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

IRONICBo requested review from antoninbas and Dyanngg April 8, 2024 17:08

Dyanngg reviewed Apr 8, 2024

View reviewed changes

IRONICBo force-pushed the 5514-ping-mesh-monitoring-tool branch from 310d69b to b866203 Compare April 9, 2024 06:25

IRONICBo requested a review from Dyanngg April 9, 2024 18:47

heanlan reviewed Apr 10, 2024

View reviewed changes

Dyanngg reviewed Apr 10, 2024

View reviewed changes

antoninbas reviewed Apr 10, 2024

View reviewed changes

IRONICBo requested review from antoninbas, Dyanngg and heanlan April 17, 2024 17:44

Dyanngg reviewed Apr 17, 2024

View reviewed changes

antoninbas reviewed Apr 18, 2024

View reviewed changes

IRONICBo added 8 commits April 23, 2024 02:08

support simple ping mesh in agent

4332b60

Signed-off-by: IRONICBo <boironic@gmail.com>

feat: update feature config and ping tool.

ff8367c

Signed-off-by: IRONICBo <boironic@gmail.com>

feat: add singleton nodelatencymonitor crd define.

34e9943

Signed-off-by: IRONICBo <boironic@gmail.com>

feat: watch nodelatencymonitor crd to start or stop monitor.

430a100

Signed-off-by: IRONICBo <boironic@gmail.com>

feat: support get pod gw0 ip.

0794875

Signed-off-by: IRONICBo <boironic@gmail.com>

feat: support NetworkPolicyOnly and fix dual stack IPs.

2b475c0

Signed-off-by: IRONICBo <boironic@gmail.com>

feat: support singleton socket.

6492b83

Signed-off-by: IRONICBo <boironic@gmail.com>

fix: fix wrong node entry.

bb9e9cd

Signed-off-by: IRONICBo <boironic@gmail.com>

IRONICBo added 2 commits April 23, 2024 02:08

fix: tidy up nodeinformer.

3c8bb56

Signed-off-by: IRONICBo <boironic@gmail.com>

feat: refactor monitor loop.

6915393

Signed-off-by: IRONICBo <boironic@gmail.com>

IRONICBo force-pushed the 5514-ping-mesh-monitoring-tool branch from f6c7714 to 6915393 Compare April 22, 2024 18:19

antoninbas reviewed Apr 23, 2024

View reviewed changes

IRONICBo commented Apr 25, 2024

View reviewed changes

IRONICBo force-pushed the 5514-ping-mesh-monitoring-tool branch from 142eef7 to cea5be0 Compare April 26, 2024 23:21

fix: fix dual stack estimation.

3c1cfef

Signed-off-by: IRONICBo <boironic@gmail.com>

IRONICBo force-pushed the 5514-ping-mesh-monitoring-tool branch from cea5be0 to 3c1cfef Compare April 27, 2024 00:53

IRONICBo mentioned this pull request May 1, 2024

Export node latency query api. #6279

Open

antoninbas reviewed May 1, 2024

View reviewed changes

fix: fix latency store update function.

09a9615

Signed-off-by: Asklv <boironic@gmail.com>

IRONICBo requested review from antoninbas and Dyanngg May 3, 2024 08:26

antoninbas reviewed May 4, 2024

View reviewed changes

fix: remove seqID and fix comments.

80c6ec3

Signed-off-by: Asklv <boironic@gmail.com>

IRONICBo requested a review from antoninbas May 5, 2024 10:52

Dyanngg reviewed May 6, 2024

View reviewed changes

pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved

pkg/agent/monitortool/latency_store.go Outdated Show resolved Hide resolved

pkg/agent/monitortool/monitor.go Outdated Show resolved Hide resolved

IRONICBo force-pushed the 5514-ping-mesh-monitoring-tool branch 2 times, most recently from be5aa62 to b44469b Compare May 7, 2024 16:57

IRONICBo requested a review from Dyanngg May 7, 2024 16:57

fix: unexport unused function.

49074e0

Signed-off-by: Asklv <boironic@gmail.com>

IRONICBo force-pushed the 5514-ping-mesh-monitoring-tool branch from b44469b to 49074e0 Compare May 8, 2024 02:55

antoninbas reviewed May 8, 2024

View reviewed changes

IRONICBo requested a review from antoninbas May 9, 2024 02:02

fix: remove unused comments.

527a0b0

Signed-off-by: Asklv <boironic@gmail.com>

IRONICBo force-pushed the 5514-ping-mesh-monitoring-tool branch from 815f399 to 527a0b0 Compare May 9, 2024 17:23

		// Notify the latency config changed
		m.latencyConfigChanged <- struct{}{}

	// isIPv4Enabled is the flag to indicate if the IPv4 is enabled.
	// isIPv4Enabled is the flag to indicate whether IPv4 is enabled.

	// isIPv6Enabled is the flag to indicate if the IPv6 is enabled.
	// isIPv6Enabled is the flag to indicate whether IPv6 is enabled.

Support simple ping mesh in agent. #6120

Are you sure you want to change the base?

Support simple ping mesh in agent. #6120

Conversation

IRONICBo commented Mar 19, 2024

This comment was marked as outdated.

This comment was marked as resolved.

IRONICBo commented Apr 8, 2024

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

IRONICBo commented Apr 15, 2024

Choose a reason for hiding this comment

IRONICBo commented Apr 22, 2024

heanlan commented Apr 24, 2024

antoninbas commented Apr 24, 2024

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

IRONICBo commented May 8, 2024

antoninbas left a 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

IRONICBo May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IRONICBo May 9, 2024 • edited

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

IRONICBo May 9, 2024 • edited

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

IRONICBo May 9, 2024 •

edited

IRONICBo May 9, 2024 •

edited

IRONICBo May 9, 2024 •

edited