Skip to content

Conversation

@evgenLevin
Copy link
Collaborator

@evgenLevin evgenLevin commented Jan 2, 2026

Summary by CodeRabbit

  • Tests
    • Enhanced MetalLB test suite with additional runtime validations for BGP session states, address pool configurations, and service status across multiple test scenarios to improve test reliability and coverage.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 2, 2026

📝 Walkthrough

Walkthrough

This PR adds comprehensive validation helpers for MetalLB BGP tests (address pool state, BGP session state, service BGP status) to common.go and integrates them throughout multiple test files. Function signatures in bfd-test.go are updated to accept peer IP parameters. A test setup function (setupTestEnv) is migrated from bgp-tests.go to common.go.

Changes

Cohort / File(s) Summary
Common validation infrastructure
tests/cnf/core/network/metallb/tests/common.go
Added four new validation functions: setupTestEnv (initializes test environment with BGPPeer, IPAddressPool, BGPAdvertisement), validateaddresspool (verifies IP pool status fields), validatebgpsessionstate (polls BGP/BFD session states), and validateservicebgpstatus (verifies service-level BGP status). Also added pool validation after L2 advertisement setup.
BFD test updates
tests/cnf/core/network/metallb/tests/bfd-test.go
Updated function signatures for testBFDFailOver and testBFDFailBack to accept peerIP string parameter. Added explicit BGP/BFD state validation calls with peer IP throughout test flows.
BGP test validations
tests/cnf/core/network/metallb/tests/bgp-connect-time-tests.go, bgp-multiservice.go, bgp-remote-as-dynamic.go, bgp-unnumbered.go, frrk8-tests.go
Inserted validation calls for address pools, BGP session states, and service BGP statuses at multiple test checkpoints. New imports added to bgp-unnumbered.go and pool-selector.go for nodes package.
Test setup refactoring
tests/cnf/core/network/metallb/tests/bgp-tests.go
Removed setupTestEnv function (now defined in common.go). Removed associated imports (fmt, define, corev1). Existing test code still references setupTestEnv, which may cause compilation errors unless resolved.
Layer 2 and MetalLB tests
tests/cnf/core/network/metallb/tests/layer2-test.go, metallb-crds.go, metallb-nodeSelector.go, pool-selector.go
Added validation calls for address pool state and BGP session states after pod creation and configuration setup. metallb-nodeSelector.go added imports for nodes and netcmd packages.
Cleanup updates
tests/cnf/core/network/metallb/tests/system-metallb.go
Added service.GetGVR() to AfterEach cleanup routine to remove Service resources during test teardown.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • gkopels
  • ajaggapa

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely summarizes the main change: adding BGP session state and address pool validation to MetalLB tests across multiple test files.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (5)
tests/cnf/core/network/metallb/tests/common.go (1)

878-891: Consider using Eventually for status validation.

The function directly checks ipAddressPool.Object.Status fields without waiting for status propagation. If there's any delay in status updates after pool creation or IP assignment, this could cause flaky tests.

Consider wrapping with Eventually similar to other validation patterns in the codebase, or ensure callers provide sufficient delay before calling this function.

🔎 Proposed enhancement
 func validateaddresspool(ipAddressPoolName string, availableIPv4, availableIPv6, assignedIPv4, assignedIPv6 int64) {
-	ipAddressPool, err := metallb.PullAddressPool(APIClient, ipAddressPoolName, NetConfig.MlbOperatorNamespace)
-	Expect(err).ToNot(HaveOccurred(), "Failed to pull IPAddressPool %s", ipAddressPoolName)
-
-	Expect(ipAddressPool.Object.Status.AvailableIPv4).To(Equal(availableIPv4),
-		"Failed to verify address pool available IPv4")
-	Expect(ipAddressPool.Object.Status.AvailableIPv6).To(Equal(availableIPv6),
-		"Failed to verify address pool available IPv6")
-	Expect(ipAddressPool.Object.Status.AssignedIPv4).To(Equal(assignedIPv4),
-		"Failed to verify address pool assigned IPv4")
-	Expect(ipAddressPool.Object.Status.AssignedIPv6).To(Equal(assignedIPv6),
-		"Failed to verify address pool assigned IPv6")
+	Eventually(func() bool {
+		ipAddressPool, err := metallb.PullAddressPool(APIClient, ipAddressPoolName, NetConfig.MlbOperatorNamespace)
+		if err != nil {
+			return false
+		}
+		return ipAddressPool.Object.Status.AvailableIPv4 == availableIPv4 &&
+			ipAddressPool.Object.Status.AvailableIPv6 == availableIPv6 &&
+			ipAddressPool.Object.Status.AssignedIPv4 == assignedIPv4 &&
+			ipAddressPool.Object.Status.AssignedIPv6 == assignedIPv6
+	}, 30*time.Second, tsparams.DefaultRetryInterval).Should(BeTrue(),
+		"Failed to verify address pool %s state: expected available=%d/%d, assigned=%d/%d",
+		ipAddressPoolName, availableIPv4, availableIPv6, assignedIPv4, assignedIPv6)
 }
tests/cnf/core/network/metallb/tests/bfd-test.go (1)

125-125: Document or use named constants for validation parameters.

The numeric parameters 239, 0, 1, 0 in validateaddresspool are magic numbers without clear meaning. Consider adding inline comments explaining what each parameter represents (e.g., total capacity, in-use, available, reserved) or using named constants.

Example:

// validateaddresspool checks: totalCapacity=239, inUse=0, available=1, reserved=0
validateaddresspool(tsparams.BGPAdvAndAddressPoolName, 239, 0, 1, 0)
tests/cnf/core/network/metallb/tests/bgp-connect-time-tests.go (1)

103-103: Consider optional parameter or overload for BFD-agnostic validation.

The validation uses "N/A" as a string literal for BFD status since BFD is not enabled. While this works, consider whether the validation helper API could be improved with optional parameters or a separate function for BGP-only validation to avoid magic strings.

Example alternatives:

// Option 1: Separate function
validatebgpsessionstateOnly("Established", ipv4metalLbIPList[0], workerNodeList)

// Option 2: Optional parameter (if Go supports it via variadic args)
validatebgpsessionstate("Established", ipv4metalLbIPList[0], workerNodeList, WithBFDStatus("Up"))
tests/cnf/core/network/metallb/tests/bgp-multiservice.go (1)

72-76: Document validation parameters for address pool state.

Similar to other files, the numeric parameters in validateaddresspool lack documentation. Consistent inline comments across all usages would improve maintainability.

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go (1)

88-88: Document address pool validation parameters.

The magic numbers 240, 0, 0, 0 in validateaddresspool should be documented. This is consistent across the test suite and would benefit from either inline comments or named constants.

📜 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 99eb18a and 7f874b4.

📒 Files selected for processing (13)
  • tests/cnf/core/network/metallb/tests/bfd-test.go
  • tests/cnf/core/network/metallb/tests/bgp-connect-time-tests.go
  • tests/cnf/core/network/metallb/tests/bgp-multiservice.go
  • tests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.go
  • tests/cnf/core/network/metallb/tests/bgp-tests.go
  • tests/cnf/core/network/metallb/tests/bgp-unnumbered.go
  • tests/cnf/core/network/metallb/tests/common.go
  • tests/cnf/core/network/metallb/tests/frrk8-tests.go
  • tests/cnf/core/network/metallb/tests/layer2-test.go
  • tests/cnf/core/network/metallb/tests/metallb-crds.go
  • tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go
  • tests/cnf/core/network/metallb/tests/pool-selector.go
  • tests/cnf/core/network/metallb/tests/system-metallb.go
💤 Files with no reviewable changes (1)
  • tests/cnf/core/network/metallb/tests/bgp-tests.go
🧰 Additional context used
🧬 Code graph analysis (5)
tests/cnf/core/network/metallb/tests/bgp-unnumbered.go (1)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (2)
  • MetallbServiceName (46-46)
  • BgpPeerName1 (32-32)
tests/cnf/core/network/metallb/tests/bgp-multiservice.go (2)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (4)
  • BGPAdvAndAddressPoolName (50-50)
  • MetallbServiceName (46-46)
  • BgpPeerName1 (32-32)
  • MetallbServiceName2 (48-48)
tests/cnf/core/network/internal/netparam/const.go (1)
  • IPSubnetInt32 (25-25)
tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go (3)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (4)
  • MetallbServiceName (46-46)
  • BgpPeerName1 (32-32)
  • MetallbServiceName2 (48-48)
  • BgpPeerName2 (34-34)
tests/cnf/core/network/metallb/internal/tsparams/mlbvars.go (1)
  • TestNamespaceName (44-44)
tests/cnf/core/network/internal/cmd/cmd.go (1)
  • RemovePrefixFromIPList (181-190)
tests/cnf/core/network/metallb/tests/pool-selector.go (2)
tests/cnf/ran/ptp/internal/iface/iface.go (1)
  • Name (22-22)
tests/cnf/core/network/metallb/internal/tsparams/mlbvars.go (1)
  • LBipRange2 (110-113)
tests/cnf/core/network/metallb/tests/metallb-crds.go (2)
tests/cnf/core/network/metallb/internal/tsparams/consts.go (3)
  • BGPAdvAndAddressPoolName (50-50)
  • MetallbServiceName (46-46)
  • BgpPeerName1 (32-32)
tests/cnf/core/network/metallb/internal/tsparams/mlbvars.go (1)
  • TestNamespaceName (44-44)
🔇 Additional comments (32)
tests/cnf/core/network/metallb/tests/system-metallb.go (1)

56-61: LGTM!

Adding service.GetGVR() to the cleanup list ensures proper teardown of Service resources created during test setup. This aligns with the test's usage of setupTestEnv which creates MetalLB services.

tests/cnf/core/network/metallb/tests/frrk8-tests.go (3)

101-105: LGTM!

The validation sequence is well-structured: first verify BGP sessions are up on the FRR pod, then validate the BGP session state and service BGP status. This provides comprehensive validation of the BGP establishment.


330-344: LGTM!

The address pool validation correctly tracks the IP allocation lifecycle: 240 available IPs initially, then 239 available and 1 assigned after the nginx pod is created with a LoadBalancer service.


521-527: Verify the validation order is intentional.

In this test, validateservicebgpstatus is called before verifyMetalLbBGPSessionsAreUPOnFrrPod, unlike other tests where the order is reversed. This could cause flaky behavior if the service BGP status depends on established BGP sessions.

Consider reordering to match the pattern in other tests (e.g., lines 436-442):

-			By("Validating the service BGP statuses")
-			validateservicebgpstatus(
-				workerNodeList, tsparams.MetallbServiceName, tsparams.TestNamespaceName, []string{tsparams.BgpPeerName1})
-
 			By("Checking that BGP session is established and up")
 			verifyMetalLbBGPSessionsAreUPOnFrrPod(frrPod, netcmd.RemovePrefixFromIPList(ipv4NodeAddrList))
 			validatebgpsessionstate("Established", "N/A", frrExternalMasterIPAddress, workerNodeList)
+
+			By("Validating the service BGP statuses")
+			validateservicebgpstatus(
+				workerNodeList, tsparams.MetallbServiceName, tsparams.TestNamespaceName, []string{tsparams.BgpPeerName1})
tests/cnf/core/network/metallb/tests/common.go (3)

426-427: LGTM!

Adding address pool validation after L2Advertisement setup ensures the pool state is verified before proceeding with the test.


791-876: LGTM!

The setupTestEnv function provides a well-structured test scaffolding with proper validation steps at key points in the setup lifecycle. The function correctly validates:

  1. Address pool after creation
  2. Address pool after service/pod deployment (showing IP allocation)
  3. Service BGP status
  4. BGP session state after establishment

893-933: LGTM with minor observation.

The two-phase validation (Eventually + Consistently) is a solid pattern for verifying state transitions. The 3-minute timeout for initial state and the consistency check ensure robust validation.

tests/cnf/core/network/metallb/tests/layer2-test.go (2)

107-107: LGTM!

The address pool validation correctly reflects the state after nginx pod deployment: 1 available IP remaining, 1 assigned for the LoadBalancer service.


138-138: LGTM!

Same validation as above, correctly placed after nginx pod setup in the speaker failure test.

tests/cnf/core/network/metallb/tests/metallb-crds.go (3)

80-80: LGTM!

The BGP session state validation correctly expects "Established" for BGP and "Up" for BFD, matching the test setup that creates and uses a BFD profile.


95-95: LGTM!

The address pool validation correctly expects 240 available IPv4 addresses, matching the pool range of 3.3.3.1-3.3.3.240.


145-148: LGTM!

Service BGP status validation is appropriately placed after service creation in the concurrent Layer2/Layer3 test.

tests/cnf/core/network/metallb/tests/bgp-unnumbered.go (7)

14-14: LGTM!

Import added for nodes.Builder type required by validatebgpsessionstate function calls.


74-74: LGTM!

Address pool validation correctly placed after pool creation with expected 240 available IPs.


136-136: LGTM!

BGP session state validation correctly expects "Up" for BFD status since BFD profile is configured (line 131 uses tsparams.BfdProfileName).


145-145: LGTM!

Address pool validation after nginx pod creation shows 239 available and 1 assigned IP.


166-173: LGTM!

Service BGP status and session state validations are appropriately placed after BGP peer creation and session establishment.


208-215: Correct BFD status for non-BFD test.

BGP session state correctly expects "N/A" for BFD status since this test creates BGPPeer without a BFD profile (line 201: bfdProfileName: "").


247-254: LGTM!

Validation sequence is correctly placed for the IBGP peering with configured peerASN test, with "Up" for BFD since BFD profile is used.

tests/cnf/core/network/metallb/tests/pool-selector.go (5)

14-14: LGTM!

Import added for nodes.Builder type required by validatebgpsessionstate function calls.


249-249: LGTM!

Address pool validation correctly placed after pool1 creation.


365-365: LGTM!

Address pool validation for dual-stack pool1.


371-371: Verify the expected available IP count for dual-stack pool2.

Similar to the single-stack case, pool2 validates with 240 available IPs here. Ensure this is consistent with the pool range definition.


255-255: No action needed - the validation value is correct.

Pool2 uses tsparams.LBipRange2[ipStack], which defines IPv4 range {"2.2.2.2", "2.2.2.240"} (239 IPs). This differs from Pool1's LBipRange1[ipStack] which uses {"1.1.1.1", "1.1.1.240"} (240 IPs). The validation count of 239 for Pool2 is correct.

tests/cnf/core/network/metallb/tests/bfd-test.go (3)

82-87: LGTM: Enhanced validation with parameterized peer IP.

The addition of session state validation in BeforeEach and the updated function signatures accepting peerIP enable more targeted validation throughout BFD failover scenarios.


328-351: Validate BGP state and failover flow with correct peer IP.

The validations correctly use masterClientPodIP for the multihop scenario, and the failover/failback tests properly pass the peer IP parameter.


415-468: Well-structured failover validation with peer IP parameter.

The refactored functions now accept peerIP and use it consistently for targeted validation. The BGP state checks correctly validate "Established"/"Up" for healthy sessions and "Connect"/"Down" for failed sessions.

tests/cnf/core/network/metallb/tests/bgp-remote-as-dynamic.go (1)

56-90: Correct BGP session state validations for established and failed scenarios.

The validations appropriately check for "Established" state in successful cases and "Idle" state when the session should fail due to misconfiguration.

tests/cnf/core/network/metallb/tests/bgp-multiservice.go (1)

114-140: Well-integrated service and session validations.

The validations for service BGP status and BGP session state are appropriately placed after peer configuration and FRR pod creation, ensuring the system is in the expected state before proceeding with traffic validation.

tests/cnf/core/network/metallb/tests/metallb-nodeSelector.go (3)

10-13: LGTM: Required imports for enhanced validation.

The imports for nodes and netcmd packages enable node-based filtering in validations and IP prefix removal utilities.


109-122: Comprehensive service BGP status validation across nodes and peers.

The validations correctly check that each service is properly announced on the appropriate nodes with the correct BGP peer associations. The node filtering ([]*nodes.Builder{workerNodeList[0]}) ensures targeted validation.


406-411: Enhanced setupTestCase with BGP session state validation.

The addition of BGP session state validations for both FRR pods after peer creation ensures the BGP sessions are established before proceeding with the test scenarios.

Comment on lines +935 to +953
func validateservicebgpstatus(nodeList []*nodes.Builder, serviceName, serviceNamespace string, peers []string) {
serviceBGPStatuses, err := metallb.ListServiceBGPStatus(APIClient)
Expect(err).ToNot(HaveOccurred(), "Failed to list ServiceBGPStatus objects")

for _, workerNode := range nodeList {
for _, status := range serviceBGPStatuses {
if status.Object != nil &&
status.Object.Status.Node == workerNode.Definition.Name &&
status.Object.Status.ServiceName == serviceName &&
status.Object.Status.ServiceNamespace == serviceNamespace {
Expect(status.Object.Status.Peers).To(ContainElements(peers),
"Failed to find peers in service BGP status with node %s, service name %s, service namespace %s",
workerNode.Definition.Name, serviceName, serviceNamespace)

break
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing failure case when no matching status is found.

The function silently succeeds if no ServiceBGPStatus matches the criteria. If the status object doesn't exist or doesn't match, the test will pass without validating anything.

🔎 Proposed fix to ensure validation occurs
 func validateservicebgpstatus(nodeList []*nodes.Builder, serviceName, serviceNamespace string, peers []string) {
 	serviceBGPStatuses, err := metallb.ListServiceBGPStatus(APIClient)
 	Expect(err).ToNot(HaveOccurred(), "Failed to list ServiceBGPStatus objects")

 	for _, workerNode := range nodeList {
+		found := false
 		for _, status := range serviceBGPStatuses {
 			if status.Object != nil &&
 				status.Object.Status.Node == workerNode.Definition.Name &&
 				status.Object.Status.ServiceName == serviceName &&
 				status.Object.Status.ServiceNamespace == serviceNamespace {
 				Expect(status.Object.Status.Peers).To(ContainElements(peers),
 					"Failed to find peers in service BGP status with node %s, service name %s, service namespace %s",
 					workerNode.Definition.Name, serviceName, serviceNamespace)

+				found = true
 				break
 			}
 		}
+		Expect(found).To(BeTrue(),
+			"No ServiceBGPStatus found for node %s, service %s/%s",
+			workerNode.Definition.Name, serviceNamespace, serviceName)
 	}
 }
📝 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.

Suggested change
func validateservicebgpstatus(nodeList []*nodes.Builder, serviceName, serviceNamespace string, peers []string) {
serviceBGPStatuses, err := metallb.ListServiceBGPStatus(APIClient)
Expect(err).ToNot(HaveOccurred(), "Failed to list ServiceBGPStatus objects")
for _, workerNode := range nodeList {
for _, status := range serviceBGPStatuses {
if status.Object != nil &&
status.Object.Status.Node == workerNode.Definition.Name &&
status.Object.Status.ServiceName == serviceName &&
status.Object.Status.ServiceNamespace == serviceNamespace {
Expect(status.Object.Status.Peers).To(ContainElements(peers),
"Failed to find peers in service BGP status with node %s, service name %s, service namespace %s",
workerNode.Definition.Name, serviceName, serviceNamespace)
break
}
}
}
}
func validateservicebgpstatus(nodeList []*nodes.Builder, serviceName, serviceNamespace string, peers []string) {
serviceBGPStatuses, err := metallb.ListServiceBGPStatus(APIClient)
Expect(err).ToNot(HaveOccurred(), "Failed to list ServiceBGPStatus objects")
for _, workerNode := range nodeList {
found := false
for _, status := range serviceBGPStatuses {
if status.Object != nil &&
status.Object.Status.Node == workerNode.Definition.Name &&
status.Object.Status.ServiceName == serviceName &&
status.Object.Status.ServiceNamespace == serviceNamespace {
Expect(status.Object.Status.Peers).To(ContainElements(peers),
"Failed to find peers in service BGP status with node %s, service name %s, service namespace %s",
workerNode.Definition.Name, serviceName, serviceNamespace)
found = true
break
}
}
Expect(found).To(BeTrue(),
"No ServiceBGPStatus found for node %s, service %s/%s",
workerNode.Definition.Name, serviceNamespace, serviceName)
}
}
🤖 Prompt for AI Agents
In tests/cnf/core/network/metallb/tests/common.go around lines 935 to 953, the
validateservicebgpstatus function silently succeeds when no ServiceBGPStatus
matches the node/service criteria; add an explicit failure when no matching
status is found. For each workerNode, track a found boolean that is set when a
matching status is located (and peers are asserted); after iterating
serviceBGPStatuses Expect(found).To(BeTrue(), ...) with a clear message
including the node name, serviceName, and serviceNamespace so the test fails if
no status object matches.

Comment on lines +323 to +327
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
verifyMetalLbBGPSessionsAreUPOnFrrPod(extFrrPod2, nodeAddrList[ipStack])
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove duplicate validation calls.

Lines 323-324 and 326-327 contain identical duplicate validation calls executed consecutively, which is redundant.

🔎 Proposed fix
 	By("Checking that BGP session is established on external FRR Pod")
 	verifyMetalLbBGPSessionsAreUPOnFrrPod(extFrrPod1, nodeAddrList[ipStack])
 	validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
-	validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
 	verifyMetalLbBGPSessionsAreUPOnFrrPod(extFrrPod2, nodeAddrList[ipStack])
 	validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
-	validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
📝 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.

Suggested change
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
verifyMetalLbBGPSessionsAreUPOnFrrPod(extFrrPod2, nodeAddrList[ipStack])
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
verifyMetalLbBGPSessionsAreUPOnFrrPod(extFrrPod2, nodeAddrList[ipStack])
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
🤖 Prompt for AI Agents
In tests/cnf/core/network/metallb/tests/pool-selector.go around lines 323 to 327
there are duplicate consecutive validation calls: two identical
validatebgpsessionstate calls for metallbAddrList[ipStack][0] (lines 323-324)
and two identical calls for metallbAddrList[ipStack][1] (lines 326-327); remove
the redundant duplicate calls so each validation is invoked only once (keep one
call per address and preserve the surrounding
verifyMetalLbBGPSessionsAreUPOnFrrPod call and overall order).

Comment on lines +454 to +458
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
verifyMetalLbBGPSessionsAreUPOnFrrPod(extFrrPod2, []string{nodeAddrList[ipv4][1], nodeAddrList[ipv6][1]})
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove duplicate validation calls in dual-stack tests.

Same issue as in the single-stack tests: lines 454-455 and 457-458 contain duplicate consecutive validation calls.

🔎 Proposed fix
 	By("Checking that BGP session is established on external FRR Pod")
 	verifyMetalLbBGPSessionsAreUPOnFrrPod(extFrrPod1, []string{nodeAddrList[ipv4][0], nodeAddrList[ipv6][0]})
 	validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
-	validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][0], []*nodes.Builder{workerNodeList[0]})
 	verifyMetalLbBGPSessionsAreUPOnFrrPod(extFrrPod2, []string{nodeAddrList[ipv4][1], nodeAddrList[ipv6][1]})
 	validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})
-	validatebgpsessionstate("Established", "N/A", metallbAddrList[ipStack][1], []*nodes.Builder{workerNodeList[1]})

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In tests/cnf/core/network/metallb/tests/pool-selector.go around lines 454 to
458, there are duplicate consecutive validatebgpsessionstate calls (lines
454-455 and 457-458) in the dual-stack test; remove the duplicated calls so each
validatebgpsessionstate invocation only appears once (keep the first of each
pair), ensuring the verifyMetalLbBGPSessionsAreUPOnFrrPod call remains unchanged
and the test logic/ordering is preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant