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
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-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
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 Cluster
participant BgpVipVerify
InstallerJob->>BgpVipWorkflow: Start optional Metal IPI BGP VIP test
BgpVipWorkflow->>Cluster: Configure IPv4 BGP VIP management
BgpVipWorkflow->>BgpVipVerify: Run verification step
BgpVipVerify->>Cluster: Check VIP state, routes, pods, and console access
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 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 |
|
[REHEARSALNOTIFIER]
Prior to this PR being merged, you will need to either run and acknowledge or opt to skip these rehearsals. Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, 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: 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.
| 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.
| echo "[5/5] console answers over the BGP-routed VIP" | ||
| console_url="$(oc whoami --show-console)" | ||
| http_code="$(curl -sk -o /dev/null -w '%{http_code}' --max-time 30 "${console_url}")" | ||
| if [[ "${http_code}" != "200" ]]; then | ||
| fail "console at ${console_url} returned HTTP ${http_code}, expected 200" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prove that the console request used the BGP ingress VIP.
Curling the console hostname only proves that some HTTP endpoint returned 200; DNS or a proxy could route elsewhere. Pin the hostname to ${ingress_vip} with curl --resolve and bypass proxies for this check.
🤖 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 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.
|
@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. |
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 validating bare-metal IPI clusters with BGP-managed API and ingress VIPs.The new workflow configures an IPv4, two-worker Equinix metal cluster with
DevPreviewNoUpgrade, BGP VIP management, and BGP ToR support. Its verification step checks Infrastructure configuration, FRR/keepalived pod state, BGP advertisements, and console access through the routed VIP.