Added initialisation of creationTimestamp when creating instances of NodeLatencyStats . #6574

hkiiita · 2024-07-30T07:45:21Z

Attempts to fix issue #6569 about creationTimestamp field is always null in NodeLatencyStats.
Awaiting review.

antoninbas

Unit tests need to be updated.
They are probably failing at the moment. You can run them locally with go test -v ./pkg/apiserver/registry/stats/nodelatencystats/.
Unit tests should validate that when Create is invoked without a timestamp, the timestamp is added, and that when Create is invoked with a timestamp, the timestamp is not modified.

antoninbas · 2024-07-30T17:26:31Z

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

+	return &statsv1alpha1.NodeLatencyStats{
+		ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.Now()},
+	}


I don't think this change is needed or even makes sense. I would not modify New as part of this PR.

Yes, worked on it.

hkiiita · 2024-07-31T11:13:38Z

Unit tests need to be updated. They are probably failing at the moment. You can run them locally with go test -v ./pkg/apiserver/registry/stats/nodelatencystats/. Unit tests should validate that when Create is invoked without a timestamp, the timestamp is added, and that when Create is invoked with a timestamp, the timestamp is not modified.

Added new tests and refactored existing ones.

hkiiita · 2024-07-31T11:19:06Z

@antoninbas Please review.

antoninbas · 2024-07-31T16:43:47Z

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

+		summary     *statsv1alpha1.NodeLatencyStats
+		nodeName    string
+		expectedObj runtime.Object
+		err         error


this field is not used, please remove

antoninbas · 2024-07-31T16:44:16Z

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

+	tests := []struct {
+		name        string
+		summary     *statsv1alpha1.NodeLatencyStats
+		nodeName    string


this field is not used, please remove

antoninbas · 2024-07-31T16:50:07Z

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

+			} else {
+				assert.NotEqual(t, tt.expectedObj, obj)
+			}


this doesn't make sense IMO. Just because a timestamp is added by Create in this case doesn't mean that assert.NotEqual is the right thing to check. Especially when you have a field called tt.expectedObj, which implies that you are expecting the function to return a specific object.

usually the best way to deal with this is to add a clock.Clock (https://pkg.go.dev/k8s.io/utils/clock#Clock) to the object being tested (in this case, to the REST object). In the "normal" case, the clock is set to RealClock, but for tests you can set it to FakeClock. This way you can freeze time and predict what the timestamp will be / should be in the returned object. There are multiple examples in the Antrea code base, so you can look for them.

antoninbas · 2024-07-31T16:50:50Z

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

+		name        string
+		summary     *statsv1alpha1.NodeLatencyStats
+		nodeName    string
+		expectedObj runtime.Object


nit: I would use the *statsv1alpha1.NodeLatencyStats type for this as well

antoninbas · 2024-07-31T16:52:09Z

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

+		nodeName    string
+		expectedObj runtime.Object
+		err         error
+		timeAdded   bool


the name is a bit confusion IMO. timeAdded to me means that the time is added by the Create function, but that's not how you use it. I would suggest something less ambiguous, such as timestampMissing.

antoninbas · 2024-07-31T16:52:23Z

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

 }

 func TestRESTGet(t *testing.T) {
+	addedTime := metav1.Now()


rename to creationTimestamp

same comment for subsequent tests

antoninbas · 2024-08-01T17:03:55Z

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

@@ -16,6 +16,7 @@ package nodelatencystats

 import (
 	"context"
+	testing2 "k8s.io/utils/clock/testing"


testing2 is not a good name for an import, use clocktesting

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

antoninbas · 2024-08-01T17:10:30Z

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

@@ -28,29 +29,56 @@ import (
 	statsv1alpha1 "antrea.io/antrea/pkg/apis/stats/v1alpha1"
 )

+var (
+	fakeClock = testing2.NewFakeClock(time.Now())


it's better to have a separate clock instance for each test IMO, as some tests may modify the clock

antoninbas · 2024-08-02T17:24:27Z

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

+			if tt.name == "create with time not added" {
+				assert.Equal(t, metav1.Time{Time: fakeClock.Now()}, obj.(*statsv1alpha1.NodeLatencyStats).CreationTimestamp)
+
+			} else {
+				assert.Equal(t, tt.summary.CreationTimestamp, obj.(*statsv1alpha1.NodeLatencyStats).CreationTimestamp)
+			}


this is not ok. You should not modify the test logic based on the test case name, otherwise it defeats the purpose of having table-driven tests. You need to use expectedObj which you are ignoring at the moment.

antoninbas · 2024-08-02T17:24:47Z

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

 	ctx := context.Background()

 	_, err := r.Create(ctx, summary, nil, nil)
 	require.NoError(t, err)
 	obj, err := r.ConvertToTable(ctx, summary, nil)
 	require.NoError(t, err)
 	assert.Equal(t, expectedCells, obj.Rows[0].Cells)
-}
+}


don't remove the trailing newline character

this has not been addressed yet

antoninbas · 2024-08-02T17:26:22Z

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

-func NewREST() *REST {
+func NewREST(clock clock.Clock) *REST {


Given that the clock is only necessary for testing, let's use the following approach instead:

func NewREST() *REST { return newRESTWithClock(cloc.RealClock{}) } func newRESTWithClock(clock clock.Clock) *REST { return &REST{ indexer: cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}), clock: clock, } }

This way in apiserver.go you can keep calling NewREST() (no clock argument required) and in tests you can call newRESTWithClock.

hkiiita · 2024-08-05T09:59:30Z

Hi @antoninbas,

I understand and am aware that this has taken a while, i appreciate your patience as I've been refining the previous commit based on your guidance. I've worked on the changes, specifically addressing the use of fakeClock and its advancement in line with using expectedObj, as rightly pointed and suggested by you.

I encountered some confusion initially regarding the fakeClock functionality and how to detect its advancement. However, after reviewing the code base and running some local experiments, I believe I have arrived at a solution that aligns with your recommendations.

Request you to please review the updated commit.

Thanks

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

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

+				PeerNodeLatencyStats: nil,
+			},
+			expectedObj: &statsv1alpha1.NodeLatencyStats{
+				ObjectMeta:           metav1.ObjectMeta{Name: "node1", CreationTimestamp: metav1.Time{Time: fakeClock.Now().Add(2 * time.Minute)}},


This is not good because your test case assumes that it will run second and that the clock will have been advanced twice as a result. A key principle of table-driven test cases is that test cases should not depend on each other. They should be runnable individually, independently, in any order, and sometimes even in parallel.

The test should be like this:

func TestRESTCreate(t *testing.T) { ctx := context.Background() now := time.Now() const timeStep = 1*time.Minute tests := []struct { name string summary *statsv1alpha1.NodeLatencyStats expectedObj *statsv1alpha1.NodeLatencyStats }{ { name: "create with existing timestamp", summary: &statsv1alpha1.NodeLatencyStats{ ObjectMeta: metav1.ObjectMeta{Name: "node1", CreationTimestamp: metav1.Time{Time: now}}, PeerNodeLatencyStats: nil, }, expectedObj: &statsv1alpha1.NodeLatencyStats{ ObjectMeta: metav1.ObjectMeta{Name: "node1", CreationTimestamp: metav1.Time{Time: now}}, PeerNodeLatencyStats: nil, }, }, { name: "create with no existing timestamp", summary: &statsv1alpha1.NodeLatencyStats{ ObjectMeta: metav1.ObjectMeta{Name: "node1"}, PeerNodeLatencyStats: nil, }, expectedObj: &statsv1alpha1.NodeLatencyStats{ // the test case advances the clock by timeStep ObjectMeta: metav1.ObjectMeta{Name: "node1", CreationTimestamp: metav1.Time{Time: now.Add(timeStep)}}, PeerNodeLatencyStats: nil, }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { fakeClock := clocktesting.NewFakeClock(now) r := newRESTWithClock(fakeClock) fakeClock.SetTime(now.Add(timeStep)) obj, err := r.Create(ctx, tt.summary, nil, nil) require.NoError(t, err) assert.Equal(t, tt.expectedObj, obj) }) } }

you can use the code as is. I also updated the test case names for more clarity.

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

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

 	ctx := context.Background()

 	_, err := r.Create(ctx, summary, nil, nil)
 	require.NoError(t, err)
 	obj, err := r.ConvertToTable(ctx, summary, nil)
 	require.NoError(t, err)
 	assert.Equal(t, expectedCells, obj.Rows[0].Cells)
-}
+}


this has not been addressed yet

antoninbas · 2024-08-05T18:06:20Z

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

@@ -210,12 +242,12 @@ func TestRESTConvertToTable(t *testing.T) {
 	}
 	expectedCells := []interface{}{"node1", 2, "1.5ms", "2ms"}

-	r := NewREST()
+	r := newRESTWithClock(fakeClock)


I don't think this test needs a fake clock, so you can remove it

antoninbas · 2024-08-05T18:07:54Z

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

@@ -105,6 +134,7 @@ func TestRESTGet(t *testing.T) {
 }

 func TestRESTDelete(t *testing.T) {
+	fakeClock := clocktesting.NewFakeClock(time.Now())


follow the example I gave above: now := time.Now()

then in each subtest, create a separate clock with fakeClock := clocktesting.NewFakeClock(now)

antoninbas · 2024-08-05T18:09:03Z

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

 }

 func TestRESTGet(t *testing.T) {
+	fakeClock := clocktesting.NewFakeClock(time.Now())


follow the example I gave above: now := time.Now()

then in each subtest, create a separate clock with fakeClock := clocktesting.NewFakeClock(now)

hkiiita · 2024-08-06T16:49:20Z

Hi, @antoninbas request you to please have a look.

antoninbas

a couple of nits, otherwise, LGTM

antoninbas · 2024-08-06T17:38:02Z

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

+		t.Run(tt.name, func(t *testing.T) {
+			fakeClock := clocktesting.NewFakeClock(now)
+			r := newRESTWithClock(fakeClock)
+			fakeClock.SetTime(now.Add(timeStep))


Suggested change

fakeClock.SetTime(now.Add(timeStep))

fakeClock.Step(timeStep)

I may have forgotten to suggest it in my previous review

antoninbas · 2024-08-06T17:38:40Z

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

@@ -219,3 +253,4 @@ func TestRESTConvertToTable(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, expectedCells, obj.Rows[0].Cells)
 }
+


remove this extra line

antoninbas · 2024-08-06T17:40:29Z

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

@@ -156,20 +189,21 @@ func TestRESTDelete(t *testing.T) {
 }

 func TestRESTList(t *testing.T) {
+	fakeClock := clocktesting.NewFakeClock(time.Now())


nit: introduce now := time.Now() for consistency with other tests and less verbosity

hkiiita · 2024-08-07T09:57:59Z

@antoninbas Yes, did the changes, please check .

antoninbas

@hkiiita the code looks good to me but you have some CI failures (for code linting). You need to run make golangci-fix locally and commit the changes.

hkiiita · 2024-08-07T18:51:15Z

@antoninbas Thanks . Yes ran the command and committed.

antoninbas · 2024-08-07T18:51:59Z

/test-all

antoninbas · 2024-08-07T23:10:49Z

@hkiiita the DCO check is failing - please sign your commits as per our contribution guideline
It may be a good opportunity to squash all your commits as well.

- Initialized fakeClock for each test case. - Refactored NewRest function to prevent API breakage and updated tests. - Improved test logic and fixed lint issues. - Made various improvements for better code readability and efficiency. Signed-off-by: HEMANT KUMAR <hkbiet@gmail.com>

hkiiita · 2024-08-08T06:33:16Z

@antoninbas Thanks for it. Squashed the old commits into a single one.

antoninbas · 2024-08-08T16:51:21Z

/skip-all

antoninbas · 2024-08-08T18:15:38Z

Thanks for your contribution!

hkiiita · 2024-08-08T18:29:27Z

Thanks for your contribution!

@antoninbas Thanks to you for being so patient and helping. Really appreciate that . On the other hand i definitely ended up learning a couple of things from this . I will continue to learn more and get better with things .

antoninbas · 2024-08-08T18:32:29Z

@hkiiita if you intend to apply to the LFX mentorship program for this term's project, make sure to complete our test task: #6590

antoninbas reviewed Jul 30, 2024

View reviewed changes

hkiiita force-pushed the main branch from fdb72e5 to 21e3995 Compare July 31, 2024 11:15

antoninbas reviewed Jul 31, 2024

View reviewed changes

antoninbas reviewed Aug 1, 2024

View reviewed changes

hkiiita force-pushed the main branch from 1edc529 to 91b7af0 Compare August 2, 2024 13:04

antoninbas reviewed Aug 2, 2024

View reviewed changes

hkiiita force-pushed the main branch from 05c102d to 6265a26 Compare August 3, 2024 17:55

antoninbas reviewed Aug 5, 2024

View reviewed changes

antoninbas reviewed Aug 6, 2024

View reviewed changes

antoninbas reviewed Aug 7, 2024

View reviewed changes

antoninbas approved these changes Aug 7, 2024

View reviewed changes

hkiiita force-pushed the main branch from 54b9aeb to 11c7f08 Compare August 8, 2024 06:12

antoninbas merged commit 6e68789 into antrea-io:main Aug 8, 2024
55 of 58 checks passed

		func NewREST() *REST {
		func NewREST(clock clock.Clock) *REST {

	fakeClock.SetTime(now.Add(timeStep))
	fakeClock.Step(timeStep)

Added initialisation of creationTimestamp when creating instances of NodeLatencyStats . #6574

Added initialisation of creationTimestamp when creating instances of NodeLatencyStats . #6574

Conversation

hkiiita commented Jul 30, 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

hkiiita commented Jul 31, 2024

hkiiita commented Jul 31, 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

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

hkiiita commented Aug 5, 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

hkiiita commented Aug 6, 2024 • edited 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

hkiiita commented Aug 7, 2024

antoninbas left a comment

Choose a reason for hiding this comment

hkiiita commented Aug 7, 2024

antoninbas commented Aug 7, 2024

antoninbas commented Aug 7, 2024

hkiiita commented Aug 8, 2024

antoninbas commented Aug 8, 2024

antoninbas commented Aug 8, 2024

hkiiita commented Aug 8, 2024

antoninbas commented Aug 8, 2024

hkiiita commented Aug 5, 2024 •

edited

Loading

hkiiita commented Aug 6, 2024 •

edited

Loading