Add nodelatencystats rest apis implementation. #6479

IRONICBo · 2024-06-23T10:04:54Z

In last PR #6392, we have defined some rest apis for nodelatencystats.

We have implemented the inner operations by cache.Indexer and support query nodelatencystats by api.

Example:

$ kubectl get nodelatencystats
NODE NAME            NUM LATENCY ENTRIES   AVG LATENCY   MAX LATENCY
kind-control-plane   2                     7.110553ms    8.94447ms
kind-worker          2                     11.177585ms   11.508751ms
kind-worker2         2                     11.356675ms   15.265629ms

pkg/apiserver/registry/stats/nodelatencystats/rest.go

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

antoninbas

some of the comments in rest_test.go apply to all test cases, even though I only commented once. Please update all test cases as necessary.

pkg/apiserver/registry/stats/nodelatencystats/rest.go

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

antoninbas · 2024-07-24T18:33:04Z

pkg/apiserver/registry/stats/nodelatencystats/rest.go

+				if targetIPLatencyCount == 1 {
+					avgLatency = currentLatency
+				} else {
+					avgLatency += (currentLatency - avgLatency) / targetIPLatencyCount
+				}


you should do avgLatency += currentLatency here, and divide at the end to avoid rounding errors.
(not worried about an overflow given that this is an int64)

Ok, I've changed it to simple accumulation.

antoninbas · 2024-07-24T18:44:18Z

pkg/apiserver/registry/stats/nodelatencystats/rest.go

+			{Name: "Max Latency", Type: "integer", Format: "int64", Description: "Name of the peer with the highest latency and the latency value(ns)."},
+			{Name: "Avg Latency", Type: "integer", Format: "int64", Description: "Average latency value across all peers(ns)."},


It may be more user-friendly to make this a string.
To convert from the integral latency value (in ns) to the string, you would use: time.Duration(latency).String()
This way the column will be displayed as a value with a user-friendly unit (e.g., 10ns, 10µs, 10ms).
With this change, you can remove the unit specification ((ns)) from the column description.

Ok, using a formatted timestamp string is a better choice.

antoninbas · 2024-07-24T18:46:12Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()


There is no need to create a cancellable context, you should just use ctx := context.Background()

same comment for other tests

Ok, I've updated all of them.

antoninbas · 2024-07-24T18:46:40Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	obj, err := r.Create(ctx, summary, nil, nil)
+	assert.NoError(t, err)


Suggested change

assert.NoError(t, err)

require.NoError(t, err)

this one was not addressed

antoninbas · 2024-07-24T18:49:39Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+		expectedErr bool
+		err         error


You don't need the boolean (expectedErr). You can check err != nil in the test.

Ok, expectedErr has been removed.

antoninbas · 2024-07-24T19:01:22Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+			_, err := r.Create(ctx, tt.summary, nil, nil)
+			assert.Nil(t, err)


Suggested change

assert.Nil(t, err)

require.NoError(t, err)

Ok, I've updated all of them.

antoninbas · 2024-07-24T19:01:34Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+			defer cancel()
+
+			_, err := r.Create(ctx, tt.summary, nil, nil)
+			assert.NoError(t, err)


antoninbas · 2024-07-24T19:01:47Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	defer cancel()
+
+	_, err := r.Create(ctx, summary, nil, nil)
+	assert.Nil(t, err)


antoninbas · 2024-07-24T19:02:10Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	_, err := r.Create(ctx, summary, nil, nil)
+	assert.Nil(t, err)
+	objs, err := r.List(ctx, options)
+	assert.NoError(t, err)


Suggested change

assert.NoError(t, err)

require.NoError(t, err)

antoninbas · 2024-07-24T19:02:17Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	defer cancel()
+
+	_, err := r.Create(ctx, summary, nil, nil)
+	assert.Nil(t, err)


antoninbas · 2024-07-24T19:02:24Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	_, err := r.Create(ctx, summary, nil, nil)
+	assert.Nil(t, err)
+	obj, err := r.ConvertToTable(ctx, summary, nil)
+	assert.NoError(t, err)


antoninbas · 2024-07-24T19:03:28Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	assert.Nil(t, err)
+	obj, err := r.ConvertToTable(ctx, summary, nil)
+	assert.NoError(t, err)
+	assert.Equal(t, expectedObj, obj.Rows[0].Object.Object)


I don't think this test is doing anything
You need to populate PeerNodeLatencyStats with some data and validate the actual rows.

Ok, I populated PeerNodeLatencyStats in this case.

antoninbas · 2024-07-25T20:01:29Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	obj, err := r.Create(ctx, summary, nil, nil)
+	assert.NoError(t, err)


this one was not addressed

antoninbas · 2024-07-25T20:03:56Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	options := &internalversion.ListOptions{
+		Limit:    10,
+		Continue: "",
+	}


remove, we should use nil as the list options in this test given that we don't support them yet

Yes, it is be better to be removed.

antoninbas · 2024-07-25T20:04:56Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+	expectedObj := &statsv1alpha1.NodeLatencyStats{
+		ObjectMeta: metav1.ObjectMeta{Name: "node1"},
+		PeerNodeLatencyStats: []statsv1alpha1.PeerNodeLatencyStats{
+			{
+				NodeName: "node2",
+				TargetIPLatencyStats: []statsv1alpha1.TargetIPLatencyStats{
+					{
+						TargetIP:                   "192.168.0.1",
+						LastSendTime:               metav1.Time{Time: mockTime},
+						LastRecvTime:               metav1.Time{Time: mockTime},
+						LastMeasuredRTTNanoseconds: 0,
+					},
+				},
+			},
+		},
+	}


I don't think defining this object is useful, and I don't think the assert.Equal(t, expectedObj, obj.Rows[0].Object.Object) is very useful either.

Okay, the two parts are really the same and can be left out of the comparison.

antoninbas · 2024-07-25T20:05:50Z

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go

+		PeerNodeLatencyStats: []statsv1alpha1.PeerNodeLatencyStats{
+			{
+				NodeName: "node2",
+				TargetIPLatencyStats: []statsv1alpha1.TargetIPLatencyStats{
+					{
+						TargetIP:                   "192.168.0.1",
+						LastSendTime:               metav1.Time{Time: mockTime},
+						LastRecvTime:               metav1.Time{Time: mockTime},
+						LastMeasuredRTTNanoseconds: 0,
+					},
+				},
+			},
+		},


please add a second Node entry to this list, and use non-zero (and distinct) values for LastMeasuredRTTNanoseconds, for higher-quality testing

Signed-off-by: Asklv <[email protected]>

Signed-off-by: Antonin Bas <[email protected]>

antoninbas · 2024-07-25T22:58:58Z

/skip-all

antoninbas reviewed Jun 24, 2024

View reviewed changes

IRONICBo requested review from antoninbas and tnqn July 2, 2024 00:49

IRONICBo force-pushed the feat/impl-node-latency-query-api branch from 481f49c to 3e38af0 Compare July 2, 2024 08:23

antoninbas mentioned this pull request Jul 22, 2024

Release 2.1.0 #6536

Merged

antoninbas reviewed Jul 22, 2024

View reviewed changes

IRONICBo requested a review from antoninbas July 24, 2024 17:22

antoninbas added this to the Antrea v2.1 release milestone Jul 24, 2024

antoninbas reviewed Jul 24, 2024

View reviewed changes

IRONICBo requested a review from antoninbas July 25, 2024 09:10

antoninbas reviewed Jul 25, 2024

View reviewed changes

IRONICBo and others added 2 commits July 25, 2024 13:53

feat: add nodelatencystats rest apis implementation.

a7064c5

Signed-off-by: Asklv <[email protected]>

Address latest round of review comments

2d54451

Signed-off-by: Antonin Bas <[email protected]>

antoninbas force-pushed the feat/impl-node-latency-query-api branch from 1d3a874 to 2d54451 Compare July 25, 2024 21:01

antoninbas approved these changes Jul 25, 2024

View reviewed changes

antoninbas merged commit 4d9953f into antrea-io:main Jul 25, 2024
58 of 61 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nodelatencystats rest apis implementation. #6479

Add nodelatencystats rest apis implementation. #6479

IRONICBo commented Jun 23, 2024 •

edited by antoninbas

Loading

antoninbas left a comment

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024 •

edited

Loading

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 24, 2024

IRONICBo Jul 25, 2024

antoninbas Jul 25, 2024

antoninbas Jul 25, 2024

IRONICBo Jul 26, 2024

antoninbas Jul 25, 2024

IRONICBo Jul 26, 2024

antoninbas Jul 25, 2024

IRONICBo Jul 26, 2024

antoninbas commented Jul 25, 2024

		{Name: "Max Latency", Type: "integer", Format: "int64", Description: "Name of the peer with the highest latency and the latency value(ns)."},
		{Name: "Avg Latency", Type: "integer", Format: "int64", Description: "Average latency value across all peers(ns)."},

		ctx, cancel := context.WithCancel(context.Background())
		defer cancel()

Add nodelatencystats rest apis implementation. #6479

Add nodelatencystats rest apis implementation. #6479

Conversation

IRONICBo commented Jun 23, 2024 • edited by antoninbas Loading

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 Jul 25, 2024 • 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

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

Choose a reason for hiding this comment

antoninbas commented Jul 25, 2024

IRONICBo commented Jun 23, 2024 •

edited by antoninbas

Loading

IRONICBo Jul 25, 2024 •

edited

Loading