Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add e2e test for the NodeLatencyMonitor feature #6612

Merged
merged 14 commits into from
Sep 6, 2024

Conversation

Rupam-It
Copy link
Contributor

This PR adds end-to-end (e2e) tests for the NodeLatencyMonitor feature, which is currently available in Alpha as part of Antrea v2.1 (#6120).

Fixes #6549.

@Rupam-It Rupam-It force-pushed the add-node-latency-monitor-tests branch from 947491a to d3c1f7b Compare August 19, 2024 04:04
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All e2e tests go under test/e2e and are written in a specific way - you could say we have a "framework" that we use for all our e2e tests. Please refer to that folder for examples. There are a lot of test files you can look at, such as test/e2e/basic_test.go.

Copy link
Contributor Author

@Rupam-It Rupam-It left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored e2e Tests: Updated the e2e tests to align with the framework established in the test/e2e directory. I’ve restructured the tests to match the format used in test/e2e/basic_test.go, ensuring consistency across our test suite. These changes should address your feedback and improve alignment with our testing standards. Thanks for your guidance, @antoninbas .

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed e2e tests are not actually testing anything. You are just validating CRUD operations on the NodeLatencyMonitor CRD. These CRUD operations are provided by K8s, not Antrea, so in essence you are testing CRD support in K8s, and not testing the NodeLatencyMonitor feature in Antrea.
The issue for this task (#6549) describes what operations the e2e tests should cover.

Comment on lines 18 to 46
// GetKubeconfigPath returns the path to the kubeconfig file used for connecting to the Kubernetes cluster.
func GetKubeconfigPath() (string, error) {
// Check if the KUBECONFIG environment variable is set
kubeconfig := os.Getenv("KUBECONFIG")
if kubeconfig == "" {
// Default to the kubeconfig path in the user's home directory if not set
kubeconfig = filepath.Join(os.Getenv("HOME"), ".kube", "config")
}

// Verify that the kubeconfig file exists
if _, err := os.Stat(kubeconfig); os.IsNotExist(err) {
return "", fmt.Errorf("kubeconfig file does not exist at path: %s", kubeconfig)
}

return kubeconfig, nil
}

// getAntreaClient returns a clientset for interacting with Antrea CRDs
func getAntreaClient(t *testing.T) *clientset.Clientset {
kubeconfig, err := GetKubeconfigPath()
failOnError(err, t)

config, err := clientcmd.BuildConfigFromFlags("", kubeconfig)
failOnError(err, t)

antreaClientset, err := clientset.NewForConfig(config)
failOnError(err, t)
return antreaClientset
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to other e2e tests. The framework already provides access to clientsets for K8s / Antrea, and there is no need for these 2 functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okk, I understand. I'm doiing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hii @antoninbas i'm so close towards my goal but
getting this error in test file
nodelatencymonitor_test.go:143: Failed to retrieve NodeLatencyStats: the server could not find the requested resource (get nodelatencystats.crd.antrea.io)
also
kubectl get nodelatencystats
error: the server doesn't have a resource type "nodelatencystats"

is there any thing i have missed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodelatencystats.crd.antrea.io does not exist as NodeLatencyStats is not a CRD

kubectl get nodelatencystats should work fine unless you are using an older version of Antrea (prior to v2.1). You should be testing against the "latest" Antrea (main branch).

When running kubectl api-resources, you should see the following:

...
antreaclusternetworkpolicystats                stats.antrea.io/v1alpha1          false        AntreaClusterNetworkPolicyStats
antreanetworkpolicystats                       stats.antrea.io/v1alpha1          true         AntreaNetworkPolicyStats
multicastgroups                                stats.antrea.io/v1alpha1          false        MulticastGroup
networkpolicystats                             stats.antrea.io/v1alpha1          true         NetworkPolicyStats
nodelatencystats                               stats.antrea.io/v1alpha1          false        NodeLatencyStats
...

Copy link
Contributor Author

@Rupam-It Rupam-It left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas following the previous feedback, i have refactored the e2e test for nodelatencymonitor
feature to focus on functional testing of Antrea .

  • Validate the correct enabling and disabling of latency probes.
  • Ensure the PingIntervalSeconds is respected and updated as expected.
  • Check that latency stats are accurately recorded and retrieved, aligning with the intended behavior of the NodeLatencyMonitor feature in Antrea.

I think it will meet the necessary requirements.

nlm := &v1alpha1.NodeLatencyMonitor{
ObjectMeta: v1.ObjectMeta{
Name: nodeLatencyMonitorName,
Namespace: nodeLatencyMonitorNS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a cluster-scoped CRD, there should be no namespace

)

func TestNodeLatencyMonitor(t *testing.T) {
skipIfNotRequired(t)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why you included this? With a single argument, I believe this is a no-op

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

however, the test should be skipped if the feature (NodeLatencyMonitor) is disabled. We have the skipIfFeatureDisabled helper function for this.


data, err := setupTest(t)
if err != nil {
t.Fatalf("Failed to set up clients: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the error message doesn't seem accurate

t.Fatalf("Failed to delete NodeLatencyMonitor CR: %v", err)
}

waitForLatencyProbesDisabled(t, data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this really serve a purpose? The resource has no finalizers, so it should be deleted immediately, and I think there is no point in querying the API again to check for deletion. Is that no what you observed?

Comment on lines 83 to 91
func testEnableLatencyProbes(t *testing.T, data *TestData) {
waitForLatencyProbesEnabled(t, data)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the point of this test is?

Comment on lines 34 to 45
t.Run("testEnableLatencyProbes", func(t *testing.T) {
testEnableLatencyProbes(t, data)
})
t.Run("testRetrieveLatencyStats", func(t *testing.T) {
testRetrieveLatencyStats(t, data)
})
t.Run("testUpdatePingInterval", func(t *testing.T) {
testUpdatePingInterval(t, data)
})
t.Run("testDisableLatencyProbes", func(t *testing.T) {
testDisableLatencyProbes(t, data)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better if subtests are independent and don't assume a specific execution order

@Rupam-It Rupam-It force-pushed the add-node-latency-monitor-tests branch 2 times, most recently from 204c0cb to 58f8ad0 Compare August 28, 2024 09:47
Copy link
Contributor Author

@Rupam-It Rupam-It left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas i have made your desire changes .
I've run the sub tests independently, they works fine.

Comment on lines 25 to 27
func generatePingInterval() int32 {
return int32(rand.Intn(21) + 30)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no reason to use a random value for the ping interval

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"k8s.io/apimachinery/pkg/api/errors"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the import to v1 here has no effect as it is already the default name. I suggest renaming it to metav1 instead, which is what we usually do.

"testing"
"time"

v1alpha1 "antrea.io/antrea/pkg/apis/crd/v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming the import to v1alpha1 here has no effect as it is already the default name. I suggest renaming it to crdv1alpha1 instead, which is what we usually do.

}

func TestNodeLatencyMonitor(t *testing.T) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for an empty line here

test/e2e/nodelatencymonitor_test.go Outdated Show resolved Hide resolved

ctx := context.Background()

_, err := data.crdClient.StatsV1alpha1().NodeLatencyStats().Create(ctx, summary, v1.CreateOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a misunderstanding here. It is Antrea that should create the NodeLatencyStats object as a result of enabling the NodeLatencyMonitor functionality. The test case should not be creating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i have some confusion here.
can you provide me documentation of it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation for the feature is available here: https://github.com/antrea-io/antrea/blob/main/docs/feature-gates.md#nodelatencymonitor

The NodeLatencyMonitor CRD is to enable ICMP probes, which will measure latency between pairs of Nodes. The latency measurements are then reported by Antrea through the NodeLatencyStats API. The e2e test should validate that when enabling NodeLatencyMonitor, invoking the NodeLatencyStats API (GET) returns appropriate measurements.

Copy link
Contributor Author

@Rupam-It Rupam-It Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to clarify,
My implementation is almost right other than the nodeLatencyStats creation part?

Or the whole function is wrong ?

}
}

func testEnableLatencyProbes(t *testing.T, data *TestData) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not do anything. It just creates the CR (which only involves the K8s API), and it does not check that NodeLatencyStats are reported correctly.

I would recommend having a single test case in this PR, which will

  1. create the CR, with a ping interval of 10s
  2. poll until NodeLatencyStats are reported correctly
  3. delete the CR

We can improve testing over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, as the flow of test case

  1. create the CR, with a ping interval of 10s

  2. delete the CR
    this are implemented

  3. poll until NodeLatencyStats are reported correctly
    functionality i have do to correction.

update ping Interval will be deleted from here..
right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have inverted steps 2 and 3

update ping Interval will be deleted from here..

yes, and you should only have one test, not 4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okk

@Rupam-It Rupam-It force-pushed the add-node-latency-monitor-tests branch from 3e765c9 to 321dbdc Compare August 29, 2024 09:27
Copy link
Contributor Author

@Rupam-It Rupam-It left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @antoninbas , this changes in test surely match with requirements.

t.Logf("NodeLatencyMonitor CR created successfully.")

// 2: Poll until(5min) NodeLatencyStats are reported correctly
ctx := context.Background()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use context.Background() here when you use context.TODO() for other API calls?


// 2: Poll until(5min) NodeLatencyStats are reported correctly
ctx := context.Background()
err = wait.PollImmediate(time.Second, 300*time.Second, func() (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what kind of checks you are doing in the condition function. You are computing max latency and average latency, but then you just log them? This is the test so it should validate the contents of the NodeLatencyStats objects to make sure they are correct.

I would suggest the following:

  • a first call to wait.PollImmediate, with a 30s timeout (after all the ping interval is set to 10s). This should validate that the NodeLatencyStats objects are available. There should be one per Node, and for each one, there should be one entry in PeerNodeLatencyStats for each Node. You can also make sure that TargetIPLatencyStats has the correct size (1 if cluster is IPv4-only or IPv6-only, 2 if cluster is dual-stack, and that each latency number is strictly positive.
  • a second call to wait.PollImmediate, again with a 30s timeout, which validates that the stats are refreshed after some time. You should perform the same validations as for the first call to wait.PollImmediate, but also check that all the LastSendTime and LastRecvTime fields have been updated.

Copy link
Contributor Author

@Rupam-It Rupam-It left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas i've done the recommended changes.

return false, err
}

if len(statsList.Items) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the length should match the number of Nodes in the cluster

}

for _, item := range statsList.Items {
if len(item.PeerNodeLatencyStats) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, the length should match the number of Nodes in the cluster

return false, nil
}
for _, peerStat := range item.PeerNodeLatencyStats {
if len(peerStat.TargetIPLatencyStats) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please read #6612 (comment) again, the expected length depends on the type of cluster

}
for _, targetStat := range peerStat.TargetIPLatencyStats {
if targetStat.LastMeasuredRTTNanoseconds <= 0 {
t.Logf("Invalid(negetive) RTT for peer %s on node %s", peerStat.NodeName, item.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
t.Logf("Invalid(negetive) RTT for peer %s on node %s", peerStat.NodeName, item.Name)
t.Logf("Invalid RTT for peer %s reported by Node %s", peerStat.NodeName, item.Name)

test/e2e/nodelatencymonitor_test.go Outdated Show resolved Hide resolved
Comment on lines 105 to 106
previousSendTime = targetStat.LastSendTime
previousRecvTime = targetStat.LastRecvTime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "previous" values must come from the previous poll. We need a previous value for each pair of Node, so you will need a map for this.

Copy link
Contributor Author

@Rupam-It Rupam-It left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas I've refine the nodelatencystats test.

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the structure of the test looks good now, so all the new comments are suggestions for smaller improvements

Comment on lines 26 to 28
nodes, err := data.clientset.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{})
require.NoError(t, err, "Failed to list nodes")

var isDualStack bool
for _, node := range nodes.Items {
if node.Spec.PodCIDR != "" && strings.Contains(node.Spec.PodCIDR, ":") {
isDualStack = true
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should be using clusterInfo.podV4NetworkCIDR and clusterInfo.podV4NetworkCIDR to determine this

we already have a framework that performs the correct checks for you

t.Logf("NodeLatencyMonitor CR created successfully.")

validateNodeLatencyStats := func(statsList *v1alpha1.NodeLatencyStatsList, initialPoll bool, previousTimes map[string]map[string]metav1.Time) (bool, error) {
if len(statsList.Items) != len(nodes.Items) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clusterInfo.numNodes stores the number of Nodes, it is populated by the framework

require.NoError(t, err, "Failed to create NodeLatencyMonitor CR")
t.Logf("NodeLatencyMonitor CR created successfully.")

validateNodeLatencyStats := func(statsList *v1alpha1.NodeLatencyStatsList, initialPoll bool, previousTimes map[string]map[string]metav1.Time) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the previousTimes map declaration before the closure definition, and remove the previousTimes parameter


for _, item := range statsList.Items {
if len(item.PeerNodeLatencyStats) != len(nodes.Items)-1 {
t.Logf("Expected %d PeerNodeLatencyStats for node %s, but found %d, retrying...", len(nodes.Items), item.Name, len(item.PeerNodeLatencyStats))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invalid log message, the expected number of len(nodes.Items)-1, not len(nodes.Items)


for _, item := range statsList.Items {
if len(item.PeerNodeLatencyStats) != len(nodes.Items)-1 {
t.Logf("Expected %d PeerNodeLatencyStats for node %s, but found %d, retrying...", len(nodes.Items), item.Name, len(item.PeerNodeLatencyStats))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: in Antrea we capitalize all K8s terms (such as Node) in comments and log messages

previousTimes[item.Name][peerStat.NodeName] = targetStat.LastSendTime
} else {
previousSendTime, sendTimeExists := previousTimes[item.Name][peerStat.NodeName]
if !sendTimeExists || previousSendTime.Equal(&targetStat.LastSendTime) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for the check, I think it would be slightly better to validate that targetStat.LastSendTime > previousSendTime

return false, err
}

if len(statsList.Items) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems redundant, given the checks already in validateNodeLatencyStats, so maybe we could remove it?

return false, err
}

if len(statsList.Items) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

err = wait.PollImmediate(time.Second, 30*time.Second, func() (bool, error) {
statsList, err := data.crdClient.StatsV1alpha1().NodeLatencyStats().List(context.TODO(), metav1.ListOptions{})
if err != nil {
t.Logf("Error while listing NodeLatencyStats: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for this log message

err = wait.PollImmediate(time.Second, 30*time.Second, func() (bool, error) {
statsList, err := data.crdClient.StatsV1alpha1().NodeLatencyStats().List(context.TODO(), metav1.ListOptions{})
if err != nil {
t.Logf("Error while listing NodeLatencyStats: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

@Rupam-It Rupam-It left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas I've removed the redundant codes & implements Antrea framework more efficiently.

Comment on lines 97 to 101
valid, validateErr := validateNodeLatencyStats(statsList, true)
if !valid {
return false, validateErr
}
return true, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace these 5 lines with return validateNodeLatencyStats(statsList, true)

Comment on lines 111 to 115
valid, validateErr := validateNodeLatencyStats(statsList, false)
if !valid {
return false, validateErr
}
return true, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace these 5 lines with return validateNodeLatencyStats(statsList, false)

test/e2e/nodelatencymonitor_test.go Outdated Show resolved Hide resolved
test/e2e/nodelatencymonitor_test.go Outdated Show resolved Hide resolved
Comment on lines 121 to 123
err = data.crdClient.CrdV1alpha1().NodeLatencyMonitors().Delete(context.TODO(), "default", metav1.DeleteOptions{})
require.NoError(t, err, "Failed to delete NodeLatencyMonitor CR")
t.Logf("NodeLatencyMonitor CR deleted successfully.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that this should be a defer statement just after the Create operation succeeds. Otherwise the test may fail (e.g., during one of the poll) and then the CR will not be deleted and will persist after the test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes , you are right .
I'm changing.

@antoninbas antoninbas changed the title test: Add e2e Tests for NodeLatencyMonitor Feature title Add e2e test for the NodeLatencyMonitor feature Sep 3, 2024
Copy link
Contributor Author

@Rupam-It Rupam-It left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoninbas I have added he defer function that will surely delete nodeltancymonitor cr(if created) after test fail or success

@antoninbas
Copy link
Contributor

@Rupam-It The DCO check is failing because at least one of your commits is not signed. This may be a good opportunity to squash all your commits and sign the one resulting commit.

Rupam-It and others added 8 commits September 4, 2024 00:11
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
… tests for NodeLatencyMonitor resources to ensure better clarity and functionality.

Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Rupam-It and others added 4 commits September 4, 2024 00:11
…me work

Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Co-authored-by: Antonin Bas <antonin.bas@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Co-authored-by: Antonin Bas <antonin.bas@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
Signed-off-by: Rupam-It <mannarupam3@gmail.com>
@Rupam-It Rupam-It force-pushed the add-node-latency-monitor-tests branch from 8554278 to 3b0d16f Compare September 3, 2024 18:41
@Rupam-It
Copy link
Contributor Author

Rupam-It commented Sep 3, 2024

@antoninbas Thank you so much for your patience and guidance throughout this process. Can you suggest me next issue to work on ?

@antoninbas
Copy link
Contributor

@Rupam-It a few CI checks are failing:

  • the new file is missing a copyright header
  • the code linting tool is reporting some issues (you can run make golangci locally to check the issues and confirm that you have fixed them)

@Rupam-It
Copy link
Contributor Author

Rupam-It commented Sep 5, 2024

Okk, I'm doing

Signed-off-by: Rupam-It <mannarupam3@gmail.com>
@Rupam-It
Copy link
Contributor Author

Rupam-It commented Sep 6, 2024

hey @antoninbas i have add the copywrite headers also change in code that passes CI checks .

package e2e

import (
// Standard library imports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a helpful comment, please remove it

@@ -1,16 +1,32 @@
// Copyright 2020 Antrea Authors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the date is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Signed-off-by: Rupam-It <mannarupam3@gmail.com>
@Rupam-It
Copy link
Contributor Author

Rupam-It commented Sep 6, 2024

I think this changes will works fine .

@antoninbas
Copy link
Contributor

/test-e2e

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thank you for your contribution @Rupam-It !

@antoninbas
Copy link
Contributor

/skip-conformance
/skip-networkpolicy

@antoninbas antoninbas merged commit 66c5b73 into antrea-io:main Sep 6, 2024
52 of 57 checks passed
@Rupam-It
Copy link
Contributor Author

Rupam-It commented Sep 7, 2024

@antoninbas Thank you for your continuous guidance and patience throughout the process. I truly appreciate your support !
see you in next issue.

@Rupam-It Rupam-It deleted the add-node-latency-monitor-tests branch September 12, 2024 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add e2e tests for the NodeLatencyMonitor feature
2 participants