Migrating Ipfailover test cases to images repo#243
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: melvinjoseph86 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 |
|
/payload-job-with-prs pull-ci-openshift-images-master-e2e-metal-ipi-tests-ext openshift/release/pull/81103 |
|
@melvinjoseph86: An error was encountered. No known errors were detected, please see the full error message for details. Full error message.
unable to get additional pr info from string: openshift/release/pull/81103: string: openshift/release/pull/81103 doesn't match expected format: org/repo#number
Please contact an administrator to resolve this issue. |
|
/payload-job-with-prs pull-ci-openshift-images-master-e2e-metal-ipi-tests-ext openshift/release#81103 |
|
@melvinjoseph86: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/payload-job-with-prs periodic-ci-openshift-images-master-e2e-metal-ipi-tests-ext openshift/release#81103 |
|
@melvinjoseph86: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
Investigation: Deserialization Error Root CauseI investigated the Root Cause:
|
melvinjoseph86
force-pushed
the
ipfailover_migration_e2e
branch
from
36578d9 to
791cb2c
Compare
WalkthroughAdds a new openshift-tests-extension (OTE) for ipfailover keepalived tests, migrated from openshift-tests-private. Includes a Go module, Makefile, multi-stage Dockerfile build stage, a test entrypoint with three suite registrations, six e2e test cases with utility helpers, an OpenShift deployment template, and a README. Changesipfailover OTE test extension
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 5 warnings)
✅ Passed checks (6 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}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
ipfailover/keepalived/Dockerfile (1)
5-5: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCopy only the inputs this builder stage actually needs.
COPY . .pulls the whole repository into the build context even though this stage only buildsipfailover/keepalived/tests. That widens the accidental secret/file exposure surface and invalidates the cache on unrelated changes.♻️ Proposed fix
WORKDIR /go/src/github.com/openshift/ipfailover -COPY . . +COPY ipfailover/keepalived/tests ./ipfailover/keepalived/tests RUN cd ipfailover/keepalived/tests && \ make build && \As per path instructions,
COPY specific files, not entire context.🤖 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 `@ipfailover/keepalived/Dockerfile` at line 5, The builder stage in the keepalived Dockerfile is over-copying the entire repository with COPY . ., even though it only needs the inputs required to build ipfailover/keepalived/tests. Replace the broad copy with explicit copies of the minimal needed files and directories so the stage stays narrowly scoped, avoids exposing unrelated files, and preserves cache reuse; use the Dockerfile stage and the tests build target as the main references when locating the change.Source: Path instructions
🤖 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 `@ipfailover/keepalived/tests/cmd/main.go`:
- Around line 25-26: Move the framework.AfterReadingAllFlags call so it runs
after root.Execute() has parsed flags, because invoking it before parsing leaves
framework.TestContext at default values and ignores kube/e2e overrides. Update
the startup flow in main so util.InitStandardFlags still happens first, then
flags are parsed via root.Execute, and only then
framework.AfterReadingAllFlags(&framework.TestContext) is called.
In `@ipfailover/keepalived/tests/e2e/ipfailover.go`:
- Around line 168-171: After deleting the current primary, do not immediately
recompute MASTER/BACKUP from the pod list; in the ipfailover e2e flow around
getPodListByLabel, ensureIpfailoverMasterBackup, waitForPrimaryPod, and
futurePrimaryPod[0], add a replica-aware wait until the replacement pod set is
fully available and stable before reading pod names again. Apply the same fix in
both affected flows so the recalculation happens only after the rollout
completes and the pod list reflects the final state.
- Around line 75-84: The IPv6 single-stack check in ipfailover e2e is too narrow
because it only inspects the first clusterNetwork CIDR and can misclassify
IPv6-first dual-stack clusters. Update the logic in the test around the current
oc-based network lookup to reuse checkIPStackType(oc) or otherwise evaluate the
full status.clusterNetwork CIDR list before calling g.Skip, so only true IPv6
single-stack clusters are skipped.
- Around line 69-73: The remote-worker skip check in the ipfailover e2e test is
using the wrong node selector, so it can miss remote-worker clusters. Update the
logic in the test block around the remote-worker gate to query
`node.openshift.io/remote-worker` directly, matching the approach used by
`exactNodeDetails()`, and then keep the skip decision based on that targeted
result instead of searching generic `nodes -l kubernetes.io/hostname` output.
In `@ipfailover/keepalived/tests/e2e/testdata/router/ipfailover.yaml`:
- Around line 28-75: The openshift-ipfailover container definition in the
ipfailover manifest is missing an explicit resource envelope. Add cpu and memory
requests and limits to the openshift-ipfailover container alongside its existing
ports, securityContext, and env settings so it conforms to the resource policy
for every container.
- Around line 14-27: The ipfailover test template hard-codes a 2-replica
deployment with a worker-only node selector, which assumes a standard
multi-worker topology. Update the ipfailover.yaml template to derive replicas
from the number of schedulable nodes and make the e2e test setup topology-aware
before rendering it, so it can handle SingleReplica, DualReplica, and
arbiter-style clusters. Use the existing ipfailover template fields and related
test setup logic to cap replica count to available schedulable nodes and avoid
assuming node-role.kubernetes.io/worker is always present.
- Around line 23-44: The ipfailover test pod spec is effectively fully
host-privileged and needs to be tightened in the openshift-ipfailover workload
definition. Update the container and pod security settings to follow the
security guidance by adding non-root and no-escalation constraints, making the
root filesystem read-only, dropping all capabilities unless explicitly required,
and disabling service account token automount unless the test truly needs it.
Keep host access only if required for the test, but scope it to the minimum
necessary privileges and document any exception in the ipfailover template
rather than leaving the default manifest overly permissive.
- Around line 69-75: The keepalived test pod spec currently defines only
livenessProbe, which lets the pod report Ready too early and makes the e2e flow
race-prone. Add a readinessProbe alongside the existing livenessProbe in the
ipfailover manifest so readiness only flips after keepalived is actually
prepared to serve, keeping ensurePodWithLabelReady in sync with the downstream
assertions in ipfailover.go.
- Around line 22-25: Remove the invalid top-level privileged field from the pod
spec in the ipfailover test manifest. The fix is to delete
spec.template.spec.privileged from the ipfailover.yaml fixture and keep the
existing containers[].securityContext.privileged setting as the only privileged
declaration. Use the ipfailover pod template structure under spec.template.spec
to locate the change.
In `@ipfailover/keepalived/tests/e2e/util.go`:
- Around line 319-331: The helper waitForPodWithLabelReady currently only checks
Ready status, so it can succeed with a single surviving pod during rollout.
Update waitForPodWithLabelReady to also verify the number of pods returned by
the label selector in addition to readiness, and keep polling until both the
expected replica count and Ready condition are met. Use the existing wait.Poll
logic and the kubectl get call in waitForPodWithLabelReady to enforce the count
before returning success.
- Around line 29-31: The worker-counting logic in the e2e helper is parsing `oc
get nodes` table text with `strings.Count`, which double-counts `Ready` inside
`NotReady` and can underreport healthy nodes. Update the node readiness check in
the helper that builds `linuxWorkerDetails` and `nodeCount` to use structured
node data from the API or JSON output, inspecting node conditions and
unschedulable state instead of matching substrings in human-readable output.
- Around line 240-249: The unicast peer list in the Nutanix/none branch is built
from the first node address, but .status.addresses[0] is not guaranteed to be an
InternalIP. Update the logic in the keepalived e2e util path that calls
oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", ...) to filter node
addresses by type == "InternalIP" before assembling OPENSHIFT_HA_UNICAST_PEERS,
and keep the existing validation that enough worker IPs were found.
- Around line 160-176: The getVipOwnerPod helper is treating the literal
“command terminated with exit code 1” as a VIP miss, which can mask real oc exec
failures and return the wrong master or an empty result. Update the logic around
oc.AsAdmin().WithoutNamespace().Run("exec").Args(...).Output() to inspect err
first, only consider exit code 1 from the command as “VIP not present,” and
explicitly retry or fail for any other error conditions; keep the selection
logic in getVipOwnerPod and its primaryNode assignment tied to successful
command results.
In `@ipfailover/keepalived/tests/go.mod`:
- Line 3: The module version in ipfailover/keepalived/tests/go.mod is newer than
the Go toolchain used by the ipfailover/keepalived/Dockerfile builder stage, so
the build may auto-download a different compiler. Update the builder image in
Dockerfile to a Go 1.25-based image, or reduce the go directive in go.mod to
match the supported compiler version. Make sure the versions stay aligned across
go.mod and the Dockerfile so the tests build with the image’s local toolchain.
---
Nitpick comments:
In `@ipfailover/keepalived/Dockerfile`:
- Line 5: The builder stage in the keepalived Dockerfile is over-copying the
entire repository with COPY . ., even though it only needs the inputs required
to build ipfailover/keepalived/tests. Replace the broad copy with explicit
copies of the minimal needed files and directories so the stage stays narrowly
scoped, avoids exposing unrelated files, and preserves cache reuse; use the
Dockerfile stage and the tests build target as the main references when locating
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| util.InitStandardFlags() | ||
| framework.AfterReadingAllFlags(&framework.TestContext) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'InitStandardFlags\(|AfterReadingAllFlags\(' ipfailover/keepalived/tests/cmd/main.go
rg -n -C3 --iglob 'main.go' 'InitStandardFlags\(|AfterReadingAllFlags\(' .Repository: openshift/images
Length of output: 972
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== main.go =="
cat -n ipfailover/keepalived/tests/cmd/main.go | sed -n '1,180p'
echo
echo "== other AfterReadingAllFlags usages =="
rg -n -C2 'AfterReadingAllFlags\(' . || true
echo
echo "== root.Execute call site =="
rg -n -C3 'root\.Execute\(\)|Execute\(\)' ipfailover/keepalived/tests/cmd/main.goRepository: openshift/images
Length of output: 6376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== framework docs references =="
rg -n -C2 'AfterReadingAllFlags' . || true
echo
echo "== related entrypoints =="
rg -n -C2 'util.InitStandardFlags\(|logs.InitLogs\(|framework.AfterReadingAllFlags\(' . --glob 'main.go' || trueRepository: openshift/images
Length of output: 2173
Move framework.AfterReadingAllFlags after flag parsing. Calling it before root.Execute() leaves framework.TestContext on defaults, so kube/e2e flag overrides are ignored.
🤖 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 `@ipfailover/keepalived/tests/cmd/main.go` around lines 25 - 26, Move the
framework.AfterReadingAllFlags call so it runs after root.Execute() has parsed
flags, because invoking it before parsing leaves framework.TestContext at
default values and ignores kube/e2e overrides. Update the startup flow in main
so util.InitStandardFlags still happens first, then flags are parsed via
root.Execute, and only then
framework.AfterReadingAllFlags(&framework.TestContext) is called.
| g.By("check the cluster has remote worker profile") | ||
| remoteWorkerDetails, _ := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", "-l", "kubernetes.io/hostname").Output() | ||
| if strings.Contains(remoteWorkerDetails, "remote-worker") { | ||
| g.Skip("Skip as ipfailover currently doesn't support on remote-worker profile") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The remote-worker gate is checking the wrong selector.
-l kubernetes.io/hostname returns every node, then this relies on "remote-worker" appearing in the rendered table output. That misses clusters where the node name does not contain that substring. Query node.openshift.io/remote-worker directly, like exactNodeDetails() already does.
Proposed fix
- remoteWorkerDetails, _ := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", "-l", "kubernetes.io/hostname").Output()
- if strings.Contains(remoteWorkerDetails, "remote-worker") {
+ remoteWorkerDetails, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", "--selector=node.openshift.io/remote-worker").Output()
+ o.Expect(err).NotTo(o.HaveOccurred())
+ if !strings.Contains(remoteWorkerDetails, "No resources found") {
g.Skip("Skip as ipfailover currently doesn't support on remote-worker profile")
}📝 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.
| g.By("check the cluster has remote worker profile") | |
| remoteWorkerDetails, _ := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", "-l", "kubernetes.io/hostname").Output() | |
| if strings.Contains(remoteWorkerDetails, "remote-worker") { | |
| g.Skip("Skip as ipfailover currently doesn't support on remote-worker profile") | |
| } | |
| g.By("check the cluster has remote worker profile") | |
| remoteWorkerDetails, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", "--selector=node.openshift.io/remote-worker").Output() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| if !strings.Contains(remoteWorkerDetails, "No resources found") { | |
| g.Skip("Skip as ipfailover currently doesn't support on remote-worker profile") | |
| } |
🤖 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 `@ipfailover/keepalived/tests/e2e/ipfailover.go` around lines 69 - 73, The
remote-worker skip check in the ipfailover e2e test is using the wrong node
selector, so it can miss remote-worker clusters. Update the logic in the test
block around the remote-worker gate to query `node.openshift.io/remote-worker`
directly, matching the approach used by `exactNodeDetails()`, and then keep the
skip decision based on that targeted result instead of searching generic `nodes
-l kubernetes.io/hostname` output.
| g.By("check whether the cluster is not ipv6 single stack") | ||
| // Get IP stack type using oc command instead of compat_otp | ||
| networkOutput, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("network", "cluster", "-o=jsonpath={.status.clusterNetworkMTU}").Output() | ||
| if err == nil && networkOutput != "" { | ||
| // Simple check - if we can get network info, check for IPv6 | ||
| cidrOutput, _ := oc.AsAdmin().WithoutNamespace().Run("get").Args("network", "cluster", "-o=jsonpath={.status.clusterNetwork[0].cidr}").Output() | ||
| if strings.Contains(cidrOutput, ":") && !strings.Contains(cidrOutput, ".") { | ||
| g.Skip("Skip as ipfailover currently doesn't support ipv6 single stack") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This skips IPv6-first dual-stack clusters as if they were single-stack.
The check only inspects status.clusterNetwork[0].cidr. If the first CIDR is IPv6 and the second is IPv4, the suite is skipped even though dual-stack is supported by the rest of this package. Reuse checkIPStackType(oc) or inspect the full CIDR list.
🤖 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 `@ipfailover/keepalived/tests/e2e/ipfailover.go` around lines 75 - 84, The IPv6
single-stack check in ipfailover e2e is too narrow because it only inspects the
first clusterNetwork CIDR and can misclassify IPv6-first dual-stack clusters.
Update the logic in the test around the current oc-based network lookup to reuse
checkIPStackType(oc) or otherwise evaluate the full status.clusterNetwork CIDR
list before calling g.Skip, so only true IPv6 single-stack clusters are skipped.
| g.By("7. Verify the virtual IP is floated onto the new MASTER node") | ||
| newPodName1 := getPodListByLabel(oc, ns, "ipfailover=hello-openshift") | ||
| newMasterNode, _ := ensureIpfailoverMasterBackup(oc, ns, newPodName1) | ||
| waitForPrimaryPod(oc, ns, newMasterNode, virtualIP) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for the replacement pod set before recalculating MASTER/BACKUP.
Both flows delete the current primary and then immediately read pod names again. There is a normal window where only the surviving replica exists, so ensureIpfailoverMasterBackup() / futurePrimaryPod[0] can race the rollout and fail nondeterministically. Add a replica-aware wait between the delete and the next pod-list read.
Also applies to: 346-353
🤖 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 `@ipfailover/keepalived/tests/e2e/ipfailover.go` around lines 168 - 171, After
deleting the current primary, do not immediately recompute MASTER/BACKUP from
the pod list; in the ipfailover e2e flow around getPodListByLabel,
ensureIpfailoverMasterBackup, waitForPrimaryPod, and futurePrimaryPod[0], add a
replica-aware wait until the replacement pod set is fully available and stable
before reading pod names again. Apply the same fix in both affected flows so the
recalculation happens only after the rollout completes and the pod list reflects
the final state.
| replicas: 2 | ||
| selector: | ||
| matchLabels: | ||
| ipfailover: hello-openshift | ||
| template: | ||
| metadata: | ||
| labels: | ||
| ipfailover: hello-openshift | ||
| spec: | ||
| serviceAccountName: ipfailover | ||
| privileged: true | ||
| hostNetwork: true | ||
| nodeSelector: | ||
| node-role.kubernetes.io/worker: "" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Avoid hard-coding a 2-replica, worker-only topology.
replicas: 2 plus node-role.kubernetes.io/worker assumes at least two schedulable workers exist. This template will stall on SingleReplica/DualReplica/arbiter-style topologies instead of adapting or being explicitly gated. Cap replicas to the schedulable node count and make the surrounding test logic topology-aware before rendering this template. As per coding guidelines, "check whether they introduce scheduling constraints assuming standard HA topology with 3+ control-plane nodes and dedicated workers" and "Cap replica counts to schedulable 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 `@ipfailover/keepalived/tests/e2e/testdata/router/ipfailover.yaml` around lines
14 - 27, The ipfailover test template hard-codes a 2-replica deployment with a
worker-only node selector, which assumes a standard multi-worker topology.
Update the ipfailover.yaml template to derive replicas from the number of
schedulable nodes and make the e2e test setup topology-aware before rendering
it, so it can handle SingleReplica, DualReplica, and arbiter-style clusters. Use
the existing ipfailover template fields and related test setup logic to cap
replica count to available schedulable nodes and avoid assuming
node-role.kubernetes.io/worker is always present.
Source: Coding guidelines
| linuxWorkerDetails, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", "--selector=node-role.kubernetes.io/worker=,kubernetes.io/os=linux").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| nodeCount := int(strings.Count(linuxWorkerDetails, "Ready")) - (int(strings.Count(linuxWorkerDetails, "SchedulingDisabled")) + int(strings.Count(linuxWorkerDetails, "NotReady"))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop counting worker readiness from table text.
strings.Count(..., "Ready") also matches the Ready inside NotReady, so a NotReady,SchedulingDisabled worker is subtracted twice. With two healthy workers plus one cordoned unhealthy worker, this helper returns 1 instead of 2 and the suite is skipped even though the precondition is met. Query structured node conditions / unschedulable state instead of parsing oc get nodes text.
🤖 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 `@ipfailover/keepalived/tests/e2e/util.go` around lines 29 - 31, The
worker-counting logic in the e2e helper is parsing `oc get nodes` table text
with `strings.Count`, which double-counts `Ready` inside `NotReady` and can
underreport healthy nodes. Update the node readiness check in the helper that
builds `linuxWorkerDetails` and `nodeCount` to use structured node data from the
API or JSON output, inspecting node conditions and unschedulable state instead
of matching substrings in human-readable output.
| func getVipOwnerPod(oc *exutil.CLI, ns string, podname []string, vip string) string { | ||
| cmd := fmt.Sprintf("ip address |grep %s", vip) | ||
| var primaryNode string | ||
| for i := 0; i < len(podname); i++ { | ||
| output, err := oc.AsAdmin().WithoutNamespace().Run("exec").Args("-n", ns, podname[i], "--", "bash", "-c", cmd).Output() | ||
| if len(podname) == 1 && output == "command terminated with exit code 1" { | ||
| e2e.Failf("The given pod is not master") | ||
| } | ||
| if output == "command terminated with exit code 1" { | ||
| e2e.Logf("This Pod %v does not have the VIP", podname[i]) | ||
| } else if strings.Contains(output, vip) { | ||
| e2e.Logf("The pod owning the VIP is %v", podname[i]) | ||
| primaryNode = podname[i] | ||
| break | ||
| } else { | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Differentiate a grep miss from an oc exec failure.
Matching the literal output "command terminated with exit code 1" makes pod restarts, not-ready containers, and transport failures look the same as “VIP not present”. That can select the wrong master or return an empty pod name. Check err, only treat exit code 1 as a miss, and retry/fail on everything else. As per path instructions, **/*.go: Never ignore error returns.
🤖 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 `@ipfailover/keepalived/tests/e2e/util.go` around lines 160 - 176, The
getVipOwnerPod helper is treating the literal “command terminated with exit code
1” as a VIP miss, which can mask real oc exec failures and return the wrong
master or an empty result. Update the logic around
oc.AsAdmin().WithoutNamespace().Run("exec").Args(...).Output() to inspect err
first, only consider exit code 1 from the command as “VIP not present,” and
explicitly retry or fail for any other error conditions; keep the selection
logic in getVipOwnerPod and its primaryNode assignment tied to successful
command results.
Source: Path instructions
| if platformtype == "nutanix" || platformtype == "none" { | ||
| getPodListByLabel(oc, oc.Namespace(), "ipfailover=hello-openshift") | ||
| workerIPAddress, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", "--selector=node-role.kubernetes.io/worker=", "-ojsonpath={.items[*].status.addresses[0].address}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| modifiedIPList := strings.Split(workerIPAddress, " ") | ||
| if len(modifiedIPList) < 2 { | ||
| e2e.Failf("There is not enough IP addresses to add as unicast peer") | ||
| } | ||
| ipList := strings.Join(modifiedIPList, ",") | ||
| cmd := fmt.Sprintf("OPENSHIFT_HA_UNICAST_PEERS=%v", ipList) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t assume .status.addresses[0] is the worker IP.
Kubernetes does not guarantee address order. On many clusters index 0 is Hostname or ExternalIP, so OPENSHIFT_HA_UNICAST_PEERS can be populated with unusable values and the Nutanix/none path never converges. Filter node addresses by type=="InternalIP" before building the peer list.
🤖 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 `@ipfailover/keepalived/tests/e2e/util.go` around lines 240 - 249, The unicast
peer list in the Nutanix/none branch is built from the first node address, but
.status.addresses[0] is not guaranteed to be an InternalIP. Update the logic in
the keepalived e2e util path that calls
oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", ...) to filter node
addresses by type == "InternalIP" before assembling OPENSHIFT_HA_UNICAST_PEERS,
and keep the existing validation that enough worker IPs were found.
| func waitForPodWithLabelReady(oc *exutil.CLI, ns, label string) error { | ||
| return wait.Poll(5*time.Second, 3*time.Minute, func() (bool, error) { | ||
| status, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("pod", "-n", ns, "-l", label, `-ojsonpath={.items[*].status.conditions[?(@.type=="Ready")].status}`).Output() | ||
| e2e.Logf("the Ready status of pod is %v", status) | ||
| if err != nil || status == "" { | ||
| e2e.Logf("failed to get pod status: %v, retrying...", err) | ||
| return false, nil | ||
| } | ||
| if strings.Contains(status, "False") { | ||
| e2e.Logf("the pod Ready status not met; wanted True but got %v, retrying...", status) | ||
| return false, nil | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
This readiness gate can pass before the replacement replica exists.
It only checks the Ready condition of the pods that already exist; it never verifies how many labeled pods are present. During a rollout, one surviving Ready pod is enough for this to return success, and callers then immediately assume a two-pod MASTER/BACKUP set. Make the helper validate the expected replica count too.
🤖 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 `@ipfailover/keepalived/tests/e2e/util.go` around lines 319 - 331, The helper
waitForPodWithLabelReady currently only checks Ready status, so it can succeed
with a single surviving pod during rollout. Update waitForPodWithLabelReady to
also verify the number of pods returned by the label selector in addition to
readiness, and keep polling until both the expected replica count and Ready
condition are met. Use the existing wait.Poll logic and the kubectl get call in
waitForPodWithLabelReady to enforce the count before returning success.
| @@ -0,0 +1,350 @@ | |||
| module github.com/openshift/ipfailover-tests-extension | |||
|
|
|||
| go 1.25.0 | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod =="
sed -n '1,6p' ipfailover/keepalived/tests/go.mod
echo
echo "== Makefile build recipe =="
sed -n '1,10p' ipfailover/keepalived/tests/Makefile
echo
echo "== Docker builder stage =="
sed -n '1,8p' ipfailover/keepalived/DockerfileRepository: openshift/images
Length of output: 910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== other Go version declarations =="
rg -n --glob 'go.mod' --glob 'Dockerfile*' --glob 'Makefile*' 'go 1\.25|golang-1\.22|GOTOOLCHAIN=auto|GOTOOLCHAIN' .
echo
echo "== files in keepalived tests =="
git ls-files 'ipfailover/keepalived/tests/*'
echo
echo "== nearby Dockerfile references =="
rg -n 'ipfailover/keepalived/tests|test-extension-builder|golang-' ipfailover/keepalived -SRepository: openshift/images
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Dockerfile (full, line-numbered) =="
cat -n ipfailover/keepalived/Dockerfile
echo
echo "== tests go.mod =="
cat -n ipfailover/keepalived/tests/go.mod
echo
echo "== tests Makefile =="
cat -n ipfailover/keepalived/tests/MakefileRepository: openshift/images
Length of output: 26386
Align the Go 1.25 module with the Go 1.22 builder image
ipfailover/keepalived/tests/go.mod:3 now requires Go 1.25.0, but ipfailover/keepalived/Dockerfile:2 still builds in ...golang-1.22.... With GOTOOLCHAIN=auto, this will pull a newer toolchain during the build instead of using the image’s compiler, making CI and offline builds dependent on network access. Bump the builder image to Go 1.25 or lower the module directive to the supported version.
🤖 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 `@ipfailover/keepalived/tests/go.mod` at line 3, The module version in
ipfailover/keepalived/tests/go.mod is newer than the Go toolchain used by the
ipfailover/keepalived/Dockerfile builder stage, so the build may auto-download a
different compiler. Update the builder image in Dockerfile to a Go 1.25-based
image, or reduce the go directive in go.mod to match the supported compiler
version. Make sure the versions stay aligned across go.mod and the Dockerfile so
the tests build with the image’s local toolchain.
|
/payload-with-prs openshift/release#81231 |
|
/test-with-release openshift/release#81231 |
|
/payload-job-with-prs openshift/release#81231 e2e-openstack-ipi-tests-ext |
|
@melvinjoseph86: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores