system-tests: update ipvlan-validation.go to support IPv6 single stack#1343
system-tests: update ipvlan-validation.go to support IPv6 single stack#1343laurayang842 wants to merge 2 commits intorh-ecosystem-edge:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughTwo IPVLAN test verification functions were refactored to use a new helper that iterates over configured IPv4/IPv6 target addresses, asserting at least one is present, skipping empty addresses, logging each used target, and delegating per-address connectivity checks to the existing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/system-tests/rdscore/internal/rdscorecommon/ipvlan-validation.go (1)
160-208: Consider extracting the repeated target-address loop into a helper.The same pattern (assert at-least-one-configured, build a 2-element slice, iterate skipping empties, log, call
verifySRIOVConnectivity) is now repeated four times acrossVerifyIPVLANConnectivityBetweenDifferentNodesandVerifyIPVLANConnectivityOnSameNode. Extracting a small helper would reduce duplication and make future additions (e.g., new address families) trivial.♻️ Example helper
func verifyConnectivityForAddresses(srcNS, dstNS, srcLabel, dstLabel, deployName string, addresses ...string) { Expect(addresses).To(ContainElement(Not(BeEmpty())), fmt.Sprintf("At least one target address (IPv4 or IPv6) must be configured for %s", deployName)) for _, targetAddress := range addresses { if targetAddress == "" { klog.V(rdscoreparams.RDSCoreLogLevel).Infof("Skipping empty address for %s", deployName) continue } klog.V(rdscoreparams.RDSCoreLogLevel).Infof("Access workload via %q", targetAddress) verifySRIOVConnectivity(srcNS, dstNS, srcLabel, dstLabel, targetAddress) } }Also minor: the
"Skipping empty address %q"log always prints an empty string ("") — it could either be dropped or reworded to identify which address family was skipped (e.g.,"Skipping empty IPv6 target for Deploy1").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/system-tests/rdscore/internal/rdscorecommon/ipvlan-validation.go` around lines 160 - 208, Extract the repeated target-address assertion+loop in VerifyIPVLANConnectivityBetweenDifferentNodes (and the similar code in VerifyIPVLANConnectivityOnSameNode) into a small helper, e.g. verifyConnectivityForAddresses(srcNS, dstNS, srcLabel, dstLabel, deployName string, addresses ...string), that asserts at least one non-empty address (using Expect(addresses).To(ContainElement(Not(BeEmpty()))) with a deployName-specific message), iterates skipping empty entries, logs a contextual skip message (include deployName or address family instead of printing the empty string), and calls verifySRIOVConnectivity(srcNS, dstNS, srcLabel, dstLabel, targetAddress); then replace each duplicated block to call this helper with the appropriate RDSCoreConfig fields and labels (ipvlanDeploy10Label, ipvlanDeploy11Label, IPVlanNSOne, IPVlanDeploy1TargetAddress, IPVlanDeploy1TargetAddressIPv6, etc.).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/ipvlan-validation.go`:
- Around line 160-208: Extract the repeated target-address assertion+loop in
VerifyIPVLANConnectivityBetweenDifferentNodes (and the similar code in
VerifyIPVLANConnectivityOnSameNode) into a small helper, e.g.
verifyConnectivityForAddresses(srcNS, dstNS, srcLabel, dstLabel, deployName
string, addresses ...string), that asserts at least one non-empty address (using
Expect(addresses).To(ContainElement(Not(BeEmpty()))) with a deployName-specific
message), iterates skipping empty entries, logs a contextual skip message
(include deployName or address family instead of printing the empty string), and
calls verifySRIOVConnectivity(srcNS, dstNS, srcLabel, dstLabel, targetAddress);
then replace each duplicated block to call this helper with the appropriate
RDSCoreConfig fields and labels (ipvlanDeploy10Label, ipvlanDeploy11Label,
IPVlanNSOne, IPVlanDeploy1TargetAddress, IPVlanDeploy1TargetAddressIPv6, etc.).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12401a4d-486e-4604-bbc0-d939cd2fbe1e
📒 Files selected for processing (1)
tests/system-tests/rdscore/internal/rdscorecommon/ipvlan-validation.go
Summary by CodeRabbit