OPNET-803: installer: add BGP VIP coexistence lanes (OVN-K route advertisements, day-2 MetalLB) - #82912
OPNET-803: installer: add BGP VIP coexistence lanes (OVN-K route advertisements, day-2 MetalLB)#82912mkowalski wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (23)
WalkthroughAdds three optional Equinix metal CI jobs for OVN-BGP VIP, MetalLB BGP VIP, and combined validation. The workflows configure BGP components, run bare-metal IPI tests, and verify sessions, routes, VIPs, and network connectivity. ChangesBGP VIP validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CIJob
participant Workflow
participant Preparation
participant Cluster
participant RouteReflector
participant Verification
CIJob->>Workflow: Start bare-metal BGP VIP workflow
Workflow->>Preparation: Configure OVN-BGP and MetalLB
Preparation->>Cluster: Apply resources and wait for readiness
Workflow->>Verification: Run route and connectivity checks
Verification->>Cluster: Read resources and service state
Verification->>RouteReflector: Check sessions and advertised routes
RouteReflector-->>Verification: Return BGP state
Verification-->>CIJob: Return aggregate status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
mkowalski
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mkowalski: This pull request references OPNET-803 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. |
1 similar comment
|
@mkowalski: This pull request references OPNET-803 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. |
Combined lane for BGP-based VIP management (enhancement 1982) and OVN-K BGP route advertisements. New baremetalds-e2e-bgp-vip-ovn-bgp workflow: dev-scripts install with BGP_VIP_MANAGEMENT=true + ENABLE_BGP_TOR=true, then the existing baremetalds-e2e-ovn-bgp-pre step (external FRR route reflector, routeAdvertisements enabled, FRRConfiguration and RouteAdvertisements CRs). Tests run the existing VIP acceptance step, a new coexistence verify step (both consumers' CRs present, every node - including control plane nodes served by frr-k8s static pods, not the DaemonSet - Established at the route reflector, per-node pod subnets advertised, pod-to-external datapath over a BGP-imported route) and the conformance suite. baremetalds-e2e-ovn-bgp-pre namespace creation made idempotent (oc create namespace fails under errexit when openshift-frr-k8s already exists, as it does on BGP-VIP-managed clusters). Wired as an optional, on-demand presubmit on openshift/installer, red by design until the feature PRs merge (installer#10718, MCO#6326, CNO#3047), like the base e2e-metal-ipi-bgp-vip lane. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
cb5c2cf to
9532220
Compare
Found by a dry run of the coexistence lane on a live dev-scripts cluster
with BGP VIP management: when frr-k8s is already deployed, the daemonset
rollout waits return immediately and the RouteAdvertisements apply races
CNO's CRD creation ('no matches for kind RouteAdvertisements'). On the
existing ovn-bgp lanes the race is masked by the daemonset creation wait.
Assisted-By: Claude Fable 5
Signed-off-by: Mat Kowalski <mko@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh`:
- Around line 29-57: Add bounded polling to the verifier checks around the
FRRConfiguration existence, Established BGP session count, and pod-route count
so each condition is re-evaluated until ready or the timeout expires. Preserve
the existing validation and diagnostic output, but only call fail after the
polling timeout, allowing normal CR and BGP reconciliation to converge before
asserting state.
- Around line 42-55: Replace the hard-coded podman exec invocations in the BGP
verification flow with the selected container runtime used by the pre-step.
Detect or reuse the remote runtime before the calls in the established-session
and pod-route checks, then apply it consistently to all four frr vtysh
executions, including diagnostic output, so Docker-only hosts work correctly.
- Around line 49-57: The verification block around pod_routes must validate the
exact expected pod CIDRs rather than counting all BGP routes. Build the expected
prefix set from each node’s assigned pod CIDR, then assert every expected prefix
exists in the route-reflector output; retain the existing diagnostic route dump
and failure behavior when any expected CIDR is missing.
- Around line 64-70: Update the curl invocation in the bgp-ra-datapath-check
command to include --fail and --show-error, ensuring HTTP 4xx/5xx responses
cause the existing reachability check to enter the fail branch.
🪄 Autofix
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: a8b09d00-7de6-47ac-90f2-189be81b58cf
⛔ 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 (9)
ci-operator/config/openshift/installer/openshift-installer-main.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/baremetalds-e2e-bgp-vip-ovn-bgp-workflow.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/baremetalds-e2e-bgp-vip-ovn-bgp-workflow.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.shci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-ref.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-ref.yamlci-operator/step-registry/baremetalds/e2e/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh`:
- Around line 316-324: Update the RouteAdvertisements CRD wait in the
pre-command script to occur before the FRR_IMAGE-driven CNO managementState
transition, keeping CNO reconciliation active while the CRD is created. Replace
the unbounded until loop with a finite overall deadline, and on timeout print
the CRD state before exiting non-zero; retain successful continuation once the
CRD reaches Established.
🪄 Autofix
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: 10aff8d8-cb96-4a92-b531-a274e2293348
⛔ 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 (9)
ci-operator/config/openshift/installer/openshift-installer-main.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/baremetalds-e2e-bgp-vip-ovn-bgp-workflow.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/baremetalds-e2e-bgp-vip-ovn-bgp-workflow.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.shci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-ref.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-ref.yamlci-operator/step-registry/baremetalds/e2e/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh
🚧 Files skipped from review as they are similar to previous changes (8)
- ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/OWNERS
- ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-ref.metadata.json
- ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-ref.yaml
- ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/baremetalds-e2e-bgp-vip-ovn-bgp-workflow.yaml
- ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/OWNERS
- ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh
- ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/baremetalds-e2e-bgp-vip-ovn-bgp-workflow.metadata.json
- ci-operator/config/openshift/installer/openshift-installer-main.yaml
|
Dry run of this lane's logic against a live dev-scripts cluster (BGP-VIP-managed, 3 masters + 2 workers) — coexistence verified end to end. Executed the
The dry run also caught a real bug, fixed in 0edc822: the Remaining untested vs CI: the conformance suite against the combined configuration, and the install itself through this workflow (pending the feature PRs, as described above). This comment was generated using AI. Please verify before acting on it. |
Verify step: bounded polling before asserting CR presence, session count and route state (BGP/CR reconciliation may still be converging when the step starts); container runtime detection matching the pre-step's docker fallback instead of hard-coded podman; assert each node's exact OVN pod subnet (from the k8s.ovn.org/node-subnets annotation) at the route reflector instead of a route count that unrelated prefixes (agnhost network) could satisfy; curl --fail --show-error so HTTP errors fail the datapath check. Pre step: make the RouteAdvertisements CRD wait finite (10 min deadline, dump CRD state on timeout) and move it before the FRR_IMAGE managementState=Unmanaged transition, which would stop CNO from ever creating the CRD. Re-validated against a live BGP-VIP dev-scripts cluster with route advertisements active: all four verify checks pass. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh (1)
12-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisable global command tracing.
bash -xandset -xenable tracing for the full remote verifier. The verifier accesses the cluster throughKUBECONFIG. Remove global tracing. Use the existing timeout diagnostics, or enable tracing only for a short non-sensitive diagnostic block.As per coding guidelines, "
**/*-commands.sh: ... default toset -euo pipefailwithout-x; only enable tracing when needed; and disable tracing around sensitive operations."Proposed fix
-ssh "${SSHOPTS[@]}" "root@${IP}" bash -x - << 'EOF' +ssh "${SSHOPTS[@]}" "root@${IP}" bash - << 'EOF' @@ -set -x🤖 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/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh` around lines 12 - 17, Disable global command tracing in the remote verifier by removing `-x` from the `bash` invocation and removing `set -x` from the heredoc script, while preserving the existing `nounset`, `errexit`, and `pipefail` behavior and relying on timeout diagnostics or narrowly scoped tracing only where necessary.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/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh`:
- Around line 76-85: Update check_pod_subnets around the rr_routes and subnet
loop to count the expected IPv4 pod subnets from node metadata and compare that
count with the number of nodes. Fail when any node lacks an IPv4 pod subnet,
including when no subnets are returned, before accepting the route-reflector
result; preserve the existing per-subnet route validation.
---
Nitpick comments:
In
`@ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh`:
- Around line 12-17: Disable global command tracing in the remote verifier by
removing `-x` from the `bash` invocation and removing `set -x` from the heredoc
script, while preserving the existing `nounset`, `errexit`, and `pipefail`
behavior and relying on timeout diagnostics or narrowly scoped tracing only
where necessary.
🪄 Autofix
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: d7bf4db9-d05c-4eb5-a73b-ea5f6af233de
📒 Files selected for processing (2)
ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.shci-operator/step-registry/baremetalds/e2e/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/baremetalds/e2e/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh
Container-Privileges pre-merge check: EXTRA_MG_ARGS=--host-network scopes to the post-run must-gather pod only (diagnostics must survive a broken BGP-advertised pod-network path); the privileged FRR/agnhost containers are in the pre-existing shared ovn-bgp-pre step and run on the ephemeral CI hypervisor, not the cluster. Both mirror the existing baremetalds-e2e-ovn-bgp-* lanes unchanged. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
|
Re the Container-Privileges pre-merge check (
Re the IPv6/disconnected warning: intentionally v4-only for now — the BGP VIP feature side of this lane is v4-proven only (dualstack is listed as follow-up work), and the lane is optional/on-demand, not part of disconnected or IPv6 job suites. The agnhost image reference matches the one already used by the shared pre step. This comment was generated using AI. Please verify before acting on it. |
Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
Third bgp-vip lane: day-2 MetalLB operator install (OLM with upstream manifest fallback) in frr-k8s-external mode on a BGP-VIP-managed cluster, peering with the existing ToR. Verifies the same-neighbor FRRConfiguration merge (no session duplication), LoadBalancer IP advertisement from every node, datapath over the BGP route, and that the VIP acceptance criteria still hold. Validated end to end against a live dev-scripts cluster before submission. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
mkowalski
changed the title
|
Pushed the second coexistence job: Dry-run evidence from a live BGP-VIP dev-scripts cluster (3 masters + 2 workers), full day-2 flow executed exactly as the pre step scripts it:
One implementation note for reviewers: MetalLB's generated FRRConfigurations carry no labels, so the verify step matches them by the This comment was generated using AI. Please verify before acting on it. |
Full coexistence lane composing the two previous ones: BGP-based VIP management + OVN-K route advertisements + day-2 MetalLB in frr-k8s-external mode on a single cluster - three FRRConfiguration producers sharing the same frr-k8s instances (static pods on masters, DaemonSet on workers) and the same external speakers. Runs all three verify steps in order, then the conformance suite. This exact producer combination was validated end to end on a live dev-scripts cluster (all three verifies green) before the lane was added. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
|
Added the third lane: This comment was generated using AI. Please verify before acting on it. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-commands.sh (2)
53-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSelect the CSV by
installedCSVinstead ofdisplayName.The wait loop matches the CSV whose
.spec.displayNameequals the literalMetalLB Operator. That display name is not an API contract and it changes between operator releases. If it changes, the loop cannot match, the step waits the full 900s, and it fails on a healthy install.Read the CSV name from the Subscription status instead. That value is set by OLM and is stable.
♻️ Proposed fix to resolve the CSV by name
deadline=$((SECONDS + 900)) - until [ "$(oc get csv -n metallb-system -o jsonpath='{.items[?(@.spec.displayName=="MetalLB Operator")].status.phase}' 2>/dev/null)" = "Succeeded" ]; do + csv_succeeded() { + local csv + csv="$(oc get subscription metallb-operator -n metallb-system -o jsonpath='{.status.installedCSV}' 2>/dev/null)" + [ -n "${csv}" ] || return 1 + [ "$(oc get csv "${csv}" -n metallb-system -o jsonpath='{.status.phase}' 2>/dev/null)" = "Succeeded" ] + } + until csv_succeeded; do if (( SECONDS >= deadline )); then oc get csv,subscription,installplan -n metallb-system || true echo "Timed out waiting for the MetalLB operator CSV" >&2 exit 1 fi sleep 15 done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-commands.sh` around lines 53 - 61, Update the MetalLB operator wait loop to resolve the CSV name from the Subscription status.installedCSV in metallb-system, then query that CSV’s status.phase for Succeeded. Remove the displayName-based selector while preserving the existing timeout, diagnostics, and polling behavior.
155-159: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWait for the
lb-echopods to become Ready.The step creates the Deployment and the Service, then exits. The verify step polls for the LoadBalancer IP and for the BGP paths, but MetalLB advertises the IP based on the Service, not on pod readiness. The single
curlin the verify step can therefore run before either replica is Ready.Add a rollout wait here, consistent with the other rollout waits in this script.
♻️ Proposed fix to gate on workload readiness
oc expose deployment lb-echo -n default --type=LoadBalancer --port=8080 --name=lb-echo \ --dry-run=client -o yaml | oc apply -f - +oc rollout status -n default deploy/lb-echo --timeout=5m🤖 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/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-commands.sh` around lines 155 - 159, After applying the lb-echo Deployment and Service, add a rollout wait for the lb-echo Deployment, using the same rollout-wait pattern already used elsewhere in this script. Ensure the command waits for both replicas to become Ready before this step exits, while leaving the existing creation commands unchanged.
🤖 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/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-commands.sh`:
- Around line 125-153: Update the IPAddressPool in the MetalLB manifest applied
by the pre-commands to use an address range outside dev-scripts’ DHCP range of
192.168.111.20-192.168.111.60, while leaving the BGPPeer peerAddress and ASN
values unchanged.
In
`@ci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/verify/baremetalds-e2e-bgp-vip-metallb-verify-commands.sh`:
- Around line 100-111: The datapath check must wait for a serving backend before
asserting reachability. In
ci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/verify/baremetalds-e2e-bgp-vip-metallb-verify-commands.sh
lines 100-111, wrap the curl in the existing poll helper while preserving the ip
route get diagnostic on failure; in
ci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-commands.sh
lines 155-159, add oc rollout status -n default deploy/lb-echo --timeout=5m
after oc expose.
- Around line 66-71: Update check_sessions to read peers from the FRR
.ipv4Unicast.peers object, with an explicit fallback to the top-level .peers for
flat output, before filtering Established sessions and comparing the count with
nodes.
---
Nitpick comments:
In
`@ci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-commands.sh`:
- Around line 53-61: Update the MetalLB operator wait loop to resolve the CSV
name from the Subscription status.installedCSV in metallb-system, then query
that CSV’s status.phase for Succeeded. Remove the displayName-based selector
while preserving the existing timeout, diagnostics, and polling behavior.
- Around line 155-159: After applying the lb-echo Deployment and Service, add a
rollout wait for the lb-echo Deployment, using the same rollout-wait pattern
already used elsewhere in this script. Ensure the command waits for both
replicas to become Ready before this step exits, while leaving the existing
creation commands unchanged.
🪄 Autofix
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: 71362e55-4276-4787-a6e9-3d4df4a6bc0c
⛔ 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 (12)
ci-operator/config/openshift/installer/openshift-installer-main.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/baremetalds-e2e-bgp-vip-metallb-workflow.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/baremetalds-e2e-bgp-vip-metallb-workflow.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/pre/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-commands.shci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-ref.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/pre/baremetalds-e2e-bgp-vip-metallb-pre-ref.yamlci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/verify/OWNERSci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/verify/baremetalds-e2e-bgp-vip-metallb-verify-commands.shci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/verify/baremetalds-e2e-bgp-vip-metallb-verify-ref.metadata.jsonci-operator/step-registry/baremetalds/e2e/bgp-vip/metallb/verify/baremetalds-e2e-bgp-vip-metallb-verify-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/config/openshift/installer/openshift-installer-main.yaml
…-operator Switch the no-catalog fallback from a pinned upstream main SHA to the openshift/metallb-operator release branch (default release-5.0): the fork's branches track OCP releases and carry the downstream API shape (frr-k8s-external backend), so the lane does not rot the way an upstream main pin would. The SCC grant stays: the fork's bin manifest ships the same uid-65534 webhook-server deployment (the productized operator only avoids it via the CSV's --external-metallb-webhook-server mode, which is OLM-only). Validated live: fork manifest applied over the dry-run cluster, operator + MetalLB rolled out, all three coexistence verifies still pass. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
|
Fallback source switched to Investigated dropping the Re-validated live: fork manifest applied over the dry-run cluster, operator + MetalLB re-rolled, all three coexistence verifies still exit 0. This comment was generated using AI. Please verify before acting on it. |
Add metallb/frr-k8s#470 (redistribute design, open) and openshift/release#82912 (coexistence lanes, open); kube-vip#1636 already shown merged. Today-line moved to Aug 5; final phase band widened to 'CI + coexistence lanes'. 23 PRs across 12 repositories. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
- both datapath curls discard the response body (-o /dev/null): the pod hostname must not land in CI logs (No-Sensitive-Data-In-Logs) - vtysh summary parsing accepts both JSON schemas (.ipv4Unicast.peers on FRR 8.x, flat .peers on 10.x) - ovn-bgp verify: require exactly one IPv4 pod subnet per node from the node-subnets annotations before trusting the route-reflector result (an absent annotation previously shrank the loop instead of failing) - metallb pool moved to 192.168.111.70-90, off the dev-scripts DHCP range (.20-.60) that the previous .30-.50 overlapped - datapath gated on backend readiness: lb-echo rollout wait in the pre step, poll-wrapped curl in the verify step Re-validated live: pool reapplied, service recycled (got .70), all three coexistence verifies exit 0. Assisted-By: Claude Fable 5 Signed-off-by: Mat Kowalski <mko@redhat.com>
|
Re the No-Sensitive-Data-In-Logs pre-merge check: fixed in a8b7143 — both datapath curls ( Same commit also addresses the round's inline findings (jq schema fallback for FRR 8.x/10.x summaries, per-node pod-subnet count guard, MetalLB pool moved off the dev-scripts DHCP range to .70-.90, and the datapath gated on lb-echo backend readiness at both the pre and verify sites). All re-validated against the live three-producer cluster: pool reapplied, service recycled (received 192.168.111.70), all three coexistence verifies exit 0. Re the IPv6/disconnected warning: unchanged position — the lanes are deliberately v4-only (matching the feature's proven scope) and optional/on-demand; not part of IPv6 or disconnected suites. This comment was generated using AI. Please verify before acting on it. |
|
[REHEARSALNOTIFIER]
A total of 125 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/label approved |
|
/pj-rehearse ack |
openshift-ci
Bot
added
the
approved
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@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
|
@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
Coexistence lanes for BGP-based VIP management (enhancement openshift/enhancements#1982, OPNET-595) — the scenarios the enhancement claims but nothing tests today: other BGP consumers sharing the cluster's frr-k8s with the VIP feature.
Job 1:
e2e-metal-ipi-bgp-vip-ovn-bgp— OVN-K route advertisementsNew
baremetalds-e2e-bgp-vip-ovn-bgpworkflow, composed from the two existing lanes:BGP_VIP_MANAGEMENT=true+ENABLE_BGP_TOR=true(DevPreviewNoUpgrade, v4, 2 workers) — same asbaremetalds-e2e-bgp-vip(OPNET-621: Add e2e-metal-ipi-bgp-vip lane for BGP-based VIP management #82698)baremetalds-e2e-ovn-bgp-prestep: external FRR route reflector,routeAdvertisements: Enabled, FRRConfiguration/RouteAdvertisements CRsThe verify step asserts what makes the lane interesting: on control plane nodes the frr-k8s DaemonSet does not run (frr-k8s runs as static pods there), so the RouteAdvertisements-generated FRRConfigurations must be merged by the static pods. It checks both consumers' CRs exist, every node is Established at the route reflector, per-node pod subnets arrive at the reflector, and a pod reaches an external endpoint over a BGP-imported route.
Tweaks to the shared
baremetalds-e2e-ovn-bgp-prestep (no behavior change for existing lanes): idempotent namespace creation, and a bounded wait for the RouteAdvertisements CRD placed before the FRR_IMAGE managementState transition.Job 2:
e2e-metal-ipi-bgp-vip-metallb— day-2 MetalLB in BGP modeNew
baremetalds-e2e-bgp-vip-metallbworkflow: same install, then a customer-like day-2 MetalLB operator install — OLM subscription when the catalog carriesmetallb-operator, falling back to SHA-pinned upstream metallb-operator manifests otherwise (pre-release OCP has no catalog entry; on the OLM path a CSV timeout fails loudly by design) — configured infrr-k8s-externalmode against the cluster'sopenshift-frr-k8s, with an IPAddressPool/BGPPeer/BGPAdvertisement peering with the same ToR speaker that serves the VIPs, plus a LoadBalancer workload.The verify step asserts the hardest merge case: MetalLB's generated FRRConfigurations must merge into the ToR neighbor already declared by the VIP configuration — exactly one Established session per node (no duplication, no flap), the LoadBalancer IP advertised from every node, reachable over the BGP path, with the VIP acceptance step re-run first.
Job 3:
e2e-metal-ipi-bgp-vip-ovn-bgp-metallb— all three producersComposition of the two lanes above on one cluster: VIP management + OVN-K route advertisements + day-2 MetalLB, three FRRConfiguration producers sharing the same frr-k8s instances and external speakers. Runs all three verify steps in order, then the conformance suite. Pure composition — no new steps; it reuses the refs from jobs 1 and 2.
Validation
Both lanes' logic was executed end to end against a live dev-scripts cluster (BGP-VIP-managed, 3 masters + 2 workers) before submission — including the full day-2 MetalLB install. All verify steps pass with three FRRConfiguration producers coexisting (VIP + OVN-K RA + MetalLB): ToR sessions stable at one per node, LB /32 and pod subnets advertised, datapaths green. The dry runs also surfaced the fixes carried here (RouteAdvertisements CRD race, namespace idempotency, MetalLB SCC grant + DEPLOY_SERVICEMONITORS requirement on the manifest path).
Both jobs are optional/on-demand presubmits on openshift/installer, red by design until the feature PRs merge (openshift/installer#10718, openshift/machine-config-operator#6326, openshift/cluster-network-operator#3047); combined-stack runs are possible today via multi-PR testing (
/testwith).This PR description was generated using AI. Please verify before acting on it.
Summary by CodeRabbit
Adds optional bare-metal workflows for
openshift/installer:baremetalds-e2e-bgp-vip-ovn-bgptests BGP VIP management with OVN-Kubernetes BGP route advertisements.baremetalds-e2e-bgp-vip-metallbtests BGP VIP management with day-two MetalLB.The workflows use the
equnix-ocp-metalprofile and verify FRR resource merging, established BGP sessions, route advertisements, VIP and LoadBalancer paths, external connectivity, and conformance.The shared OVN-BGP preparation step creates the
openshift-frr-k8snamespace idempotently and waits for theRouteAdvertisementsCRD before applying resources. The MetalLB setup supports OLM and fallback installation paths and configures the required SCC, monitoring, and FRR-K8s resources.