Porting Duplicate MAC Test to Origin - #31478
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shreyasbe The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughChangesBare-metal EgressIP failover validation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EgressIPTest
participant OVNKubernetesNode
participant EgressIP
participant NetworkProbe
EgressIPTest->>EgressIP: assign bare-metal EgressIP
EgressIPTest->>OVNKubernetesNode: delete egress node ovnkube-node pod
OVNKubernetesNode-->>EgressIP: migrate EgressIP
EgressIPTest->>NetworkProbe: probe EgressIP MAC responses
NetworkProbe-->>EgressIPTest: return observed MACs
EgressIPTest->>OVNKubernetesNode: verify nftables cleanup and pod recovery
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-ci
Bot
added
the
ready-for-human-review
Signed-off-by: Shreyas Be <52690686+shreyasbe@users.noreply.github.com>
2516552 to
60733c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
test/extended/networking/egressip.go (2)
639-642: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore only the labels that this test added.
The
AfterEachremovesk8s.ovn.org/egress-assignablefrom every worker node. If a node carried that label before this test ran, the test leaves the cluster in a different state and can affect later specs. Track the nodes that the test labels, and remove the label only from those nodes.🤖 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/extended/networking/egressip.go` around lines 639 - 642, Update the egress-label cleanup in the test’s AfterEach block to track which worker nodes were labeled by this test and remove k8s.ovn.org/egress-assignable only from those nodes. Preserve pre-existing labels on all other nodes, using the existing worker-node setup and labeling flow to maintain the tracked-node set.
738-762: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
oc debugpolling is too slow to observe a transient nftables chain.The ticker fires every 200 ms, but each iteration starts a new debug pod on the node. Pod creation and scheduling normally take several seconds. The effective probe interval is therefore seconds, not 200 ms. The
egressip-dropchain exists only during the shutdown window ofovnkube-node, so the check can miss it and the test becomes flaky.Consider one long-lived debug session that loops inside the node shell, or a privileged host-network pod that is created before the deletion and polls locally.
🤖 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/extended/networking/egressip.go` around lines 738 - 762, Replace the per-tick oc.AsAdmin().Run("debug") invocation in the nftChainFound polling goroutine with a long-lived node debug session established before polling, and execute a shell loop inside that session to repeatedly check nftChainCheckCmd locally at the required interval. Ensure the session remains active through ovnkube-node termination, signals nftChainFound when the command detects “FOUND,” and still exits cleanly via stopChecking with proper resource cleanup.
🤖 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/extended/networking/egressip_helpers.go`:
- Around line 1873-1880: Update findNodeEgressIPsBaremetal around the ipnetStr
selection and getFirstFreeIPs call to prevent IPv6 CIDRs from reaching unbounded
address enumeration. Restrict this bare-metal allocation path to IPv4, or
validate the selected CIDR’s prefix length before calling getFirstFreeIPs and
return a clear error for unsupported IPv6 ranges.
- Around line 1839-1846: Handle the error returned by listEgressIPs in the
surrounding helper instead of ignoring it: return or propagate the error
immediately when it is non-nil, and only iterate over egressipList.Items to
populate reservedIPs after a successful call.
- Around line 1790-1824: Add a preflight check before the retry loop that
verifies the selected arping or ndisc6 binary is available in the probe pod,
using the existing adminExecInPod execution context. If the check fails, return
immediately with an error explicitly naming the missing binary; only enter the
MAC-probing loop after presence is confirmed.
In `@test/extended/networking/egressip.go`:
- Around line 771-780: Update the shutdown verification around the nftChainFound
select to close stopChecking exactly once: remove the timeout-branch close and
register a defer immediately after starting the checker goroutine to close the
channel. Preserve the existing timeout failure behavior and successful
verification flow.
- Around line 838-846: Update the nftables cleanup verification around
adminExecInPod so it always validates the command output, including when the
command returns a non-zero error because the table is absent. Retain only the
“No such file” matcher, and fail explicitly for unexpected adminExecInPod errors
instead of logging success unconditionally.
- Around line 694-705: Update the networking test flow around
findBridgePhysicalInterface to resolve and retain separate physical interface
names for egressNode1Name, egressNode2Name, and the probe node. Use each node’s
matching interface for getNodeInterfaceMAC, the discovery command, and
checkForDuplicateMACOnNode instead of reusing egressNode1Name’s interface.
---
Nitpick comments:
In `@test/extended/networking/egressip.go`:
- Around line 639-642: Update the egress-label cleanup in the test’s AfterEach
block to track which worker nodes were labeled by this test and remove
k8s.ovn.org/egress-assignable only from those nodes. Preserve pre-existing
labels on all other nodes, using the existing worker-node setup and labeling
flow to maintain the tracked-node set.
- Around line 738-762: Replace the per-tick oc.AsAdmin().Run("debug") invocation
in the nftChainFound polling goroutine with a long-lived node debug session
established before polling, and execute a shell loop inside that session to
repeatedly check nftChainCheckCmd locally at the required interval. Ensure the
session remains active through ovnkube-node termination, signals nftChainFound
when the command detects “FOUND,” and still exits cleanly via stopChecking with
proper resource cleanup.
🪄 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: Pro Plus
Run ID: e8d13884-9dab-49b5-8305-d877c36cc8dd
📒 Files selected for processing (2)
test/extended/networking/egressip.gotest/extended/networking/egressip_helpers.go
| for i := 0; i < maxChecks; i++ { | ||
| var cmd string | ||
| var macRegex *regexp.Regexp | ||
|
|
||
| if isIPv6 { | ||
| cmd = fmt.Sprintf("ndisc6 -1 -w 1000 %s %s 2>&1", egressIP, interfaceName) | ||
| macRegex = macRegexNdisc6 | ||
| } else { | ||
| cmd = fmt.Sprintf("arping -c 1 -I %s %s 2>&1", interfaceName, egressIP) | ||
| macRegex = macRegexArping | ||
| } | ||
|
|
||
| output, execErr := adminExecInPod(oc, "openshift-ovn-kubernetes", probePodInfo.podName, probePodInfo.containerName, cmd) | ||
| if execErr != nil { | ||
| framework.Logf("Check %d/%d: %s command returned error: %v; output: %s", i+1, maxChecks, toolName, execErr, output) | ||
| } | ||
|
|
||
| matches := macRegex.FindStringSubmatch(output) | ||
| if len(matches) >= 2 { | ||
| respondingMAC := strings.ToLower(strings.TrimSpace(matches[1])) | ||
| if respondingMAC == oldMAC { | ||
| return fmt.Errorf("DUPLICATE MAC DETECTED on check %d: Old node MAC %s responded to %s for egress IP %s after migration. "+ | ||
| "The nftables drop rules should have prevented this response", i+1, oldMAC, toolName, egressIP) | ||
| } else if respondingMAC == expectedMAC { | ||
| foundExpected = true | ||
| framework.Logf("Check %d/%d: New node MAC %s is responding (expected)", i+1, maxChecks, expectedMAC) | ||
| } else { | ||
| return fmt.Errorf("unexpected MAC %s (not old %s or expected %s)", respondingMAC, oldMAC, expectedMAC) | ||
| } | ||
| } | ||
|
|
||
| if i < maxChecks-1 { | ||
| time.Sleep(checkInterval) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether other tests in this repo already rely on arping/ndisc6 inside ovnkube pods,
# and how they guard for tool availability.
rg -n -C4 -w 'arping|ndisc6' --type=goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching egressip_helpers:\n'
fd -a 'egressip_helpers\.go$' . || true
printf '\nRelevant references to arping/ndisc6 in tracked files (text search):\n'
rg -n -C3 -w 'arping|ndisc6' . || true
printf '\nRelevant references to tools/arping/ndisc6 in Dockerfiles/YAML manifests:\n'
rg -n -C3 'arping|ndisc6' --glob 'Dockerfile*' --glob '*.yaml' --glob '*.yml' . || true
printf '\nInspect function context around egressip_helpers.go lines 1720-1850:\n'
file="$(fd 'egressip_helpers\.go$' . | head -n1)"
if [ -n "${file:-}" ]; then
sed -n '1720,1850p' "$file" | nl -ba -v1720
fiRepository: openshift/origin
Length of output: 4585
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'List tracked OpenShift images:\n'
git ls-files | rg '(^images/.*Dockerfile|.*Dockerfile$|(^|/)images/)' | sed -n '1,120p'
printf '\nRelevant context around helper function:\n'
file="$(git ls-files | rg 'test/extended/networking/egressip_helpers\.go$' | head -n1)"
sed -n '1720,1850p' "$file"
printf '\nRelevant context around callsite:\n'
sed -n '610,735p' test/extended/networking/egressip.go
printf '\nSearch for arping/ndisc6 availability assertions and adminExecInPod usages:\n'
rg -n -C3 'adminExecInPod|withTool|tools|Command|arping|ndisc6' test/extended --glob '*.go' | sed -n '1,220p'Repository: openshift/origin
Length of output: 31020
🌐 Web query:
ovnkube-node container image contains arping ndisc6
💡 Result:
The ovnkube-node container image generally contains the tools arping and ndisc6 [1][2], though their presence depends on the base distribution used to build the image (e.g., Fedora, RHEL, or Ubuntu) and the specific version of the OVN-Kubernetes project [3][4][5]. These utilities are commonly included in the container images used by OVN-Kubernetes because they are frequently employed in diagnostic and troubleshooting scripts, such as those used for network tracing and connectivity verification (e.g., validating gateway reachability) [1][2]. For example, diagnostic scripts within the project use arping for IPv4 address resolution and ndisc6 for IPv6 neighbor discovery [1][2]. While these tools are standard in the environments used for these containers, if you find they are missing in a specific deployment, it is often due to variations in the underlying base image packages [4]. You can verify the contents of a specific image by checking the Dockerfile or the package list (often managed via 'more-pkgs' or similar files) in the respective OVN-Kubernetes repository branch [6][3][4].
Citations:
- 1: https://github.com/kubeovn/kube-ovn/blob/master/dist/images/kubectl-ko
- 2: kubeovn/kube-ovn@33272f1
- 3: https://github.com/openshift/ovn-kubernetes/blob/1243011ac02038e3bc254e670d50554ed4c0cf88/Dockerfile
- 4: https://github.com/openshift/ovn-kubernetes/blob/49cca4708e73f4bbee3960e761dfe82049032837/Dockerfile.rhel9
- 5: https://ovn-kubernetes.io/developer-guide/image-build/
- 6: https://github.com/openshift/ovn-kubernetes/blob/d7d9a5611ff326a13674a44b6782b5b4de98cda7/Dockerfile
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files with adminExecInPod definitions/usages:\n'
rg -n -C4 'func adminExecInPod|execOptions|adminExecInPod' --glob '*.go' .
printf '\nRelevant helper implementations:\n'
for file in $(rg -l 'func adminExecInPod\(' --glob '*.go' .); do
echo "--- $file"
sed -n '1,240p' "$file"
doneRepository: openshift/origin
Length of output: 17739
Add a clear binary-presence assertion before probing legacy responses.
arping and ndisc6 are queried from the openshift-ovn-kubernetes pod with adminExecInPod, so a missing tool currently exits via a no such file or directory execution error and later falls through to the generic “did not observe expected MAC” path. Fail before the retry loop with a message that names the missing arping/ndisc6 binary.
🤖 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/extended/networking/egressip_helpers.go` around lines 1790 - 1824, Add a
preflight check before the retry loop that verifies the selected arping or
ndisc6 binary is available in the probe pod, using the existing adminExecInPod
execution context. If the check fails, return immediately with an error
explicitly naming the missing binary; only enter the MAC-probing loop after
presence is confirmed.
| egressipList, err := listEgressIPs(oc) | ||
| if err == nil { | ||
| for _, egressip := range egressipList.Items { | ||
| for _, ip := range egressip.Spec.EgressIPs { | ||
| reservedIPs = append(reservedIPs, ip) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not swallow the listEgressIPs error.
If listEgressIPs fails, reservedIPs misses all EgressIPs that are already in use. The function then can allocate an IP that another EgressIP object owns, which makes the test flaky and can disturb other objects. Return the error instead.
As per path instructions: "Never ignore error returns".
🐛 Proposed fix
egressipList, err := listEgressIPs(oc)
- if err == nil {
- for _, egressip := range egressipList.Items {
- for _, ip := range egressip.Spec.EgressIPs {
- reservedIPs = append(reservedIPs, ip)
- }
- }
- }
+ if err != nil {
+ return nil, fmt.Errorf("failed to list EgressIPs: %v", err)
+ }
+ for _, egressip := range egressipList.Items {
+ reservedIPs = append(reservedIPs, egressip.Spec.EgressIPs...)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| egressipList, err := listEgressIPs(oc) | |
| if err == nil { | |
| for _, egressip := range egressipList.Items { | |
| for _, ip := range egressip.Spec.EgressIPs { | |
| reservedIPs = append(reservedIPs, ip) | |
| } | |
| } | |
| } | |
| egressipList, err := listEgressIPs(oc) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to list EgressIPs: %v", err) | |
| } | |
| for _, egressip := range egressipList.Items { | |
| reservedIPs = append(reservedIPs, egressip.Spec.EgressIPs...) | |
| } |
🤖 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/extended/networking/egressip_helpers.go` around lines 1839 - 1846,
Handle the error returned by listEgressIPs in the surrounding helper instead of
ignoring it: return or propagate the error immediately when it is non-nil, and
only iterate over egressipList.Items to populate reservedIPs after a successful
call.
Source: Path instructions
| ipnetStr := nodeEgressIPConfigs[0].IFAddr.IPv4 | ||
| if ipnetStr == "" { | ||
| ipnetStr = nodeEgressIPConfigs[0].IFAddr.IPv6 | ||
| } | ||
| freeIPs, err := getFirstFreeIPs(ipnetStr, reservedIPs, configv1.NonePlatformType, 1) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to find free EgressIP for node %s: %v", nodeName, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect SubnetIPs to determine whether it bounds enumeration for large (IPv6) prefixes.
rg -n -A30 'func SubnetIPs' --type=goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate egressip_helpers and relevant funcs =="
git ls-files | rg 'test/extended/networking/egressip_helpers\.go$|SubnetIPs|FreeIP|getFirstFreeIPs' || true
echo
echo "== File stats and outline around concern =="
if [ -f test/extended/networking/egressip_helpers.go ]; then
wc -l test/extended/networking/egressip_helpers.go
sed -n '1830,1905p' test/extended/networking/egressip_helpers.go | cat -n
fi
echo
echo "== Search symbol in all tracked files =="
rg -n 'SubnetIPs|getFirstFreeIPs|FirstFree|freeIPs' --type=go | sed -n '1,220p'Repository: openshift/origin
Length of output: 2806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate function definitions with fixed files =="
rg -n 'func (getFirstFreeIPs|findNodeEgressIPsBaremetal|getNodeEgressIPConfiguration|SubnetIPs)' test/extended/networking/egressip_helpers.go
echo
sed -n '1600,1895p' test/extended/networking/egressip_helpers.go | cat -n
echo
echo "== Check vendored/source locations for SubnetIPs and network APIs =="
rg -n '\bSubnetIPs\b' . --glob '*.go' | sed -n '1,200p'
echo
echo "== Git status and dependencies =="
git status --short
git diff --name-only
git diff -- test/extended/networking/egressip_helpers.go | sed -n '1,240p'Repository: openshift/origin
Length of output: 13476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== getFirstFreeIPs and util.go SubnetIPs implementation =="
sed -n '960,1025p' test/extended/networking/egressip_helpers.go | cat -n
sed -n '540,585p' test/extended/networking/util.go | cat -n
echo
echo "== Behavioral/static verifier for IPv6 /64 enumeration path =="
python3 - <<'PY'
# Read-only structural verifier: report whether util.go SubnetIPs contains a prefix-length guard before loop start.
with open('test/extended/networking/util.go', 'r') as f:
lines = f.readlines()
start_idx = None
for i, line in enumerate(lines):
if 'func SubnetIPs(ipnet net.IPNet)' in line:
start_idx = i
break
if start_idx is None:
print("SubnetIPs definition not found")
raise SystemExit(1)
# Collect function body lines until next func/method/type or package/package-level brace scope ending (rough).
func_lines = lines[start_idx:]
body_lines = []
brace_depth = 0
started = False
for line in func_lines:
# include opening curly after signature
if '{' in line:
started = True
if not started:
continue
body_lines.append(line.rstrip())
brace_depth += line.count('{')
brace_depth -= line.count('}')
if brace_depth == 0:
break
print("SubnetIPs line range:", start_idx+1, "to", start_idx+len(body_lines))
print("Body:")
for lineno, line in enumerate(body_lines, start=start_idx+1):
print(f"{lineno}: {line}")
print("\nHas prefix-length guard before any loop/call/return:")
body = '\n'.join(body_lines)
for name in ['for', 'range', 'append', 'return']:
print(f"-- contains {name}: {name in body}")
print("-- contains mask length comparison:", 'masklen' in body or '/8' in body or '/128' in body or 'PrefixLen' in body)
PYRepository: openshift/origin
Length of output: 6055
Restrict findNodeEgressIPsBaremetal to IPv4 or protect IPv6 allocation.
getFirstFreeIPs enumerates every address in SubnetIPs, and that function has no prefix-length guard. Passing an IPv6 node egress CIDR such as /64 can make the allocation fail after OOM/time exhaustion. Add an explicit guard before the enum or restrict this path to IPv4.
🤖 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/extended/networking/egressip_helpers.go` around lines 1873 - 1880,
Update findNodeEgressIPsBaremetal around the ipnetStr selection and
getFirstFreeIPs call to prevent IPv6 CIDRs from reaching unbounded address
enumeration. Restrict this bare-metal allocation path to IPv4, or validate the
selected CIDR’s prefix length before calling getFirstFreeIPs and return a clear
error for unsupported IPv6 ranges.
| g.By("6. Getting br-ex physical interface name") | ||
| physInterface, err := findBridgePhysicalInterface(oc, egressNode1Name, "br-ex") | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| framework.Logf("Using physical interface: %s", physInterface) | ||
|
|
||
| g.By("7. Getting MAC addresses of egress node 1 and egress node 2") | ||
| mac1, err := getNodeInterfaceMAC(oc, egressNode1Name, physInterface) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| mac2, err := getNodeInterfaceMAC(oc, egressNode2Name, physInterface) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| framework.Logf("Egress node 1 (%s) MAC: %s", egressNode1Name, mac1) | ||
| framework.Logf("Egress node 2 (%s) MAC: %s", egressNode2Name, mac2) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Resolve the physical interface name per node.
Line 695 discovers the br-ex physical interface on egressNode1Name only. The test then reuses that name for egressNode2Name (line 702) and for the probe node (lines 717 and 802). On bare metal, NIC names can differ between nodes, for example eno1 on one node and enp2s0f0 on another. The MAC lookup or the probe then fails for a reason that is unrelated to the behavior under test.
Call findBridgePhysicalInterface for each node and use the matching name in each command.
🐛 Proposed change
- g.By("6. Getting br-ex physical interface name")
- physInterface, err := findBridgePhysicalInterface(oc, egressNode1Name, "br-ex")
- o.Expect(err).NotTo(o.HaveOccurred())
- framework.Logf("Using physical interface: %s", physInterface)
-
- g.By("7. Getting MAC addresses of egress node 1 and egress node 2")
- mac1, err := getNodeInterfaceMAC(oc, egressNode1Name, physInterface)
- o.Expect(err).NotTo(o.HaveOccurred())
- mac2, err := getNodeInterfaceMAC(oc, egressNode2Name, physInterface)
- o.Expect(err).NotTo(o.HaveOccurred())
+ g.By("6. Getting br-ex physical interface name on each node")
+ iface1, err := findBridgePhysicalInterface(oc, egressNode1Name, "br-ex")
+ o.Expect(err).NotTo(o.HaveOccurred())
+ iface2, err := findBridgePhysicalInterface(oc, egressNode2Name, "br-ex")
+ o.Expect(err).NotTo(o.HaveOccurred())
+ probeInterface, err := findBridgePhysicalInterface(oc, probeNodeName, "br-ex")
+ o.Expect(err).NotTo(o.HaveOccurred())
+
+ g.By("7. Getting MAC addresses of egress node 1 and egress node 2")
+ mac1, err := getNodeInterfaceMAC(oc, egressNode1Name, iface1)
+ o.Expect(err).NotTo(o.HaveOccurred())
+ mac2, err := getNodeInterfaceMAC(oc, egressNode2Name, iface2)
+ o.Expect(err).NotTo(o.HaveOccurred())Then pass probeInterface to the discovery command at line 717 and to checkForDuplicateMACOnNode at line 802.
🤖 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/extended/networking/egressip.go` around lines 694 - 705, Update the
networking test flow around findBridgePhysicalInterface to resolve and retain
separate physical interface names for egressNode1Name, egressNode2Name, and the
probe node. Use each node’s matching interface for getNodeInterfaceMAC, the
discovery command, and checkForDuplicateMACOnNode instead of reusing
egressNode1Name’s interface.
| g.By("12. Verifying nftables chain egressip-drop exists on egress node 1 during shutdown") | ||
| select { | ||
| case found := <-nftChainFound: | ||
| o.Expect(found).To(o.BeTrue(), "nftables chain egressip-drop should be found on egress node 1") | ||
| framework.Logf("Nftables chain egressip-drop verified on node %s", egressNode1Name) | ||
| case <-time.After(60 * time.Second): | ||
| close(stopChecking) | ||
| framework.Failf("Timed out waiting for nftables chain egressip-drop on node %s", egressNode1Name) | ||
| } | ||
| close(stopChecking) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid the double close of stopChecking.
The timeout branch closes stopChecking at line 777 and line 780 closes it again. Today framework.Failf panics through Ginkgo, so line 780 is not reached. The code depends on that panic behavior. If the failure path ever returns, the second close panics with "close of closed channel".
Close the channel once with a defer placed right after the goroutine starts.
🐛 Proposed change
<-goroutineReady
+ defer close(stopChecking)
framework.Logf("Nftables chain monitoring goroutine started")
@@
case <-time.After(60 * time.Second):
- close(stopChecking)
framework.Failf("Timed out waiting for nftables chain egressip-drop on node %s", egressNode1Name)
}
- close(stopChecking)🤖 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/extended/networking/egressip.go` around lines 771 - 780, Update the
shutdown verification around the nftChainFound select to close stopChecking
exactly once: remove the timeout-branch close and register a defer immediately
after starting the checker goroutine to close the channel. Preserve the existing
timeout failure behavior and successful verification flow.
| nftCheckCmd := "nft list table netdev ovn-kubernetes-egressip 2>&1" | ||
| nftOutput, nftErr := adminExecInPod(oc, "openshift-ovn-kubernetes", newPodInfo.podName, newPodInfo.containerName, nftCheckCmd) | ||
| if nftErr == nil { | ||
| o.Expect(nftOutput).To(o.Or( | ||
| o.ContainSubstring("No such file or directory"), | ||
| o.ContainSubstring("No such file"), | ||
| ), "nftables egress IP table should be deleted after cleanup") | ||
| } | ||
| framework.Logf("Nftables table cleaned up on node %s", egressNode1Name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The nftables cleanup check passes when the command fails.
nft list table netdev ovn-kubernetes-egressip exits non-zero when the table is absent. adminExecInPod then returns an error, so nftErr != nil and the assertion at lines 841-844 is skipped. Line 846 still logs success. The cleanup verification therefore passes without checking anything, both when the table is gone and when the exec fails for an unrelated reason.
Assert on the output in both cases, and treat an unexpected error as a failure. The two ContainSubstring matchers also overlap; keep only "No such file".
🐛 Proposed change
- nftCheckCmd := "nft list table netdev ovn-kubernetes-egressip 2>&1"
- nftOutput, nftErr := adminExecInPod(oc, "openshift-ovn-kubernetes", newPodInfo.podName, newPodInfo.containerName, nftCheckCmd)
- if nftErr == nil {
- o.Expect(nftOutput).To(o.Or(
- o.ContainSubstring("No such file or directory"),
- o.ContainSubstring("No such file"),
- ), "nftables egress IP table should be deleted after cleanup")
- }
+ // `nft list table` exits non-zero when the table is absent, so ignore the exit
+ // status and assert on the combined output instead.
+ nftCheckCmd := "nft list table netdev ovn-kubernetes-egressip 2>&1 || true"
+ nftOutput, nftErr := adminExecInPod(oc, "openshift-ovn-kubernetes", newPodInfo.podName, newPodInfo.containerName, nftCheckCmd)
+ o.Expect(nftErr).NotTo(o.HaveOccurred())
+ o.Expect(nftOutput).To(o.ContainSubstring("No such file"),
+ "nftables egress IP table should be deleted after cleanup")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nftCheckCmd := "nft list table netdev ovn-kubernetes-egressip 2>&1" | |
| nftOutput, nftErr := adminExecInPod(oc, "openshift-ovn-kubernetes", newPodInfo.podName, newPodInfo.containerName, nftCheckCmd) | |
| if nftErr == nil { | |
| o.Expect(nftOutput).To(o.Or( | |
| o.ContainSubstring("No such file or directory"), | |
| o.ContainSubstring("No such file"), | |
| ), "nftables egress IP table should be deleted after cleanup") | |
| } | |
| framework.Logf("Nftables table cleaned up on node %s", egressNode1Name) | |
| // `nft list table` exits non-zero when the table is absent, so ignore the exit | |
| // status and assert on the combined output instead. | |
| nftCheckCmd := "nft list table netdev ovn-kubernetes-egressip 2>&1 || true" | |
| nftOutput, nftErr := adminExecInPod(oc, "openshift-ovn-kubernetes", newPodInfo.podName, newPodInfo.containerName, nftCheckCmd) | |
| o.Expect(nftErr).NotTo(o.HaveOccurred()) | |
| o.Expect(nftOutput).To(o.ContainSubstring("No such file"), | |
| "nftables egress IP table should be deleted after cleanup") | |
| framework.Logf("Nftables table cleaned up on node %s", egressNode1Name) |
🤖 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/extended/networking/egressip.go` around lines 838 - 846, Update the
nftables cleanup verification around adminExecInPod so it always validates the
command output, including when the command returns a non-zero error because the
table is absent. Retain only the “No such file” matcher, and fail explicitly for
unexpected adminExecInPod errors instead of logging success unconditionally.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/extended/networking/egressip.go`:
- Around line 664-667: In the EgressIP validation flow after
findNodeEgressIPsBaremetal returns, assert that nodeEgressIPMap[egressNode1Name]
contains at least one address before indexing element 0. Keep the existing error
assertion and logging behavior, but report a clear test failure when no EgressIP
was allocated instead of allowing an index-out-of-range panic.
- Around line 593-607: Update the platform check in the egress IP test to read
the type from infra.Status.PlatformStatus.Type instead of
infra.Spec.PlatformSpec.Type. Guard against a nil PlatformStatus and preserve
the existing cloud-platform skip behavior when the status type matches an
unsupported platform.
- Around line 733-764: Update the nftables monitoring goroutine around
nftChainCheckCmd to use a separate CLI instance from the main test path,
preventing concurrent mutation of shared oc command state during Output
execution. Also increase the ticker interval beyond 200ms to allow each oc debug
invocation to complete without queuing repeated commands and adding unnecessary
cluster load.
- Around line 711-726: Replace the isIPv6-based branching in the discovery and
duplicate-MAC validation flow with a family check derived from
net.ParseIP(egressIPStr).To4() == nil. Use that result to select the IPv6 ndisc6
command and regex for IPv6 allocated addresses, while preserving the arping path
for IPv4 addresses, including dual-stack clusters.
🪄 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: Pro Plus
Run ID: 56378f26-947d-44f6-bb53-ee831699ca04
📒 Files selected for processing (2)
test/extended/networking/egressip.gotest/extended/networking/egressip_helpers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended/networking/egressip_helpers.go
| g.By("Checking platform type - this test requires L2 network adjacency") | ||
| infra, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| cloudType := infra.Spec.PlatformSpec.Type | ||
| cloudPlatforms := []configv1.PlatformType{ | ||
| configv1.AWSPlatformType, | ||
| configv1.GCPPlatformType, | ||
| configv1.AzurePlatformType, | ||
| configv1.OpenStackPlatformType, | ||
| } | ||
| for _, cp := range cloudPlatforms { | ||
| if cloudType == cp { | ||
| skipper.Skipf("This test requires L2 network adjacency (baremetal); cloud platform %s is not supported", cloudType) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare Spec.PlatformSpec.Type vs Status.PlatformStatus.Type usage in this repo's tests.
rg -nP --type=go -C2 '(Spec\.PlatformSpec\.Type|Status\.PlatformStatus\.Type)' test/ | head -80Repository: openshift/origin
Length of output: 5204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- target file context ---\n'
sed -n '550,630p' test/extended/networking/egressip.go
printf '\n--- Infrastructure definitions and usages in egressip.go ---\n'
rg -n --type=go -C3 'Infrastructure|Infrastructures|Spec\.PlatformSpec|Status\.PlatformStatus' test/extended/networking/egressip.go
printf '\n--- focused repo usage: Infrastructures and PlatformStatus vs Spec.PlatformSpec ---\n'
rg -n --type=go -C2 'AdminConfigClient\(\)\.ConfigV1\(\)\.Infrastructures\(\)\.Get|Status\.PlatformStatus\.Type|Spec\.PlatformSpec\.Type' test | head -200Repository: openshift/origin
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file context ---'
sed -n '550,630p' test/extended/networking/egressip.go
printf '%s\n' ''
printf '%s\n' '--- Infrastructure definitions and usages in egressip.go ---'
rg -n --type=go -C3 'Infrastructure|Infrastructures|Spec\.PlatformSpec|Status\.PlatformStatus' test/extended/networking/egressip.go
printf '%s\n' ''
printf '%s\n' '--- focused repo usage: Infrastructures and PlatformStatus vs Spec.PlatformSpec ---'
rg -n --type=go -C2 'AdminConfigClient\(\)\.ConfigV1\(\)\.Infrastructures\(\)\.Get|Status\.PlatformStatus\.Type|Spec\.PlatformSpec\.Type' test | head -200Repository: openshift/origin
Length of output: 21107
Read the platform type from Status.PlatformStatus, not Spec.PlatformSpec.
Spec.PlatformSpec.Type can remain empty on installed clusters, so cloud platforms may not match the skip list and the test can run where L2 adjacency is unavailable. Use infra.Status.PlatformStatus.Type, with a nil PlatformStatus guard.
🤖 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/extended/networking/egressip.go` around lines 593 - 607, Update the
platform check in the egress IP test to read the type from
infra.Status.PlatformStatus.Type instead of infra.Spec.PlatformSpec.Type. Guard
against a nil PlatformStatus and preserve the existing cloud-platform skip
behavior when the status type matches an unsupported platform.
| nodeEgressIPMap, err := findNodeEgressIPsBaremetal(oc, clientset, []string{egressNode1Name}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| egressIPStr := nodeEgressIPMap[egressNode1Name][0] | ||
| framework.Logf("Allocated EgressIP: %s for node %s", egressIPStr, egressNode1Name) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assert that an EgressIP was allocated before indexing.
Line 666 indexes nodeEgressIPMap[egressNode1Name][0] without a length check. If findNodeEgressIPsBaremetal returns no address for the node, the test panics with an index-out-of-range error instead of reporting a clear failure.
🛡️ Proposed change
nodeEgressIPMap, err := findNodeEgressIPsBaremetal(oc, clientset, []string{egressNode1Name})
o.Expect(err).NotTo(o.HaveOccurred())
+ o.Expect(nodeEgressIPMap).To(o.HaveKey(egressNode1Name))
+ o.Expect(nodeEgressIPMap[egressNode1Name]).NotTo(o.BeEmpty(),
+ fmt.Sprintf("no free EgressIP found for node %s", egressNode1Name))
egressIPStr := nodeEgressIPMap[egressNode1Name][0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nodeEgressIPMap, err := findNodeEgressIPsBaremetal(oc, clientset, []string{egressNode1Name}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| egressIPStr := nodeEgressIPMap[egressNode1Name][0] | |
| framework.Logf("Allocated EgressIP: %s for node %s", egressIPStr, egressNode1Name) | |
| nodeEgressIPMap, err := findNodeEgressIPsBaremetal(oc, clientset, []string{egressNode1Name}) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| o.Expect(nodeEgressIPMap).To(o.HaveKey(egressNode1Name)) | |
| o.Expect(nodeEgressIPMap[egressNode1Name]).NotTo(o.BeEmpty(), | |
| fmt.Sprintf("no free EgressIP found for node %s", egressNode1Name)) | |
| egressIPStr := nodeEgressIPMap[egressNode1Name][0] | |
| framework.Logf("Allocated EgressIP: %s for node %s", egressIPStr, egressNode1Name) |
🤖 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/extended/networking/egressip.go` around lines 664 - 667, In the EgressIP
validation flow after findNodeEgressIPsBaremetal returns, assert that
nodeEgressIPMap[egressNode1Name] contains at least one address before indexing
element 0. Keep the existing error assertion and logging behavior, but report a
clear test failure when no EgressIP was allocated instead of allowing an
index-out-of-range panic.
| var discoveryCmd string | ||
| var macRegex *regexp.Regexp | ||
| if isIPv6 { | ||
| discoveryCmd = fmt.Sprintf("ndisc6 -1 -w 1000 %s %s 2>&1", egressIPStr, physInterface) | ||
| macRegex = regexp.MustCompile(`Target link-layer address:\s+([0-9a-fA-F]{1,2}:[0-9a-fA-F]{1,2}:[0-9a-fA-F]{1,2}:[0-9a-fA-F]{1,2}:[0-9a-fA-F]{1,2}:[0-9a-fA-F]{1,2})`) | ||
| } else { | ||
| discoveryCmd = fmt.Sprintf("arping -c 1 -I %s %s 2>&1", physInterface, egressIPStr) | ||
| macRegex = regexp.MustCompile(`\[([0-9a-fA-F:]+)\]`) | ||
| } | ||
| output, err := adminExecInPod(oc, "openshift-ovn-kubernetes", probePodInfo.podName, probePodInfo.containerName, discoveryCmd) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "network discovery should succeed before migration") | ||
| matches := macRegex.FindStringSubmatch(output) | ||
| o.Expect(matches).To(o.HaveLen(2), fmt.Sprintf("should extract MAC from discovery output: %s", output)) | ||
| macBeforeMigration := strings.ToLower(strings.TrimSpace(matches[1])) | ||
| framework.Logf("MAC before migration: %s, expected node 1 MAC: %s", macBeforeMigration, mac1) | ||
| o.Expect(macBeforeMigration).To(o.Equal(mac1), "EgressIP should resolve to egress node 1 MAC before migration") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine which IP family findNodeEgressIPsBaremetal returns on dual-stack clusters.
ast-grep run --pattern 'func findNodeEgressIPsBaremetal($$$) { $$$ }' --lang go test/extended/networking/egressip_helpers.goRepository: openshift/origin
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)egressip(_helpers)?\.go$|test/extended/(include\.go)$' || true
echo "== egressip.go outline =="
ast-grep outline test/extended/networking/egressip.go --match 'egress' --view expanded || true
echo "== helpers outline =="
if [ -f test/extended/networking/egressip_helpers.go ]; then
ast-grep outline test/extended/networking/egressip_helpers.go --view expanded || true
fi
echo "== relevant searches =="
rg -n "findNodeEgressIPsBaremetal|isIPv6|egressIPStr|hasIPv|baremetal|dual-stack|dual stack|dualstack" test/extended/networking/egressip.go test/extended/networking/egressip_helpers.go 2>/dev/null || trueRepository: openshift/origin
Length of output: 11727
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test context 620-810 =="
sed -n '620,810p' test/extended/networking/egressip.go
echo "== findNodeEgressIPs 882-940 =="
sed -n '882,940p' test/extended/networking/egressip_helpers.go
echo "== baremetal allocator 1834-1940 =="
sed -n '1834,1945p' test/extended/networking/egressip_helpers.go
echo "== checkForDuplicateMACOnNode 1767-1815 =="
sed -n '1767,1815p' test/extended/networking/egressip_helpers.go
echo "== behavioral probe: current dual-stack selector and address-family selection =="
python3 - <<'PY'
IP_STR = "fd12:3456:7890:abcd::1"
for is_ipv6 in [False, True]:
import subprocess
cmd = "arping -c 1 -I eth0 fd12:3456:7890:abcd::1 2>&1" if not is_ipv6 else "ndisc6 -1 -w 1000 fd12:3456:7890:abcd::1 eth0 2>&1"
res = subprocess.run(cmd.split(), stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True)
print(is_ipv6, res.returncode, res.stdout.splitlines()[0] if res.stdout else "")
PYRepository: openshift/origin
Length of output: 16004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== getFirstFreeIPs 982-1052 =="
sed -n '982,1052p' test/extended/networking/egressip_helpers.go
echo "== getNodeEgressIPConfiguration 1053-1070 =="
sed -n '1053,1070p' test/extended/networking/egressip_helpers.go
echo "== GetIPAddressFamily occurrences =="
rg -n "func GetIPAddressFamily|GetIPAddressFamily\\(" -S .
echo "== deterministic family selection behavior from source comments =="
python3 - <<'PY'
from pathlib import Path
p = Path('test/extended/networking/egressip.go')
text = p.read_text()
for key in ['hasIPv4, hasIPv6, err = GetIPAddressFamily(oc)', 'getFirstFreeIPs']:
print(key, text.find(key))
PYRepository: openshift/origin
Length of output: 4921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== util GetIPAddressFamily 550-630 =="
sed -n '550,630p' test/extended/networking/util.go
echo "== check duplicate family source around line 1486 util.go =="
sed -n '1470,1505p' test/extended/networking/util.go
echo "== getTargetProtocolHostPort 1630-1670 =="
sed -n '1630,1670p' test/extended/networking/egressip_helpers.goRepository: openshift/origin
Length of output: 3969
🌐 Web query:
OVN-Kubernetes node egress-ipconfig annotation IPv6IPv4 dual-stack EgressIP allocation findNodeEgressIPsBaremetal
💡 Result:
In OVN-Kubernetes, the cloud.network.openshift.io/egress-ipconfig annotation is used to provide metadata about a node's networking capabilities—specifically interface information, subnets (ifaddr), and IP address capacity—in public cloud environments (AWS, Azure, Google Cloud) [1][2][3][4]. This annotation allows the EgressIP controller to understand how many egress IPs can be assigned to a node and on which interface [1][3][4]. Regarding IPv4/IPv6 dual-stack and EgressIP allocation: 1. Dual-Stack Configuration: For clusters configured for dual-stack networking, a single EgressIP object can support both IPv4 and IPv6 addresses [1][2][3][5]. Red Hat does not support using two separate EgressIP objects to achieve dual-stack functionality (e.g., one for IPv4 and one for IPv6) [1][2][3][6]. 2. Egress IP Allocation: Egress IPs are assigned to nodes that have been labeled with k8s.ovn.org/egress-assignable="" [7][8][9][5]. The OVN-Kubernetes controller tracks these assignments [10][11]. While the term findNodeEgressIPsBaremetal is not a standard documented API or command, the assignment process involves the EgressIP controller (running in ovnkube-cluster-manager) periodically checking for reachable nodes with the required label to host the assigned egress IPs [8]. If a node is no longer reachable, the controller revalidates and moves the egress IPs to another usable node [8]. 3. Platform Support: While the cloud.network.openshift.io/egress-ipconfig annotation is specific to public cloud environments, the EgressIP feature is also supported on bare-metal clusters [9]. On bare-metal, the requirement to label nodes with k8s.ovn.org/egress-assignable="" remains the primary mechanism for directing where egress traffic is handled [7][9].
Citations:
- 1: https://docs.okd.io/latest/networking/ovn_kubernetes_network_provider/configuring-egress-ips-ovn.html
- 2: https://docs.redhat.com/en/documentation/openshift_container_platform/4.20/html/ovn-kubernetes_network_plugin/configuring-egress-ips-ovn
- 3: https://docs.redhat.com/en/documentation/openshift_container_platform/4.21/html/ovn-kubernetes_network_plugin/configuring-egress-ips-ovn
- 4: https://docs.okd.io/4.19/networking/ovn_kubernetes_network_provider/configuring-egress-ips-ovn.html
- 5: https://docs.okd.io/4.16/networking/ovn_kubernetes_network_provider/configuring-egress-ips-ovn.html
- 6: openshift/openshift-docs@41c1af7
- 7: https://docs.okd.io/4.18/networking/ovn_kubernetes_network_provider/configuring-egress-ips-ovn.html
- 8: https://ovn-kubernetes.io/features/cluster-egress-controls/egress-ip/
- 9: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/ovn-kubernetes_network_plugin/configuring-egress-ips-ovn
- 10: https://github.com/ovn-kubernetes/ovn-kubernetes/blob/82f167a3920c8c3cd0687ceb3e7a5ba64372be69/go-controller/pkg/ovn/egressip.go
- 11: https://github.com/ovn-kubernetes/ovn-kubernetes/blob/04c66114b57f/go-controller/pkg/ovn/egressip.go
Select the discovery command from the allocated EgressIP family.
isIPv6 is true only for IPv6-only clusters, while findNodeEgressIPsBaremetal still tries IPv6 first when no IPv4 range is configured. On a dual-stack cluster, an allocated IPv6 egressIPStr then follows the arping path and can fail. Use net.ParseIP(egressIPStr).To4() == nil for the discovery and duplicate-MAC checks instead.
🤖 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/extended/networking/egressip.go` around lines 711 - 726, Replace the
isIPv6-based branching in the discovery and duplicate-MAC validation flow with a
family check derived from net.ParseIP(egressIPStr).To4() == nil. Use that result
to select the IPv6 ndisc6 command and regex for IPv6 allocated addresses, while
preserving the arping path for IPv4 addresses, including dual-stack clusters.
| g.By("10. Starting goroutine to monitor nftables chain creation during pod deletion") | ||
| nftChainFound := make(chan bool, 1) | ||
| stopChecking := make(chan bool, 1) | ||
| goroutineReady := make(chan bool, 1) | ||
| nftChainCheckCmd := "nft list chains 2>/dev/null | grep -q egressip-drop && echo FOUND || echo NOTFOUND" | ||
| go func() { | ||
| defer close(nftChainFound) | ||
| goroutineReady <- true | ||
| ticker := time.NewTicker(200 * time.Millisecond) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-stopChecking: | ||
| return | ||
| case <-ticker.C: | ||
| // Use oc debug to run on the node directly since the ovnkube-node pod may be terminating | ||
| result, debugErr := oc.AsAdmin().Run("debug").Args( | ||
| "node/"+egressNode1Name, | ||
| "--", | ||
| "chroot", "/host", | ||
| "/bin/bash", "-c", | ||
| nftChainCheckCmd, | ||
| ).Output() | ||
| if debugErr == nil && strings.Contains(result, "FOUND") { | ||
| nftChainFound <- true | ||
| return | ||
| } | ||
| } | ||
| } | ||
| }() | ||
| <-goroutineReady | ||
| framework.Logf("Nftables chain monitoring goroutine started") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect exutil.CLI for mutable state that makes concurrent use unsafe.
fd -t f 'cli.go' test/extended/util | xargs -r rg -nP -C3 '(func \(c \*CLI\) (AsAdmin|Run|Args|WithoutNamespace)|type CLI struct)'Repository: openshift/origin
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate egressip.go and util files =="
fd -t f 'egressip.go|cli.go' test | sed -n '1,120p'
echo
echo "== locate CLI type/AsAdmin/Run/Args definitions =="
rg -n "type CLI struct|func \\(c \\*CLI\\) (AsAdmin|Run|Args|WithoutNamespace|SetNamespace)" test -SRepository: openshift/origin
Length of output: 720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read egressip.go around goroutine and later oc/clientset usage =="
sed -n '700,810p' test/extended/networking/egressip.go
echo
echo "== inspect relevant CLI implementation files =="
for f in $(rg -l "type CLI struct|func \\(c \\*CLI\\) Run" test -S); do
echo "--- $f ---"
sed -n '1,220p' "$f"
doneRepository: openshift/origin
Length of output: 13052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runOcc/Output/AsAdmin/SetNamespace implementation =="
sed -n '240,320p' test/extended/util/client.go
sed -n '860,1025p' test/extended/util/client.go
echo
echo "== occurrences of runOcWithRetry/Output usages in surrounding egressip.go =="
rg -n "runOcWithRetry|oc\\.Output\\(|oc\\.SetNamespace|\\.Output\\(" test/extended/networking/egressip.go test/extended/util/client.goRepository: openshift/origin
Length of output: 9021
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== client.go outputs/runOcc implementation =="
sed -n '970,1055p' test/extended/util/client.go
echo
echo "== helper definitions in egressip.go =="
sed -n '150,205p' test/extended/networking/egressip.go
echo
echo "== read-only structural check: goroutine call chain and later runOcWithRetry/Output calls =="
python3 - <<'PY'
from pathlib import Path
p = Path('test/extended/networking/egressip.go')
s = p.read_text()
lines = s.splitlines()
calls = []
for i,l in enumerate(lines,1):
if 'go func()' in l or 'oc.AsAdmin().Run("debug")' in l or 'oc.AsAdmin()' in l:
calls.append((i,l.strip()))
print('\n'.join(f'{i}:{line}' for i,line in calls))
PYRepository: openshift/origin
Length of output: 6750
Do not share one CLI instance between the monitor goroutine and the main test path.
The goroutine and later test helpers both derive commands from oc, while CLI#Args and CLI#start mutate command state. Concurrent runOcWithRetry(...) calls and .Output() from the monitor can mix argument/execution state. Build a separate CLI for the monitor, or keep the monitor out of concurrent CLI invocations. Also, increase the polling interval from 200ms; each oc debug launch is too heavy to complete in that window, so the loop queues work and adds cluster load.
🤖 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/extended/networking/egressip.go` around lines 733 - 764, Update the
nftables monitoring goroutine around nftChainCheckCmd to use a separate CLI
instance from the main test path, preventing concurrent mutation of shared oc
command state during Output execution. Also increase the ticker interval beyond
200ms to allow each oc debug invocation to complete without queuing repeated
commands and adding unnecessary cluster load.
|
/testwith openshift/ovn-kubernetes/main/e2e-metal-ipi-serial-ovn-ipv4 openshift/ovn-kubernetes#3357 |
|
@shreyasbe, |
|
/testwith openshift/ovn-kubernetes/main/e2e-metal-ipi-serial-ovn-ipv4 openshift/ovn-kubernetes#3357 |
|
@shreyasbe, |
|
Scheduling required tests: |
1 participant
Summary by CodeRabbit