OPNET-803: installer: add e2e-metal-ipi-bgp-vip-ovn-bgp coexistence lane - #82912
OPNET-803: installer: add e2e-metal-ipi-bgp-vip-ovn-bgp coexistence lane#82912mkowalski wants to merge 2 commits into
Conversation
WalkthroughAdds an optional Equinix metal CI job for combined BGP VIP and OVN BGP routing. The workflow prepares the cluster, runs IPI tests, verifies BGP state and pod connectivity, and handles an existing FRR-K8s namespace. ChangesOVN BGP VIP bare-metal E2E
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CIJob as e2e-metal-ipi-bgp-vip-ovn-bgp
participant Workflow as baremetalds-e2e-bgp-vip-ovn-bgp
participant Preparation as OVN BGP preparation
participant Verify as OVN BGP verification
participant Cluster as Equinix metal cluster
CIJob->>Workflow: starts configured workflow
Workflow->>Preparation: prepares FRR-K8s and BGP environment
Preparation->>Cluster: applies namespace and waits for CRD
Workflow->>Verify: runs BGP VIP and OVN BGP checks
Verify->>Cluster: checks BGP resources, sessions, and routes
Verify->>Cluster: tests pod access to external agnhost
Cluster-->>Verify: returns validation results
Verify-->>CIJob: exits success or failure
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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
| echo "[1/4] both BGP consumers own FRRConfiguration CRs in openshift-frr-k8s" | ||
| for cr in bgp-vip receive-filtered; do | ||
| if ! oc get frrconfiguration -n openshift-frr-k8s "${cr}" &>/dev/null; then | ||
| fail "FRRConfiguration '${cr}' not found in openshift-frr-k8s" | ||
| fi | ||
| done | ||
|
|
||
| echo "[2/4] every node has an Established BGP session to the route reflector" | ||
| # The route reflector (external 'frr' container, 192.168.111.3) peers with | ||
| # every node. On the control plane the RouteAdvertisements-generated | ||
| # FRRConfiguration must be merged by the frr-k8s *static pods* (the frr-k8s | ||
| # DaemonSet does not run there), so established master sessions prove the | ||
| # static-pod CR merge works alongside the VIP configuration. | ||
| established="$(podman exec frr vtysh -c 'show bgp ipv4 unicast summary json' \ | ||
| | jq '[.peers[] | select(.state=="Established")] | length')" | ||
| if [[ "${established:-0}" -ne "${nodes}" ]]; then | ||
| podman exec frr vtysh -c 'show bgp ipv4 unicast summary' | ||
| fail "route reflector has ${established:-0} Established session(s), expected ${nodes} (one per node)" | ||
| fi | ||
|
|
||
| echo "[3/4] every node's pod subnet is advertised to the route reflector" | ||
| cluster_network="$(oc get network.config cluster -o jsonpath='{.status.clusterNetwork[0].cidr}')" | ||
| pod_routes="$(podman exec frr vtysh -c "show bgp ipv4 unicast json" \ | ||
| | jq --arg net "${cluster_network}" '[.routes | keys[] | select(. != $net)] | length')" | ||
| # each node advertises its own host subnet out of the cluster network | ||
| if [[ "${pod_routes:-0}" -lt "${nodes}" ]]; then | ||
| podman exec frr vtysh -c 'show bgp ipv4 unicast' | ||
| fail "route reflector sees ${pod_routes:-0} pod subnet route(s), expected at least ${nodes}" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh'
printf '%s\n' '--- verifier script ---'
cat -n "$file"
printf '%s\n' '--- related BGP-VIP OVN-BGP files ---'
git ls-files 'ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/**'
printf '%s\n' '--- preparation and verifier references ---'
rg -n -C 5 'FRRConfiguration|RouteAdvertisements|baremetalds-e2e-bgp-vip-ovn-bgp-verify|podman exec frr|sleep|retry|until|timeout' \
ci-operator/step-registry/baremetalds/e2e/bgp-vipRepository: openshift/release
Length of output: 20309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all OVN-BGP preparation references ---'
rg -n -C 8 'RouteAdvertisements|FRRConfiguration|routeAdvertisements|openshift-frr-k8s' ci-operator/step-registry
printf '%s\n' '--- step references for the preparation chain ---'
rg -n -C 8 'baremetalds-e2e-ovn-bgp-pre|e2e-ovn-bgp-pre|ovn-bgp-pre' ci-operator/step-registry
printf '%s\n' '--- verifier timing and retry constructs ---'
python3 - <<'PY'
from pathlib import Path
path = Path("ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh")
text = path.read_text()
for number, line in enumerate(text.splitlines(), 1):
if any(token in line for token in ("oc get", "podman exec", "until", "retry", "sleep", "timeout", "wait", "for ")):
print(f"{number}: {line}")
PYRepository: openshift/release
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pre='ci-operator/step-registry/baremetalds/e2e/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh'
workflow='ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/baremetalds-e2e-bgp-vip-ovn-bgp-workflow.yaml'
printf '%s\n' '--- preparation resource application and tail ---'
sed -n '310,430p' "$pre"
printf '%s\n' '--- workflow ordering ---'
cat -n "$workflow" | sed -n '1,45p'
printf '%s\n' '--- deterministic check of verifier reads and waits ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if re.search(r'\b(oc|podman)\b', line) or re.search(r'\b(until|while|sleep|retry|wait|timeout)\b', line, re.I):
print(f"{i}: {line}")
PYRepository: openshift/release
Length of output: 6447
Wait for BGP convergence before asserting route-advertisement state.
The preparation step waits for the CRD but applies FRRConfiguration and RouteAdvertisements immediately before it exits. The verifier performs single reads for the CRs, BGP sessions, and pod routes. Poll these conditions with a bounded timeout before failing to avoid false failures during normal reconciliation.
🤖 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 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.
| established="$(podman exec frr vtysh -c 'show bgp ipv4 unicast summary json' \ | ||
| | jq '[.peers[] | select(.state=="Established")] | length')" | ||
| if [[ "${established:-0}" -ne "${nodes}" ]]; then | ||
| podman exec frr vtysh -c 'show bgp ipv4 unicast summary' | ||
| fail "route reflector has ${established:-0} Established session(s), expected ${nodes} (one per node)" | ||
| fi | ||
|
|
||
| echo "[3/4] every node's pod subnet is advertised to the route reflector" | ||
| cluster_network="$(oc get network.config cluster -o jsonpath='{.status.clusterNetwork[0].cidr}')" | ||
| pod_routes="$(podman exec frr vtysh -c "show bgp ipv4 unicast json" \ | ||
| | jq --arg net "${cluster_network}" '[.routes | keys[] | select(. != $net)] | length')" | ||
| # each node advertises its own host subnet out of the cluster network | ||
| if [[ "${pod_routes:-0}" -lt "${nodes}" ]]; then | ||
| podman exec frr vtysh -c 'show bgp ipv4 unicast' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
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/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh
do
echo "===== ${f} ====="
wc -l "${f}"
cat -n "${f}"
done
echo "===== related runtime and SHARED_DIR references ====="
rg -n -C 3 'CONTAINER_CLI|podman|docker|SHARED_DIR|frr' ci-operator/step-registry/baremetalds/e2eRepository: openshift/release
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== workflow ====="
cat -n ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/baremetalds-e2e-bgp-vip-ovn-bgp-workflow.yaml
echo "===== verifier/pre references ====="
rg -n -C 5 \
'baremetalds-e2e-(ovn-bgp-pre|bgp-vip-ovn-bgp-verify)|ovn-bgp-pre|ovn-bgp-verify' \
ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp \
ci-operator/step-registry/baremetalds/e2e/ovn/bgp
echo "===== runtime command occurrences in the two scripts ====="
python3 - <<'PY'
from pathlib import Path
files = [
Path("ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh"),
Path("ci-operator/step-registry/baremetalds/e2e/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh"),
]
for path in files:
text = path.read_text()
print(f"{path}:")
for i, line in enumerate(text.splitlines(), 1):
if "podman" in line or "docker" in line or "CLI" in line:
print(f" {i}: {line}")
PYRepository: openshift/release
Length of output: 20431
Use the selected container runtime for all exec calls.
The pre-step falls back to Docker, but this verifier hard-codes podman exec at lines 42, 45, 51, and 55. On Docker-only hosts, the verifier cannot access frr. Detect the runtime on the remote host and use it for all four calls.
🤖 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 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.
| echo "[3/4] every node's pod subnet is advertised to the route reflector" | ||
| cluster_network="$(oc get network.config cluster -o jsonpath='{.status.clusterNetwork[0].cidr}')" | ||
| pod_routes="$(podman exec frr vtysh -c "show bgp ipv4 unicast json" \ | ||
| | jq --arg net "${cluster_network}" '[.routes | keys[] | select(. != $net)] | length')" | ||
| # each node advertises its own host subnet out of the cluster network | ||
| if [[ "${pod_routes:-0}" -lt "${nodes}" ]]; then | ||
| podman exec frr vtysh -c 'show bgp ipv4 unicast' | ||
| fail "route reflector sees ${pod_routes:-0} pod subnet route(s), expected at least ${nodes}" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target='ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh'
printf '%s\n' '--- target context ---'
cat -n "$target" | sed -n '1,90p'
printf '%s\n' '--- related AGNHOST_SUBNET_V4 references ---'
rg -n -C 5 'AGNHOST_SUBNET_V4|show bgp ipv4 unicast json|pod_routes|clusterNetwork' ci-operator/step-registryRepository: openshift/release
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
prep='ci-operator/step-registry/baremetalds/e2e/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh'
target='ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh'
printf '%s\n' '--- preparation route configuration ---'
cat -n "$prep" | sed -n '1,115p'
cat -n "$prep" | sed -n '350,395p'
printf '%s\n' '--- references to the verification step and preparation step ---'
rg -n -C 3 'baremetalds-e2e-(ovn-bgp-pre|bgp-vip-ovn-bgp-verify)|ovn-bgp-pre|bgp-vip-ovn-bgp-verify' \
ci-operator/step-registry/baremetalds ci-operator/jobs ci-operator/config 2>/dev/null | head -200
printf '%s\n' '--- deterministic jq behavior probe ---'
python3 - <<'PY'
import json
import subprocess
routes = {
"10.128.0.0/14": {},
"10.128.0.0/23": {},
"10.128.2.0/23": {},
"10.128.4.0/23": {},
"172.20.0.0/16": {},
"192.0.2.0/24": {},
}
payload = json.dumps({"routes": routes})
query = r'[.routes | keys[] | select(. != $net)] | length'
result = subprocess.run(
["jq", "--arg", "net", "10.128.0.0/14", query],
input=payload,
text=True,
capture_output=True,
check=True,
)
print("route keys:", ", ".join(routes))
print("count excluding cluster aggregate:", result.stdout.strip())
print("expected pod CIDRs present:", [
cidr for cidr in ("10.128.0.0/23", "10.128.2.0/23", "10.128.4.0/23")
if cidr in routes
])
missing_one = dict(routes)
del missing_one["10.128.4.0/23"]
result = subprocess.run(
["jq", "--arg", "net", "10.128.0.0/14", query],
input=json.dumps({"routes": missing_one}),
text=True,
capture_output=True,
check=True,
)
print("count with one pod CIDR missing and unrelated prefixes retained:", result.stdout.strip())
PYRepository: openshift/release
Length of output: 15480
Assert every expected pod CIDR, not only the route count.
pod_routes counts every .routes key except the exact cluster-network aggregate. The preparation step advertises 172.20.0.0/16 (AGNHOST_SUBNET_V4) through the reflector, so this non-pod prefix is included. Thus N-1 pod routes plus unrelated routes can satisfy pod_routes >= nodes. Build the expected set from each node's assigned pod CIDR and assert that every expected prefix exists.
🤖 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 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.
| if oc run bgp-ra-datapath-check --restart=Never --attach --rm --pod-running-timeout=5m \ | ||
| --image=registry.k8s.io/e2e-test-images/agnhost:2.53 --command -- \ | ||
| curl --max-time 20 -s http://172.20.0.100:8000/hostname; then | ||
| echo "agnhost reachable from pod network" | ||
| else | ||
| fail "pod could not reach agnhost 172.20.0.100:8000 over the BGP-imported route" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(git ls-files 'ci-operator/step-registry/baremetalds/e2e/bgp-vip/ovn-bgp/verify/baremetalds-e2e-bgp-vip-ovn-bgp-verify-commands.sh')
printf '%s\n' "$file"
cat -n "$file" | sed -n '1,100p'
printf '\nCurl behavior and available options:\n'
curl --version | head -n 2
curl --help all | rg -- '--fail|--show-error|--silent|-s'Repository: openshift/release
Length of output: 7803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 --glob '*.sh' --glob '*.yaml' --glob '*.yml' \
'172\.20\.0\.100:8000/hostname|agnhost reachable|pod could not reach agnhost|agnhost:2\.53|--fail.*curl|curl .*--fail' .Repository: openshift/release
Length of output: 50374
Make the HTTP check fail on error responses.
curl -s returns success for HTTP 4xx and 5xx responses. Add --fail --show-error before treating the endpoint as reachable.
🤖 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 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.
|
[REHEARSALNOTIFIER]
A total of 123 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: |
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
| # The RouteAdvertisements CRD is created by CNO while reconciling the | ||
| # routeAdvertisements enablement. On clusters where frr-k8s is already | ||
| # deployed (e.g. BGP-based VIP management) the daemonset rollout waits above | ||
| # return immediately, so wait for the CRD explicitly before applying CRs. | ||
| echo "Waiting for the RouteAdvertisements CRD..." | ||
| until oc wait --for condition=Established crd/routeadvertisements.k8s.ovn.org --timeout 2m &> /dev/null; do | ||
| sleep 5 | ||
| done | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the CRD wait finite and keep CNO reconciliation active.
The --timeout 2m option limits one oc wait call. The surrounding until loop starts another call after each failure. If the CRD is not created or never becomes Established, this step runs until the external job timeout and hides the failure.
When FRR_IMAGE is set, the managementState: Unmanaged transition at Line 296 can stop CNO before it creates this CRD. Move this wait before that transition. Add a finite overall deadline and print the CRD state before exiting non-zero.
Suggested bounded wait
echo "Waiting for the RouteAdvertisements CRD..."
-until oc wait --for condition=Established crd/routeadvertisements.k8s.ovn.org --timeout 2m &> /dev/null; do
+deadline=$((SECONDS + 600))
+until oc wait --for condition=Established crd/routeadvertisements.k8s.ovn.org --timeout 10s &> /dev/null; do
+ if (( SECONDS >= deadline )); then
+ oc get crd/routeadvertisements.k8s.ovn.org -o yaml || true
+ echo "Timed out waiting for routeadvertisements.k8s.ovn.org" >&2
+ exit 1
+ fi
sleep 5
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/ovn/bgp/pre/baremetalds-e2e-ovn-bgp-pre-commands.sh`
around lines 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.
|
@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
Combined lane for BGP-based VIP management (enhancement openshift/enhancements#1982, OPNET-595) and OVN-Kubernetes BGP route advertisements — the coexistence scenario the enhancement claims but nothing tests today.
New
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 on the baremetal bridge,routeAdvertisements: Enabled, FRRConfiguration/RouteAdvertisements CRs — same as thebaremetalds-e2e-ovn-bgp-*lanesThe new verify step asserts what makes this lane interesting: on control plane nodes the frr-k8s DaemonSet does not run (anti-affinity; frr-k8s runs as static pods there), so the RouteAdvertisements-generated FRRConfiguration must be merged by the static pods. It checks both consumers' CRs exist, every node (masters included) 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.
One tweak to a shared step:
baremetalds-e2e-ovn-bgp-prenamespace creation made idempotent (oc create namespacefails undererrexitwhenopenshift-frr-k8salready exists, as it does on BGP-VIP-managed clusters). No behavior change for the existing lanes.Wired as an optional, on-demand presubmit on openshift/installer. Like the base
e2e-metal-ipi-bgp-viplane, it is 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 an optional
baremetalds-e2e-bgp-vip-ovn-bgppresubmit workflow foropenshift/installer.The workflow:
The shared OVN-BGP preparation step now creates the
openshift-frr-k8snamespace idempotently and waits for theRouteAdvertisementsCRD before applying route-advertisement resources.