add OpenPERouter baremetal E2E deploy-verify lane - #83156
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughThe CI configuration updates OpenPerOuter bundle builds and adds a dual-stack Equinix metal end-to-end test. The workflow applies extra-network firewall rules, deploys the operator bundle, creates an ChangesOpenPerOuter bare-metal validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI workflow
participant Host as dev-scripts host
participant SDK as Operator SDK and OLM
participant Cluster as OpenShift cluster
CI->>Host: Apply extra-network SSH reject rules
CI->>SDK: Install Operator SDK and deploy OO_BUNDLE
SDK->>Cluster: Create OpenPERouter namespace and resource
Cluster-->>SDK: Report deployments and daemonsets
SDK-->>CI: Return pod readiness status
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
![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
🤖 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/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-commands.sh`:
- Line 18: Remove the -x option from the remote shell setup in
ci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-commands.sh:18-18
and
ci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.sh:14-14,
keeping both scripts at set -euo pipefail by default.
In
`@ci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.sh`:
- Around line 63-71: Update the pod validation block in the baremetal
openperouter E2E command to inspect each non-completed pod’s Ready condition,
not just its phase. Treat Running pods with a non-ready condition such as 0/1 as
failures, report them through NOT_READY, and print the success message only when
every non-completed pod is Ready.
- Around line 51-56: Update the DaemonSet creation loop in the controller/router
wait flow to enforce a 300-second deadline; if oc get daemonset does not succeed
within that period, print an error and exit nonzero. Preserve the existing
polling interval and rollout status handling after creation.
🪄 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: Pro Plus
Run ID: 8375c2c2-6920-4b4d-8844-c4f217b8eb90
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift-kni/openperouter/openshift-kni-openperouter-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (9)
ci-operator/config/openshift-kni/openperouter/openshift-kni-openperouter-main.yamlci-operator/step-registry/baremetalds/openperouter-e2e/OWNERSci-operator/step-registry/baremetalds/openperouter-e2e/baremetalds-openperouter-e2e-workflow.yamlci-operator/step-registry/baremetalds/openperouter-e2e/iptables/OWNERSci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-commands.shci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-ref.yamlci-operator/step-registry/baremetalds/openperouter-e2e/test/OWNERSci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.shci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-ref.yaml
b35033d to
5c78f49
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
5c78f49 to
b2556bb
Compare
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
b2556bb to
479b385
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.sh (1)
68-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck Pod phase and Ready condition directly.
The grep accepts a
1/1 Terminatingrow. It also rejects a healthy pod with more than five containers. Query the Pod phase and theReadycondition instead of parsing displayed readiness counts.Proposed fix
-NOT_READY=$(oc get pods -n openshift-openperouter-system --no-headers | grep -v "Completed" | grep -v "1/1\|2/2\|3/3\|4/4\|5/5" || true) +NOT_READY="$(oc get pods -n openshift-openperouter-system \ + -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.status.phase}{"\t"}{range .status.conditions[?(@.type=="Ready")]}{.status}{end}{"\n"}{end}' | + awk -F '\t' '$2 != "Succeeded" && ($2 != "Running" || $3 != "True")')"🤖 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/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.sh` around lines 68 - 76, Replace the readiness-count parsing in the pod verification block with direct checks of each pod’s phase and Ready condition. Update the logic around NOT_READY to identify pods only when phase is not Running or the Ready condition is not True, so terminating pods and healthy pods with any container count are handled correctly.
🤖 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/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.sh`:
- Around line 20-21: Update the Operator SDK download curl command in the test
setup to fail on HTTP errors and enforce both connection and total request
timeouts, while preserving the existing output path and versioned download URL.
---
Duplicate comments:
In
`@ci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.sh`:
- Around line 68-76: Replace the readiness-count parsing in the pod verification
block with direct checks of each pod’s phase and Ready condition. Update the
logic around NOT_READY to identify pods only when phase is not Running or the
Ready condition is not True, so terminating pods and healthy pods with any
container count are handled correctly.
🪄 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: Pro Plus
Run ID: 2f3b5e68-485b-4dac-9fa3-5930ffc353d9
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift-kni/openperouter/openshift-kni-openperouter-main-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (12)
ci-operator/config/openshift-kni/openperouter/openshift-kni-openperouter-main.yamlci-operator/step-registry/baremetalds/openperouter-e2e/OWNERSci-operator/step-registry/baremetalds/openperouter-e2e/baremetalds-openperouter-e2e-workflow.metadata.jsonci-operator/step-registry/baremetalds/openperouter-e2e/baremetalds-openperouter-e2e-workflow.yamlci-operator/step-registry/baremetalds/openperouter-e2e/iptables/OWNERSci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-commands.shci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-ref.metadata.jsonci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-ref.yamlci-operator/step-registry/baremetalds/openperouter-e2e/test/OWNERSci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.shci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-ref.metadata.jsonci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- ci-operator/step-registry/baremetalds/openperouter-e2e/test/OWNERS
- ci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-ref.yaml
- ci-operator/step-registry/baremetalds/openperouter-e2e/baremetalds-openperouter-e2e-workflow.yaml
- ci-operator/step-registry/baremetalds/openperouter-e2e/OWNERS
- ci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-commands.sh
- ci-operator/config/openshift-kni/openperouter/openshift-kni-openperouter-main.yaml
- ci-operator/step-registry/baremetalds/openperouter-e2e/iptables/OWNERS
- ci-operator/step-registry/baremetalds/openperouter-e2e/test/baremetalds-openperouter-e2e-test-ref.yaml
| curl -sLo /usr/local/bin/operator-sdk \ | ||
| "https://github.com/operator-framework/operator-sdk/releases/download/${OPERATOR_SDK_VERSION}/operator-sdk_linux_amd64" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound and validate the Operator SDK download.
curl -sLo accepts HTTP error responses and has no connection or total timeout. A stalled GitHub request can consume the full 3600-second step timeout. Fail on HTTP errors and set bounded timeouts.
Proposed fix
-curl -sLo /usr/local/bin/operator-sdk \
+curl --fail --silent --show-error --location --retry 3 \
+ --connect-timeout 30 --max-time 300 \
+ -o /usr/local/bin/operator-sdk \
"https://github.com/operator-framework/operator-sdk/releases/download/${OPERATOR_SDK_VERSION}/operator-sdk_linux_amd64"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| curl -sLo /usr/local/bin/operator-sdk \ | |
| "https://github.com/operator-framework/operator-sdk/releases/download/${OPERATOR_SDK_VERSION}/operator-sdk_linux_amd64" | |
| curl --fail --silent --show-error --location --retry 3 \ | |
| --connect-timeout 30 --max-time 300 \ | |
| -o /usr/local/bin/operator-sdk \ | |
| "https://github.com/operator-framework/operator-sdk/releases/download/${OPERATOR_SDK_VERSION}/operator-sdk_linux_amd64" |
🤖 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/openperouter-e2e/test/baremetalds-openperouter-e2e-test-commands.sh`
around lines 20 - 21, Update the Operator SDK download curl command in the test
setup to fail on HTTP errors and enforce both connection and total request
timeouts, while preserving the existing output path and versioned download URL.
479b385 to
e8ce01b
Compare
|
/pj-rehearse |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
e8ce01b to
354c49a
Compare
|
/pj-rehearse |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
354c49a to
0e38b9a
Compare
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
| TOSWITCH1_NETWORK_SUBNET_V4='192.168.11.0/24' | ||
| TOSWITCH1_NETWORK_SUBNET_V6='2001:db8:11::/64' | ||
| TOSWITCH2_NETWORK_SUBNET_V4='192.168.12.0/24' | ||
| TOSWITCH2_NETWORK_SUBNET_V6='2001:db8:12::/64' |
There was a problem hiding this comment.
who consume these variables?
script at ci-operator/step-registry/baremetalds/openperouter-e2e/iptables/baremetalds-openperouter-e2e-iptables-commands.sh has the same values. maybe there we should use the env variables
| - dockerfile_path: operator/bundle.Dockerfile.openshift | ||
| - context_dir: operator | ||
| dockerfile_path: bundle.Dockerfile |
There was a problem hiding this comment.
since the current PR is not really executing tests, can we deploy the Dockerfile.openshift image? Does it have deployment issues too?
There was a problem hiding this comment.
yeah, let's use the openshift dockerfile, so we have a clear signal of the bits that will land downstream.
There was a problem hiding this comment.
we are using Dockerfile.openshift for the build itself, and that is what is mentioned in the substitutions (so that the end result is that we do use the openshift build from the given PR).
as for using bundle.Dockerfile vs bundle.Dockerfile.openshift (which wrap the olm manifests), bundle.Dockerfile.openshift did some problems, and given that they are practically the same (manifests wise, minus the overlay pins that konflux does that I think aren't related to a given PR) I flipped to bundle.Dockerfile.this is the same as the other projects under telco5g-konflux e.g oran-o2ims and lifecycle-agent, although they don't have bundle.Dockerfile.openshift (but their bundle.Dockerfile is pretty much the same as ours)
There was a problem hiding this comment.
I understand.
I don't like testing a bundle that is not the one that goes to production, but the konflux build for the bundle is so complicated that using the operator/bundle.Dockerfile.openshift file for Prow jobs has a risk of not testing the right code (the PR code).
I'm ok on keeping this.
Unfortunately, we can't add a comment to these file
6974e9a to
6b07c58
Compare
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
6b07c58 to
e64aa8e
Compare
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
e64aa8e to
f7fc8b3
Compare
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
3229118 to
0a8d42e
Compare
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
| TOSWITCH2_NETWORK_SUBNET_V4='192.168.12.0/24' | ||
| TOSWITCH2_NETWORK_SUBNET_V6='2001:db8:12::/64' | ||
| NUM_WORKERS=2 | ||
| ENABLE_LOCAL_REGISTRY=true |
There was a problem hiding this comment.
| ENABLE_LOCAL_REGISTRY=true | |
| ENABLE_LOCAL_REGISTRY=true | |
| OPENSHIFT_INSTALL_GATHER_BOOTSTRAP=false |
can you try this? if it solves the long install time, we can get rid of the iptables rules
There was a problem hiding this comment.
you're right about the iptables fix not working correctly 😅
compared this to metallb's and there gather takes 2 mins where here it took 90.
about setting the gather_bootstrap var, I've already tried it (before trying the iptables approach) and sadly it does not work, the reason being dev-scripts related:
it is hardcoded in
I wanted to avoid doing changes to the global devscripts script, even if it is harmless like changing to:
export OPENSHIFT_INSTALL_GATHER_BOOTSTRAP=${OPENSHIFT_INSTALL_GATHER_BOOTSTRAP:-true}
I'll try another hack with iptables and if it doesn't work again then we should propose^^ in a separate pr (or just go that route because it's cleaner, and better and will allow us to just use the devscripts-chain instead of hacking a custom step in-between)
There was a problem hiding this comment.
yep, didn't work. as we discussed, we should try submitting the pr to fix that dev-scripts env var, and ultimately here disable the gather bootstrap step, which will also allow us to use the ofcir chain like metallb instead of unpacking just to insert a hack (that doesn't even work perfectly)
Baseline for the openperouter-e2e-metal test. Currently deploys the operator via OLM bundle on a baremetal OCP cluster and verifies all components (operator, webhook, controller, router) come up healthy. The full E2E lane will extend this with containerlab fabric setup and test execution. The bundle image is now built using bundle.Dockerfile rather than bundle.Dockerfile.openshift (like the other telco-5g projects using konflux here). The workflow inserts an iptables pre step before devscripts-setup. EXTRA_NETWORK_NAMES creates additional NICs on each node. The installer's bootstrap gather collects ALL NIC IPs and SSHes to each for log gathering. Since the extra networks are unreachable via SSH, this avoids timeouts on attempts to gather them. This is the reason we unpacked the dev-scripts chain instead of calling it directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
0a8d42e to
a94a3a0
Compare
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse |
|
@oribon: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/lgtm I'll open another PR with the improvements described here |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oribon, zeeke 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 |
|
@oribon: 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. |
3 participants
Baseline for the openperouter-e2e-metal test. Currently deploys the operator via OLM bundle on a baremetal OCP cluster and verifies all components (operator, webhook, controller, router) come up healthy. The full E2E lane will extend this with containerlab fabric setup and test execution.
The bundle image is now built using bundle.Dockerfile rather than bundle.Dockerfile.openshift (like the other telco-5g projects using konflux here).
The workflow inserts an iptables pre step before devscripts-setup. EXTRA_NETWORK_NAMES creates additional NICs on each node. The installer's bootstrap gather collects ALL NIC IPs and SSHes to each for log gathering. Since the extra networks are unreachable via SSH, this avoids timeouts on attempts to gather them.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Signed-off-by: Ori Braunshtein obraunsh@redhat.com
Summary by CodeRabbit
Adds the
openperouter-e2e-metalOpenShift CI lane for OpenPERouter.build05Equinix bare-metal profile.