Skip to content

Conversation

@EdDev
Copy link
Contributor

@EdDev EdDev commented Sep 25, 2025

What this PR does / why we need it:

Tests which are marked with the ipv6 marker are expecting the cluster to support IPv6. If it does not support IPv6, the infra is considered incorrect and therefore no tests should execute.

This change refactors the IPv4 sanity check and extends it to cover IPv6 as well.

Follow up changes should convert usages of
fail_if_not_ipv6_supported_cluster to a test marker.

Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:

Summary by CodeRabbit

  • Tests

    • Expanded network sanity checks to cover both IPv4 and IPv6 support.
    • Added per-family cluster support detection for each IP family.
    • Improved validation messages and logs to clearly indicate which IP family is being tested.
  • Refactor

    • Consolidated IP-family verification into a single, family-agnostic flow to reduce duplication and simplify maintenance.

@coderabbitai
Copy link

coderabbitai bot commented Sep 25, 2025

Walkthrough

Replaces an IPv4-specific verifier with a generalized _verify_ip_family(family, is_supported_in_cluster) and updates network_sanity to accept ipv6_supported_cluster. Invocations now validate both ipv4 and ipv6; _verify_ipv4 was removed and logs/messages reference the specific IP family.

Changes

Cohort / File(s) Summary
Network test fixtures
tests/network/conftest.py
Added _verify_ip_family(family, is_supported_in_cluster), removed _verify_ipv4, updated network_sanity signature to include ipv6_supported_cluster, replaced IPv4-only checks with calls for both ipv4 and ipv6, and updated logging/failure messages to reference the specific IP family. _verify_sriov unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

verified, can-be-merged, lgtm-yossisegev, lgtm-servolkov

Suggested reviewers

  • Anatw
  • servolkov
  • yossisegev

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description only fills in the “What this PR does / why we need it” section and leaves out the required “Short description” and “More details” headings, while also omitting issue references and the jira-ticket field. Because it does not follow the repository’s full template structure or provide all requested information, the description is incomplete. Please add the missing “Short description” and “More details” sections, populate the “Which issue(s) this PR fixes” heading if applicable, and include a value for “jira-ticket” (or write “NONE” if there is no ticket) to fully comply with the repository template.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly captures the primary change by indicating that network sanity tests are being extended to validate IPv6 support, and it correctly references the tests, net, and sanity scope without extraneous detail. It is specific enough for a teammate to understand that the PR adds IPv6 family validation and aligns with the changeset. The phrasing is concise and directly related to the main refactoring and feature addition described in the code.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 53a2560 and 782efc3.

📒 Files selected for processing (1)
  • tests/network/conftest.py (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/network/conftest.py (1)
tests/conftest.py (2)
  • ipv6_supported_cluster (1962-1964)
  • ipv4_supported_cluster (1956-1958)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: build-container
  • GitHub Check: tox
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
🔇 Additional comments (3)
tests/network/conftest.py (3)

227-227: LGTM! Parameter addition integrates cleanly.

The addition of ipv6_supported_cluster to the fixture signature is consistent with the existing ipv4_supported_cluster parameter and properly sourced from the session-scoped fixture in tests/conftest.py.


314-320: Excellent refactoring! Clean abstraction for IP family validation.

The generalized _verify_ip_family function eliminates code duplication and follows the established pattern used by other verification helpers (_verify_dpdk, _verify_service_mesh, etc.). The logic correctly validates that tests marked with a specific IP family marker (e.g., ipv4, ipv6) can only run on clusters that support that family. The error and success messages clearly reference the specific IP family being validated.


347-348: LGTM! IPv4 and IPv6 validation properly invoked.

Both IP families are now validated in the session-scoped network_sanity fixture, ensuring that tests marked with ipv4 or ipv6 will only run on appropriately configured clusters. This replaces the old _verify_ipv4() call and extends coverage to IPv6 as intended.


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.

@openshift-virtualization-qe-bot-2

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: A tracking issue is created for this PR and will be closed when the PR is merged or closed
  • Pre-commit Checks: pre-commit runs automatically if .pre-commit-config.yaml exists
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. LGTM Count: Minimum 2 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No WIP, hold, or conflict labels
  5. Verified: PR must be marked as verified (if verification is enabled)

📊 Review Process

Approvers and Reviewers

Approvers:

  • EdDev

Reviewers:

  • Anatw
  • EdDev
  • azhivovk
  • servolkov
  • yossisegev
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
  • automerge

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is automatically removed on each new commit
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

@openshift-virtualization-qe-bot-2

azhivovk can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"}

Copy link

@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: 0

🧹 Nitpick comments (1)
tests/network/conftest.py (1)

316-321: Fix the logging text to reflect the actual IP family.

When _verify_ip_family runs for IPv6, the log still says “running IPV4 tests…”, and the success message prints “an ipv6 supported cluster.” Please make the messages depend on the family argument so operators aren’t misled.

-        LOGGER.info("Verifying if the cluster supports running IPV4 tests...")
+        LOGGER.info("Verifying if the cluster supports running %s tests...", family.upper())
         if not is_supported:
             failure_msgs.append(f"{family} is not supported in this cluster")
         else:
-            LOGGER.info(f"Validated network lane is running against an {family} supported cluster")
+            LOGGER.info("Validated network lane is running against a %s-supported cluster", family.upper())
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a827c2 and 7ba87f6.

📒 Files selected for processing (1)
  • tests/network/conftest.py (3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rlobillo
PR: RedHatQE/openshift-virtualization-tests#1984
File: tests/install_upgrade_operators/network_policy/test_network_policy_components.py:0-0
Timestamp: 2025-09-12T09:39:12.961Z
Learning: IPv6 support is not needed for network policy tests in the openshift-virtualization-tests repository, as tests would fail earlier if the destination IP is IPv6.
📚 Learning: 2025-08-28T12:30:40.692Z
Learnt from: servolkov
PR: RedHatQE/openshift-virtualization-tests#1776
File: tests/network/bgp/conftest.py:35-54
Timestamp: 2025-08-28T12:30:40.692Z
Learning: The BGP test suite in tests/network/bgp/ relies on session-scoped validation in tests/network/conftest.py via the network_sanity fixture's _verify_bgp() function, which validates required environment variables (VLAN_TAG, EXTERNAL_FRR_STATIC_IPV4, BGP_CLUSTER_DOMAIN_GROUP) before any BGP tests run, making individual fixture-level validation redundant.

Applied to files:

  • tests/network/conftest.py
🧬 Code graph analysis (1)
tests/network/conftest.py (1)
tests/conftest.py (2)
  • ipv6_supported_cluster (1956-1958)
  • ipv4_supported_cluster (1950-1952)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: can-be-merged
  • GitHub Check: can-be-merged
  • GitHub Check: build-container
  • GitHub Check: tox
  • GitHub Check: can-be-merged

@EdDev
Copy link
Contributor Author

EdDev commented Oct 15, 2025

Basing the verification on #2177 (comment) as no changes have been added since.

/verified

@openshift-virtualization-qe-bot-3
Copy link
Contributor

@coderabbitai

Test execution plan request details

As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan.
You will create a change request comment on the 1st line of the 1st file in the pr with the test execution plan.
If you fail to run or post a comment, retry.

Analysis Requirements:

  1. Examine code changes in each modified file
  2. Identify affected code paths, functions, and classes
  3. Analyze pytest-specific elements: fixtures (scope, dependencies), parametrization, markers, conftest changes
  4. Trace test dependencies through imports, shared utilities, and fixture inheritance
  5. Detect new tests introduced in the PR

Your deliverable:
Your change request comment will be based on the following requirements:

Test Execution Plan

  • path/to/test_file.py - When the entire test file needs verification
  • path/to/test_file.py::TestClass::test_method - When specific test(s) needed
  • path/to/test_file.py::test_function - When specific test(s) needed
  • -m marker - When specific marker(s) can be used to cover multiple cases.

Guidelines:

  • Include only tests directly affected by the changes
  • Use a full file path only if ALL tests in that file require verification
  • Use file path + test name if only specific tests are needed
  • If a test marker can cover multiple files/tests, provide the marker
  • Balance coverage vs over-testing - Keep descriptions minimal
  • Do not add a follow-up comment in the PR, only the change request one

request,
istio_system_namespace,
cluster_network_mtu,
network_overhead,
Copy link

Choose a reason for hiding this comment

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

Test Execution Plan

Based on the changes to the network_sanity fixture that add IPv6 family stack validation:

Recommended approach:

  • -m ipv6 - Most efficient approach to verify all tests requiring IPv6 support

Alternative specific test paths:

  • tests/network/connectivity/test_ovs_linux_bridge.py::TestLinuxBridgeConnectivity::test_ipv6_linux_bridge
  • tests/network/connectivity/test_ovs_linux_bridge.py::TestOvsBridgeConnectivity::test_ipv6_ovs_bridge
  • tests/observability/metrics/test_general_metrics.py::TestVirtHCOSingleStackIpv6::test_metric_kubevirt_hco_single_stack_ipv6

Rationale:
The modified network_sanity fixture now validates IPv6 cluster support. All tests marked with ipv6 depend on this fixture and will be validated against IPv6 capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the verification described here are covering this change.

@coderabbitai
Copy link

coderabbitai bot commented Oct 15, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Create a review comment on line 1 of the first modified file with the test execution plan

# Get the PR number from the URL
PR_NUMBER=2177

# Get the commit SHA of the HEAD
COMMIT_SHA=$(git rev-parse HEAD)

# Create a review comment on line 1 of tests/network/conftest.py
gh api \
  --method POST \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  /repos/RedHatQE/openshift-virtualization-tests/pulls/${PR_NUMBER}/reviews \
  -f event='COMMENT' \
  -f commit_id="${COMMIT_SHA}" \
  -f body="**Test Execution Plan**

Based on the changes to the \`network_sanity\` fixture that add IPv6 family stack validation:

**Recommended approach:**
- \`-m ipv6\` - Most efficient approach to verify all tests requiring IPv6 support

**Alternative specific test paths:**
- \`tests/network/connectivity/test_ovs_linux_bridge.py::TestLinuxBridgeConnectivity::test_ipv6_linux_bridge\`
- \`tests/network/connectivity/test_ovs_linux_bridge.py::TestOvsBridgeConnectivity::test_ipv6_ovs_bridge\`
- \`tests/observability/metrics/test_general_metrics.py::TestVirtHCOSingleStackIpv6::test_metric_kubevirt_hco_single_stack_ipv6\`

**Rationale:**
The modified \`network_sanity\` fixture now validates IPv6 cluster support. All tests marked with \`ipv6\` depend on this fixture and will be validated against IPv6 capabilities." \
  -f comments='[{"path":"tests/network/conftest.py","position":1,"body":"**Test Execution Plan**\n\nBased on the changes to the `network_sanity` fixture that add IPv6 family stack validation:\n\n**Recommended approach:**\n- `-m ipv6` - Most efficient approach to verify all tests requiring IPv6 support\n\n**Alternative specific test paths:**\n- `tests/network/connectivity/test_ovs_linux_bridge.py::TestLinuxBridgeConnectivity::test_ipv6_linux_bridge`\n- `tests/network/connectivity/test_ovs_linux_bridge.py::TestOvsBridgeConnectivity::test_ipv6_ovs_bridge`\n- `tests/observability/metrics/test_general_metrics.py::TestVirtHCOSingleStackIpv6::test_metric_kubevirt_hco_single_stack_ipv6`\n\n**Rationale:**\nThe modified `network_sanity` fixture now validates IPv6 cluster support. All tests marked with `ipv6` depend on this fixture and will be validated against IPv6 capabilities."}]'

Length of output: 2106


🏁 Script executed:

#!/bin/bash
# Create a review comment on line 1 of the first modified file with the test execution plan

PR_NUMBER=2177
COMMIT_SHA=$(git rev-parse HEAD)

# Create a properly formatted JSON payload
cat > review_payload.json <<'EOF'
{
  "commit_id": "COMMIT_SHA_PLACEHOLDER",
  "event": "COMMENT",
  "comments": [
    {
      "path": "tests/network/conftest.py",
      "position": 1,
      "body": "**Test Execution Plan**\n\nBased on the changes to the `network_sanity` fixture that add IPv6 family stack validation:\n\n**Recommended approach:**\n- `-m ipv6` - Most efficient approach to verify all tests requiring IPv6 support\n\n**Alternative specific test paths:**\n- `tests/network/connectivity/test_ovs_linux_bridge.py::TestLinuxBridgeConnectivity::test_ipv6_linux_bridge`\n- `tests/network/connectivity/test_ovs_linux_bridge.py::TestOvsBridgeConnectivity::test_ipv6_ovs_bridge`\n- `tests/observability/metrics/test_general_metrics.py::TestVirtHCOSingleStackIpv6::test_metric_kubevirt_hco_single_stack_ipv6`\n\n**Rationale:**\nThe modified `network_sanity` fixture now validates IPv6 cluster support. All tests marked with `ipv6` depend on this fixture and will be validated against IPv6 capabilities."
    }
  ]
}
EOF

# Replace the placeholder with actual commit SHA
sed -i "s/COMMIT_SHA_PLACEHOLDER/${COMMIT_SHA}/" review_payload.json

# Create the review using the JSON payload
gh api \
  --method POST \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  /repos/RedHatQE/openshift-virtualization-tests/pulls/${PR_NUMBER}/reviews \
  --input review_payload.json

echo "Review comment created successfully"

Length of output: 1763


@openshift-virtualization-qe-bot-3, I've successfully created a change request comment on line 1 of tests/network/conftest.py with the test execution plan.

Test Execution Plan Summary:

Recommended approach:

  • -m ipv6 - Most efficient way to verify all tests requiring IPv6 support

Alternative specific test paths:

  • tests/network/connectivity/test_ovs_linux_bridge.py::TestLinuxBridgeConnectivity::test_ipv6_linux_bridge
  • tests/network/connectivity/test_ovs_linux_bridge.py::TestOvsBridgeConnectivity::test_ipv6_ovs_bridge
  • tests/observability/metrics/test_general_metrics.py::TestVirtHCOSingleStackIpv6::test_metric_kubevirt_hco_single_stack_ipv6

The plan focuses on tests that depend on the modified network_sanity fixture, which now includes IPv6 cluster support validation. Using the -m ipv6 marker is the most efficient approach as it covers all affected tests in a single run.


🧠 Learnings used
Learnt from: servolkov
PR: RedHatQE/openshift-virtualization-tests#1776
File: libs/net/node_network.py:25-31
Timestamp: 2025-08-20T23:43:28.117Z
Learning: In the RedHatQE/openshift-virtualization-tests project, servolkov's team always uses bare metal (BM) clusters with IPv4 setup in their testing environment, making defensive checks for IPv4 data presence potentially redundant in their networking code.

@rnetser
Copy link
Collaborator

rnetser commented Oct 15, 2025

/approve
/lgtm

@rnetser rnetser merged commit 6983e31 into RedHatQE:main Oct 15, 2025
20 checks passed
@openshift-virtualization-qe-bot-2

Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-2177.

@openshift-virtualization-qe-bot-2

New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants