Skip to content

USHIFT-7382: Add MTU boundary tests for C2CC and C2CC+IPsec#7024

Open
agullon wants to merge 6 commits into
openshift:mainfrom
agullon:c2cc-ipsec-mtu-tests
Open

USHIFT-7382: Add MTU boundary tests for C2CC and C2CC+IPsec#7024
agullon wants to merge 6 commits into
openshift:mainfrom
agullon:c2cc-ipsec-mtu-tests

Conversation

@agullon

@agullon agullon commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Add Robot Framework tests to exercise different MTU sizes for C2CC (Cluster-to-Cluster Connectivity) with and without IPsec, covering both IPv4 and IPv6.

What this PR adds

3 new test suites under test/suites/c2cc/extra/:

Suite Description Test count
ipsec.robot DF-bit boundary tests at 1500 MTU through IPsec tunnels 15
mtu.robot Plain C2CC at jumbo MTU (9000), no IPsec 8
ipsec-mtu.robot IPsec at jumbo MTU with pod MTU 8900 (ESP headroom) 13

12 new scenario scripts (el98 + el102 × IPv4 + IPv6 × 3 suites):

  • c2cc-ipsec-ipv4/ipv6 — IPsec functional + DF-bit tests at 1500 MTU
  • c2cc-mtu-ipv4/ipv6 — jumbo frame (9000) C2CC without IPsec
  • c2cc-ipsec-mtu-ipv4/ipv6 — jumbo frame C2CC with IPsec + pod MTU 8900

Test approach

Tests use df-bit-send.py, a Python3 script that sends UDP datagrams with the DF bit set using IP_PMTUDISC_DO (IPv4) / IPV6_DONTFRAG (IPv6). The script is piped via stdin to nettest-pod through oc exec. This validates MTU enforcement at the pod interface level without requiring NET_RAW capability — the nettest-pod runs as non-root under restricted PodSecurity.

The "should fail" assertions check for the specific EMSGSIZE error to distinguish genuine MTU rejection from infrastructure failures. Curl failure assertions also verify recognizable error modes (timeout/refused/unreachable).

All DF-bit tests exercise all 6 cluster pair directions (a→b, a→c, b→a, b→c, c→a, c→b).

Jumbo MTU infrastructure

Setting jumbo MTU on bootc VMs required solving several layered problems:

  1. NIC MTU: Three mechanisms set the guest NIC to 9000 before MicroShift starts:

    • NetworkManager conf.d (default ethernet MTU for auto-created connections)
    • NM dispatcher script (ip link set during connection activation, failures logged via logger)
    • systemd oneshot service (final safety net, script at /etc/ because /usr/ is read-only on bootc)
  2. OVN pod MTU: Explicit mtu in /etc/microshift/ovn.yaml injected via kickstart, because C2CC VMs have no default route and MicroShift's auto-detection falls back to 1500.

  3. Guest virtio-net MTU: launch_vm --network_mtu propagates the MTU to QEMU's virtio-net driver via virt-install's mtu.size sub-option. A libvirt network's <mtu> element alone only affects the host-side bridge and tap devices.

  4. IPv6 jumbo networks: Separate jumbo-ipv6 libvirt network with MTU 9000. Scenarios use the default IPv4 WEB_SERVER_URL for hypervisor-side kickstart fetch (libvirt's nftables rules block non-DNS traffic to on-the-fly IPv6 bridge addresses). VMs resolve the mirror registry via hostname → /etc/hosts → IPv6 bridge IP.

  5. Race-safe network creation: c2cc_create_jumbo_network and c2cc_create_jumbo_ipv6_network tolerate concurrent callers (multiple parallel scenarios hitting them simultaneously) — each step ignores "already done" errors and a final active-state check catches genuine failures.

MTU boundary values

Scenario Pod MTU Pass boundary Reject boundary
1500 MTU (ipsec) ESP-adjusted PMTU 1450B 1472B (exceeds ESP-adjusted PMTU)
Jumbo plain (mtu) 8922 (9000−78 headroom) 8872B 8895B / 8972B
Jumbo+IPsec (ipsec-mtu) 8900 (set via ovn.yaml) 8852B (max IPv6 at 8900) 8873B (Phase 2) / 8972B

CI infrastructure changes

  • --check disk_size=off: Skip virt-install's false-positive pool capacity check. With 8 concurrent scenarios (24 VMs × 20G nominal), thin-provisioned qcow2 sizes exceed the backing filesystem's capacity check, but actual usage never exceeds ~370G on a 398G disk.
  • Architecture-based scenario split: Each arch runs one RHEL version to halve the scenario count (~12 per job instead of 23). x86 runs el98, ARM runs el102. The assignment rotates per commit (first hex digit of HEAD) so both combinations get exercised over time.

Also included

  • Renamed c2cc-ipsecc2cc-ipsec-ipv4 for consistency with new IPv6 variants
  • IPv6 fixes in shared keywords: nftables ip6 daddr, curl bracket notation for IPv6 addresses

Test plan

  • All 12 new scenarios pass on x86 CI (e2e-aws-tests-bootc-c2cc)
  • All 12 new scenarios pass on ARM CI (e2e-aws-tests-bootc-c2cc-arm)
  • Existing IPsec tests (15/15) pass on both architectures
  • IPv4 and IPv6 variants produce identical results
  • Phase 2 pod MTU verification (8900) passes after MicroShift restart
  • verify-rf (robocop check + format) passes
  • verify-shell (shellcheck) passes

🤖 Generated with Claude Code

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2026
@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agullon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2026
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a hardened nettest-pod Kubernetes manifest, wires it into c2cc test workload deployment/readiness waits, introduces Robot Framework keywords for DF-bit UDP send/verify, pod MTU reading, and large-payload POST checks, and adds new/extended test cases across ipsec.robot and a new mtu.robot suite validating IPsec MTU boundaries.

Changes

IPsec MTU/DF-bit test coverage

Layer / File(s) Summary
Nettest pod manifest and workload wiring
test/assets/c2cc/nettest-pod.yaml, test/resources/c2cc.resource
Adds a hardened nettest-pod manifest and applies/waits for it alongside existing test workloads across clusters.
DF-bit and MTU verification keywords
test/resources/ipsec.resource
Adds keywords for DF-bit UDP send/verify (pass and fail variants), reading pod interface MTU, and large payload POST verification.
Existing IPsec suite MTU test additions
test/suites/c2cc/ipsec/ipsec.robot
Extends an existing test with large payload checks and adds several new DF-bit/MTU boundary test cases plus a minor cleanup trim.
Jumbo MTU test suite
test/suites/c2cc/ipsec/mtu.robot
Adds a new suite validating IPsec MTU behavior across physical and pod MTU reconfiguration phases, including setup, test cases, redeployment, and teardown keywords.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested reviewers: jerpeter1, kasturinarra

🚥 Pre-merge checks | ✅ 12 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning mtu.robot calls nonexistent Test Connectivity Between Clusters; the repo defines Verify Connectivity Between Clusters, so the suite will error. Rename those calls to Verify Connectivity Between Clusters (or add the missing keyword) and rerun the MTU suite.
Ipv6 And Disconnected Network Test Compatibility ⚠️ Warning New helpers hardcode socket.AF_INET and unbracketed http://${ip}:8080 URLs; nettest-pod also pulls from registry.access.redhat.com. Make the MTU helpers IP-family-aware (AF_INET6/bracketed hosts or net.JoinHostPort) and use an internal/mirrored image for nettest-pod.
✅ Passed checks (12 passed)
Check name Status Explanation
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.
Stable And Deterministic Test Names ✅ Passed Added Robot test names are static/descriptive; no pod names, IPs, namespaces, timestamps, or random IDs appear in titles.
Microshift Test Compatibility ✅ Passed PASS: PR only adds Robot/YAML C2CC tests and a Pod manifest; no Ginkgo tests, unsupported OpenShift APIs, or MicroShift-skip issues found.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No added test assumes multiple nodes or HA; the new Robot MTU checks only use per-cluster pods/IPsec, with no SNO-specific multi-node dependency.
Topology-Aware Scheduling Compatibility ✅ Passed Changed files add only a plain Pod and Robot test keywords; no nodeSelector, affinity, topology spread, replica, or PDB assumptions were introduced.
Ote Binary Stdout Contract ✅ Passed PR only changes shell/Robot test assets; no main/init/TestMain/RunSpecs code or stdout writes were added in process-level binary code.
No-Weak-Crypto ✅ Passed No MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or custom crypto/comparison issues found; IPsec config uses AES256-SHA2_256.
Container-Privileges ✅ Passed nettest-pod.yaml uses allowPrivilegeEscalation:false, drops ALL caps, runs as UID 10001/non-root, and sets no privileged/host namespace flags.
No-Sensitive-Data-In-Logs ✅ Passed No new sensitive logging appears in the PR; added files are manifests/tests, and existing debug logs predate these changes.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding MTU boundary tests for C2CC and C2CC+IPsec.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@agullon

agullon commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-tests-bootc-c2cc
/test e2e-aws-tests-bootc-c2cc-arm

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
putComment timed out

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

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 (3)
test/bin/c2cc_common.sh (1)

363-383: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Unchecked command results in configure_pod_mtu/restart_microshift_and_wait.

Neither the tee write in configure_pod_mtu nor the systemctl restart microshift in restart_microshift_and_wait check run_command_on_vm's return code. Failures will only surface later (if at all) via wait_for_greenboot_on_hosts/the bigmtu.robot MTU assertion, with a less specific error than at the point of failure. Existing helpers in this file (e.g. full_vm_name) use || return 1 for this purpose.

🩹 Proposed fix
 configure_pod_mtu() {
     local -r mtu=$1
     for host in host1 host2 host3; do
         run_command_on_vm "${host}" "sudo tee /etc/microshift/ovn.yaml > /dev/null <<EOF
 mtu: ${mtu}
-EOF"
+EOF" || { echo "${host}: failed to write ovn.yaml" >&2; return 1; }
     done
 }
 restart_microshift_and_wait() {
     local -r junit_label="${1:-bigmtu_greenboot}"
     for host in host1 host2 host3; do
-        run_command_on_vm "${host}" "sudo systemctl restart microshift"
+        run_command_on_vm "${host}" "sudo systemctl restart microshift" || { echo "${host}: failed to restart microshift" >&2; return 1; }
     done
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/bin/c2cc_common.sh` around lines 363 - 383, The `configure_pod_mtu` and
`restart_microshift_and_wait` helpers ignore failures from `run_command_on_vm`,
so add explicit return-code propagation for the `tee` write and the `systemctl
restart microshift` command. Use the same `|| return 1` pattern already used by
helpers like `full_vm_name` so these functions fail immediately at the point of
error rather than only surfacing later in `wait_for_greenboot_on_hosts` or
tunnel checks.
test/assets/c2cc/nettest-pod.yaml (1)

8-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Harden nettest-pod further per manifest guidelines.

Missing readOnlyRootFilesystem: true, container resource limits, and automountServiceAccountToken: false (this pod doesn't need API access). As per path instructions: "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Resource limits (cpu, memory) on every container", and "automountServiceAccountToken: false unless needed".

🔒 Proposed hardening
 spec:
   terminationGracePeriodSeconds: 0
+  automountServiceAccountToken: false
   containers:
   - name: nettest
     image: registry.access.redhat.com/ubi9/ubi:9.6
     command: ["sleep", "infinity"]
     securityContext:
       allowPrivilegeEscalation: false
+      readOnlyRootFilesystem: true
       capabilities:
         drop:
         - ALL
       runAsNonRoot: true
       runAsUser: 10001
       seccompProfile:
         type: RuntimeDefault
+    resources:
+      limits:
+        cpu: 100m
+        memory: 64Mi
+      requests:
+        cpu: 50m
+        memory: 32Mi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/assets/c2cc/nettest-pod.yaml` around lines 8 - 19, The nettest pod spec
is missing required hardening settings; update the nettest container and pod
manifest to match the security guidelines. In the nettest container definition,
add readOnlyRootFilesystem: true alongside the existing securityContext
settings, and add cpu/memory resource limits for the container. Also set
automountServiceAccountToken: false at the pod level since this test pod does
not need API access.

Source: Path instructions

test/resources/ipsec.resource (1)

143-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Docstring claims EMSGSIZE is asserted, but the check only verifies absence of "OK".

The docstring says this keyword Asserts EMSGSIZE (Message too long), but Should Not Contain ${stdout} OK will also pass for any unrelated failure (pod not found, oc CLI error, python traceback for a different reason, transient exec failure). This decouples the assertion from the actual MTU-boundary condition it's meant to verify, making the test pass for the wrong reason.

Also note the near-duplicate Python one-liner between this keyword and Ping With DF Bit And Verify (Lines 130-141) — worth extracting into a shared variable/keyword to avoid divergence if the socket options ever need adjusting.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/resources/ipsec.resource` around lines 143 - 151, The `Ping With DF Bit
Should Fail` keyword currently only checks that `${stdout}` does not contain
"OK", so it can pass for unrelated errors instead of proving EMSGSIZE; update
the assertion to verify the expected "Message too long"/EMSGSIZE failure from
the `Oc On Cluster`/`oc exec` command path. Keep the check tied to the actual
DF-bit UDP send behavior in this keyword, and consider extracting the repeated
Python socket one-liner shared with `Ping With DF Bit And Verify` into a common
variable or helper keyword to avoid future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/resources/ipsec.resource`:
- Around line 130-152: The DF-bit UDP checks in Ping With DF Bit And Verify only
validate local send success via the inline Python3 `socket.sendto()` path, so
oversize packets can still print OK even when the encapsulated packet is later
dropped. Update these helpers to verify delivery from the peer side by adding an
echo/ack check or by retrying the send in the `Oc On Cluster` command before
asserting success/failure, and keep the assertions in `Should Contain` / `Should
Not Contain` aligned with that delivery confirmation.

In `@test/suites/c2cc-ipsec/bigmtu.robot`:
- Around line 69-73: The Jumbo MTU Large TCP Transfer case is calling a keyword
that is only defined in the sibling suite file, so it is not available here.
Move Send Large Payload And Verify out of the ipsec.robot *** Keywords ***
section into the shared ipsec.resource file, then update both bigmtu.robot and
ipsec.robot to use the shared keyword so the suite import can resolve it.

---

Nitpick comments:
In `@test/assets/c2cc/nettest-pod.yaml`:
- Around line 8-19: The nettest pod spec is missing required hardening settings;
update the nettest container and pod manifest to match the security guidelines.
In the nettest container definition, add readOnlyRootFilesystem: true alongside
the existing securityContext settings, and add cpu/memory resource limits for
the container. Also set automountServiceAccountToken: false at the pod level
since this test pod does not need API access.

In `@test/bin/c2cc_common.sh`:
- Around line 363-383: The `configure_pod_mtu` and `restart_microshift_and_wait`
helpers ignore failures from `run_command_on_vm`, so add explicit return-code
propagation for the `tee` write and the `systemctl restart microshift` command.
Use the same `|| return 1` pattern already used by helpers like `full_vm_name`
so these functions fail immediately at the point of error rather than only
surfacing later in `wait_for_greenboot_on_hosts` or tunnel checks.

In `@test/resources/ipsec.resource`:
- Around line 143-151: The `Ping With DF Bit Should Fail` keyword currently only
checks that `${stdout}` does not contain "OK", so it can pass for unrelated
errors instead of proving EMSGSIZE; update the assertion to verify the expected
"Message too long"/EMSGSIZE failure from the `Oc On Cluster`/`oc exec` command
path. Keep the check tied to the actual DF-bit UDP send behavior in this
keyword, and consider extracting the repeated Python socket one-liner shared
with `Ping With DF Bit And Verify` into a common variable or helper keyword to
avoid future drift.
🪄 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: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: ad866bf9-8cb2-4661-84c1-89b6ba52bf02

📥 Commits

Reviewing files that changed from the base of the PR and between 0e09fc5 and 7e16b1a.

📒 Files selected for processing (8)
  • test/assets/c2cc/nettest-pod.yaml
  • test/bin/c2cc_common.sh
  • test/resources/c2cc.resource
  • test/resources/ipsec.resource
  • test/scenarios-bootc/c2cc/el102-src@c2cc-ipsec.sh
  • test/scenarios-bootc/c2cc/el98-src@c2cc-ipsec.sh
  • test/suites/c2cc-ipsec/bigmtu.robot
  • test/suites/c2cc-ipsec/ipsec.robot

Comment thread test/resources/ipsec.resource
Comment thread test/suites/c2cc/ipsec/bigmtu.robot Outdated
@agullon

agullon commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Addressed CodeRabbit nitpick findings:

  • c2cc_common.sh:363-383 (unchecked run_command_on_vm returns): Won't fix — script runs with set -e, so run_command_on_vm failure already aborts. Same pattern as configure_c2cc_host which doesn't use || return 1 either.
  • nettest-pod.yaml:8-19 (missing readOnlyRootFilesystem, resource limits, automountServiceAccountToken): Won't fix — neither curl-pod.yaml nor hello-microshift.yaml uses any of these. Following the existing C2CC test pod pattern.
  • ipsec.resource:143-151 (docstring claims EMSGSIZE but only checks absence of "OK"): Won't fix — the "should pass" tests run first, so if Python/oc is broken, those fail before the "should fail" tests are reached. A false positive from an unrelated failure is caught upstream.

@agullon

agullon commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-tests-bootc-c2cc
/test e2e-aws-tests-bootc-c2cc-arm

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/resources/ipsec.resource (1)

161-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Capture stderr for easier failure diagnosis.

Unlike the DF-bit keywords (which redirect 2>&1), this command doesn't capture curl/dd stderr into ${stdout}, so a failed Should Contain Hello from assertion won't show why (e.g., curl connection error, timeout).

♻️ Suggested tweak
     ${stdout}=    Oc On Cluster
     ...    ${alias}
-    ...    oc exec curl-pod -n ${NAMESPACES}[${alias}] -- sh -c 'dd if=/dev/zero bs=${size} count=1 2>/dev/null | curl -sS --max-time 15 --data-binary `@-` http://${ip}:8080/cgi-bin/hello'
+    ...    oc exec curl-pod -n ${NAMESPACES}[${alias}] -- sh -c 'dd if=/dev/zero bs=${size} count=1 2>/dev/null | curl -sS --max-time 15 --data-binary `@-` http://${ip}:8080/cgi-bin/hello' 2>&1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/resources/ipsec.resource` around lines 161 - 167, The Send Large Payload
And Verify keyword currently does not capture curl/dd stderr, so failures in the
Oc On Cluster command are hard to diagnose. Update the shell command inside this
keyword to redirect stderr into stdout the same way the DF-bit keywords do, so
${stdout} includes curl or dd error details before the Should Contain assertion
runs. Keep the change localized to Send Large Payload And Verify and preserve
the existing behavior otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/resources/ipsec.resource`:
- Around line 161-167: The Send Large Payload And Verify keyword currently does
not capture curl/dd stderr, so failures in the Oc On Cluster command are hard to
diagnose. Update the shell command inside this keyword to redirect stderr into
stdout the same way the DF-bit keywords do, so ${stdout} includes curl or dd
error details before the Should Contain assertion runs. Keep the change
localized to Send Large Payload And Verify and preserve the existing behavior
otherwise.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: b50d1ba5-ac25-443a-85f8-de8ec321520a

📥 Commits

Reviewing files that changed from the base of the PR and between 7e16b1a and 0211fa4.

📒 Files selected for processing (2)
  • test/resources/ipsec.resource
  • test/suites/c2cc-ipsec/ipsec.robot
💤 Files with no reviewable changes (1)
  • test/suites/c2cc-ipsec/ipsec.robot

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2026
@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch from 53b1ace to 096f15f Compare July 9, 2026 13:40
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
test/suites/c2cc/ipsec/mtu.robot (2)

101-112: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Fragile default-interface detection.

ip route show default | awk '{print $5}' | head -1 assumes exactly one relevant default route; with dual-stack or ECMP routes, head -1 may pick an interface that isn't the one actually carrying traffic, silently mis-targeting the MTU change.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/suites/c2cc/ipsec/mtu.robot` around lines 101 - 112, The
default-interface lookup in Configure Jumbo MTU On All Clusters is too fragile
because it relies on a single routed result and head -1. Update the Command On
Cluster command used to determine ${iface} so it selects the actual egress
interface more robustly, and keep the rest of the MTU change/verification flow
unchanged. Use the Configure Jumbo MTU On All Clusters keyword and the ${iface}
assignment as the main place to fix this.

60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Stateful setup disguised as a test case.

Reconfigure Pod MTU To 8900 mutates cluster infra (config, restart, redeploy) as a regular test case rather than a [Setup]/suite fixture. If this test is filtered out, skipped, or fails partway, later Phase 3 tests (Verify Pod MTU Configuration, Jumbo MTU At Reduced Pod MTU, etc.) will silently execute against an undefined MTU state and produce misleading pass/fail results, with no repair path in Teardown. The suite comment acknowledges TEST_RANDOMIZATION=none as the safeguard, but partial failure of this one test case is not handled by the current teardown/setup structure.

Consider moving this into the suite structure as an explicit [Setup] for a nested test block, or ensure downstream tests defensively re-verify pod MTU rather than assuming success of a prior sibling test case.

Also applies to: 86-90

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/suites/c2cc/ipsec/mtu.robot` around lines 60 - 66, The MTU
reconfiguration step is currently implemented as a regular test case in
Reconfigure Pod MTU To 8900, which makes later Phase 3 tests depend on a sibling
test’s success. Move the cluster mutation logic into suite-level setup for the
Phase 3 block, or into a dedicated nested [Setup] fixture, so Configure Pod MTU
On All Clusters, Restart MicroShift On All Clusters, and Redeploy Test Workloads
always run before Verify Pod MTU Configuration and Jumbo MTU At Reduced Pod MTU.
If you keep the test case, add defensive re-verification in the downstream
checks so they do not assume the prior state transition succeeded.

Source: Learnings

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@test/suites/c2cc/ipsec/mtu.robot`:
- Around line 101-112: The default-interface lookup in Configure Jumbo MTU On
All Clusters is too fragile because it relies on a single routed result and head
-1. Update the Command On Cluster command used to determine ${iface} so it
selects the actual egress interface more robustly, and keep the rest of the MTU
change/verification flow unchanged. Use the Configure Jumbo MTU On All Clusters
keyword and the ${iface} assignment as the main place to fix this.
- Around line 60-66: The MTU reconfiguration step is currently implemented as a
regular test case in Reconfigure Pod MTU To 8900, which makes later Phase 3
tests depend on a sibling test’s success. Move the cluster mutation logic into
suite-level setup for the Phase 3 block, or into a dedicated nested [Setup]
fixture, so Configure Pod MTU On All Clusters, Restart MicroShift On All
Clusters, and Redeploy Test Workloads always run before Verify Pod MTU
Configuration and Jumbo MTU At Reduced Pod MTU. If you keep the test case, add
defensive re-verification in the downstream checks so they do not assume the
prior state transition succeeded.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c8d367af-0c47-4a80-8c6a-7b8945ef3cf4

📥 Commits

Reviewing files that changed from the base of the PR and between 096f15f and 8ff6553.

📒 Files selected for processing (1)
  • test/suites/c2cc/ipsec/mtu.robot

@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch from f631e3f to a06baac Compare July 9, 2026 14:34
@agullon

agullon commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-tests-bootc-c2cc
/test e2e-aws-tests-bootc-c2cc-arm

@agullon agullon changed the title WIP: Add MTU boundary tests for C2CC IPsec Add MTU boundary tests for C2CC IPsec Jul 9, 2026
@agullon agullon changed the title Add MTU boundary tests for C2CC IPsec WIP: Add MTU boundary tests for C2CC IPsec Jul 9, 2026
@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch from a06baac to 958ac82 Compare July 9, 2026 18:56
@agullon

agullon commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-tests-bootc-c2cc
/test e2e-aws-tests-bootc-c2cc-arm

@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch from 958ac82 to 8cdb214 Compare July 9, 2026 20:52
@agullon

agullon commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-tests-bootc-c2cc
/test e2e-aws-tests-bootc-c2cc-arm

@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch 2 times, most recently from 176c9b3 to 78b9d67 Compare July 10, 2026 06:34
@agullon

agullon commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-tests-bootc-c2cc
/test e2e-aws-tests-bootc-c2cc-arm

@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch 3 times, most recently from a590b9c to 8570b5a Compare July 10, 2026 09:34
@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch from 6e4224f to 7031a0f Compare July 14, 2026 14:49
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2026
@agullon
agullon marked this pull request as ready for review July 14, 2026 14:53
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2026
@agullon
agullon marked this pull request as draft July 14, 2026 14:53
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2026
@openshift-ci
openshift-ci Bot requested review from eslutsky and jerpeter1 July 14, 2026 14:54
@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch from 7031a0f to c336b53 Compare July 14, 2026 14:54
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2026
@agullon
agullon marked this pull request as ready for review July 14, 2026 14:55
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2026
@openshift-ci
openshift-ci Bot requested review from ggiguash and pmtk July 14, 2026 14:56
@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch 6 times, most recently from f2f06d5 to daddcb0 Compare July 16, 2026 08:32
@agullon

agullon commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

/retest

agullon added 6 commits July 16, 2026 13:25
Add infrastructure to support jumbo frame (MTU 9000) testing in C2CC
scenarios:

- nettest-pod: non-root pod for UDP DF-bit testing without NET_RAW
- df-bit-send.py: sends DF-bit UDP datagrams using IP_PMTUDISC_DO
  (IPv4) or IPV6_DONTFRAG (IPv6), auto-detects address family
- jumbo/jumbo-ipv6 libvirt networks with MTU 9000
- inject_kickstart_mtu(): configures guest NIC MTU via NM conf.d,
  NM dispatcher, and systemd oneshot; writes pod MTU to ovn.yaml
- c2cc_create_jumbo_network/ipv6: race-safe network creation — each
  step tolerates concurrent callers and a final active-state check
  catches genuine failures
- launch_vm --network_mtu: propagates MTU to QEMU's virtio-net driver
- --check disk_size=off: skip false-positive pool capacity check when
  thin-provisioned qcow2 nominal sizes exceed backing filesystem
- Deploy nettest-pod alongside existing test workloads

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

pre-commit.check-secrets: ENABLED
Add Robot Framework keywords for MTU boundary testing and IPv6 support:

- Ping With DF Bit And Verify / Should Fail: pipe df-bit-send.py via
  stdin to nettest-pod; assert EMSGSIZE for rejection
- Get Pod Interface MTU: read pod eth0 MTU via sysfs
- Send Large Payload And Verify: TCP payload via curl POST (head -c)
- Ping/Large Payload Between All Clusters: iterate all 6 cluster pairs
- Fix nftables enforcement for IPv6 (ip6 daddr vs ip daddr)
- Fix curl URLs for IPv6 (bracket notation)
- Add curl failure mode assertions (timeout/refused/unreachable)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

pre-commit.check-secrets: ENABLED
Add ipsec.robot (15 tests) covering ESP encapsulation, cross-cluster
connectivity, source IP preservation, policy enforcement, plaintext
rejection, host-to-pod rejection, DF-bit MTU boundary at 1500, DNS
resolution, and tunnel recovery after restart.

DF-bit tests validate the ESP-adjusted PMTU using UDP datagrams with
IP_PMTUDISC_DO across all 6 cluster pair directions.

Rename c2cc-ipsec scenarios to c2cc-ipsec-ipv4 for consistency with
new c2cc-ipsec-ipv6 variants that run the same suite over IPv6.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

pre-commit.check-secrets: ENABLED
Add mtu.robot (8 tests) validating MTU boundary behavior for plain
C2CC traffic at jumbo frame size (9000 MTU) without IPsec.

Tests verify DF-bit acceptance and rejection at the pod MTU boundary
(8922 = NIC 9000 - 78B headroom), 64KB TCP transfers, and full
cross-cluster connectivity across all 6 cluster pairs.

Includes IPv4 and IPv6 scenario scripts for both el98 and el102.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

pre-commit.check-secrets: ENABLED
Add ipsec-mtu.robot (13 tests) validating MTU boundary behavior
through IPsec tunnel-mode ESP encapsulation at jumbo frame size
(9000 MTU) with the recommended pod MTU of 8900.

Phase 1 validates DF-bit boundaries, ESP encapsulation, and TCP
transfers at pod MTU 8900 (set via kickstart ovn.yaml).
Phase 2 re-applies the config, restarts MicroShift, and verifies
the boundaries survive a restart cycle with tunnel reestablishment.

Includes IPv4 and IPv6 scenario scripts for both el98 and el102.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

pre-commit.check-secrets: ENABLED
Each arch now runs only one RHEL version: x86 runs el98 (RHEL 9.8),
ARM runs el102 (RHEL 10.2). The assignment rotates per commit using
the first hex digit of HEAD so both combinations get exercised over
time. Both arch jobs derive the same flip from the same merge commit.

This halves the scenario count per CI job (23 → ~12), preventing
/tmp exhaustion from parallel's output buffering and reducing VM
resource contention on the c7g.metal instances.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

pre-commit.check-secrets: ENABLED
@agullon
agullon force-pushed the c2cc-ipsec-mtu-tests branch from daddcb0 to 2969e02 Compare July 16, 2026 11:47
@agullon

agullon commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-ci

openshift-ci Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@agullon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-tests-bootc-periodic-el9 2969e02 link true /test e2e-aws-tests-bootc-periodic-el9
ci/prow/e2e-aws-tests-bootc-el10 2969e02 link true /test e2e-aws-tests-bootc-el10
ci/prow/e2e-aws-tests-bootc-c2cc 2969e02 link true /test e2e-aws-tests-bootc-c2cc
ci/prow/e2e-aws-tests-bootc-periodic-el10 2969e02 link true /test e2e-aws-tests-bootc-periodic-el10

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants