move sriov test cases from cnf-gotests #998
Conversation
📝 WalkthroughWalkthroughAdds two Ginkgo SR-IOV test suites (IPv4 and IPv6), extensive SR-IOV test helpers and parameters, and updates two test container image tags to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
92b5bfd to
63119bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
455-462: Replacetime.Sleepwith proper completion polling.Using a hardcoded
time.Sleep(10 * time.Second)is brittle. The pod command runsmodprobe sctp && sleep 5, so the container should naturally exit after ~5 seconds. Consider usingEventuallyto poll for pod completion or verify the module is loaded.- // Wait for the pod to complete and then delete it - time.Sleep(10 * time.Second) - _, err = debugPod.DeleteAndWait(netparam.DefaultTimeout) + // Wait for the modprobe command to complete by polling pod phase + Eventually(func() bool { + updatedPod, pullErr := pod.Pull(APIClient, debugPod.Definition.Name, tsparams.TestNamespaceName) + if pullErr != nil { + return false + } + return updatedPod.Object.Status.Phase == "Succeeded" + }, 30*time.Second, 2*time.Second).Should(BeTrue(), + "SCTP enabler pod on node %s should complete", node.Definition.Name) + + _, err = debugPod.DeleteAndWait(netparam.DefaultTimeout)Alternatively, if the pod doesn't transition to
Succeededdue to the container spec, you could verify the module is loaded by checking/proc/moduleson the host.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between aa40bb0 and 63119bd1315980073998d35ee8ebd0217d103583.
📒 Files selected for processing (2)
tests/cnf/core/network/internal/netconfig/default.yaml(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv4.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/sriov/tests/sriov-ipv4.go
[error] 27-27: Comment should end in a period (godot)
🪛 GitHub Check: build
tests/cnf/core/network/sriov/tests/sriov-ipv4.go
[failure] 341-341:
Comment should end in a period (godot)
[failure] 321-321:
Comment should end in a period (godot)
[failure] 59-59:
Comment should end in a period (godot)
[failure] 55-55:
Comment should end in a period (godot)
[failure] 51-51:
Comment should end in a period (godot)
[failure] 45-45:
Comment should end in a period (godot)
[failure] 41-41:
Comment should end in a period (godot)
[failure] 37-37:
Comment should end in a period (godot)
[failure] 31-31:
Comment should end in a period (godot)
[failure] 27-27:
Comment should end in a period (godot)
🔇 Additional comments (6)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (5)
66-107: LGTM!Well-structured test suite setup with proper prerequisite validation (worker nodes, SR-IOV interfaces) and clean namespace lifecycle management.
109-167: LGTM!The Same Node Same PF context properly tests two MTU scenarios (500 and 9000) with isolated VF ranges on a single PF, and includes appropriate cleanup in AfterAll.
169-228: LGTM!The cross-PF connectivity test is well-structured, testing communication between different physical functions on the same node with standard MTU.
230-285: LGTM!The cross-node connectivity test properly validates SR-IOV communication between different worker nodes, complementing the same-node test scenarios.
409-440: LGTM!The traffic test helper is well-organized, testing ICMP, TCP, UDP, and SCTP connectivity with appropriate MTU-adjusted packet sizes. The separation of concerns between server listener setup and client traffic generation is clean.
tests/cnf/core/network/internal/netconfig/default.yaml (1)
3-3: Verify compatibility of the v4.20 container image with the test suite.The v4.20 tag exists on the registry (last modified October 2025). However, this represents a significant version jump from v4.15.2. Confirm the new image is compatible with your tests and consider whether the unchanged dpdk_test_container (still at v4.15.0) needs updating as well.
gkopels
force-pushed
the
add-sriov-table-tests
branch
3 times, most recently
from
537eb44 to
722d2cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (4)
454-476: Improve SCTP enabler pod synchronization.The function uses
time.Sleep(10 * time.Second)for synchronization instead of properly waiting for the pod to complete. This makes the test slower and less reliable.Consider using
WaitUntilConditionor checking pod phase/completion status instead of arbitrary sleep. For example:Expect(err).ToNot(HaveOccurred(), "Failed to create SCTP enabler pod on node %s", node.Definition.Name) - // Wait for the pod to complete and then delete it - time.Sleep(10 * time.Second) + // Wait for the pod to complete (the command sleeps for 5 seconds, so completion indicates success) + Eventually(func() bool { + debugPod, err := pod.Pull(APIClient, debugPod.Definition.Name, tsparams.TestNamespaceName) + if err != nil { + return false + } + return debugPod.Object.Status.Phase == "Succeeded" || + time.Since(debugPod.Object.Status.StartTime.Time) > 15*time.Second + }, 30*time.Second, 2*time.Second).Should(BeTrue()) + _, err = debugPod.DeleteAndWait(netparam.DefaultTimeout) Expect(err).ToNot(HaveOccurred(), "Failed to delete SCTP enabler pod on node %s", node.Definition.Name)
301-314: Consider simplifying constant parameters.Static analysis indicates that
numVfs(line 301) andtotalVfs(line 327) always receive the value 6. Since these parameters don't vary, consider either hardcoding them or calculatingtotalVfsfrom the VF range.For example, you could calculate
totalVfsfrom the policy's VF range:-func waitForSriovPoliciesStable(workerNodes []*nodes.Builder, sriovInterface string, totalVfs int) { +func waitForSriovPoliciesStable(workerNodes []*nodes.Builder, sriovInterface string) { + // All policies in this test use 6 VFs (ranges 0-2 and 3-5) + const totalVfs = 6 By("Waiting for VFs to be created") err := sriovenv.WaitUntilVfsCreated(workerNodes, sriovInterface, totalVfs, 5*time.Minute) Expect(err).ToNot(HaveOccurred(), "Failed waiting for VFs to be created") }And update calls to remove the parameter:
- waitForSriovPoliciesStable(workerNodeList, sriovInterfacePF1, 6) + waitForSriovPoliciesStable(workerNodeList, sriovInterfacePF1)Also applies to: 327-331
328-329: Add whitespace for readability.The wsl linter flags missing whitespace before assignment statements on lines 329, 412, 416, 432, and 437.
Apply this diff to add blank lines:
func waitForSriovPoliciesStable(workerNodes []*nodes.Builder, sriovInterface string, totalVfs int) { By("Waiting for VFs to be created") + err := sriovenv.WaitUntilVfsCreated(workerNodes, sriovInterface, totalVfs, 5*time.Minute) Expect(err).ToNot(HaveOccurred(), "Failed waiting for VFs to be created") }func cleanupSriovResources() { By("Removing all SR-IOV networks") + err := sriovoperator.RemoveAllSriovNetworks(APIClient, NetConfig.SriovOperatorNamespace, tsparams.DefaultTimeout) Expect(err).ToNot(HaveOccurred(), "Failed to remove SR-IOV networks") By("Removing all SR-IOV policies and waiting for stability") + err = sriovoperator.RemoveAllPoliciesAndWaitForSriovAndMCPStable( APIClient, NetConfig.CnfMcpLabel, NetConfig.SriovOperatorNamespace, tsparams.MCOWaitTimeout) Expect(err).ToNot(HaveOccurred(), "Failed to remove SR-IOV policies") }// 1. ICMP - verify basic connectivity first By("Verifying ICMP connectivity") + err := cmd.ICMPConnectivityCheck(clientPod, []string{serverIP}) Expect(err).ToNot(HaveOccurred(), "ICMP connectivity check failed for MTU %d", mtu) // 2. TCP unicast (server listening on port 5001) By("Verifying TCP connectivity") + _, err = clientPod.ExecCommand([]string{"bash", "-c", fmt.Sprintf("testcmd --protocol=tcp --port=5001 --interface=net1 --server=%s --mtu=%d", serverIPAddress, packetSize)})Also applies to: 411-412, 415-416, 431-432, 436-437
404-406: Add blank line before return.The nlreturn linter requires a blank line before return statements for readability.
Apply this diff:
_, err := serverPod.ExecCommand([]string{"bash", "-c", "pgrep -f testcmd"}) + return err }, 30*time.Second, 2*time.Second).Should(Succeed(), "Server pod %s should have testcmd listeners running", serverPod.Definition.Name)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 63119bd1315980073998d35ee8ebd0217d103583 and 722d2cc11d4298fb704aef77a06152fcae56eb93.
📒 Files selected for processing (2)
tests/cnf/core/network/internal/netconfig/default.yaml(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv4.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cnf/core/network/internal/netconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (3)
tests/system-tests/internal/link/link.go (1)
NewBuilder(48-77)tests/cnf/ran/ptp/internal/iface/iface.go (1)
Name(22-22)tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (2)
CreateSriovNetworkAndWaitForNADCreation(59-68)WaitUntilVfsCreated(152-180)
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/sriov/tests/sriov-ipv4.go
[error] 450-450: The line is 121 characters long, which exceeds the maximum of 120 characters. (lll)
🪛 GitHub Check: build
tests/cnf/core/network/sriov/tests/sriov-ipv4.go
[failure] 437-437:
missing whitespace above this line (invalid statement above assign) (wsl_v5)
[failure] 432-432:
missing whitespace above this line (invalid statement above assign) (wsl_v5)
[failure] 416-416:
missing whitespace above this line (invalid statement above assign) (wsl_v5)
[failure] 412-412:
missing whitespace above this line (invalid statement above assign) (wsl_v5)
[failure] 329-329:
missing whitespace above this line (invalid statement above assign) (wsl_v5)
[failure] 327-327:
waitForSriovPoliciesStable - totalVfs always receives 6 (unparam)
[failure] 316-316:
createSriovNetwork - mtu is unused (unparam)
[failure] 301-301:
createSriovPolicy - numVfs always receives 6 (unparam)
[failure] 405-405:
return with no blank line before (nlreturn)
[failure] 450-450:
The line is 121 characters long, which exceeds the maximum of 120 characters. (lll)
| func createSriovNetwork(name, resourceName string, mtu int) { | ||
| networkBuilder := sriov.NewNetworkBuilder( | ||
| APIClient, name, NetConfig.SriovOperatorNamespace, | ||
| tsparams.TestNamespaceName, resourceName). | ||
| WithMacAddressSupport(). | ||
| WithStaticIpam() | ||
|
|
||
| err := sriovenv.CreateSriovNetworkAndWaitForNADCreation(networkBuilder, tsparams.WaitTimeout) | ||
| Expect(err).ToNot(HaveOccurred(), "Failed to create SR-IOV network %s", name) | ||
| } |
There was a problem hiding this comment.
Critical: MTU parameter is not applied to SR-IOV network.
The mtu parameter is accepted but never used. The SR-IOV network is created without MTU configuration, which means all networks will use the default MTU regardless of what's specified in the tests.
Apply this diff to configure MTU on the network:
func createSriovNetwork(name, resourceName string, mtu int) {
networkBuilder := sriov.NewNetworkBuilder(
APIClient, name, NetConfig.SriovOperatorNamespace,
tsparams.TestNamespaceName, resourceName).
WithMacAddressSupport().
- WithStaticIpam()
+ WithStaticIpam().
+ WithMTU(mtu)
err := sriovenv.CreateSriovNetworkAndWaitForNADCreation(networkBuilder, tsparams.WaitTimeout)
Expect(err).ToNot(HaveOccurred(), "Failed to create SR-IOV network %s", name)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func createSriovNetwork(name, resourceName string, mtu int) { | |
| networkBuilder := sriov.NewNetworkBuilder( | |
| APIClient, name, NetConfig.SriovOperatorNamespace, | |
| tsparams.TestNamespaceName, resourceName). | |
| WithMacAddressSupport(). | |
| WithStaticIpam() | |
| err := sriovenv.CreateSriovNetworkAndWaitForNADCreation(networkBuilder, tsparams.WaitTimeout) | |
| Expect(err).ToNot(HaveOccurred(), "Failed to create SR-IOV network %s", name) | |
| } | |
| func createSriovNetwork(name, resourceName string, mtu int) { | |
| networkBuilder := sriov.NewNetworkBuilder( | |
| APIClient, name, NetConfig.SriovOperatorNamespace, | |
| tsparams.TestNamespaceName, resourceName). | |
| WithMacAddressSupport(). | |
| WithStaticIpam(). | |
| WithMTU(mtu) | |
| err := sriovenv.CreateSriovNetworkAndWaitForNADCreation(networkBuilder, tsparams.WaitTimeout) | |
| Expect(err).ToNot(HaveOccurred(), "Failed to create SR-IOV network %s", name) | |
| } |
🧰 Tools
🪛 GitHub Check: build
[failure] 316-316:
createSriovNetwork - mtu is unused (unparam)
🤖 Prompt for AI Agents
In tests/cnf/core/network/sriov/tests/sriov-ipv4.go around lines 316 to 325, the
createSriovNetwork function accepts an mtu parameter but never applies it to the
SR-IOV network; update the network builder chain to call the builder method that
sets MTU (e.g., WithMTU(mtu) or the equivalent provided by
sriov.NewNetworkBuilder) before creating the network so the MTU is configured;
ensure the call is added inline with the existing
WithMacAddressSupport().WithStaticIpam() chain and compile to verify the builder
method name matches the SDK.
722d2cc to
21f9361
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
480-503: Refactor to use pod status wait instead of arbitrary sleep.The function uses
time.Sleep(10 * time.Second)to wait for SCTP module loading to complete. This is fragile and could fail if pod startup is delayed.Consider using
WaitUntilConditionor similar to wait for pod completion status:debugPod, err := pod.NewBuilder( APIClient, fmt.Sprintf("sctp-enable-%s", node.Definition.Name), tsparams.TestNamespaceName, NetConfig.CnfNetTestContainer). DefineOnNode(node.Definition.Name). WithPrivilegedFlag(). WithHostPid(true). WithHostNetwork(). - RedefineDefaultCMD([]string{"bash", "-c", "nsenter -t 1 -m -u -n -i modprobe sctp && sleep 5"}). - CreateAndWaitUntilRunning(netparam.DefaultTimeout) + RedefineDefaultCMD([]string{"bash", "-c", "nsenter -t 1 -m -u -n -i modprobe sctp"}). + WithRestartPolicy("Never"). + CreateAndWaitUntilRunning(netparam.DefaultTimeout) Expect(err).ToNot(HaveOccurred(), "Failed to create SCTP enabler pod on node %s", node.Definition.Name) - // Wait for the pod to complete and then delete it. - time.Sleep(10 * time.Second) + // Wait for the pod to complete + err = debugPod.WaitUntilCondition("Succeeded", netparam.DefaultTimeout) + Expect(err).ToNot(HaveOccurred(), "SCTP enabler pod did not complete on node %s", node.Definition.Name) _, err = debugPod.DeleteAndWait(netparam.DefaultTimeout) Expect(err).ToNot(HaveOccurred(), "Failed to delete SCTP enabler pod on node %s", node.Definition.Name)This ensures the module is loaded before proceeding and eliminates the race condition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 722d2cc11d4298fb704aef77a06152fcae56eb93 and 21f9361dc31ed64e1f11726fbc7d45fb9793f78d.
📒 Files selected for processing (2)
tests/cnf/core/network/internal/netconfig/default.yaml(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv4.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cnf/core/network/internal/netconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (4)
tests/system-tests/internal/link/link.go (1)
NewBuilder(48-77)tests/cnf/ran/ptp/internal/iface/iface.go (1)
Name(22-22)tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (2)
CreateSriovNetworkAndWaitForNADCreation(59-68)WaitUntilVfsCreated(152-180)tests/internal/sriovoperator/sriovenv.go (2)
RemoveAllSriovNetworks(156-176)RemoveAllPoliciesAndWaitForSriovAndMCPStable(180-195)
🔇 Additional comments (9)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (9)
26-65: LGTM! Well-organized constant definitions.The constants are clearly documented and logically grouped by purpose (MTU values, VF configuration, resource names, etc.). The godot linter issues from previous reviews have been properly addressed.
78-103: Solid test setup with appropriate prerequisite validation.The BeforeAll hook properly validates the cluster has sufficient resources (2+ worker nodes, 2+ SR-IOV interfaces) before proceeding with tests.
112-311: Well-structured test contexts covering key SR-IOV scenarios.The three contexts (Same Node Same PF, Same Node Different PF, Different Node) provide comprehensive coverage of SR-IOV connectivity patterns. Each context properly manages its own setup and teardown.
316-330: LGTM! Policy creation correctly configures MTU and VF range.The function properly creates SR-IOV policies with MTU and VF range configuration. The
nolint:unparamdirective is justified sincenumVfsis consistentlytotalVFsacross all calls.
351-407: Excellent pod creation helpers with proper network configuration.Both
createClientPodandcreateServerPodcorrectly configure SR-IOV secondary networks with static IPs and MACs. The server pod setup is particularly well-designed, running multiple protocol listeners (TCP/UDP/SCTP) in background processes with appropriate packet sizing.
418-427: Good use of Eventually pattern for async readiness check.The function properly waits for testcmd processes to be running rather than using arbitrary sleep delays. This is more robust than time-based waits.
429-440: Proper cleanup order for SR-IOV resources.The function correctly removes networks before removing policies, which is the proper teardown sequence for SR-IOV resources.
442-478: Comprehensive traffic validation with appropriate packet sizing.The function tests all required protocols (ICMP, TCP, UDP, SCTP) with packet sizes correctly adjusted for MTU (mtu - 100 accounts for headers). The sequential testing order is logical, starting with ICMP to verify basic connectivity.
332-341: The suggested fix is incorrect and based on a misunderstanding of the SR-IOV Network Operator architecture.The original review proposes adding
.WithMTU(mtu)to the network builder, butsriov.NetworkBuilderdoes not support aWithMTUmethod. More importantly, MTU configuration in the SR-IOV Network Operator is not managed at the network level—it is managed at the node policy level viaSriovNetworkNodePolicy.The current code is correct:
createSriovPolicy()configures MTU on the node policy (line 107:.WithMTU(mtu))- The SR-IOV Network Operator automatically applies that MTU to the physical function and its virtual functions
createSriovNetwork()creates theSriovNetworkresource, which references the policy but does not itself specify MTUThe
SriovNetwork/ generatedNetworkAttachmentDefinitiondoes not have an MTU field; MTU inheritance from the policy happens automatically at the operator level. The test design correctly separates concerns: policies manage node-level MTU configuration, and networks reference those policies.
gkopels
force-pushed
the
add-sriov-table-tests
branch
2 times, most recently
from
0327a55 to
77aea09
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
333-342: MTU parameter was removed but previous review flagged it as needed.A past review comment flagged that the
mtuparameter was unused and suggested addingWithMTU(mtu)to configure MTU on the network. However, the current implementation removed themtuparameter entirely. If MTU configuration on the network is actually required (separate from the policy-level MTU), this should be addressed.The same concern applies as noted in the IPv6 file review - verify whether MTU needs to be set on the SR-IOV network in addition to the policy.
🧹 Nitpick comments (2)
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (2)
498-506: Replace time.Sleep with proper readiness check.Using
time.Sleep(10 * time.Second)is fragile and may cause flakiness. The pod withmodprobe sctp && sleep 5should complete quickly, so waiting explicitly is redundant if the command succeeded.Consider removing or reducing the sleep, or use a proper wait mechanism:
- // Wait for the pod to complete and then delete it. - time.Sleep(10 * time.Second) - _, err = debugPod.DeleteAndWait(netparam.DefaultTimeout)
1-24: Consider extracting shared helper functions to reduce duplication.The IPv6 test suite contains significant duplication with the IPv4 test suite (
sriov-ipv4.go). Functions likecreateClientPod,createServerPod,deleteTestPods,waitForServerReady,cleanupSriovResources,runTrafficTests, andenableSCTPOnWorkersare nearly identical with only minor naming differences (prefixipv6).Consider extracting these into a shared helper file (e.g.,
sriov_helpers.go) to reduce maintenance burden and ensure consistency. The IP family could be passed as a parameter where needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/cnf/core/network/internal/netconfig/default.yaml(1 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv4.go(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv6.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (2)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (2)
CreateSriovNetworkAndWaitForNADCreation(59-68)WaitUntilVfsCreated(152-180)tests/internal/sriovoperator/sriovenv.go (1)
RemoveAllSriovNetworks(156-176)
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (2)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (2)
CreateSriovNetworkAndWaitForNADCreation(59-68)WaitUntilVfsCreated(152-180)tests/internal/sriovoperator/sriovenv.go (2)
RemoveAllSriovNetworks(156-176)RemoveAllPoliciesAndWaitForSriovAndMCPStable(180-195)
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[error] 828-828: gofmt: File is not properly formatted.
🪛 GitHub Check: build
tests/cnf/core/network/sriov/tests/sriov-ipv6.go
[failure] 69-69:
File is not properly formatted (gofmt)
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[failure] 828-828:
File is not properly formatted (gofmt)
🔇 Additional comments (6)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (3)
452-456: Same verification needed for ICMPConnectivityCheck with CIDR notation.Similar to the IPv6 test,
serverIPcontains the CIDR prefix (e.g.,192.168.10.2/24). Ensurecmd.ICMPConnectivityCheckhandles this correctly.
78-104: Good test setup with proper validation.The BeforeAll setup properly validates prerequisites (worker nodes count, SR-IOV interfaces availability) before proceeding with test execution. The SCTP enablement ensures the kernel module is loaded for SCTP traffic tests.
153-159: Clean resource cleanup pattern.The AfterAll properly cleans up test pods before SR-IOV resources, ensuring correct cleanup order.
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (2)
453-457: > Likely an incorrect or invalid review comment.
333-342: No changes needed; function follows established codebase patterns.The network builder consistently does not set MTU across all test files in the codebase. MTU configuration is handled at the SR-IOV policy level (via
WithMTUon policy builders), which sets the VF MTU. The network resource creation does not require MTU configuration, as evidenced by all existing network builders following this same pattern. The function is correct as-is.Likely an incorrect or invalid review comment.
tests/cnf/core/network/internal/netconfig/default.yaml (1)
3-3: Verify the container image tag v4.20 exists and is available.The container tag
v4.20was newly added in this file. Ensure this version exists onquay.io/ocp-edge-qe/eco-gotests-network-clientand is compatible with the SR-IOV test suites. No CI/CD validation found in the codebase for this container tag.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Remove trailing blank lines to fix gofmt failure.
The pipeline is failing because of these trailing blank lines. Go files should not have excessive trailing whitespace after the last function.
Remove lines 828-847 (all trailing blank lines after the closing brace of verifyBlockedRoutes).
}, 60*time.Second, 5*time.Second).Should(Not(ContainSubstring(blockedPrefixes)),
"Failed the blocked route was received.")
}
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-🧰 Tools
🪛 GitHub Actions: Makefile CI
[error] 828-828: gofmt: File is not properly formatted.
🪛 GitHub Check: build
[failure] 828-828:
File is not properly formatted (gofmt)
🤖 Prompt for AI Agents
In tests/cnf/core/network/metallb/tests/frrk8-tests.go around lines 828 to 847,
there are excessive trailing blank lines after the closing brace of the
verifyBlockedRoutes function causing gofmt failures; remove all blank lines from
828 through 847 so the file ends immediately after the function's closing brace
(ensure no extra newline blocks remain) and re-run gofmt/go vet to confirm the
formatting error is resolved.
63c38db to
bc931bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
828-847: Remove trailing blank lines to fix gofmt failure (duplicate).This issue was already flagged in a previous review. The pipeline continues to fail because these trailing blank lines remain. Please remove all blank lines from 828 through 847.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yaml(1 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(1 hunks)tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go(1 hunks)tests/cnf/core/network/sriov/internal/tsparams/consts.go(1 hunks)tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv4.go(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv6.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cnf/core/network/internal/netconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (10)
EnableSCTPOnWorkers(596-622)CreateSriovPolicyWithMTU(423-439)WaitUntilVfsCreated(152-180)CreateSriovNetworkWithStaticIPAM(442-452)CreateTestClientPod(455-471)CreateTestServerPod(474-510)WaitForServerReady(529-541)DeleteTestPods(513-526)CleanupAllSriovResources(544-553)RunTrafficTest(556-593)
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[error] 828-828: golangci-lint: File is not properly formatted (gofmt).
🪛 GitHub Check: build
tests/cnf/core/network/sriov/tests/sriov-ipv4.go
[failure] 165-165:
The line is 125 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 160-160:
The line is 123 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 156-156:
The line is 123 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 114-114:
The line is 125 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 110-110:
The line is 125 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 105-105:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 101-101:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 97-97:
The line is 123 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 93-93:
The line is 121 characters long, which exceeds the maximum of 120 characters. (lll)
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[failure] 828-828:
File is not properly formatted (gofmt)
🔇 Additional comments (10)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
25-323: Well-structured SR-IOV IPv4 test suite.The test suite is well-organized with clear separation of concerns:
- Comprehensive setup/teardown in BeforeAll/AfterAll hooks
- Three distinct test contexts covering Same PF, Different PF, and Different Node scenarios
- Proper MTU handling at the policy level with corresponding traffic validation
- SCTP enablement and resource cleanup flows
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (1)
25-323: LGTM: IPv6 test suite mirrors IPv4 structure.The IPv6 test suite properly mirrors the IPv4 implementation with appropriate IPv6 addressing. The consistent structure across both IP versions makes the test suite maintainable and comprehensive.
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (1)
63-86: LGTM: Test configuration variables are well-defined.The new variables provide clear parameterization for MTU testing with appropriate VF distribution (6 VFs split into two groups for MTU 500 and MTU 9000 testing).
tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
36-68: LGTM: Well-organized test constants.The constants are logically organized by test context (Same PF, Different PF, Different Node) and MTU configuration, with clear and descriptive naming that enhances test readability.
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (6)
422-452: LGTM: SR-IOV policy and network creation helpers.The MTU configuration at the policy level (CreateSriovPolicyWithMTU) properly configures the VFs, which is the primary mechanism for MTU control in SR-IOV. The network creation focuses on IPAM and MAC addressing, which is appropriate.
454-510: LGTM: Client and server pod creation helpers.The server pod design efficiently runs multiple protocol listeners (TCP, UDP, SCTP) in a single container using background processes. The packet size calculation (MTU - 100) appropriately accounts for protocol headers.
555-593: LGTM: Comprehensive traffic validation.The traffic test function validates connectivity across all major protocols (ICMP, TCP, UDP, SCTP) with MTU-aware packet sizing and clear error reporting.
595-622: LGTM: SCTP enablement implementation.The function properly loads the SCTP kernel module using privileged pods with nsenter, and correctly cleans up the enabler pods after the module is loaded.
512-553: LGTM: Pod management and cleanup helpers.The pod deletion and server readiness functions properly handle resource cleanup and validation, with appropriate error handling.
624-676: LGTM: Resource cleanup and utility functions.The cleanup functions comprehensively remove SR-IOV resources (networks and policies) and properly wait for MCP stabilization. The removePrefix utility is a simple and effective helper for IP address handling.
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
bc931bb to
ad76b68
Compare
gkopels
changed the title
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
ad76b68 to
26dc2d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/cnf/core/network/metallb/tests/frrk8-tests.go (1)
828-847: Remove trailing blank lines to fix gofmt failure.The pipeline is failing because of these trailing blank lines at the end of the file. This issue was previously flagged and must be resolved.
🔎 Proposed fix
Remove all blank lines from 828 through 847:
}, 60*time.Second, 5*time.Second).Should(Not(ContainSubstring(blockedPrefixes)), "Failed the blocked route was received.") } - - - - - - - - - - - - - - - - - - -tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
145-149: Fix line length violations in Different PF context.Lines 145 and 149 exceed the 120-character limit.
🔎 Proposed fix
By("Creating SR-IOV Network Node Policies for MTU 500") - err = sriovenv.CreateSriovPolicyWithMTU("policy-diffpf1-mtu500", tsparams.SriovResourceDiffPF1MTU500, sriovInterfacePF1, - tsparams.MTU500, tsparams.TotalVFs, tsparams.VFStartMTU500, tsparams.VFEndMTU500) + err = sriovenv.CreateSriovPolicyWithMTU( + "policy-diffpf1-mtu500", tsparams.SriovResourceDiffPF1MTU500, sriovInterfacePF1, + tsparams.MTU500, tsparams.TotalVFs, tsparams.VFStartMTU500, tsparams.VFEndMTU500) Expect(err).ToNot(HaveOccurred(), "Failed to create SR-IOV policy for PF1 MTU 500") - err = sriovenv.CreateSriovPolicyWithMTU("policy-diffpf2-mtu500", tsparams.SriovResourceDiffPF2MTU500, sriovInterfacePF2, - tsparams.MTU500, tsparams.TotalVFs, tsparams.VFStartMTU500, tsparams.VFEndMTU500) + err = sriovenv.CreateSriovPolicyWithMTU( + "policy-diffpf2-mtu500", tsparams.SriovResourceDiffPF2MTU500, sriovInterfacePF2, + tsparams.MTU500, tsparams.TotalVFs, tsparams.VFStartMTU500, tsparams.VFEndMTU500) Expect(err).ToNot(HaveOccurred(), "Failed to create SR-IOV policy for PF2 MTU 500")
🧹 Nitpick comments (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (1)
422-684: Fix line length violation on line 457.The code structure and helper functions are well-organized, but line 457 exceeds the 120-character limit.
🔎 Proposed fix for line 457
-// CreateTestClientPod creates a client pod with SR-IOV interface. +// CreateTestClientPod creates a client pod with SR-IOV interface. // ipAddresses can be a single IP or multiple IPs for dual-stack (e.g., []string{"192.168.1.1/24", "2001::1/64"}). -func CreateTestClientPod(name, nodeName, networkName string, ipAddresses []string, macAddress string) (*pod.Builder, error) { +func CreateTestClientPod( + name, nodeName, networkName string, ipAddresses []string, macAddress string) (*pod.Builder, error) {Otherwise, the helper functions are well-structured:
- Clear separation of concerns (policy/network creation, pod lifecycle, traffic testing, cleanup)
- Proper error handling and logging
- MTU-aware packet sizing (mtu-100 accounting for headers)
- Background testcmd listeners for TCP/UDP/SCTP on server pods
- Comprehensive cleanup with MCP stability checks
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tests/cnf/core/network/internal/netconfig/default.yaml(1 hunks)tests/cnf/core/network/metallb/tests/frrk8-tests.go(1 hunks)tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go(1 hunks)tests/cnf/core/network/sriov/internal/tsparams/consts.go(1 hunks)tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv4.go(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv6.go(1 hunks)tests/ocp/sriov/internal/ocpsriovconfig/default.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cnf/core/network/internal/netconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (11)
ValidateSriovInterfaces(29-56)EnableSCTPOnWorkers(600-626)CreateSriovPolicyWithMTU(423-439)WaitUntilVfsCreated(152-180)CreateSriovNetworkWithStaticIPAM(442-453)CreateTestClientPod(457-473)CreateTestServerPod(478-514)WaitForServerReady(533-545)DeleteTestPods(517-530)CleanupAllSriovResources(548-557)RunTrafficTest(560-597)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (4)
tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
TestNamespaceName(7-7)tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (2)
WaitTimeout(17-17)DefaultTimeout(19-19)tests/ocp/sriov/internal/sriovocpenv/pods.go (1)
ICMPConnectivityCheck(44-73)tests/internal/cluster/cluster.go (1)
WaitForMcpStable(315-329)
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (3)
tests/cnf/core/network/sriov/internal/tsparams/consts.go (21)
LabelSuite(5-5)SriovResourceSamePFMTU500(37-37)SriovResourceSamePFMTU9000(38-38)SriovNetworkSamePFMTU500(41-41)SriovNetworkSamePFMTU9000(42-42)ClientPodMTU500(65-65)ServerPodMTU500(66-66)ClientPodMTU9000(67-67)ServerPodMTU9000(68-68)SriovResourceDiffPF1MTU500(45-45)SriovResourceDiffPF2MTU500(46-46)SriovResourceDiffPF1MTU9000(47-47)SriovResourceDiffPF2MTU9000(48-48)SriovNetworkDiffPF1MTU500(51-51)SriovNetworkDiffPF2MTU500(52-52)SriovNetworkDiffPF1MTU9000(53-53)SriovNetworkDiffPF2MTU9000(54-54)SriovResourceDiffNodeMTU500(57-57)SriovResourceDiffNodeMTU9000(58-58)SriovNetworkDiffNodeMTU500(61-61)SriovNetworkDiffNodeMTU9000(62-62)tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (10)
EnableSCTPOnWorkers(600-626)CreateSriovPolicyWithMTU(423-439)WaitUntilVfsCreated(152-180)CreateSriovNetworkWithStaticIPAM(442-453)CreateTestClientPod(457-473)CreateTestServerPod(478-514)WaitForServerReady(533-545)DeleteTestPods(517-530)CleanupAllSriovResources(548-557)RunTrafficTest(560-597)tests/cnf/ran/ptp/internal/iface/iface.go (1)
Name(22-22)
🪛 GitHub Actions: Makefile CI
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[error] 828-828: gofmt: File is not properly formatted. Run 'gofmt -w' to fix.
🪛 GitHub Check: build
tests/cnf/core/network/sriov/tests/sriov-ipv4.go
[failure] 149-149:
The line is 123 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 145-145:
The line is 123 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 103-103:
The line is 125 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 99-99:
The line is 125 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 94-94:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 90-90:
The line is 122 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 86-86:
The line is 123 characters long, which exceeds the maximum of 120 characters. (lll)
[failure] 82-82:
The line is 121 characters long, which exceeds the maximum of 120 characters. (lll)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go
[failure] 457-457:
The line is 125 characters long, which exceeds the maximum of 120 characters. (lll)
tests/cnf/core/network/metallb/tests/frrk8-tests.go
[failure] 828-828:
File is not properly formatted (gofmt)
🔇 Additional comments (5)
tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
35-69: LGTM! Well-organized SR-IOV test constants.The new constants are well-structured, consistently named, and logically grouped by test scenario (Same PF, Different PF, Different Node) and MTU values (500, 9000). This provides clear parameterization for the MTU-based SR-IOV connectivity tests.
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (1)
59-107: LGTM! Comprehensive test parameter additions.The new variables are well-documented and provide comprehensive test parameterization:
- Dual-stack IP groups for IPv4/IPv6 testing
- Bare server IPs (without CIDR prefix) for testcmd binding
- Additional MAC addresses for multi-pod scenarios
- MTU test values (500 and 9000) for jumbo frame testing
- VF configuration splitting 6 VFs between two MTU ranges (0-2 and 3-5)
This setup enables thorough MTU-aware SR-IOV connectivity testing across multiple scenarios.
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (1)
24-326: LGTM! Well-structured IPv6 SR-IOV test suite.The test suite is well-organized with:
- Comprehensive BeforeAll setup (node discovery, interface validation, SCTP enablement)
- Three logical test contexts covering key scenarios:
- Same Node Same PF (MTU 500 & 9000)
- Same Node Different PF (cross-PF connectivity)
- Different Node (cross-node connectivity)
- Proper resource lifecycle management with AfterAll cleanup
- Explicit server readiness checks before traffic tests
- Consistent use of helper functions from sriovenv package
The test design mirrors the IPv4 test suite structure, providing symmetric coverage for both IP families.
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
24-314: Overall: Well-structured IPv4 SR-IOV test suite.Despite the line length issues, the test suite design is solid:
- Mirrors the IPv6 test structure for consistency
- Comprehensive coverage of Same Node Same PF, Same Node Different PF, and Different Node scenarios
- Proper resource lifecycle management
- MTU-aware testing (500 and 9000)
Once the line length issues are fixed, this will be production-ready.
tests/ocp/sriov/internal/ocpsriovconfig/default.yaml (1)
3-3: Container image consolidation from sriov-specific to generic network client.The test container image has been updated from
quay.io/ocp-edge-qe/eco-gotests-sriov-client:v4.15.2toquay.io/ocp-edge-qe/eco-gotests-network-client:v4.20. This change is part of a broader consolidation effort where both the OCP SR-IOV and CNF network test configurations now use the same unifiednetwork-clientimage. While the version bump to v4.20 is significant, ensure that this image is validated in your test environment and that it provides equivalent or enhanced capabilities for SR-IOV testing compared to the previous sriov-specific variant.
gkopels
force-pushed
the
add-sriov-table-tests
branch
2 times, most recently
from
82f4718 to
0b14f44
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (2)
62-70: Consider consistent timeouts for VF creation.PF1 uses 10 minutes while PF2 uses 5 minutes for waiting on VFs. Since policies are created together upfront (causing a single node reboot), the VF creation on both PFs should complete around the same time. Consider using the same timeout for consistency, or add a comment explaining the difference.
176-198: Inconsistent pod naming convention.This context uses hardcoded pod names (e.g.,
"client-diffpf-mtu500") while the "Same Node Same PF" context uses constants fromtsparams(e.g.,tsparams.ClientPodMTU500). Consider defining constants for these pod names intsparamsfor consistency and maintainability.tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (1)
546-560: Consider continuing deletion on individual failures.Currently, the function returns immediately if any pod deletion fails, leaving remaining pods undeleted. For cleanup scenarios, it might be better to attempt deleting all pods and aggregate errors.
🔎 Proposed enhancement for robust cleanup
func DeleteTestPods(pods ...*pod.Builder) error { + var errs []error for _, podBuilder := range pods { if podBuilder != nil { klog.V(90).Infof("Deleting pod %s", podBuilder.Definition.Name) _, err := podBuilder.Delete() if err != nil { - return fmt.Errorf("failed to delete pod %s: %w", podBuilder.Definition.Name, err) + errs = append(errs, fmt.Errorf("failed to delete pod %s: %w", podBuilder.Definition.Name, err)) } } } + if len(errs) > 0 { + return fmt.Errorf("failed to delete some pods: %v", errs) + } + return nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yaml(1 hunks)tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go(1 hunks)tests/cnf/core/network/sriov/internal/tsparams/consts.go(1 hunks)tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv4.go(1 hunks)tests/cnf/core/network/sriov/tests/sriov-ipv6.go(1 hunks)tests/ocp/sriov/internal/ocpsriovconfig/default.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/cnf/core/network/sriov/internal/tsparams/consts.go
- tests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (1)
tests/ocp/sriov/internal/sriovocpenv/pods.go (1)
ICMPConnectivityCheck(44-73)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (11)
EnableSCTPOnWorkers(630-656)WaitUntilVfsCreated(152-180)CleanupAllSriovResources(578-587)CreateSriovNetworkWithStaticIPAM(442-453)CreateTestClientPod(486-503)CreateTestServerPod(508-544)WaitForServerReady(563-575)DeleteTestPods(547-560)DeleteSriovNetworks(456-482)RunTrafficTest(590-627)CreateSriovPolicyWithMTU(423-439)
🔇 Additional comments (21)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (5)
1-22: LGTM!Imports and constants are well-organized. The
serverReadyTimeoutconstant is appropriately scoped for this test file.
73-77: LGTM!Proper cleanup of SR-IOV resources in
AfterAllwith appropriate timeout.
80-148: LGTM!The "Same Node Same PF" context follows good test patterns with proper setup, teardown, and traffic verification for both MTU scenarios.
233-307: LGTM!The "Different Node" context properly tests cross-node SR-IOV connectivity with client and server pods on separate worker nodes.
310-340: LGTM!Well-designed helper function that creates all SR-IOV policies upfront to minimize node reboots. The VF range partitioning (0-2 for MTU 500, 3-5 for MTU 9000) is correctly configured.
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (3)
1-33: LGTM!Imports and variable declarations are well-structured and properly formatted, consistent with the IPv4 test suite.
35-77: LGTM!BeforeAll/AfterAll follows the same well-structured pattern as the IPv4 test suite.
319-349: LGTM!The
createAllIPv6SriovPolicieshelper correctly uses thepolicy-ipv6-prefix to differentiate from IPv4 policies.tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (2)
59-77: LGTM!Well-organized dual-stack and bare IP address constants with proper documentation.
82-101: LGTM!MAC addresses, MTU values, and VF range constants are well-defined with clear documentation. The VF ranges (0-2 and 3-5) correctly partition the 6 total VFs.
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (10)
422-439: LGTM!Clean implementation of the policy creation helper with MTU and VF range configuration.
441-453: LGTM!Properly configures the SR-IOV network with static IPAM, IP address support, and MAC address support.
455-482: LGTM!Robust network deletion with proper handling of non-existent networks and NAD cleanup verification.
484-503: LGTM!Well-documented client pod creation with proper dual-stack support.
505-544: LGTM!Well-designed server pod with multi-protocol listeners. The
packetSize = mtu - 100accounts for protocol headers, matching the calculation used inRunTrafficTest.
562-575: LGTM!Simple and effective server readiness check using process detection.
577-587: LGTM!Proper cleanup order: networks first, then policies, followed by MCP stability verification.
589-627: LGTM!Comprehensive traffic testing across all protocols (ICMP, TCP, UDP, SCTP) with proper IP formatting for each tool.
629-656: LGTM!Proper SCTP kernel module loading using nsenter to access host namespace. Cleanup is handled correctly.
658-714: LGTM!Helper functions are well-implemented. The
removePrefixfunction is simple and effective, and the cleanup helpers properly preserve the default policy while ensuring MCP stability.tests/cnf/core/network/internal/netconfig/default.yaml (1)
3-3: Container image version update verified and appropriate.The
v4.20image exists in the Quay.io registry and is properly tagged. This update is part of a consistent standardization across test configurations—both CNF network and SR-IOV tests now useeco-gotests-network-client:v4.20. Thedpdk_test_containerremaining atv4.15.0is appropriate as it uses a separate image.
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
0b14f44 to
9f09011
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (1)
629-656: Consider reducing redundant sleep delays.The pod command includes
sleep 5aftermodprobe, and there's an additionaltime.Sleep(10 * time.Second)after pod creation (line 647). The combined 15+ seconds per node may be excessive. Consider reducing one of these delays if testing shows it's unnecessary.🔎 Suggested simplification
- RedefineDefaultCMD([]string{"bash", "-c", "nsenter -t 1 -m -u -n -i modprobe sctp && sleep 5"}). + RedefineDefaultCMD([]string{"bash", "-c", "nsenter -t 1 -m -u -n -i modprobe sctp"}). CreateAndWaitUntilRunning(netparam.DefaultTimeout) if err != nil { return fmt.Errorf("failed to create SCTP enabler pod on node %s: %w", node.Definition.Name, err) } - time.Sleep(10 * time.Second) + time.Sleep(5 * time.Second)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yamltests/cnf/core/network/sriov/internal/sriovenv/sriovenv.gotests/cnf/core/network/sriov/internal/tsparams/consts.gotests/cnf/core/network/sriov/internal/tsparams/sriovvars.gotests/cnf/core/network/sriov/tests/sriov-ipv4.gotests/cnf/core/network/sriov/tests/sriov-ipv6.gotests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cnf/core/network/sriov/tests/sriov-ipv4.go
- tests/cnf/core/network/sriov/tests/sriov-ipv6.go
- tests/cnf/core/network/internal/netconfig/default.yaml
🔇 Additional comments (16)
tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
35-98: Well-structured constants for SR-IOV test infrastructure.The new constants are logically organized by:
- Resource names per PF and MTU combination
- Network names by test context (Same PF, Different PF, Different Node)
- Pod name prefixes for connectivity tests
The MTU value choices are appropriate—500 for IPv4-only tests, 1280 as the minimum MTU required by IPv6 (RFC 8200), and 9000 for jumbo frame testing.
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (2)
96-107: Overlapping VF ranges for MTU500 and MTU1280.Both
VFStartMTU500/VFEndMTU500(0-2) andVFStartMTU1280/VFEndMTU1280(0-2) use the same VF index range. This works if MTU 500 (IPv4-only) and MTU 1280 (IPv6) tests are never run concurrently on the same PF, but could cause resource conflicts if they are.Please confirm this overlap is intentional because these MTU configurations target mutually exclusive test runs (IPv4-only vs IPv6).
59-67: Dual-stack IP slices are well-structured.Clean approach for managing dual-stack addressing by composing the slices from the individual address variables.
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (12)
422-439: LGTM!Clean wrapper around the policy builder with appropriate MTU and VF range configuration.
441-453: LGTM!Properly configures the network with static IPAM and address support, then waits for NAD creation.
455-482: LGTM!Good defensive handling of non-existent networks and proper cleanup sequence (delete network, wait for NAD deletion).
484-503: LGTM!Clean client pod creation with proper network attachment and wait semantics.
505-544: Server pod setup is functional but relies on timing for startup.The
sleep 5delay before starting listeners is a simple approach for network readiness. Consider using a retry mechanism or network interface check if flaky test failures occur.The background process approach (
&) means listener startup failures won't be immediately detected—they'll only surface when traffic tests fail.
546-560: LGTM!Proper nil checks and error propagation.
562-575: LGTM!Simple and effective readiness check using process detection.
577-587: LGTM!Correct cleanup order—networks before policies—and appropriate MCP stability wait.
589-627: LGTM!Comprehensive traffic testing covering ICMP, TCP, UDP, and SCTP with proper error context.
658-667: LGTM!Simple and efficient CIDR prefix removal.
669-694: LGTM!Proper cleanup of all SR-IOV networks with NAD deletion wait.
696-714: LGTM!Correctly preserves the "default" policy and waits for MCP stability after cleanup.
tests/ocp/sriov/internal/ocpsriovconfig/default.yaml (1)
3-3: Verify this new configuration file uses the correct container image.This adds a new SR-IOV configuration file that references
eco-gotests-network-client:v4.20. Confirm this image exists in the registry and contains the required network test utilities for the SR-IOV test suite.Likely an incorrect or invalid review comment.
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
9f09011 to
04d8693
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (4)
516-562: Hardcoded 5-second sleep may be insufficient or excessive.The
sleep 5before starting listeners is a fixed delay that may not be reliable across different cluster conditions. Consider using a more robust readiness check or making this configurable.Also, the
packetSize := mtu - 100calculation is duplicated here and inRunTrafficTest. Consider extracting this to a helper constant or function.🔎 Consider extracting packet size calculation
// PacketSizeFromMTU calculates the packet size for testcmd based on MTU. // Subtracts 100 bytes to account for protocol headers. const mtuHeaderOverhead = 100 func packetSizeFromMTU(mtu int) int { return mtu - mtuHeaderOverhead }
647-674: Sequential pod creation and fixed sleep adds significant latency.The function creates pods sequentially for each node with a 10-second sleep after each. For clusters with many worker nodes, this could add substantial test setup time.
Consider:
- Creating pods in parallel across nodes
- Using a more targeted wait (e.g., checking if module is loaded) instead of fixed sleep
580-593: Server readiness check only verifies process existence, not listener availability.
pgrep -f testcmdconfirms the process is running but doesn't verify that the TCP/UDP/SCTP listeners are actually accepting connections. This could lead to race conditions where traffic tests start before listeners are ready.🔎 Consider verifying listener ports are open
func WaitForServerReady(serverPod *pod.Builder, timeout time.Duration) error { klog.V(90).Infof("Waiting for server pod %s to be ready", serverPod.Definition.Name) return wait.PollUntilContextTimeout(context.TODO(), 2*time.Second, timeout, true, func(ctx context.Context) (bool, error) { - _, err := serverPod.ExecCommand([]string{"bash", "-c", "pgrep -f testcmd"}) + // Check that all listeners are up by verifying listening ports + _, err := serverPod.ExecCommand([]string{"bash", "-c", + "pgrep -f testcmd && ss -tlnp | grep -q 5001 && ss -ulnp | grep -q 5002"}) if err != nil { return false, nil } return true, nil }) }
676-685: Consider usingstrings.Cutfor simpler IP prefix removal.The manual iteration can be replaced with a more idiomatic Go approach.
🔎 Simplified implementation
func removePrefix(ipWithPrefix string) string { - for i, c := range ipWithPrefix { - if c == '/' { - return ipWithPrefix[:i] - } - } - - return ipWithPrefix + ip, _, _ := strings.Cut(ipWithPrefix, "/") + return ip }tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
35-71: Consider adding a skip condition for insufficient worker nodes earlier.The test validates
len(workerNodeList) >= 2but proceeds to set up SCTP and policies before this check could skip the suite. If the cluster has fewer than 2 workers, it will fail after potentially modifying cluster state.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yamltests/cnf/core/network/sriov/internal/sriovenv/sriovenv.gotests/cnf/core/network/sriov/internal/tsparams/consts.gotests/cnf/core/network/sriov/internal/tsparams/sriovvars.gotests/cnf/core/network/sriov/tests/sriov-ipv4.gotests/cnf/core/network/sriov/tests/sriov-ipv6.gotests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cnf/core/network/internal/netconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (5)
EnableSCTPOnWorkers(648-674)CreateSriovNetworkWithStaticIPAM(443-454)DeleteSriovNetworks(457-483)RunTrafficTest(608-645)CreateSriovPolicyWithMTU(424-440)
🔇 Additional comments (10)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (1)
422-440: LGTM! Well-structured policy creation helper.The function correctly creates an SR-IOV policy with MTU and VF range configuration. The logging and error propagation are appropriate.
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (2)
24-33: LGTM! Well-organized test suite structure.The test suite is properly organized with:
- Ordered execution for sequential tests
- Shared variables at suite level for pod builders
- ContinueOnFailure for comprehensive test coverage
310-340: LGTM! Consolidated policy creation minimizes reboots.The helper correctly creates all four policies upfront, which is the recommended approach to minimize node reboots during SR-IOV policy application.
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (2)
19-22: IPv6 server ready timeout is 2x the IPv4 timeout.The IPv6
ipv6ServerReadyTimeoutis 60 seconds while IPv4 uses 30 seconds. If this is intentional (e.g., IPv6 setup takes longer), consider documenting why. Otherwise, consider using a shared constant.
319-350: LGTM! IPv6-specific policy creation with appropriate MTU.The helper correctly uses MTU 1280 (IPv6 minimum) and creates policies with distinct
ipv6-prefixes to avoid conflicts with IPv4 policies.tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
35-98: LGTM! Well-organized and documented constants.The constants are:
- Logically grouped by purpose (resources, networks, pods)
- Consistently named with clear prefixes indicating PF/MTU/context
- Properly documented with godoc comments explaining IPv4 vs IPv6 usage
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (3)
52-76: LGTM! IPv6 address duplication issue has been fixed.The secondary IPv6 addresses now use a different subnet (
2001:200::/64) compared to the primary (2001:100::/64), preventing IP conflicts in multi-network tests. The bare addresses also correctly match their CIDR counterparts.
96-107: VF ranges for MTU 500 and MTU 1280 overlap (VFs 0-2).Both
VFStartMTU500/VFEndMTU500andVFStartMTU1280/VFEndMTU1280use VFs 0-2. This is acceptable since IPv4 (MTU 500) and IPv6 (MTU 1280) tests run in separate test suites with independent policy creation. Verify this is the intended design.
60-67: Dual-stack arrays are correctly composed from individual addresses.The arrays properly combine the corresponding IPv4 and IPv6 addresses for each address set (primary and secondary).
tests/ocp/sriov/internal/ocpsriovconfig/default.yaml (1)
3-3: Container image exists and is publicly available.The new image
quay.io/ocp-edge-qe/eco-gotests-network-client:v4.20exists in the registry and was recently created (Dec 17, 2025). However, verify that the image provides all required test utilities (testcmd, etc.) by either pulling the image locally or checking the release notes/documentation for the v4.20 version.
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
04d8693 to
ab44be0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (2)
91-117: Shared MTU 9000 network/pod names between IPv4 and IPv6 suites.The IPv6 tests reuse
SriovNetworkSamePFMTU9000,ClientPodMTU9000, andServerPodMTU9000constants (lines 92, 108-116), which are also used by the IPv4 tests. While the tests run sequentially with cleanup between contexts, this creates fragility if:
- Cleanup fails silently
- Tests are run with label filtering (e.g., only IPv6 MTU 9000)
- Future parallel execution is enabled
Consider using IPv6-specific constants (e.g.,
SriovNetworkSamePFMTU9000IPv6) or adding an IPv6 prefix to pod names for complete isolation.
140-141: Critical:FIt(focused test) will skip all other tests!
FItis a Ginkgo focused test that causes only this spec to run, skipping all other tests in the suite. This appears to be leftover debugging code that must be changed toItbefore merging.🔎 Fix the focused test
- FIt("Verify SR-IOV IPv6 connectivity with Static IPAM and Static MAC", + It("Verify SR-IOV IPv6 connectivity with Static IPAM and Static MAC", reportxml.ID("31804"), Label("ipv6", "static-ipam", "static-mac"), func() {
🧹 Nitpick comments (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (1)
656-683: Consider replacing fixed sleeps with a readiness check.The function uses
time.Sleep(10 * time.Second)(line 674) after pod creation. Combined with thesleep 5in the pod command, this adds 15 seconds of fixed delay per node. For reliability across different cluster speeds, consider checking that the SCTP module is actually loaded (e.g., verify/proc/modulescontainssctp) instead of relying on fixed delays.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yamltests/cnf/core/network/sriov/internal/sriovenv/sriovenv.gotests/cnf/core/network/sriov/internal/tsparams/consts.gotests/cnf/core/network/sriov/internal/tsparams/sriovvars.gotests/cnf/core/network/sriov/tests/sriov-ipv4.gotests/cnf/core/network/sriov/tests/sriov-ipv6.gotests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/ocp/sriov/internal/ocpsriovconfig/default.yaml
- tests/cnf/core/network/internal/netconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (5)
tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
TestNamespaceName(7-7)tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (2)
WaitTimeout(17-17)DefaultTimeout(19-19)tests/cnf/core/network/internal/netparam/netvars.go (1)
DefaultTimeout(17-17)tests/ocp/sriov/internal/sriovocpenv/pods.go (1)
ICMPConnectivityCheck(44-73)tests/internal/cluster/cluster.go (1)
WaitForMcpStable(315-329)
🔇 Additional comments (7)
tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
35-98: Well-organized constants for SR-IOV test parameters.The new constants are logically grouped (resources by PF, networks by test scenario, pod prefixes by MTU), consistently named, and properly documented. This structure supports the IPv4/IPv6 test suites effectively.
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (2)
519-548: Verify the header overhead calculation for packet size.The packet size is calculated as
mtu - 100(line 524), which accounts for protocol headers. For ICMP/TCP/UDP over IPv4/IPv6, this should be sufficient, but for SCTP with additional chunk headers, the margin may be tight at lower MTUs.Consider documenting why 100 bytes is used for header overhead, or making it a named constant for clarity.
422-483: Good structure for SR-IOV resource management helpers.The new functions (
CreateSriovPolicyWithMTU,CreateSriovNetworkWithStaticIPAM,DeleteSriovNetworks) follow consistent patterns, handle errors properly, and integrate well with the existing SR-IOV environment utilities.tests/cnf/core/network/sriov/tests/sriov-ipv4.go (2)
62-70: Asymmetric timeouts for VF creation on PF1 vs PF2.The wait for VFs on PF1 uses 10 minutes (line 64) while PF2 uses 5 minutes (line 69). Since both PFs are being configured simultaneously with similar policies, consider using consistent timeouts unless there's a specific reason for the difference.
24-77: Well-structured IPv4 test suite with efficient policy provisioning.The test suite correctly uses the
Ordereddecorator to ensure sequential execution, creates all SR-IOV policies upfront to minimize node reboots, and includes comprehensive setup/teardown at both suite and context levels. The structure aligns well with the IPv6 counterpart.tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (1)
52-107: IPv6 address updates and new test parameters look correct.The IPv6 addresses now use distinct subnets (
2001:100::/64and2001:200::/64), addressing the previous duplicate address concern. The new bare addresses, dual-stack collections, and MTU/VF constants are properly defined and consistent with their CIDR counterparts.tests/cnf/core/network/sriov/tests/sriov-ipv6.go (1)
320-351: Good approach: IPv6 policies use distinct names.The
createAllIPv6SriovPoliciesfunction correctly uses"policy-ipv6-"prefix for all policy names (lines 327, 334, 341, 348), avoiding conflicts with IPv4 policies. This is the right pattern that should also be applied to network and pod names.
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
ab44be0 to
b03b7c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (1)
656-683: Consider removing the extra sleep.Line 674 adds a 10-second sleep after the pod is already running. Since
CreateAndWaitUntilRunningensures the pod is in Running state and the command includessleep 5, the additional 10-second delay may be unnecessary. The module loading viamodprobe sctpshould complete quickly within the pod's execution.🔎 Proposed change to remove extra sleep
debugPod, err := pod.NewBuilder( APIClient, fmt.Sprintf("sctp-enable-%s", node.Definition.Name), tsparams.TestNamespaceName, NetConfig.CnfNetTestContainer). DefineOnNode(node.Definition.Name). WithPrivilegedFlag(). WithHostPid(true). WithHostNetwork(). RedefineDefaultCMD([]string{"bash", "-c", "nsenter -t 1 -m -u -n -i modprobe sctp && sleep 5"}). CreateAndWaitUntilRunning(netparam.DefaultTimeout) if err != nil { return fmt.Errorf("failed to create SCTP enabler pod on node %s: %w", node.Definition.Name, err) } - time.Sleep(10 * time.Second) - _, err = debugPod.DeleteAndWait(netparam.DefaultTimeout) if err != nil { return fmt.Errorf("failed to delete SCTP enabler pod on node %s: %w", node.Definition.Name, err) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yamltests/cnf/core/network/sriov/internal/sriovenv/sriovenv.gotests/cnf/core/network/sriov/internal/tsparams/consts.gotests/cnf/core/network/sriov/internal/tsparams/sriovvars.gotests/cnf/core/network/sriov/tests/sriov-ipv4.gotests/cnf/core/network/sriov/tests/sriov-ipv6.gotests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cnf/core/network/internal/netconfig/default.yaml
- tests/cnf/core/network/sriov/internal/tsparams/consts.go
- tests/cnf/core/network/sriov/tests/sriov-ipv6.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (10)
EnableSCTPOnWorkers(657-683)WaitUntilVfsCreated(153-181)CreateSriovNetworkWithStaticIPAM(443-454)CreateTestClientPod(487-514)CreateTestServerPod(519-562)WaitForServerReady(581-593)DeleteTestPods(565-578)DeleteSriovNetworks(457-483)RunTrafficTest(608-654)CreateSriovPolicyWithMTU(424-440)
🔇 Additional comments (12)
tests/ocp/sriov/internal/ocpsriovconfig/default.yaml (1)
3-3: LGTM!The container image update to
eco-gotests-network-client:v4.20aligns with the broader image refresh across network/SR-IOV test configurations.tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (1)
52-76: LGTM!The IPv6 addressing has been properly corrected to use distinct subnets (2001:100::/64 and 2001:200::/64), and the bare IP addresses correctly match their CIDR counterparts. The dual-stack arrays provide a clean interface for dual-stack pod configuration.
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (7)
6-6: LGTM!The
stringsimport is correctly added and used for IPv6 address detection in theCreateTestServerPodfunction.
423-440: LGTM!The function properly configures SR-IOV policies with MTU and VF range settings using the builder pattern.
442-483: LGTM!Both functions handle SR-IOV network lifecycle correctly:
CreateSriovNetworkWithStaticIPAMproperly configures static IPAM with IP/MAC supportDeleteSriovNetworksgracefully handles non-existent networks and ensures NAD cleanup
485-562: LGTM!Both pod creation functions are well-implemented:
- Client pod uses proper
sleep infinitycommand- Server pod correctly handles IPv6 bracket notation for TCP binding
- Packet size calculation (
mtu - 100) accounts for protocol headers- Multi-protocol listeners (TCP/UDP/SCTP) are spawned in background with proper syntax
564-605: LGTM!Resource management functions are correctly implemented:
DeleteTestPodssafely handles nil pods and propagates errorsWaitForServerReadyuses process checking to verify listener readinessCleanupAllSriovResourcesfollows correct cleanup order (networks before policies)
607-654: LGTM!The traffic test function provides comprehensive connectivity validation across all protocols (ICMP, TCP, UDP, SCTP) with proper MTU handling and error reporting.
685-741: LGTM!The helper functions implement proper cleanup logic:
removePrefixcorrectly strips CIDR notationremoveSriovNetworksensures NAD cleanup after network deletionremoveSriovPoliciesAndWaitForStabilitycorrectly preserves the "default" policy and waits for MCP stabilitytests/cnf/core/network/sriov/tests/sriov-ipv4.go (3)
24-77: LGTM!The suite setup follows Ginkgo best practices:
- Validates cluster prerequisites (minimum 2 worker nodes and 2 SR-IOV interfaces)
- Enables required kernel module (SCTP)
- Creates all SR-IOV policies upfront to minimize node reboots
- Properly waits for VF creation on both PFs
- Ensures cleanup in AfterAll
80-307: LGTM!The three test contexts provide comprehensive SR-IOV IPv4 connectivity coverage:
- Same Node Same PF: Tests basic intra-node communication via single PF
- Same Node Different PF: Validates cross-PF connectivity on the same node
- Different Node: Verifies inter-node SR-IOV communication
Each context follows proper Ginkgo patterns with setup, test execution, and cleanup. The test IDs (31801, 31802, 31803) and labels provide good traceability.
310-340: LGTM!The helper function efficiently creates all SR-IOV policies upfront, minimizing node reboots. The VF ranges (0-2 for MTU 500, 3-5 for MTU 9000) are non-overlapping within each PF and correctly partition the total 6 VFs. This design allows all three test contexts to reuse the same policies without additional reconfigurations.
gkopels
force-pushed
the
add-sriov-table-tests
branch
2 times, most recently
from
3dd8b01 to
22487de
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yamltests/cnf/core/network/sriov/internal/sriovenv/sriovenv.gotests/cnf/core/network/sriov/internal/tsparams/consts.gotests/cnf/core/network/sriov/internal/tsparams/sriovvars.gotests/cnf/core/network/sriov/tests/sriov-ipv4.gotests/cnf/core/network/sriov/tests/sriov-ipv6.gotests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/ocp/sriov/internal/ocpsriovconfig/default.yaml
- tests/cnf/core/network/internal/netconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (9)
EnableSCTPOnWorkers(755-781)WaitUntilVfsCreated(153-181)CleanupAllSriovResources(687-696)CreateSriovNetworkWithStaticIPAM(443-454)CreateSriovNetworkWithWhereaboutsIPAM(459-474)DeleteTestPods(618-631)DeleteSriovNetworks(477-503)CleanupTestResources(635-647)CreateSriovPolicyWithMTU(424-440)
🔇 Additional comments (25)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (7)
1-17: LGTM!The package declaration and imports are well-organized with proper grouping of standard library, Ginkgo/Gomega, and project-specific imports.
19-66: Well-structured test setup with upfront policy creation.The BeforeAll block properly:
- Lists worker nodes and validates minimum count
- Gets and validates SR-IOV interfaces
- Enables SCTP kernel module
- Creates all SR-IOV policies upfront to minimize node reboots
- Waits for VF creation on both PFs
The approach of creating all policies upfront is a good optimization that reduces test execution time.
68-72: LGTM!AfterAll properly cleans up all SR-IOV resources with appropriate timeout.
74-142: LGTM!The "Same Node Same PF" context is well-structured:
- BeforeAll creates the required SR-IOV networks
- AfterEach cleans up test pods between test runs
- AfterAll deletes the SR-IOV networks
- The test case properly creates client/server pods for both MTU 500 and MTU 9000, then runs traffic tests
144-260: LGTM!The "Same Node Different PF" context properly tests:
- Client on PF1 and server on PF2
- Both static IPAM/MAC and Whereabouts IPAM scenarios
- Both MTU 500 and MTU 9000 configurations
The Whereabouts test dynamically discovers the server IP, which is the correct approach for dynamic IP assignment.
262-327: LGTM!The "Different Node" context correctly tests cross-node SR-IOV connectivity with pods scheduled on different worker nodes.
330-360: LGTM!The
createAllSriovPolicieshelper function is well-documented and efficiently creates all four SR-IOV policies (PF1/PF2 × MTU500/MTU9000) upfront to trigger a single node reboot instead of multiple reboots per context.tests/cnf/core/network/sriov/tests/sriov-ipv6.go (5)
1-17: LGTM!Imports are properly organized and consistent with the IPv4 test file.
19-72: LGTM!The BeforeAll and AfterAll blocks follow the same efficient pattern as the IPv4 tests, creating all policies upfront and using MTU 1280 (the IPv6 minimum MTU) instead of MTU 500.
74-175: LGTM!The "Same Node Same PF" context properly tests both static IPAM/MAC and Whereabouts IPAM scenarios for IPv6 with MTU 1280 and MTU 9000.
177-249: LGTM!The "Same Node Different PF" context correctly tests IPv6 connectivity between different PFs on the same node.
321-352: LGTM!The
createAllIPv6SriovPolicieshelper is well-documented and correctly notes that IPv6 requires minimum MTU of 1280. The VF ranges (0-3 for MTU 1280, 4-5 for MTU 9000) do not overlap, which is correct.tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
35-131: Well-organized constants with clear naming conventions.The new constants are:
- Properly grouped by purpose (SR-IOV resources, network names, pod names, IPAM configurations)
- Clearly labeled for IPv4 vs IPv6 usage (e.g., MTU500 for IPv4, MTU1280 for IPv6 minimum)
- Following consistent naming patterns
- Documented with godot-compliant comments
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (3)
43-107: LGTM!The per-context IP addresses are properly organized and each context uses distinct subnets:
- Context 1: 192.168.0.x/192.168.1.x (IPv4), 2001:100::/2001:200:: (IPv6)
- Context 2: 192.168.10.x/192.168.11.x (IPv4), 2001:110::/2001:210:: (IPv6)
- Context 3: 192.168.20.x/192.168.21.x (IPv4), 2001:120::/2001:220:: (IPv6)
This avoids IP conflicts between contexts.
109-175: LGTM!Bare IP addresses and MAC addresses are properly organized per context with no conflicts. Each context uses a distinct range of MAC addresses (Context 1: 0x01-04, Context 2: 0x11-14, Context 3: 0x21-24).
177-197: VF ranges for MTU 500 and MTU 1280 overlap, which is acceptable for sequential test execution.The VF ranges are:
- MTU 500 (IPv4): VFs 0-2
- MTU 1280 (IPv6): VFs 0-3
- MTU 9000: VFs 4-5
The overlap between MTU 500 and MTU 1280 is acceptable because:
- IPv4 tests use MTU 500 + MTU 9000
- IPv6 tests use MTU 1280 + MTU 9000
- Each suite's
AfterAllcallsCleanupAllSriovResourceswhich removes all policies before the next suite runsThis design allows both suites to maximize VF usage (6 total) while maintaining isolation through cleanup.
Please confirm that IPv4 and IPv6 test suites are intended to run sequentially (not in parallel) and that cleanup between suites prevents VF range conflicts.
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (9)
422-440: LGTM!The
CreateSriovPolicyWithMTUfunction correctly creates an SR-IOV policy with MTU and VF range configuration using the builder pattern.
442-474: LGTM!Both network creation functions are well-implemented:
CreateSriovNetworkWithStaticIPAMenables IP address and MAC address supportCreateSriovNetworkWithWhereaboutsIPAMcorrectly builds the IPAM JSON for whereabouts
476-503: LGTM!The
DeleteSriovNetworksfunction properly handles the case where a network doesn't exist and waits for NAD deletion after removing each network.
505-591: Consider adding timeout handling for the shell command in server mode.The server pod command uses an inline shell script that discovers the IP dynamically. If the
ipcommand fails or takes too long, the testcmd processes may not start correctly. The current implementation relies onsleep 5to give time for IP assignment, which should be sufficient in most cases.The logic for handling IPv6 addresses (adding brackets for TCP) is correct.
593-631: LGTM!The client/server pod creation wrappers and deletion function are clean and well-documented. The server pod wrapper correctly waits for testcmd readiness.
649-684: LGTM!
WaitForServerReadyusespgrepto check if testcmd is running, andGetPodIPFromInterfacecorrectly extracts the IP from the interface output without the prefix.
698-752: Good traffic test implementation with comprehensive protocol coverage.The
RunTrafficTestfunction tests ICMP, TCP, UDP, and SCTP connectivity. The approach of usingmtu - 100for packet size to account for headers is reasonable.One note: The ICMP test adds
/32or/128prefix to the bare IP forICMPConnectivityCheck, which expects CIDR notation. This is correctly handled.
754-781: LGTM!The
EnableSCTPOnWorkersfunction correctly usesnsenterto load the SCTP kernel module on the host via a privileged pod.
783-839: LGTM!The cleanup functions are well-implemented:
removePrefixis a simple helper to strip CIDR notationremoveSriovNetworksproperly waits for NAD deletionremoveSriovPoliciesAndWaitForStabilityskips the "default" policy and waits for MCP stability
| It("Verify SR-IOV IPv6 connectivity between different nodes", | ||
| reportxml.ID("31806"), Label("ipv6", "static-ipam", "static-mac", "diff-node"), func() { |
There was a problem hiding this comment.
Duplicate report ID 31806 used for different test cases.
The report ID 31806 is used both here (line 307) and for the Whereabouts IPAM test on line 152. Each test case should have a unique report ID for proper tracking and reporting.
🔎 Suggested fix
It("Verify SR-IOV IPv6 connectivity between different nodes",
- reportxml.ID("31806"), Label("ipv6", "static-ipam", "static-mac", "diff-node"), func() {
+ reportxml.ID("31808"), Label("ipv6", "static-ipam", "static-mac", "diff-node"), func() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| It("Verify SR-IOV IPv6 connectivity between different nodes", | |
| reportxml.ID("31806"), Label("ipv6", "static-ipam", "static-mac", "diff-node"), func() { | |
| It("Verify SR-IOV IPv6 connectivity between different nodes", | |
| reportxml.ID("31808"), Label("ipv6", "static-ipam", "static-mac", "diff-node"), func() { |
🤖 Prompt for AI Agents
In tests/cnf/core/network/sriov/tests/sriov-ipv6.go around lines 306-307, the
test uses reportxml.ID("31806") which duplicates the Whereabouts IPAM test at
line 152; change this report ID to a unique unused ID (e.g., "31807" or another
free ID in the test suite), update only this test's reportxml.ID invocation, and
scan for any other duplicate IDs to ensure each test case has a distinct report
ID.
22487de to
bf0900d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (2)
606-642: Complex server command logic handles both static and dynamic IP scenarios.The server pod command generation correctly:
- Discovers IP at runtime for Whereabouts IPAM scenarios
- Wraps IPv6 addresses in brackets for TCP binding
- Uses
mtu - 100for packet size to account for headersThe bash script approach is necessary for dynamic IP discovery but makes debugging harder. Consider adding a brief comment explaining the
-100offset calculation.🔎 Suggested documentation improvement
if isServer { containerName = "server" - // Use mtu-100 for packet size to match client (accounting for headers). + // Use mtu-100 for packet size: 20 bytes IP header + 8-60 bytes transport header + buffer. packetSize := mtu - 100
714-727: Consider more specific process check forWaitForServerReady.Using
pgrep -f testcmdmay match unintended processes. A more specific check likepgrep -f "testcmd.*-listen"would ensure only the server listeners are detected.🔎 Suggested improvement
return wait.PollUntilContextTimeout(context.TODO(), 2*time.Second, timeout, true, func(ctx context.Context) (bool, error) { - _, err := serverPod.ExecCommand([]string{"bash", "-c", "pgrep -f testcmd"}) + _, err := serverPod.ExecCommand([]string{"bash", "-c", "pgrep -f 'testcmd.*-listen'"}) if err != nil { return false, nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yamltests/cnf/core/network/sriov/internal/sriovenv/sriovenv.gotests/cnf/core/network/sriov/internal/tsparams/consts.gotests/cnf/core/network/sriov/internal/tsparams/sriovvars.gotests/cnf/core/network/sriov/tests/sriov-ipv4.gotests/cnf/core/network/sriov/tests/sriov-ipv6.gotests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cnf/core/network/sriov/tests/sriov-ipv6.go
- tests/cnf/core/network/internal/netconfig/default.yaml
- tests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (3)
tests/cnf/core/network/sriov/internal/tsparams/consts.go (22)
LabelSuite(5-5)SriovNetworkSamePFMTU500(55-55)SriovResourcePF1MTU500(40-40)SriovNetworkSamePFMTU9000(59-59)SriovResourcePF1MTU9000(44-44)SriovNetworkVlanSamePFMTU500(135-135)WhereaboutsIPv4Range(121-121)WhereaboutsIPv4Gateway(123-123)ClientPodVlan(147-147)ServerPodVlan(149-149)SriovNetworkDiffPFClientMTU500(64-64)SriovNetworkDiffPFClientMTU9000(72-72)SriovNetworkDiffPFServerMTU500(66-66)SriovResourcePF2MTU500(46-46)SriovNetworkDiffPFServerMTU9000(74-74)SriovResourcePF2MTU9000(50-50)SriovNetworkWhereaboutsDiffPFClientMTU500(107-107)SriovNetworkWhereaboutsDiffPFServerMTU500(109-109)ClientPodWhereabouts(128-128)ServerPodWhereabouts(130-130)SriovNetworkDiffNodeMTU500(79-79)SriovNetworkDiffNodeMTU9000(83-83)tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (5)
EnableSCTPOnWorkers(820-846)GetPodIPFromInterface(732-749)CreateSriovNetworkWithWhereaboutsIPAM(487-502)PolicyConfig(424-432)CreateSriovPolicies(436-449)tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (32)
TotalVFs(185-185)MCOWaitTimeout(25-25)ClientIPv4IPAddress(46-46)ClientMacAddress(147-147)ServerIPv4IPAddress(48-48)ServerIPv4Bare(114-114)ServerMacAddress(149-149)MTU500(178-178)ClientIPv4IPAddress2(50-50)ClientMacAddress2(151-151)ServerIPv4IPAddress2(52-52)ServerIPv4Bare2(116-116)ServerMacAddress2(153-153)MTU9000(182-182)ClientIPv4IPAddressCtx2(65-65)ClientMacAddressCtx2(158-158)ServerIPv4IPAddressCtx2(67-67)ServerMacAddressCtx2(160-160)ClientMacAddress2Ctx2(162-162)ServerIPv4Bare2Ctx2(127-127)ClientIPv4IPAddressCtx3(84-84)ServerIPv4BareCtx3(136-136)ServerMacAddressCtx3(171-171)ClientIPv4IPAddress2Ctx3(88-88)ClientMacAddress2Ctx3(173-173)ServerIPv4IPAddress2Ctx3(90-90)ServerIPv4Bare2Ctx3(138-138)ServerMacAddress2Ctx3(175-175)VFStartMTU500(187-187)VFEndMTU500(189-189)VFStartMTU9000(195-195)VFEndMTU9000(197-197)
🔇 Additional comments (14)
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (3)
43-98: Well-structured multi-context IP addressing scheme.The IP address allocation follows a clear pattern:
- Context 1 (Same Node Same PF): 192.168.0.x, 192.168.1.x, 2001:100::/64, 2001:200::/64
- Context 2 (Same Node Different PF): 192.168.10.x, 192.168.11.x, 2001:110::/64, 2001:210::/64
- Context 3 (Different Node): 192.168.20.x, 192.168.21.x, 2001:120::/64, 2001:220::/64
All subnets are now unique, avoiding IP conflicts between contexts.
184-197: VF ranges for MTU500 and MTU1280 still overlap.
VFStartMTU500/VFEndMTU500(0-2) overlaps withVFStartMTU1280/VFEndMTU1280(0-3). Since MTU500 is used by IPv4 tests and MTU1280 by IPv6 tests, this works if the suites run exclusively. However, if both test suites ever run in the same session targeting the same PFs, the policies will conflict.Please confirm that the IPv4 and IPv6 test suites are designed to run mutually exclusively (e.g., via label selection like
--ginkgo.label-filter="ipv4"vs"ipv6").
100-107: LGTM!Dual-stack IP slices correctly combine the IPv4 and IPv6 addresses for each context.
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (5)
30-66: Good pattern: Creating all policies upfront to minimize node reboots.The approach of creating all SR-IOV policies in
BeforeAlland waiting for VF creation is efficient - it triggers a single node reboot instead of multiple reboots per context. The staggered timeouts (10min for PF1, 5min for PF2) correctly account for the reboot happening after PF1 policy creation.
179-295: LGTM!The "Same Node Different PF" context correctly:
- Creates separate networks using different PF resources (PF1 for client, PF2 for server)
- Uses Context 2 addresses to avoid IP conflicts with Context 1
- Includes both static IPAM and Whereabouts IPAM test cases
297-362: LGTM!The "Different Node" context correctly places client and server pods on separate worker nodes and uses Context 3 addresses. The combined resource cleanup in
AfterAllis appropriate since pods are created inBeforeAll.
365-411: LGTM!The
createAllSriovPolicieshelper consolidates policy creation, improving code organization and enabling batch policy application for efficient node reboot handling.
91-98: This review comment is incorrect.ECO_CNF_CORE_NET_VLANis a mandatory environment variable documented in the project README, not an optional configuration. The test correctly fails duringBeforeAllif it is not set, which is the intended behavior. No skip condition is needed.Likely an incorrect or invalid review comment.
tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
35-150: LGTM!The constants are well-organized with clear naming conventions:
- Resource names include PF number and MTU (e.g.,
sriovpf1mtu500)- Network names indicate context and MTU (e.g.,
sriov-net-samepf-mtu500)- Pod prefixes are descriptive (e.g.,
client-mtu500)The Whereabouts IPAM ranges use distinct subnets (
192.168.100.0/24for IPv4,2001:300::/64for IPv6) that don't overlap with the static IP ranges.tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (5)
434-449: LGTM!
CreateSriovPoliciescorrectly batches policy creation and uses proper error wrapping. The caller is responsible for waiting for VF creation, which is the intended design for minimizing node reboots.
763-817: LGTM!
RunTrafficTestcomprehensively validates connectivity using ICMP, TCP, UDP, and SCTP. The CIDR prefix addition forICMPConnectivityCheckis handled correctly for both IPv4 (/32) and IPv6 (/128).
819-846: LGTM!The SCTP kernel module enabler correctly uses
nsenterwith host PID namespace to load the module on each worker node. The privileged pod approach is appropriate for this kernel-level operation.
886-904: LGTM!The cleanup function correctly preserves the "default" SR-IOV policy while removing test-created policies. Waiting for MCP stability ensures the cluster is ready for subsequent test runs.
470-482: MTU configuration is handled at the policy level, not the network level.SR-IOV MTU is properly configured via
CreateSriovPolicyWithMTU(), which sets MTU on the policy builder. The network definitions (IPAM, IP/MAC support, VLAN) are separate from MTU configuration—the network inherits MTU from the underlying VF policy. NoWithMTU()call is needed on the network builder.Likely an incorrect or invalid review comment.
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
bf0900d to
d9a9617
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/cnf/core/network/sriov/tests/sriov-ipv6.go (1)
315-316: Duplicate report ID31806still present.As noted in a previous review, this test uses the same
reportxml.ID("31806")as the Whereabouts IPAM test on line 152. Each test should have a unique report ID for proper tracking.🔎 Suggested fix
It("Verify SR-IOV IPv6 connectivity between different nodes", - reportxml.ID("31806"), Label("ipv6", "static-ipam", "static-mac", "diff-node"), func() { + reportxml.ID("31809"), Label("ipv6", "static-ipam", "static-mac", "diff-node"), func() {
🧹 Nitpick comments (3)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (2)
606-642: Complex server command may be fragile with dynamic IP discovery.The dynamic IP discovery command (
ip -o addr show net1 | awk '{print $4}' | cut -d'/' -f1 | head -1) works correctly for both IPv4 and IPv6. However, thehead -1may pick a link-local IPv6 address if it's listed first. Consider filtering for global addresses.🔎 More robust IP discovery (optional)
if serverBindIP == "" { // Dynamic IP: discover IP from net1 interface at runtime. podCmd = []string{"bash", "-c", fmt.Sprintf( "sleep 5; "+ - "SERVER_IP=$(ip -o addr show net1 | awk '{print $4}' | cut -d'/' -f1 | head -1); "+ + "SERVER_IP=$(ip -o -f inet addr show net1 | awk '{print $4}' | cut -d'/' -f1 | head -1); "+ + "if [ -z \"$SERVER_IP\" ]; then SERVER_IP=$(ip -o -f inet6 addr show net1 scope global | awk '{print $4}' | cut -d'/' -f1 | head -1); fi; "+ "echo \"Discovered server IP: $SERVER_IP\"; "+This change first tries to get IPv4, then falls back to global-scope IPv6 (excluding link-local addresses).
819-846: SCTP enablement approach is reasonable but sleep timing is arbitrary.The use of
nsenterto load the SCTP kernel module on the host is a good approach. However, thesleep 10after pod creation is arbitrary. Consider verifying that the module is loaded (e.g., check/proc/modulesorlsmod) rather than relying on a fixed sleep.🔎 Optional: Verify module loaded (nitpick)
RedefineDefaultCMD([]string{"bash", "-c", "nsenter -t 1 -m -u -n -i modprobe sctp && sleep 5"}). CreateAndWaitUntilRunning(netparam.DefaultTimeout) if err != nil { return fmt.Errorf("failed to create SCTP enabler pod on node %s: %w", node.Definition.Name, err) } - time.Sleep(10 * time.Second) + // Wait for module to be loaded + _, err = debugPod.ExecCommand([]string{"bash", "-c", "nsenter -t 1 -m lsmod | grep -q sctp"}) + if err != nil { + klog.V(90).Infof("Warning: Could not verify SCTP module loaded on node %s", node.Definition.Name) + }tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (1)
184-197: VF ranges for MTU 500 and MTU 1280 overlap (VFs 0-2).The VF ranges
VFStartMTU500=0, VFEndMTU500=2andVFStartMTU1280=0, VFEndMTU1280=3overlap at indices 0-2. This works because IPv4 tests exclusively use MTU 500 while IPv6 tests use MTU 1280, and each test suite runs its ownAfterAllcleanup viaCleanupAllSriovResources. However, this design creates implicit coupling between test suites.Consider using non-overlapping VF ranges (e.g.,
VFEndMTU500=1, VFStartMTU1280=2, VFEndMTU1280=3) for clearer test isolation and to prevent issues if tests ever run in parallel.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yamltests/cnf/core/network/sriov/internal/sriovenv/sriovenv.gotests/cnf/core/network/sriov/internal/tsparams/consts.gotests/cnf/core/network/sriov/internal/tsparams/sriovvars.gotests/cnf/core/network/sriov/tests/sriov-ipv4.gotests/cnf/core/network/sriov/tests/sriov-ipv6.gotests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (3)
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (1)
WaitTimeout(17-17)tests/ocp/sriov/internal/sriovocpenv/pods.go (1)
ICMPConnectivityCheck(44-73)tests/internal/cluster/cluster.go (1)
WaitForMcpStable(315-329)
🔇 Additional comments (14)
tests/cnf/core/network/internal/netconfig/default.yaml (1)
3-3: Version v4.20 image confirmed to exist and is properly aligned.The container image update to v4.20 is verified in the Quay.io registry and aligns with the corresponding update in tests/ocp/sriov/internal/ocpsriovconfig/default.yaml. The changes are appropriate.
tests/cnf/core/network/sriov/internal/tsparams/consts.go (1)
35-150: Well-organized constant definitions for SR-IOV test infrastructure.The new constants are well-structured with clear categorization (resources, networks, pods, IPAM configs) and follow consistent naming patterns. The documentation is comprehensive with proper godot-compliant comments.
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (5)
19-28: Test suite structure follows Ginkgo best practices.The test suite is properly configured with
Orderedfor sequential execution andContinueOnFailurefor resilience. The variable declarations are well-organized.
30-72: Efficient test setup with upfront policy creation.The approach of creating all SR-IOV policies in
BeforeAllto minimize node reboots is a good optimization. The different timeouts for PF1 (10m) and PF2 (5m) make sense since PF2 setup starts after PF1.
75-177: Same Node Same PF context is well-structured.Good test isolation with network setup in
BeforeAll, pod cleanup inAfterEach, and network cleanup inAfterAll. The VLAN test correctly retrieves the dynamically assigned server IP usingGetPodIPFromInterface.
298-362: Different Node context creates pods in BeforeAll.Note that unlike other contexts, the "Different Node" context creates pods in
BeforeAllrather than inItblocks. This is reasonable for cross-node tests where pod placement (client on node 0, server on node 1) needs to be established once and reused. However, if additional test cases are added to this context that require different pod configurations, the current structure would need refactoring.
365-411: Clean helper function for policy creation.The
createAllSriovPoliciesfunction properly encapsulates policy configuration and delegates tosriovenv.CreateSriovPolicies. This aligns well with the similarcreateAllIPv6SriovPoliciesfunction in the IPv6 test file.tests/cnf/core/network/sriov/tests/sriov-ipv6.go (2)
19-28: IPv6 test suite properly uses MTU 1280 minimum.The test suite correctly uses MTU 1280 instead of MTU 500 for IPv6, respecting the IPv6 minimum MTU requirement. The structure mirrors the IPv4 test suite appropriately.
330-377: IPv6 policy helper properly documented.The comment on line 332 clearly explains why MTU 1280 is used instead of MTU 500 for IPv6 tests. This is helpful for maintainability.
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (1)
43-98: Well-organized per-context IP address constants.The IP address organization with distinct subnets per context (Context 1:
192.168.0.x, Context 2:192.168.10.x, Context 3:192.168.20.x) ensures proper isolation between test contexts. The same pattern is applied to IPv6 addresses.tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (4)
422-449: Well-designed PolicyConfig struct and bulk creation function.The
PolicyConfigstruct provides a clean abstraction for policy configuration, andCreateSriovPoliciesenables batch policy creation to minimize node reboots. Good error handling with wrapped errors.
763-817: Comprehensive traffic testing covering ICMP, TCP, UDP, and SCTP.The
RunTrafficTestfunction tests all four traffic types with consistent packet size calculation (mtu - 100). Good use of detailed logging at V(90) level for debugging. Error messages include command output for easier troubleshooting.
751-761: Cleanup order is correct: networks before policies.The
CleanupAllSriovResourcesfunction correctly removes networks before policies. This order is important because SR-IOV networks reference policies, and deleting policies first could leave dangling references.
729-749: GetPodIPFromInterface utility is clean and well-documented.The function correctly strips the CIDR prefix and validates non-empty results. The use of
head -1is acceptable here since Whereabouts assigns a specific IP that should be first in the listing.
gkopels
force-pushed
the
add-sriov-table-tests
branch
from
d9a9617 to
2fb9949
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (1)
184-197: Document that MTU500 and MTU1280 VF ranges are mutually exclusive.The VF ranges for MTU500 (0-2) and MTU1280 (0-3) overlap. This works because
CreateAllSriovPoliciesuses theisIPv6flag to choose between them—IPv4 tests use MTU500, IPv6 tests use MTU1280. Consider adding a comment here to clarify this mutual exclusivity for future maintainers.🔎 Suggested documentation
+ // Note: VFStartMTU500/VFEndMTU500 and VFStartMTU1280/VFEndMTU1280 ranges overlap intentionally. + // IPv4 tests use MTU500 VF range, IPv6 tests use MTU1280 VF range. + // These test suites are mutually exclusive (controlled by isIPv6 flag in CreateAllSriovPolicies). + // TotalVFs represents the total number of VFs to create. TotalVFs = 6tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (2)
674-710: Consider using a structured approach for server command construction.The shell command construction with
fmt.Sprintfis functional but could be fragile. The dynamic IP discovery using shell commands (ip -o addr show) works but relies on specific output format.The current implementation is acceptable for test code, but consider extracting the command construction into a helper function for better testability if this grows more complex.
920-947: Consider reducing the hardcoded sleep duration.The 10-second sleep after loading the SCTP module may be excessive. Consider using a verification mechanism instead, or at minimum, extract this as a configurable constant.
🔎 Suggested improvement
+const sctpModuleLoadWaitTime = 10 * time.Second + func EnableSCTPOnWorkers(workerNodes []*nodes.Builder) error { for _, node := range workerNodes { // ... pod creation ... - time.Sleep(10 * time.Second) + time.Sleep(sctpModuleLoadWaitTime) // ... pod deletion ... } return nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/cnf/core/network/internal/netconfig/default.yamltests/cnf/core/network/sriov/internal/sriovenv/sriovenv.gotests/cnf/core/network/sriov/internal/tsparams/consts.gotests/cnf/core/network/sriov/internal/tsparams/sriovvars.gotests/cnf/core/network/sriov/tests/sriov-ipv4.gotests/cnf/core/network/sriov/tests/sriov-ipv6.gotests/ocp/sriov/internal/ocpsriovconfig/default.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cnf/core/network/sriov/tests/sriov-ipv6.go
🧰 Additional context used
🧬 Code graph analysis (2)
tests/cnf/core/network/sriov/tests/sriov-ipv4.go (1)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (15)
ValidateSriovInterfaces(30-57)EnableSCTPOnWorkers(921-947)CreateAllSriovPolicies(437-500)WaitUntilVfsCreated(153-181)CleanupAllSriovResources(853-862)CreateSriovNetworkWithStaticIPAM(539-550)CreateSriovNetworkWithVLANAndWhereabouts(590-607)DeleteTestPods(751-764)DeleteSriovNetworks(610-636)CreateTestClientPod(728-731)CreateTestServerPod(735-748)RunTrafficTest(866-918)GetPodIPFromInterface(833-850)CreateSriovNetworkWithWhereaboutsIPAM(555-570)CleanupTestResources(768-780)
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (6)
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (9)
MTU1280(180-180)VFStartMTU1280(191-191)VFEndMTU1280(193-193)MTU500(178-178)VFStartMTU500(187-187)VFEndMTU500(189-189)TotalVFs(185-185)MTU9000(182-182)DefaultTimeout(19-19)tests/cnf/core/network/sriov/internal/tsparams/consts.go (6)
SriovResourcePF1MTU1280(45-45)SriovResourcePF2MTU1280(51-51)SriovResourcePF1MTU500(43-43)SriovResourcePF2MTU500(49-49)SriovResourcePF1MTU9000(47-47)TestNamespaceName(7-7)tests/ocp/sriov/internal/tsparams/consts.go (1)
TestNamespaceName(7-7)tests/ocp/sriov/internal/tsparams/ocpsriovvars.go (1)
DefaultTimeout(20-20)tests/cnf/core/network/internal/netparam/netvars.go (1)
DefaultTimeout(17-17)tests/ocp/sriov/internal/sriovocpenv/pods.go (1)
ICMPConnectivityCheck(44-73)
🔇 Additional comments (24)
tests/ocp/sriov/internal/ocpsriovconfig/default.yaml (1)
3-3: Image name changed fromeco-gotests-sriov-clienttoeco-gotests-network-client.The container image has been changed from
eco-gotests-sriov-client:v4.15.2toeco-gotests-network-client:v4.20. This aligns with the CNF network test container update. Verify that the network-client image contains all necessary SR-IOV test tooling (e.g.,testcmdbinary).tests/cnf/core/network/sriov/internal/tsparams/consts.go (4)
35-53: LGTM! Well-organized SR-IOV resource constants.The constants are clearly named and organized by PF and MTU values. The consolidation strategy (grouping by PF to minimize reboots) is documented in the comments.
55-101: LGTM! Network and pod name constants are well-structured.The naming convention is consistent across all contexts (Same PF, Different PF, Different Node) and MTU values, making the test code more maintainable.
103-133: LGTM! Whereabouts IPAM configuration constants.The IPv4 (
192.168.100.0/24) and IPv6 (2001:300::/64) ranges are appropriate for testing with Whereabouts IPAM.
135-152: LGTM! VLAN test constants complete the test coverage matrix.VLAN network names for Same PF and Different PF contexts are properly defined with clear naming.
tests/cnf/core/network/sriov/internal/tsparams/sriovvars.go (4)
43-60: LGTM! Context 1 IP addresses are well-defined with unique subnets.IPv4 uses
192.168.0.x/24and192.168.1.x/24(MTU 9000), IPv6 uses2001:100::/64and2001:200::/64(MTU 9000). The subnets are distinct and won't conflict.
62-98: LGTM! Context 2 and Context 3 IP addresses use unique subnets.Each context has distinct IP ranges that avoid conflicts:
- Context 2:
192.168.10.x,192.168.11.x,2001:110::/64,2001:210::/64- Context 3:
192.168.20.x,192.168.21.x,2001:120::/64,2001:220::/64
100-142: LGTM! DualStack slices and bare addresses are correctly defined.The bare IP addresses (without CIDR prefix) align with their corresponding CIDR addresses, which is necessary for server binding.
144-175: LGTM! MAC addresses are unique across all contexts.Each context uses a distinct MAC address range:
- Context 1:
20:04:0f:f1:88:0x- Context 2:
20:04:0f:f1:88:1x- Context 3:
20:04:0f:f1:88:2xtests/cnf/core/network/sriov/tests/sriov-ipv4.go (6)
19-28: LGTM! Test suite declaration with appropriate options.Using
Orderedensures sequential execution of contexts, andContinueOnFailureallows subsequent tests to run even if earlier ones fail, which is useful for comprehensive test reporting.
30-67: Well-structured BeforeAll with single-reboot policy creation.Creating all SR-IOV policies upfront and waiting for VF creation before running tests is an efficient approach that minimizes node reboots.
69-73: AfterAll cleanup handles SR-IOV resource removal.The cleanup correctly uses
CleanupAllSriovResourceswith MCO wait timeout for policy removal and MCP stability.
76-178: LGTM! Same Node Same PF context covers static IPAM and VLAN/Whereabouts tests.The context properly:
- Creates networks in BeforeAll
- Cleans up pods in AfterEach
- Deletes networks in AfterAll
- Tests both static IP/MAC and dynamic IP/MAC (Whereabouts) scenarios
181-296: LGTM! Same Node Different PF context tests cross-PF connectivity.The test correctly uses Context 2 IP/MAC addresses and creates separate client/server networks on PF1/PF2 respectively.
299-363: LGTM! Different Node context validates cross-node SR-IOV connectivity.The context correctly places client and server pods on different worker nodes and uses Context 3 addresses.
tests/cnf/core/network/sriov/internal/sriovenv/sriovenv.go (8)
422-432: LGTM! PolicyConfig struct provides clear configuration for SR-IOV policies.The struct captures all necessary parameters for policy creation with VF range support.
434-500: LGTM! CreateAllSriovPolicies consolidates policy creation for efficiency.The function correctly chooses MTU and VF ranges based on the
isIPv6flag, creating policies for both PFs in a single batch to minimize node reboots.
502-536: LGTM! Policy creation functions are well-structured.
CreateSriovPoliciesiterates through configurations andCreateSriovPolicyWithMTUcreates individual policies with proper MTU and VF range settings.
538-607: LGTM! Network creation helpers cover all required IPAM and VLAN scenarios.The functions properly configure:
- Static IPAM with IP/MAC address support
- Whereabouts IPAM with dynamic IP assignment
- VLAN tagging with optional Whereabouts IPAM
609-636: LGTM! DeleteSriovNetworks handles missing networks gracefully.The function skips networks that don't exist and properly waits for NAD deletion after removing each network.
782-795: LGTM! WaitForServerReady polls for testcmd process availability.Using
pgrep -f testcmdis a reliable way to check if the server listeners are running.
864-918: LGTM! RunTrafficTest provides comprehensive connectivity validation.The function tests all four protocols (ICMP, TCP, UDP, SCTP) with appropriate packet sizes and detailed logging for debugging.
987-1005: LGTM! Policy cleanup correctly preserves the default policy.The function iterates through all policies, deletes non-default ones, and waits for MCP stability.
tests/cnf/core/network/internal/netconfig/default.yaml (1)
3-3: Verify that the container image v4.20 is published and compatible with the test environment.The container image version has been updated to
v4.20. Confirm this version is available in the quay.io registry and is compatible with your test environment before merging.
1 participant
Summary by CodeRabbit
Tests
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.