Add e2e test case for SecondaryNetwork of SR-IOV type #6922

wenqiq · 2025-01-14T16:40:14Z

Add a script to create a Kubernetes testbed on AWS VM Nodes with SR-IOV interface configured.
Add e2e test codes for SR-IOV case to validate the traffic between two Pods with secondary network enabled.
Add a Jenkins job named cloud-antrea-test-aws-sriov-secondary-network in Jenkins service, and add github comment trigger to trigger the e2e tests for SecondaryNetwork with SR-IOV on demand.

The complete test process record:
https://jenkins.antrea.io/job/cloud-antrea-test-aws-sriov-secondary-network/16/

ci/aws/test-sr-iov-secondary-network-aws.sh

ci/jenkins/jobs/projects-cloud.yaml

luolanzone · 2025-01-21T08:23:43Z

test/e2e-secondary-network/infra/sriov-secondary-networks.yml

+  - cidr: "172.31.22.0/24"
+  subnetInfo:
+    gateway: "172.31.16.1"


Where are these IPs from? or it's totally a private CIDR which can be defined by us without any limitation?

test/e2e/framework.go

ci/aws/assign_ip_to_instance_networkinterface.sh

ci/aws/test-sriov-secondary-network-aws.sh

ci/aws/assign_ip_to_instance_networkinterface.sh

test/e2e-secondary-network/secondary_network_test.go

tnqn · 2025-01-22T14:18:57Z

test/e2e-secondary-network/secondary_network_test.go

+	t.Run("testCreateTestPodOnNode", func(t *testing.T) {
+		err := testData.createPods(t, e2eTestData.GetTestNamespace())
+		assert.NoError(t, err)
+	})
+	t.Run("testAssignPodIP", func(t *testing.T) {
+		err := testData.assignIP()
+		if err != nil {
+			t.Fatalf("Error when assign IP to ec2 instance: %v", err)
+		}
+	})
+	t.Run("testpingBetweenInterfaces", func(t *testing.T) {


These are 3 steps of a single test, shouldn't be 3 tests.

Updated. I saw the testSecondaryNetwork function use two tests and updated them.

ci/aws/test-sriov-secondary-network-aws.sh

antoninbas · 2025-01-30T23:50:52Z

ci/aws/test-sriov-secondary-network-aws.sh

+install_kubernetes() {
+    local node_ip=$1
+    echo "Installing Kubernetes on node $node_ip..."
+    ssh -o StrictHostKeyChecking=no -i "$AWS_EC2_SSH_KEY_NAME" ubuntu@$node_ip << EOF


would it be realistic to use the Ansible playbook we already have (over SSH) instead of having that long list of commands?

It seems that integrating Ansible involves significant changes and requires considerable effort. Given the scope of these modifications, it would be more efficient to implement them in a separate pull request (PR).

ci/aws/test-sriov-secondary-network-aws.sh

test/e2e-secondary-network/secondary_network_test.go

test/e2e/framework.go

antoninbas · 2025-01-31T00:25:37Z

test/e2e-secondary-network/secondary_network_test.go

+	return out.String(), nil
+}
+
+func (data *testData) assignIP() error {


it would be good if we could find a way to "isolate" the AWS-specific stuff in a single function and have the test script provide an "aws" flag when running the tests.

The function can iterate over the Pods, retrieve the Node, lookup the eni label and perform the IP assignment, all in one place. We don't need to build the mapping from Pod to eni ahead of time.

antoninbas · 2025-01-31T00:28:42Z

test/e2e-secondary-network/secondary_network_test.go

+}
+
+// createNetworkAttachmentDefinition creates a NetworkAttachmentDefinition in the specified namespace.
+func createNetworkAttachmentDefinition(client networkclient.Interface, namespace, name, config string) error {


could we let the script create the NetworkAttachmentDefinition with kubectl, like we do in ci/kind/test-secondary-network-kind.sh. This is also how we create the IPPool. I don't see anything "dynamic" here so I am not sure why we are doing it programmatically?

We should probably follow the existing model and create it in the default namespace?

Since the Namespace where the test Pod is located is randomly generated, the NetworkAttachmentDefinition needs to be in the same Namespace. Otherwise, it won`t work.

go.mod

XinShuYang · 2025-02-12T05:48:39Z

ci/test-sriov-secondary-network-aws.sh

+    RUN_ALL=false
+    shift
+    ;;
+    --cleanup-only)


Is there another cleanup job in job yaml?

There is no separate cleanup job. When the cleanup-only parameter is specified, it allows for deleting specified EC2 resources in an AWS cluster.

Then how can we ensure that the cleanup task always runs, regardless of whether the test function exits as expected? I didn't see any trap-like design either. We should prevent cloud resource leaks especially for unexpected job failure, as they may cause continuous costs.

When running tests, the EC2 resources are automatically deleted if they succeed. However, if the tests fail, it's necessary to retain the testing environment for troubleshooting purposes and manually delete the EC2 resources afterward.

This isn't the normal way to run Antrea CI, we had related discussion in #5734 (comment) . I’m not sure if I missed any discussion about this feature, which allows users to log in to the testbed for debugging instead of collecting logs? And it's difficult to ensure that the developer can manually delete the resource each time unexpected failures occur. cc @tnqn @antoninbas

Please make sure that the clean up always happens by default, even when the test fails. Otherwise we are very likely to leak resources in the AWS account when running the Jenkins job.
It is ok to request for clean up to be skipped when running the script manually. That should already be possible , as I can do:

./ci/test-sriov-secondary-network-aws.sh --setup-only # Inspect the testbed, maybe run the tests manually with go test again ./ci/test-sriov-secondary-network-aws.sh --cleanup-only

test/e2e-secondary-network/secondary_network_test.go

test/e2e-secondary-network/framework.go

antoninbas · 2025-02-12T23:31:35Z

test/e2e-secondary-network/secondary_network_test.go

+			"ippools": ["pool1"]
+		}
+	}`
+	err = awsTestData.createNetworkAttachmentDefinition(e2eTestData.GetTestNamespace(), "sriov-net1", configJSON)


please revisit my earlier comment about creating the NetworkAttachmentDefinition from sriov-secondary-networks.yml and removing createNetworkAttachmentDefinition. I don't see any reason why this would work for the VLAN tests and not for the SRIOV tests. It should be possible to have the NetworkAttachmentDefinition in the default Namespace and the Pods in the test Namespace. This is what we have for the VLAN e2e tests, and the "k8s.v1.cni.cncf.io/networks" annotation should work the same independently of the network type.

XinShuYang · 2025-02-13T05:08:40Z

ci/jenkins/jobs/projects-cloud.yaml

@@ -1608,3 +1608,60 @@
                default-excludes: true
                fingerprint: false
                only-if-success: false
+      - 'cloud-{name}-{test_name}':
+          disabled: false
+          triggers: []


Is this Jenkins job triggered by GitHub comments or a timer?

Given that this is a relatively resource-intensive pipeline, triggering it via comments or scheduled tasks may not be ideal. Would it be more appropriate for the project maintainers to trigger it as needed? cc @antoninbas @tnqn

I'll let @luolanzone comment as well but maybe we could the following:

run the test weekly - of course it's very important for resource cleanup to always work consistently as we don't want to leak EC2 instances

trigger the job manually from the Jenkins UI prior to merging a PR which impacts secondary network support, and potentially prior to releasing a new Antrea version

I would prefer to have a comment trigger to allow users to trigger the e2e tests on demand. Can we validate who trigger it and only pass through when the name is in the whitelist If we are concerning about the resource impact?
@XinShuYang any suggestion for the name validator?

ci/test-sriov-secondary-network-aws.sh

antoninbas

More comments, in particular for the bash script, but the Go code is mostly looking good to me

antoninbas · 2025-02-14T23:18:46Z

ci/jenkins/jobs/projects-cloud.yaml

@@ -1608,3 +1608,60 @@
                default-excludes: true
                fingerprint: false
                only-if-success: false
+      - 'cloud-{name}-{test_name}':
+          disabled: false
+          triggers: []


I'll let @luolanzone comment as well but maybe we could the following:

run the test weekly - of course it's very important for resource cleanup to always work consistently as we don't want to leak EC2 instances

trigger the job manually from the Jenkins UI prior to merging a PR which impacts secondary network support, and potentially prior to releasing a new Antrea version

pkg/flowaggregator/s3uploader/s3uploader_test.go

test/e2e-secondary-network/providers/aws/ec2.go

test/e2e-secondary-network/secondary_network_test.go

antoninbas · 2025-02-14T23:29:38Z

ci/test-sriov-secondary-network-aws.sh

+    RUN_ALL=false
+    shift
+    ;;
+    --cleanup-only)


Please make sure that the clean up always happens by default, even when the test fails. Otherwise we are very likely to leak resources in the AWS account when running the Jenkins job.
It is ok to request for clean up to be skipped when running the script manually. That should already be possible , as I can do:

./ci/test-sriov-secondary-network-aws.sh --setup-only # Inspect the testbed, maybe run the tests manually with go test again ./ci/test-sriov-secondary-network-aws.sh --cleanup-only

ci/test-sriov-secondary-network-aws.sh

antoninbas · 2025-02-14T23:35:08Z

ci/test-sriov-secondary-network-aws.sh

+    chmod -R g-w build/images/ovs
+    chmod -R g-w build/images/base


we should not have to run these commands on the local machine. What are they for? Are they required for Jenkins?

cc @XinShuYang

@antoninbas It seems they were first introduced in #2009. Do you still remember the reason behind it? According to the comments, it was implemented to address potential Docker caching issues.

ci/test-sriov-secondary-network-aws.sh

XinShuYang · 2025-02-18T04:10:34Z

ci/jenkins/jobs/projects-cloud.yaml

@@ -1608,3 +1608,137 @@
                default-excludes: true
                fingerprint: false
                only-if-success: false
+      - 'cloud-{name}-{test_name}-aws':


We can use cloud-{name}-{test_name}-cleanup for all related templates.

The template uses the CLUSTERNAME to delete a cluster, which is not working for deleting the EC2 in AWS.

- job-template: name: 'cloud-{name}-{test_name}-cleanup' node: '{node}' triggers: '{triggers}' builders: '{builders}' parameters: - string: default: '' description: The cluster name of the last finished build. name: CLUSTERNAME trim: 'false'

ci/jenkins/jobs/projects-cloud.yaml

ci/jenkins/jobs/job-templates.yaml

luolanzone · 2025-02-12T03:14:52Z

ci/jenkins/jobs/projects-cloud.yaml

@@ -1608,3 +1608,60 @@
                default-excludes: true
                fingerprint: false
                only-if-success: false
+      - 'cloud-{name}-{test_name}':


What's the comment to trigger this job? I didn't see it's being defined

test-sriov-secondary-network-e2e
https://jenkins.antrea.io/job/antrea-sriov-secondary-network-e2e-for-pull-request/2

luolanzone · 2025-02-17T03:01:04Z

ci/jenkins/jobs/projects-cloud.yaml

@@ -1608,3 +1608,60 @@
                default-excludes: true
                fingerprint: false
                only-if-success: false
+      - 'cloud-{name}-{test_name}':
+          disabled: false
+          triggers: []


I would prefer to have a comment trigger to allow users to trigger the e2e tests on demand. Can we validate who trigger it and only pass through when the name is in the whitelist If we are concerning about the resource impact?
@XinShuYang any suggestion for the name validator?

ci/jenkins/jobs/projects-cloud.yaml

pkg/flowaggregator/s3uploader/s3uploader_test.go

test/e2e-secondary-network/secondary_network_test.go

ci/test-sriov-secondary-network-aws.sh

wenqiq · 2025-02-19T06:02:08Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-19T07:20:24Z

/test-sriov

XinShuYang · 2025-02-19T10:05:01Z

/test-sriov-secondary-network-e2e

XinShuYang · 2025-02-19T10:08:09Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-19T11:34:55Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-19T11:57:29Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-19T15:37:41Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-19T16:26:37Z

/test-sriov-secondary-network-e2e

XinShuYang · 2025-02-20T06:51:31Z

ci/jenkins/jobs/projects-cloud.yaml

+          org_list: '{antrea_org_list}'
+          white_list: '{antrea_white_list}'
+          only_trigger_phrase: true
+          trigger_permit_all: true


trigger_permit_all: true means all users can trigger this job.

Thanks. Updated.

luolanzone

LGTM overall, a few nits

ci/test-sriov-secondary-network-aws.sh

luolanzone · 2025-02-20T08:55:33Z

ci/test-sriov-secondary-network-aws.sh

+# Function to install Kubernetes on a node
+function install_kubernetes() {
+    local node_ip=$1
+    until ssh -o StrictHostKeyChecking=no -i "$AWS_EC2_SSH_KEY_NAME" "ubuntu@$node_ip" exit; do


Looks like the script will be blocked here if there is a ssh issue, please consider to use timeout and fail the installation if a reasonable time is passed.

ci/test-sriov-secondary-network-aws.sh

luolanzone · 2025-02-20T09:07:50Z

@antoninbas @tnqn can you take a look again? Thanks

wenqiq · 2025-02-20T13:47:28Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-20T14:13:17Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-20T14:30:54Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-20T14:43:29Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-20T14:48:39Z

/test-sriov-secondary-network-e2e

wenqiq · 2025-02-20T14:57:52Z

/test-sriov-secondary-network-e2e

Signed-off-by: Wenqi Qiu <[email protected]>

wenqiq · 2025-02-20T15:50:01Z

/test-sriov-secondary-network-e2e

luolanzone added this to the Antrea v2.3 release milestone Jan 15, 2025

luolanzone added the area/secondary-network Issues or PRs related to support for secondary networks in Antrea label Jan 15, 2025

wenqiq force-pushed the add-sriov-e2e branch 4 times, most recently from 758c687 to 1f77538 Compare January 20, 2025 03:06

wenqiq changed the title ~~[WIP]Add e2e test case for SecondaryNetwork of SR-IOV type~~ Add e2e test case for SecondaryNetwork of SR-IOV type Jan 20, 2025

wenqiq mentioned this pull request Jan 20, 2025

Add e2e for SecondaryNetwork with SR-IOV #6915

Open

wenqiq marked this pull request as ready for review January 21, 2025 00:59

luolanzone requested review from antoninbas and tnqn January 21, 2025 07:33

luolanzone reviewed Jan 21, 2025

View reviewed changes

tnqn reviewed Jan 22, 2025

View reviewed changes

wenqiq force-pushed the add-sriov-e2e branch 2 times, most recently from aea2610 to 62000e7 Compare January 27, 2025 08:04

antoninbas reviewed Jan 31, 2025

View reviewed changes

luolanzone reviewed Feb 10, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

wenqiq force-pushed the add-sriov-e2e branch 2 times, most recently from 277ec59 to b5e18b1 Compare February 11, 2025 05:37

XinShuYang reviewed Feb 12, 2025

View reviewed changes

wenqiq force-pushed the add-sriov-e2e branch from c0b1757 to 4b2a25a Compare February 12, 2025 06:02

antoninbas reviewed Feb 12, 2025

View reviewed changes

wenqiq force-pushed the add-sriov-e2e branch from 9299b31 to 0610d8c Compare February 13, 2025 03:42

XinShuYang reviewed Feb 13, 2025

View reviewed changes

ci/test-sriov-secondary-network-aws.sh Show resolved Hide resolved

wenqiq force-pushed the add-sriov-e2e branch from 55a8ec8 to 4508b2e Compare February 13, 2025 11:53

antoninbas reviewed Feb 14, 2025

View reviewed changes

antoninbas mentioned this pull request Feb 15, 2025

[SecondaryNetwork] Require resourceName annotations for SRIOV #6999

Merged

wenqiq force-pushed the add-sriov-e2e branch 2 times, most recently from 073f95c to 323e957 Compare February 18, 2025 00:00

XinShuYang reviewed Feb 18, 2025

View reviewed changes

ci/jenkins/jobs/projects-cloud.yaml Outdated Show resolved Hide resolved

XinShuYang reviewed Feb 18, 2025

View reviewed changes

ci/jenkins/jobs/job-templates.yaml Outdated Show resolved Hide resolved

luolanzone reviewed Feb 18, 2025

View reviewed changes

wenqiq force-pushed the add-sriov-e2e branch from 0ab381e to 614ae06 Compare February 19, 2025 16:26

XinShuYang reviewed Feb 20, 2025

View reviewed changes

wenqiq force-pushed the add-sriov-e2e branch from 614ae06 to 2d21d24 Compare February 20, 2025 07:57

luolanzone reviewed Feb 20, 2025

View reviewed changes

wenqiq force-pushed the add-sriov-e2e branch from a93c5d2 to a31d393 Compare February 20, 2025 14:30

wenqiq force-pushed the add-sriov-e2e branch from 48eefe5 to 6ad8de2 Compare February 20, 2025 14:48

wenqiq force-pushed the add-sriov-e2e branch from 0f1e295 to 7137b41 Compare February 20, 2025 15:31

Add e2e test case for SecondaryNetwork of SR-IOV type

6ab5756

Signed-off-by: Wenqi Qiu <[email protected]>

wenqiq force-pushed the add-sriov-e2e branch from 7137b41 to 6ab5756 Compare February 20, 2025 15:49

Add e2e test case for SecondaryNetwork of SR-IOV type #6922

Are you sure you want to change the base?

Add e2e test case for SecondaryNetwork of SR-IOV type #6922

Conversation

wenqiq commented Jan 14, 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

wenqiq 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

XinShuYang Feb 13, 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

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

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

wenqiq commented Feb 19, 2025

wenqiq commented Feb 19, 2025

XinShuYang commented Feb 19, 2025

XinShuYang commented Feb 19, 2025

wenqiq commented Feb 19, 2025

wenqiq commented Feb 19, 2025

wenqiq commented Feb 19, 2025

wenqiq commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone left a 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 20, 2025

wenqiq commented Feb 20, 2025

wenqiq commented Feb 20, 2025

wenqiq commented Feb 20, 2025

wenqiq commented Feb 20, 2025

wenqiq commented Feb 20, 2025

wenqiq commented Feb 20, 2025

wenqiq commented Feb 20, 2025

wenqiq commented Jan 14, 2025 •

edited

Loading

wenqiq Feb 12, 2025 •

edited

Loading

XinShuYang Feb 13, 2025 •

edited

Loading