Skip to content

system-tests: update sriov-pod-level-bond.go for IPv6 single stack#1345

Merged
yprokule merged 3 commits intorh-ecosystem-edge:mainfrom
laurayang842:podlevelbond
Apr 28, 2026
Merged

system-tests: update sriov-pod-level-bond.go for IPv6 single stack#1345
yprokule merged 3 commits intorh-ecosystem-edge:mainfrom
laurayang842:podlevelbond

Conversation

@laurayang842
Copy link
Copy Markdown
Contributor

@laurayang842 laurayang842 commented Apr 23, 2026

Summary by CodeRabbit

  • New Features

    • Validation ensures each bond interface family has both IP and mask set; single IP-family operation (IPv4 or IPv6) supported.
    • Network requests include only the IP families that are configured.
  • Bug Fixes

    • Corrected multiple user-facing log and error messages and improved assertion clarity.
    • Failover target selection favors IPv4 first and fails fast if no address is configured.
    • Fixed incorrect deployment name in a server-pod retrieval error message.
  • Refactor

    • Consolidated topology orchestration and unified workload verification flows.
    • Centralized TCP traffic verification and reuse across test variants.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e8fac008-e060-4d41-9f67-9327a6fb7f7d

📥 Commits

Reviewing files that changed from the base of the PR and between 9ec81d2 and 2867890.

📒 Files selected for processing (1)
  • tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go

📝 Walkthrough

Walkthrough

Consolidates pod-level bond test orchestration and TCP verification in one file; enforces IPv4/IPv6 per-family IP/mask consistency; builds Multus IP requests only for configured families; selects a single failover target IP (IPv4-preferred); and fixes several log/error message texts.

Changes

Cohort / File(s) Summary
Pod-Level Bond Test Refactor
tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go
Adds runPodLevelBondTopologyCase and expectTCPPass; rewires four Verify* functions to call the new topology helper; validates per-family IP/subnet presence; constructs Multus IPRequest only for configured IP families; centralizes TCP traffic generation/parsing for both directions; picks IPv4-first targetIP for failover; fixes multiple log/error message strings and one deployment name in an error.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • yprokule
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: updating sriov-pod-level-bond.go to support IPv6 single-stack configurations alongside IPv4.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@laurayang842 laurayang842 changed the title system-tests: update sriov-pod-level-bond.go to support IPv6 single s… system-tests: update sriov-pod-level-bond.go for IPv6 single stack Apr 23, 2026
@laurayang842 laurayang842 marked this pull request as ready for review April 23, 2026 00:23
Copy link
Copy Markdown
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: 2

🧹 Nitpick comments (1)
tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go (1)

1329-1341: Failover targetIP selection reads well; minor nit on dead code.

IPv4-first preference with a clear failure when neither family is configured is the right behavior for single-stack and dual-stack configs alike. Nit: Fail(...) panics, so the trailing return on line 1340 is unreachable — harmless but you can drop it if you want to match the style used elsewhere.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go`
around lines 1329 - 1341, The switch that selects targetIP (using
RDSCoreConfig.PodLevelBondDeploymentTwoIPv4 and PodLevelBondDeploymentTwoIPv6)
calls Fail(...) on the default branch which panics, making the subsequent
"return" unreachable; remove the trailing "return" after the Fail(...) in the
default branch to eliminate the dead code and match surrounding style while
keeping the IPv4/IPv6 selection logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go`:
- Around line 1118-1122: The By description for the IPv6 server→client test is
wrong; update the call to expectTCPPass(serverPodObj, clientIPv6, ...) so the
human-readable step string reads "Send data from the server container to the
IPv6 address used by the client container" (matching the IPv4 counterpart) to
accurately reflect the direction being tested.
- Around line 1267-1270: The doc comment for
VerifyPodLevelBondWorkloadsOnSameNodeSamePF is incorrect: it says "different
PFs" while the function calls runPodLevelBondTopologyCase(true, true) which is
same node, same PF; update the comment above the
VerifyPodLevelBondWorkloadsOnSameNodeSamePF function to accurately describe that
it verifies TCP traffic on the same node and the same PF (or otherwise match the
boolean arguments passed to runPodLevelBondTopologyCase), leaving the function
name and call unchanged.

---

Nitpick comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go`:
- Around line 1329-1341: The switch that selects targetIP (using
RDSCoreConfig.PodLevelBondDeploymentTwoIPv4 and PodLevelBondDeploymentTwoIPv6)
calls Fail(...) on the default branch which panics, making the subsequent
"return" unreachable; remove the trailing "return" after the Fail(...) in the
default branch to eliminate the dead code and match surrounding style while
keeping the IPv4/IPv6 selection logic intact.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 076ceed0-10e9-4539-b18a-1de60e41f3ae

📥 Commits

Reviewing files that changed from the base of the PR and between 4990ef3 and 8280de3.

📒 Files selected for processing (1)
  • tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go

Comment thread tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go Outdated
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@laurayang842
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 23, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
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: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go`:
- Around line 577-584: The concatenated strings in the klog.V(100).Infof call
and the subsequent fmt.Errorf inside the function that checks bond active
interfaces produce ";current" without a space; update the message concatenation
used in klog.V(100).Infof and fmt.Errorf (the lines referencing
clientPod.Definition.Name, clientPod.Definition.Namespace, newInterfaceName,
interfaceName) so there is a space between the semicolon and "current" (either
add a trailing space to the first literal or a leading space to the second
literal) to ensure the log and error messages read correctly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 774ec2f6-fcc9-4b18-9ed0-8aabe304acd1

📥 Commits

Reviewing files that changed from the base of the PR and between 8280de3 and 9ec81d2.

📒 Files selected for processing (1)
  • tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go

Comment thread tests/system-tests/rdscore/internal/rdscorecommon/sriov-pod-level-bond.go Outdated
Copy link
Copy Markdown
Collaborator

@yprokule yprokule left a comment

Choose a reason for hiding this comment

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

/lgtm

@yprokule yprokule merged commit 225f998 into rh-ecosystem-edge:main Apr 28, 2026
3 checks passed
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.

2 participants