OPNET-621: Add e2e-metal-ipi-bgp-vip lane for BGP-based VIP management - #82698
OPNET-621: Add e2e-metal-ipi-bgp-vip lane for BGP-based VIP management#82698mkowalski wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mkowalski: This pull request references OPNET-621 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/pj-rehearse ack |
|
@mkowalski: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughAdds an optional Metal IPI E2E job that configures BGP-based VIP management, verifies routing and console access, and runs on the ChangesBGP VIP end-to-end test
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InstallerJob
participant BgpVipWorkflow
participant MetalCluster
participant BgpVipVerify
InstallerJob->>BgpVipWorkflow: Start optional Metal IPI BGP VIP test
BgpVipWorkflow->>MetalCluster: Configure IPv4 BGP VIP management
BgpVipWorkflow->>BgpVipVerify: Run verification step
BgpVipVerify->>MetalCluster: Check VIP state, routes, pods, and console access
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required shell defaults and narrow tracing.
Use
set -euo pipefail; the current script enables tracing twice (bash -xandset -x). Keep tracing off unless it is needed, and scope it narrowly.As per coding guidelines, step-registry command scripts should default to
set -euo pipefailwithout-x.Also applies to: 12-17
🤖 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 `@ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh` around lines 1 - 4, Update the script’s shell options to use the required default `set -euo pipefail` without tracing, and remove any duplicate `bash -x` or `set -x` usage in the referenced command sections. If debugging is necessary, scope tracing only around the specific commands that require it.Source: Coding guidelines
🤖 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
`@ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh`:
- Around line 47-66: Update the VIP path assertions around paths_for, api_paths,
and ingress_paths to require exact counts: API paths must equal masters, and
ingress paths must equal workers. Always evaluate the ingress VIP assertion,
including when the worker pool is absent, and fail when the count is missing or
differs from the expected value.
- Around line 68-73: Update the console check near the final verification step
to invoke curl with --resolve for the console hostname mapped to ${ingress_vip},
and bypass configured proxies for this request. Preserve the existing HTTPS
request, timeout, status-code validation, and failure message while ensuring the
hostname is routed specifically through the BGP ingress VIP.
- Around line 33-38: Replace the pod-name count check in the frr-k8s
verification step with validation that each control-plane node has a ready
frr-k8s pod scheduled via spec.nodeName. Exclude Pending, CrashLoopBackOff,
duplicate, and stale pods, and fail unless every node selected by
node-role.kubernetes.io/master is covered by exactly one ready pod.
---
Nitpick comments:
In
`@ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh`:
- Around line 1-4: Update the script’s shell options to use the required default
`set -euo pipefail` without tracing, and remove any duplicate `bash -x` or `set
-x` usage in the referenced command sections. If debugging is necessary, scope
tracing only around the specific commands that require it.
🪄 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: cbc42988-d2f4-4ac9-9ddf-7fcb125c5c8a
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/installer/openshift-installer-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (8)
ci-operator/config/openshift/installer/openshift-installer-main.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/baremetalds-e2e-bgp-vip-workflow.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/baremetalds-e2e-bgp-vip-workflow.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.shci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-ref.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-ref.yaml
| echo "[2/5] frr-k8s static pods run on every control plane node" | ||
| masters="$(oc get nodes -l node-role.kubernetes.io/master -o name | wc -l)" | ||
| static_pods="$(oc get pods -n openshift-frr-k8s -o name | grep -c 'frr-k8s-ostest-master' || true)" | ||
| if [[ "${static_pods}" -lt "${masters}" ]]; then | ||
| fail "expected ${masters} frr-k8s static pods on masters, found ${static_pods}" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Check frr-k8s readiness and node placement, not just pod-name counts.
A Pending or CrashLoopBackOff pod is counted, and duplicate/stale names can satisfy static_pods >= masters without one ready pod on every control-plane node. Validate pod readiness and spec.nodeName for each control-plane node. The step documentation requires frr-k8s pods to run on every control-plane node.
🤖 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
`@ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh`
around lines 33 - 38, Replace the pod-name count check in the frr-k8s
verification step with validation that each control-plane node has a ready
frr-k8s pod scheduled via spec.nodeName. Exclude Pending, CrashLoopBackOff,
duplicate, and stale pods, and fail unless every node selected by
node-role.kubernetes.io/master is covered by exactly one ready pod.
There was a problem hiding this comment.
Fixed: the check now iterates the control plane nodes and requires the frr-k8s- static pod on each to have condition Ready=True — Pending/CrashLooping pods and name-count tricks no longer satisfy it. Re-validated verbatim against a live BGP-VIP cluster.
This reply was written with AI assistance. Please verify before acting on it.
There was a problem hiding this comment.
@mkowalski, I am verifying the current script before I mark this finding as addressed.
You are interacting with an AI system.
| echo "[4/5] ToR sees the VIP routes over BGP" | ||
| api_vip="$(oc get infrastructure cluster -o jsonpath='{.status.platformStatus.baremetal.apiServerInternalIPs[0]}')" | ||
| ingress_vip="$(oc get infrastructure cluster -o jsonpath='{.status.platformStatus.baremetal.ingressIPs[0]}')" | ||
| workers="$(oc get nodes -l node-role.kubernetes.io/worker -o name | wc -l)" | ||
| podman exec bgp-tor vtysh -c 'show ip bgp' | ||
| paths_for() { | ||
| podman exec bgp-tor vtysh -c "show ip bgp $1/32" | sed -n 's/^Paths: (\([0-9]*\) available.*/\1/p' | ||
| } | ||
| api_paths="$(paths_for "${api_vip}")" | ||
| if [[ -z "${api_paths}" ]]; then | ||
| fail "API VIP ${api_vip}/32 not advertised to the ToR" | ||
| elif [[ "${api_paths}" -lt "${masters}" ]]; then | ||
| fail "API VIP advertised from ${api_paths} node(s), expected ${masters} (one per master)" | ||
| fi | ||
| if [[ "${workers}" -gt 0 ]]; then | ||
| ingress_paths="$(paths_for "${ingress_vip}")" | ||
| if [[ -z "${ingress_paths}" ]]; then | ||
| fail "ingress VIP ${ingress_vip}/32 not advertised to the ToR" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Require the expected BGP path count for both VIPs.
Line 58 accepts any API path count greater than or equal to the master count. Lines 61-66 accept a single ingress path and skip the ingress assertion entirely when zero workers exist. Require the expected number of unique paths, and fail when the configured worker pool is absent.
🤖 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
`@ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh`
around lines 47 - 66, Update the VIP path assertions around paths_for,
api_paths, and ingress_paths to require exact counts: API paths must equal
masters, and ingress paths must equal workers. Always evaluate the ingress VIP
assertion, including when the worker pool is absent, and fail when the count is
missing or differs from the expected value.
There was a problem hiding this comment.
Fixed: the API VIP now requires exactly one BGP path per master, and the ingress VIP exactly one path per node hosting a Ready default-router pod (the set of advertisers by design — router placement, not raw worker count, determines advertisement). Zero workers is now a hard failure since this lane always deploys workers. Re-validated live: 3/3 API paths, 2/2 ingress paths.
This reply was written with AI assistance. Please verify before acting on it.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh"
ast-grep outline "$file" --items all
printf '\n--- Relevant BGP VIP assertion block ---\n'
sed -n '40,90p' "$file"Length of output: 3132
@mkowalski, the API assertion now requires exactly one path per master. The worker-absence check also now fails as required.
One issue remains in the ingress assertion. router_nodes selects pods with status.phase=Running, but it does not require the Pod Ready condition. Also, if no selected router pods exist, the script accepts zero ingress paths.
Require at least one Ready default-router pod, then compare ingress_paths with the number of unique nodes that host those Ready pods. This preserves the router-placement contract and prevents a false pass before ingress routing is ready.
✏️ Learnings added
Learnt from: mkowalski
Repo: openshift/release PR: 82698
File: ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh:0-0
Timestamp: 2026-07-31T13:30:11.899Z
Learning: For `ci-operator/step-registry/baremetalds/e2e/bgp-vip/verify/baremetalds-e2e-bgp-vip-verify-commands.sh`, ingress VIP BGP advertisements originate from nodes that host Ready `default-router` pods. The expected ingress BGP path count is therefore the count of unique nodes hosting those Ready router pods, not the raw worker-node count.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
New baremetalds-e2e-bgp-vip workflow: dev-scripts baremetal IPI install with BGP_VIP_MANAGEMENT=true + ENABLE_BGP_TOR=true (DevPreviewNoUpgrade, v4, 2 workers), followed by a verification step asserting the feature's acceptance criteria instead of the conformance suite: vipManagement=BGP on the Infrastructure CR, frr-k8s static pods (and no keepalived) on the control plane, the API VIP advertised to the ToR from every master and the ingress VIP from the workers, and the console answering over the BGP-routed path. Wired as an optional, on-demand presubmit on openshift/installer. The job is expected to fail until the feature PRs merge (installer#10718, MCO#6326, CNO#3047) and the dev-scripts knob lands (openshift-metal3/dev-scripts#1939); on installer#10718 itself the payload carries the PR's installer, so /test e2e-metal-ipi-bgp-vip exercises the flow end to end. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
openshift-merge-bot
Bot
removed
the
rehearsals-ack
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@mkowalski: all tests passed! 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. |
|
/pj-rehearse ack This will not pass because it's a test for feature during implementation. |
|
@mkowalski: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
2 participants
Adds a CI lane for BGP-based VIP management on baremetal IPI (enhancement 1982, DevPreviewNoUpgrade):
baremetalds-e2e-bgp-vipworkflow — standard baremetalds/ofcir install withBGP_VIP_MANAGEMENT=true+ENABLE_BGP_TOR=true(dev-scripts knob: OPNET-787: Add BGP_VIP_MANAGEMENT one-click knob openshift-metal3/dev-scripts#1939; ToR speaker: dev-scripts#1929), IPv4, 2 workers,FEATURE_SET=DevPreviewNoUpgradebaremetalds-e2e-bgp-vip-verifystep — replaces the conformance suite with the feature's acceptance criteria:vipManagement: BGPon the Infrastructure CR, frr-k8s static pods and zero keepalived pods on the control plane, the API VIP advertised to the hypervisor ToR from every master and the ingress VIP from the workers (BGP path counts via vtysh), console HTTP 200 over the BGP-routed VIP. The script body has been validated verbatim against a live BGP-VIP cluster (passes, and the failure branches trigger on synthetic bad input)e2e-metal-ipi-bgp-vipon openshift/installer (always_run: false, optional: true)Expected red for now: the flow needs openshift/installer#10718 + openshift/machine-config-operator#6326 + openshift/cluster-network-operator#3047 and the dev-scripts knob to merge. On installer#10718 itself the ephemeral payload carries the PR's installer, so
/test e2e-metal-ipi-bgp-vipthere exercises the real flow — the lane doubles as a merge-progress indicator and goes green as the stack lands. Follow-ups once green: extend the presubmit to MCO/CNO, add a conformance variant and a nightly periodic.Rehearsals for the new job will fail by design (see above).
This PR was prepared with AI assistance. Please verify before acting on it.
Summary by CodeRabbit
Adds an optional, on-demand
e2e-metal-ipi-bgp-vippresubmit lane to OpenShift CI for bare-metal IPI clusters with BGP-managed API and ingress VIPs.The workflow provisions an IPv4, two-worker Equinix metal cluster with
DevPreviewNoUpgrade,BGP_VIP_MANAGEMENT=true, andENABLE_BGP_TOR=true. Verification checks Infrastructure configuration, FRR and keepalived pod state, BGP advertisements, and console access through the routed VIP.