USHIFT-7382: Add MTU boundary tests for C2CC and C2CC+IPsec#7024
USHIFT-7382: Add MTU boundary tests for C2CC and C2CC+IPsec#7024agullon wants to merge 6 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a hardened ChangesIPsec MTU/DF-bit test coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test e2e-aws-tests-bootc-c2cc |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/bin/c2cc_common.sh (1)
363-383: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnchecked command results in
configure_pod_mtu/restart_microshift_and_wait.Neither the
teewrite inconfigure_pod_mtunor thesystemctl restart microshiftinrestart_microshift_and_waitcheckrun_command_on_vm's return code. Failures will only surface later (if at all) viawait_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 1for 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 winHarden nettest-pod further per manifest guidelines.
Missing
readOnlyRootFilesystem: true, container resource limits, andautomountServiceAccountToken: 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 winDocstring 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} OKwill also pass for any unrelated failure (pod not found,ocCLI 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
📒 Files selected for processing (8)
test/assets/c2cc/nettest-pod.yamltest/bin/c2cc_common.shtest/resources/c2cc.resourcetest/resources/ipsec.resourcetest/scenarios-bootc/c2cc/el102-src@c2cc-ipsec.shtest/scenarios-bootc/c2cc/el98-src@c2cc-ipsec.shtest/suites/c2cc-ipsec/bigmtu.robottest/suites/c2cc-ipsec/ipsec.robot
|
Addressed CodeRabbit nitpick findings:
|
|
/test e2e-aws-tests-bootc-c2cc |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/resources/ipsec.resource (1)
161-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture 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 failedShould Contain Hello fromassertion 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
📒 Files selected for processing (2)
test/resources/ipsec.resourcetest/suites/c2cc-ipsec/ipsec.robot
💤 Files with no reviewable changes (1)
- test/suites/c2cc-ipsec/ipsec.robot
openshift-ci
Bot
added
the
needs-rebase
53b1ace to
096f15f
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/suites/c2cc/ipsec/mtu.robot (2)
101-112: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFragile default-interface detection.
ip route show default | awk '{print $5}' | head -1assumes exactly one relevant default route; with dual-stack or ECMP routes,head -1may 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 tradeoffStateful setup disguised as a test case.
Reconfigure Pod MTU To 8900mutates 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 inTeardown. The suite comment acknowledgesTEST_RANDOMIZATION=noneas 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
📒 Files selected for processing (1)
test/suites/c2cc/ipsec/mtu.robot
f631e3f to
a06baac
Compare
|
/test e2e-aws-tests-bootc-c2cc |
agullon
changed the title
agullon
changed the title
a06baac to
958ac82
Compare
|
/test e2e-aws-tests-bootc-c2cc |
958ac82 to
8cdb214
Compare
|
/test e2e-aws-tests-bootc-c2cc |
176c9b3 to
78b9d67
Compare
|
/test e2e-aws-tests-bootc-c2cc |
a590b9c to
8570b5a
Compare
f2f06d5 to
daddcb0
Compare
|
/retest |
daddcb0 to
2969e02
Compare
|
/retest |
| sock.sendto(b"A" * size, (dest, 9999)) | ||
| print("OK") |
There was a problem hiding this comment.
If the sendto fails, it won't reach OK and we'll get an exception?
Should we also close the socket? Just for correctness :)
There was a problem hiding this comment.
I agree, I'll add a try:, except: and finally: blocks.
| @@ -0,0 +1,178 @@ | |||
| *** Settings *** | |||
There was a problem hiding this comment.
We still have the test/suites/c2cc/ipsec/ipsec.robot - should we delete the old file? I don't see it being used anywhere
There was a problem hiding this comment.
I agree, I'll remove it.
| restorecon -R /etc/NetworkManager/dispatcher.d/ 2>&1 || logger -t jumbo-mtu "restorecon failed for dispatcher.d" | ||
|
|
||
| # Systemd service -- runs before microshift.service. | ||
| # Script in /etc/ because /usr/ is read-only on bootc images. |
There was a problem hiding this comment.
But is the system readonly during installation? I think it might not be yet, only after rebooting
| # to attach to the console. `unbuffer` provides the TTY. | ||
| # shellcheck disable=SC2086 | ||
| if ${timeout_install} unbuffer sudo virt-install \ | ||
| --check disk_size=off \ |
There was a problem hiding this comment.
I'll drop it, this is not needed with current approach. I forgot to remove it.
This tells virt-install to not check if is enough space in the hypervisor for a new VM. I added it because there was a limit in hypervisor disk with more than 55 VMs. But we are not creating that many VMs anymore.
| DF Bit 8400B Passes At Jumbo MTU | ||
| [Documentation] Mid-range jumbo payload on all cluster pairs. | ||
| Ping DF Bit Between All Clusters 8400 | ||
|
|
||
| DF Bit 8872B Passes At Jumbo MTU | ||
| [Documentation] 8872B + 28B = 8900B, within the pod MTU (8922, set via ovn.yaml). | ||
| Ping DF Bit Between All Clusters 8872 | ||
|
|
||
| DF Bit 8850B Passes At Jumbo MTU | ||
| [Documentation] Near pod MTU (8922, NIC MTU 9000 minus 78B headroom, set via | ||
| ... ovn.yaml) — accepted on all cluster pairs. | ||
| Ping DF Bit Between All Clusters 8850 |
There was a problem hiding this comment.
These tests take 2 seconds each, but are they necessary? To me, it's either within the limit (under the max value), or it's not - testing three cases below the max is redundant I think.
There was a problem hiding this comment.
I agree, I'll remove all but 8872B test.
| DF Bit 8895B Rejected Above Pod MTU | ||
| [Documentation] 8895B + 28B (IPv4) = 8923B > 8922B pod MTU. Rejected. | ||
| ... Also 8895B + 48B (IPv6) = 8943B > 8922B. | ||
| Ping DF Bit Should Fail Between All Clusters 8895 | ||
|
|
||
| DF Bit 8972B Rejected Well Above Pod MTU | ||
| [Documentation] 8972B + 28B = 9000B, well above the pod MTU (8922B). | ||
| Ping DF Bit Should Fail Between All Clusters 8972 |
There was a problem hiding this comment.
Similar comment. I think "Above" and "Well Above" are redundant from "mathematical" point of view.
These things are well into production for many years, so I don't think we need to double check linux networking :)
There was a problem hiding this comment.
I agree, I'll remove 8972B test.
| [Documentation] Re-write pod MTU 8900 via ovn.yaml (same value as kickstart), | ||
| ... restart MicroShift, and redeploy workloads. Validates the config | ||
| ... survives a restart cycle with IPsec tunnel reestablishment. | ||
| Configure Pod MTU On All Clusters 8900 |
There was a problem hiding this comment.
If the ovn.yaml already had mtu: 8900, what does writing it again to the file gives us? It's the same value, nothing changes, so it's no-op, you could just restart microshift.
Validates the config survives a restart cycle
I don't understand - the ovn.yaml file is not consumend on boot, so if you restart microshift or powercycle the host, it won't change and it will still contain the mtu: 8900 set via kickstart.
Since it's the same value, I think ovnk won't reconfigure anything so I'm assuming the IPSec connection isn't dropped at all.
If we'd want to exercise that scenario (Pod MTU reconfiguration -> IPSec works) then maybe it we should change it to e.g. 8899?
There was a problem hiding this comment.
yep, it was a mistake, I refactored all this robot file
| END | ||
| FOR ${alias} IN @{ALL_CLUSTERS} | ||
| Wait Until Keyword Succeeds 5m 15s | ||
| ... Command On Cluster ${alias} microshift healthcheck --timeout=30s |
There was a problem hiding this comment.
I think that 30s timeout proves that "reconfiguration" from 8900 to 8900 does nothing. If the Pods' interfaces were to be reconfigured, the Pods would be restarted and 30s is very optimistic that all Pods will be ready in that time.
b9abef6 to
8525891
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
c6ef38e to
2ac8c85
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
2ac8c85 to
c07013b
Compare
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; pod MTU is configured from the test suite, not kickstart - 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 - 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 - Verify Pod MTU On All Clusters: verify expected MTU on all clusters - Configure Pod MTU On All Clusters: write ovn.yaml on all clusters - Restart MicroShift On All Clusters: restart and wait for health - Redeploy Test Workloads: delete and recreate for new MTU - 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 (10 tests) covering ESP encapsulation, cross-cluster connectivity, source IP preservation, policy enforcement, plaintext rejection, host-to-pod rejection, MTU boundary and TCP transfers at 1500, DNS resolution, and tunnel recovery after restart. Move ipsec.robot from suites/c2cc/ipsec/ to suites/c2cc/extra/ and 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 (1 test) validating MTU boundary behavior for plain C2CC traffic at jumbo frame size (9000 MTU) without IPsec. Configures pod MTU to 9000 from the test suite, then verifies DF-bit acceptance and rejection at the boundary, 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 (2 tests) validating MTU boundary behavior through IPsec tunnel-mode ESP encapsulation at jumbo frame size (9000 MTU) with pod MTU reconfiguration. Phase 1 configures pod MTU 9000 (matching NIC MTU) from the test suite, validates DF-bit boundaries, ESP encapsulation, and large TCP transfers. Phase 2 reconfigures to 8900 (recommended ESP headroom), restarts MicroShift, and verifies OVN-K applies the new MTU with correct boundary enforcement. 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
c07013b to
fc1bd69
Compare
| sys.exit(1) | ||
| finally: | ||
| sock.close() |
There was a problem hiding this comment.
Do you think finally will execute or sys.exit will short circut the whole app and return early?
| Ping With DF Bit And Verify | ||
| [Documentation] Send a DF-bit UDP datagram via df-bit-send.py and assert success. | ||
| ... Auto-detects IPv4/IPv6. See the script for protocol details. | ||
| [Arguments] ${alias} ${dest_ip} ${payload_size} | ||
| ${stdout}= Oc On Cluster | ||
| ... ${alias} | ||
| ... oc exec -i nettest-pod -n ${NAMESPACES}[${alias}] -- python3 - ${dest_ip} ${payload_size} < ${DF_SCRIPT} 2>&1 | ||
| ... allow_fail=${TRUE} | ||
| Should Contain ${stdout} OK | ||
| ... DF-bit UDP send of ${payload_size}B to ${dest_ip} failed: ${stdout} | ||
|
|
||
| Ping With DF Bit Should Fail | ||
| [Documentation] Send a UDP datagram with DF bit set. Asserts EMSGSIZE (Message too long). | ||
| ... Proves the datagram exceeds the ESP-adjusted PMTU. Auto-detects IPv4 vs | ||
| ... IPv6 from the destination address. Checks for the specific EMSGSIZE error | ||
| ... to distinguish MTU rejection from other failures. | ||
| [Arguments] ${alias} ${dest_ip} ${payload_size} | ||
| ${stdout}= Oc On Cluster | ||
| ... ${alias} | ||
| ... oc exec -i nettest-pod -n ${NAMESPACES}[${alias}] -- python3 - ${dest_ip} ${payload_size} < ${DF_SCRIPT} 2>&1 || true | ||
| ... allow_fail=${TRUE} | ||
| Should Not Contain ${stdout} OK | ||
| ... DF-bit UDP send of ${payload_size}B should have been rejected but succeeded | ||
| Should Contain ${stdout} Message too long | ||
| ... DF-bit UDP send of ${payload_size}B expected EMSGSIZE but got: ${stdout} | ||
|
|
||
| Get Pod Interface MTU | ||
| [Documentation] Return the MTU of the pod's eth0 interface. | ||
| [Arguments] ${alias} ${pod} ${ns} | ||
| ${stdout}= Oc On Cluster ${alias} | ||
| ... oc exec ${pod} -n ${ns} -- cat /sys/class/net/eth0/mtu | ||
| ${mtu}= Strip String ${stdout} | ||
| RETURN ${mtu} | ||
|
|
||
| Send Large Payload And Verify | ||
| [Documentation] Send a large payload via curl POST and verify it succeeds. | ||
| [Arguments] ${alias} ${ip} ${size} | ||
| ${is_ipv6}= Evaluate ':' in '''${ip}''' | ||
| ${url}= Set Variable If | ||
| ... ${is_ipv6} | ||
| ... http://[${ip}]:8080/cgi-bin/hello | ||
| ... http://${ip}:8080/cgi-bin/hello | ||
| ${stdout}= Oc On Cluster | ||
| ... ${alias} | ||
| ... oc exec curl-pod -n ${NAMESPACES}[${alias}] -- sh -c 'head -c ${size} /dev/zero | curl -sS --max-time 15 --data-binary @- ${url}' | ||
| Should Contain ${stdout} Hello from | ||
|
|
||
| Ping DF Bit Between All Clusters | ||
| [Documentation] Send DF-bit UDP payload across all 6 cluster pairs. | ||
| [Arguments] ${size} | ||
| FOR ${src} ${dst} IN | ||
| ... cluster-a cluster-b | ||
| ... cluster-a cluster-c | ||
| ... cluster-b cluster-a | ||
| ... cluster-b cluster-c | ||
| ... cluster-c cluster-a | ||
| ... cluster-c cluster-b | ||
| ${ip}= Get Hello Pod IP ${dst} | ||
| Ping With DF Bit And Verify ${src} ${ip} ${size} | ||
| END | ||
|
|
||
| Ping DF Bit Should Fail Between All Clusters | ||
| [Documentation] Send DF-bit UDP payload expecting rejection on all 6 pairs. | ||
| [Arguments] ${size} | ||
| FOR ${src} ${dst} IN | ||
| ... cluster-a cluster-b | ||
| ... cluster-a cluster-c | ||
| ... cluster-b cluster-a | ||
| ... cluster-b cluster-c | ||
| ... cluster-c cluster-a | ||
| ... cluster-c cluster-b | ||
| ${ip}= Get Hello Pod IP ${dst} | ||
| Ping With DF Bit Should Fail ${src} ${ip} ${size} | ||
| END | ||
|
|
||
| Large Payload Between All Clusters | ||
| [Documentation] Send large TCP payload across all 6 cluster pairs. | ||
| [Arguments] ${size} | ||
| FOR ${src} ${dst} IN | ||
| ... cluster-a cluster-b | ||
| ... cluster-a cluster-c | ||
| ... cluster-b cluster-a | ||
| ... cluster-b cluster-c | ||
| ... cluster-c cluster-a | ||
| ... cluster-c cluster-b | ||
| ${ip}= Get Hello Pod IP ${dst} | ||
| Send Large Payload And Verify ${src} ${ip} ${size} | ||
| END | ||
|
|
||
| Configure Pod MTU On All Clusters | ||
| [Documentation] Write /etc/microshift/ovn.yaml with the given MTU value. | ||
| [Arguments] ${mtu} | ||
| FOR ${alias} IN @{ALL_CLUSTERS} | ||
| Command On Cluster | ||
| ... ${alias} | ||
| ... bash -c 'echo "mtu: ${mtu}" > /etc/microshift/ovn.yaml' | ||
| END | ||
|
|
||
| Restart MicroShift On All Clusters | ||
| [Documentation] Restart MicroShift on all clusters and wait for health. | ||
| FOR ${alias} IN @{ALL_CLUSTERS} | ||
| Command On Cluster ${alias} systemctl restart microshift | ||
| END | ||
| FOR ${alias} IN @{ALL_CLUSTERS} | ||
| Wait Until Keyword Succeeds 5m 15s | ||
| ... Command On Cluster ${alias} microshift healthcheck --timeout=30s | ||
| END | ||
|
|
||
| Verify Pod MTU On All Clusters | ||
| [Documentation] Verify the pod network interface reports the expected MTU. | ||
| [Arguments] ${expected_mtu} | ||
| FOR ${alias} IN @{ALL_CLUSTERS} | ||
| ${mtu}= Get Pod Interface MTU ${alias} nettest-pod ${NAMESPACES}[${alias}] | ||
| Should Be Equal As Strings ${mtu} ${expected_mtu} | ||
| ... ${alias}: pod MTU is ${mtu}, expected ${expected_mtu} | ||
| END | ||
|
|
||
| Redeploy Test Workloads | ||
| [Documentation] Delete and redeploy test workloads so pods pick up the new MTU. | ||
| Cleanup Test Workloads | ||
| Deploy Test Workloads |
There was a problem hiding this comment.
Aren't these keywords in general more related to C2CC than ipsec?
| Resource ../../../resources/kubeconfig.resource | ||
| Resource ../../../resources/oc.resource | ||
| Resource ../../../resources/c2cc.resource | ||
| Resource ../../../resources/ipsec.resource |
There was a problem hiding this comment.
Other side of having MTU related keywords in ipsec.resource
| # Cluster A (host1): non-overlapping CIDRs | ||
| CLUSTER_A_POD_CIDR="fd01::/48" | ||
| CLUSTER_A_SVC_CIDR="fd02::/112" | ||
| CLUSTER_A_DOMAIN="cluster-a.remote" | ||
|
|
||
| # Cluster B (host2): non-overlapping CIDRs | ||
| CLUSTER_B_POD_CIDR="fd04::/48" | ||
| CLUSTER_B_SVC_CIDR="fd05::/112" | ||
| CLUSTER_B_DOMAIN="cluster-b.remote" | ||
|
|
||
| # Cluster C (host3): non-overlapping CIDRs | ||
| CLUSTER_C_POD_CIDR="fd07::/48" | ||
| CLUSTER_C_SVC_CIDR="fd08::/112" | ||
| CLUSTER_C_DOMAIN="cluster-c.remote" |
There was a problem hiding this comment.
I'm wondering if we should move these variables to c2cc_common.sh and somehow "enable" them for ipv6 scenarios, because now they're duplicated in each ipv6 scenario
| c2cc_create_jumbo_network() { | ||
| local -r netconfig="${ROOTDIR}/test/assets/network/jumbo-network.xml" | ||
| sudo virsh net-define "${netconfig}" 2>/dev/null || true | ||
| sudo virsh net-start jumbo 2>/dev/null || true | ||
| sudo virsh net-autostart jumbo 2>/dev/null || true | ||
| sudo virsh net-info jumbo | grep "Active:.*yes" >/dev/null | ||
| } | ||
|
|
||
| # c2cc_create_jumbo_ipv6_network creates a single-stack IPv6 libvirt network | ||
| # with MTU 9000 for jumbo frame testing. Idempotent — skips if the network | ||
| # already exists. Kept separate from the shared VM_IPV6_NETWORK ("ipv6") so | ||
| # jumbo MTU doesn't affect other IPv6 scenarios. | ||
| c2cc_create_jumbo_ipv6_network() { | ||
| local -r netconfig="${ROOTDIR}/test/assets/network/jumbo-ipv6-network.xml" | ||
| sudo virsh net-define "${netconfig}" 2>/dev/null || true | ||
| sudo virsh net-start jumbo-ipv6 2>/dev/null || true | ||
| sudo virsh net-autostart jumbo-ipv6 2>/dev/null || true | ||
| sudo virsh net-info jumbo-ipv6 | grep "Active:.*yes" >/dev/null | ||
|
|
||
| # Add the bridge's IPv6 subnet to the firewall trusted zone so VMs | ||
| # can reach the hypervisor's services (registry mirror via hostname). | ||
| # Re-applied on every run in case the firewall was reset since creation. | ||
| local bridge | ||
| bridge=$(sudo virsh net-info jumbo-ipv6 | grep '^Bridge:' | awk '{print $2}') | ||
| if [[ -z "${bridge}" ]]; then | ||
| echo "ERROR: Could not determine bridge name for jumbo-ipv6 network" >&2 | ||
| return 1 | ||
| fi | ||
| local ip | ||
| for ip in $(ip addr show "${bridge}" | grep "scope global" | awk '{print $2}'); do | ||
| sudo firewall-cmd --permanent --zone=trusted --add-source="${ip}" | ||
| done | ||
| sudo firewall-cmd --reload |
There was a problem hiding this comment.
Not a big deal but wanted to point out: usually we create networks in the manage_hypervisor_config.sh.
Do you think we should move it there? If 2 scenarios (either two el98 or two el102) want to create that network, would we get race conditions?
| if [[ "$(uname -m)" == "x86_64" ]]; then | ||
| if [[ "${flip}" -eq 0 ]]; then | ||
| delete_ver=el102 | ||
| else | ||
| delete_ver=el98 | ||
| fi | ||
| else | ||
| if [[ "${flip}" -eq 0 ]]; then | ||
| delete_ver=el98 | ||
| else | ||
| delete_ver=el102 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Shorter version, use if you want - I don't mind what we have, just was wondering how to improve it
if [[ "$(uname -m)" == "x86_64" ]]; then
vers=(el102 el98)
else
vers=(el98 el102)
fi
delete_ver=${vers[flip]}
|
|
||
| # IPsec tests have ordering dependencies (setup verification must pass before | ||
| # enforcement tests), so disable randomization. | ||
| export TEST_RANDOMIZATION=none |
There was a problem hiding this comment.
I think we could drop this, c2cc_common.sh has export TEST_RANDOMIZATION=suites by default, and since we run only 1 suite (single file ipsec.robot, not whole dir), this is redundant.
| IPsec At Jumbo MTU 9000 | ||
| [Documentation] Configure pod MTU to 9000 (matching NIC MTU), deploy | ||
| ... workloads, and validate DF-bit boundaries, ESP encapsulation, | ||
| ... large TCP transfers, and full connectivity through IPsec tunnels. | ||
| Configure Pod MTU On All Clusters 9000 | ||
| Restart MicroShift And Wait For IPsec | ||
| Deploy Test Workloads | ||
| Verify Pod MTU On All Clusters 9000 | ||
| Verify ESP Encapsulation On All Clusters | ||
| Ping DF Bit Between All Clusters 64 | ||
| Ping DF Bit Between All Clusters 8952 | ||
| Ping DF Bit Should Fail Between All Clusters 8973 | ||
| Large Payload Between All Clusters 65536 | ||
|
|
||
| IPsec After Pod MTU Reconfiguration To 8900 |
There was a problem hiding this comment.
I need some insight into these two test cases. I see that what's different is that UDP datagram with DF is smaller and we check that the reconfiguration works.
But both tests exercise the the same "distance from the max" value? Do we need both? If yes, can we use RF templates?
| Configure Pod MTU On All Clusters 9000 | ||
| Restart MicroShift On All Clusters | ||
| Deploy Test Workloads | ||
| Verify Pod MTU On All Clusters 9000 | ||
| Verify Full C2CC Connectivity | ||
| Ping DF Bit Between All Clusters 64 | ||
| Ping DF Bit Between All Clusters 8952 | ||
| Ping DF Bit Should Fail Between All Clusters 8973 | ||
| Large Payload Between All Clusters 65536 |
There was a problem hiding this comment.
Very similar to ipsec-mtu.robot, but I don't have idea on common keywords without creating a mess, so we can leave that as it is
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@agullon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
3 participants
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 test suites under
test/suites/c2cc/extra/:ipsec.robotmtu.robotipsec-mtu.robot12 new scenario scripts (el98 + el102 × IPv4 + IPv6 × 3 suites):
c2cc-ipsec-ipv4/ipv6— IPsec functional + DF-bit tests at 1500 MTUc2cc-mtu-ipv4/ipv6— jumbo frame (9000) C2CC without IPsecc2cc-ipsec-mtu-ipv4/ipv6— jumbo frame C2CC with IPsec + pod MTU reconfigurationTest approach
Tests use
df-bit-send.py, a Python3 script that sends UDP datagrams with the DF bit set usingIP_PMTUDISC_DO(IPv4) /IPV6_DONTFRAG(IPv6). The script is piped via stdin tonettest-podthroughoc exec. This validates MTU enforcement at the pod interface level without requiringNET_RAWcapability — thenettest-podruns as non-root under restricted PodSecurity.The "should fail" assertions check for the specific
EMSGSIZEerror 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). Boundary values are chosen to reject on both IPv4 (28B header overhead) and IPv6 (48B), so each test works correctly on both single-stack scenario variants.
IPsec-MTU two-phase test
The
ipsec-mtu.robotsuite validates pod MTU reconfiguration under IPsec:Jumbo MTU infrastructure
Setting jumbo MTU on bootc VMs required solving several layered problems:
NIC MTU: Three mechanisms set the guest NIC to 9000 before MicroShift starts:
conf.d(default ethernet MTU for auto-created connections)ip link setduring connection activation, failures logged vialogger)/etc/because/usr/is read-only on bootc)OVN pod MTU: Configured from the Robot test suite via
Configure Pod MTU On All Clusterskeyword (writes ovn.yaml, restarts MicroShift). The kickstart only sets NIC MTU — pod MTU is set to 9000 (= NIC MTU) because the Geneve tunnel is local in single-node MicroShift and requires no headroom subtraction. C2CC VMs have no default route, so MicroShift's auto-detection falls back to 1500 without an explicit ovn.yaml.Guest virtio-net MTU:
launch_vm --network_mtupropagates the MTU to QEMU's virtio-net driver via virt-install'smtu.sizesub-option. A libvirt network's<mtu>element alone only affects the host-side bridge and tap devices.IPv6 jumbo networks: Separate
jumbo-ipv6libvirt network with MTU 9000. Scenarios use the default IPv4WEB_SERVER_URLfor 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.Race-safe network creation:
c2cc_create_jumbo_networkandc2cc_create_jumbo_ipv6_networktolerate 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
Shared keywords
MTU-related keywords are shared via
ipsec.resource:Configure Pod MTU On All Clusters— write ovn.yamlRestart MicroShift On All Clusters— restart and wait for healthVerify Pod MTU On All Clusters— verify pod eth0 MTU matches expectedRedeploy Test Workloads— delete and recreate pods for new MTUCI infrastructure changes
Also included
ipsec.robotfromsuites/c2cc/ipsec/tosuites/c2cc/extra/c2cc-ipsec→c2cc-ipsec-ipv4for consistency with new IPv6 variantsip6 daddr, curl bracket notation for IPv6 addressesTest plan
e2e-aws-tests-bootc-c2cc)e2e-aws-tests-bootc-c2cc-arm)verify-rf(robocop check + format) passesverify-shell(shellcheck) passes🤖 Generated with Claude Code