Add a CNO secondary encap CI lane - #81614
Conversation
WalkthroughThis PR adds an optional Metal IPI E2E job for OVN secondary encapsulation. It defines the workflow, discovers node secondary-interface IPs, injects ChangesOVN secondary encapsulation CI flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant JobConfig
participant Workflow
participant PreScript
participant NodePod
participant ConfigMap
participant ValidateScript
participant OVS
JobConfig->>Workflow: starts e2e-metal-ipi-ovn-secondary-encap
Workflow->>PreScript: runs pre step
PreScript->>NodePod: reads secondary interface and IP
PreScript->>ConfigMap: writes per-node OVN_ENCAP_IP overrides
PreScript->>NodePod: restarts ovnkube-node and verifies mounted overrides
Workflow->>ValidateScript: runs validation step
ValidateScript->>NodePod: locates each ready node pod
ValidateScript->>OVS: reads ovs-vsctl state and external_ids
ValidateScript->>ValidateScript: compares ovn-encap-ip and Geneve peer data
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yaml (1)
5-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame subnet value hardcoded twice.
192.168.221.0/24appears both as the standaloneEXTRANET_NETWORK_SUBNET_V4env var (line 10) and again inline inside theEXTRA_NETWORK_CONFIGheredoc (line 13). Since these are two separate literals, an update to one without the other would silently desync the extranet provisioning config from what the pre-step/validate steps expect.🤖 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/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yaml` around lines 5 - 13, The extranet subnet value is duplicated in the workflow env setup, so keep it defined from a single source instead of hardcoding it twice. Update the cluster-network-operator-ovn-secondary-encap workflow’s env block so EXTRANET_NETWORK_SUBNET_V4 and the EXTRA_NETWORK_CONFIG content both reference the same subnet value consistently, using the existing extranet config symbols to avoid future drift.
🤖 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/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-commands.sh`:
- Around line 15-26: Tighten the EXTRANET_NETWORK_SUBNET_V4 validation in
cluster-network-operator-ovn-secondary-encap-pre-commands.sh: the current */24
glob plus %0/24 stripping in the subnet/prefix setup is too loose and can
produce an empty secondary_prefix. Update the validation to use a single
anchored format check in the existing shell logic around secondary_prefix and
escaped_secondary_prefix, then derive the prefix only after the value is
confirmed to be in the expected x.x.x.0/24 form. Also ensure the value
interpolated into the remote bash -c awk match is always safely escaped before
use.
- Around line 119-124: The pod lookup in the secondary-encap pre-commands script
is still causing the shell to exit under set -e when no matching ovnkube-node
pod exists. Update the oc get call inside the while loop that assigns pod_name
to tolerate the empty-result case without failing, so the subsequent if [[ -z
"${pod_name}" ]] fallback can run and print pod=<missing>.
In
`@ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh`:
- Around line 39-61: The node validation loop in
cluster-network-operator-ovn-secondary-encap-validate-commands.sh relies on
command substitutions that can exit early under set -e, so failures from oc or
get_ovnkube_container stop the script before validation_failed is updated.
Update the logic around pod_name, container_name, and actual_ip to capture
failures explicitly and mark validation_failed instead of aborting the loop,
while preserving the per-node validation flow in this script.
---
Nitpick comments:
In
`@ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yaml`:
- Around line 5-13: The extranet subnet value is duplicated in the workflow env
setup, so keep it defined from a single source instead of hardcoding it twice.
Update the cluster-network-operator-ovn-secondary-encap workflow’s env block so
EXTRANET_NETWORK_SUBNET_V4 and the EXTRA_NETWORK_CONFIG content both reference
the same subnet value consistently, using the existing extranet config symbols
to avoid future drift.
🪄 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: a4b9d3e1-a0de-41d5-aebb-1879caac1620
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/cluster-network-operator/openshift-cluster-network-operator-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (14)
ci-operator/config/openshift/cluster-network-operator/openshift-cluster-network-operator-master.yamlci-operator/step-registry/cluster-network-operator/OWNERSci-operator/step-registry/cluster-network-operator/ovn/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yamlci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-commands.shci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.yamlci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.shci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.yaml
|
/pj-rehearse |
|
@SchSeba: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-cluster-network-operator-master-e2e-metal-ipi-ovn-secondary-encap |
|
@SchSeba: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SchSeba, 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 |
openshift-ci
Bot
added
the
needs-rebase
c32399f to
d73c04d
Compare
openshift-ci
Bot
removed
lgtm
|
New changes are detected. LGTM label has been removed. |
d73c04d to
45cd87c
Compare
|
/pj-rehearse max |
|
@SchSeba: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
45cd87c to
c7c8dc5
Compare
|
@SchSeba did we run the test-with-pr option and is there a link to a successful run of this job that I can look at? |
|
oh and you would need to address coderabbit comments |
I assume we are having a e2e test that actually tests east-west traffic over the configured geneve encap IP right? I don't see that detail in PR describe - that's what we promise through doing this so hope that's getting added.. |
c7c8dc5 to
4973dfd
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. |
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/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh`:
- Around line 71-84: Extend the validation workflow after
cluster-network-operator-ovn-secondary-encap-pre with a cross-node Geneve
traffic test that generates traffic between pods on different nodes and verifies
it uses the configured encapsulation IPs. Keep the existing
external_ids:ovn-encap-ip validation unchanged, and fail the validation when
cross-node Geneve traffic cannot be established or observed.
🪄 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: 9f29ef11-d5b2-49fe-a075-b29aa1e3f644
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/cluster-network-operator/openshift-cluster-network-operator-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (14)
ci-operator/config/openshift/cluster-network-operator/openshift-cluster-network-operator-master.yamlci-operator/step-registry/cluster-network-operator/OWNERSci-operator/step-registry/cluster-network-operator/ovn/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yamlci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-commands.shci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.yamlci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.shci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (13)
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/OWNERS
- ci-operator/config/openshift/cluster-network-operator/openshift-cluster-network-operator-master.yaml
- ci-operator/step-registry/cluster-network-operator/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.metadata.json
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.metadata.json
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.metadata.json
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-commands.sh
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/OWNERS
| if ! actual_ip=$(oc exec -n "${namespace}" "${pod_name}" -c "${container_name}" -- ovs-vsctl get Open_vSwitch . external_ids:ovn-encap-ip | tr -d '"'); then | ||
| echo "unable to read ovn-encap-ip for node ${node_name}" >&2 | ||
| validation_failed=1 | ||
| continue | ||
| fi | ||
| if [[ "${actual_ip}" != "${expected_ip}" ]]; then | ||
| echo "encap IP mismatch on ${node_name}: expected ${expected_ip}, got ${actual_ip}" >&2 | ||
| validation_failed=1 | ||
| continue | ||
| fi | ||
|
|
||
| if ! grep -Fq "${expected_ip}" "${show_file}"; then | ||
| echo "ovs-vsctl show output for ${node_name} did not contain ${expected_ip}; using external_ids:ovn-encap-ip as the source of truth" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect this workflow's commands for a post-override data-plane assertion.
fd -t f -e yaml -e sh . \
ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap |
sort |
xargs -r rg -n -i -C 4 \
'ping|iperf|netperf|curl|nc |geneve|encap|east.?west|cross.?node'Repository: openshift/release
Length of output: 27093
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow ---'
cat -n ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yaml
printf '%s\n' '--- validation script ---'
sed -n '45,95p' ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh
printf '%s\n' '--- candidate reusable traffic steps ---'
rg -n -i -g '*.yaml' -g '*.sh' \
'east.?west|cross.?node|iperf|netperf|geneve|ovn.*traffic|traffic.*ovn|connectivity|network.*test' \
ci-operator/step-registry |
head -n 300Repository: openshift/release
Length of output: 50373
Add a cross-node Geneve traffic test after cluster-network-operator-ovn-secondary-encap-pre.
The workflow only runs the validation step, which checks external_ids:ovn-encap-ip. It does not prove that east-west traffic uses the configured encapsulation IP.
🤖 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/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh`
around lines 71 - 84, Extend the validation workflow after
cluster-network-operator-ovn-secondary-encap-pre with a cross-node Geneve
traffic test that generates traffic between pods on different nodes and verifies
it uses the configured encapsulation IPs. Keep the existing
external_ids:ovn-encap-ip validation unchanged, and fail the validation when
cross-node Geneve traffic cannot be established or observed.
|
/pj-rehearse --pull=openshift/cluster-network-operator#2998 pull-ci-openshift-cluster-network-operator-master-e2e-metal-ipi-ovn-secondary-encap |
|
@SchSeba: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@SchSeba: job(s): --pull=openshift/cluster-network-operator#2998 either don't exist or were not found to be affected, and cannot be rehearsed |
4973dfd to
0f0798d
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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh (1)
101-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle a zero-match port count without exiting the validation loop.
At Line 101,
grepreturns status 1 when nolocal_ipmatches. Withpipefailanderrexit, the assignment exits the script before Lines 102-105 report the failed count or validate remaining nodes.This has the same failure mode as the previously fixed command substitutions, but it remains at this count operation. Treat grep status 1 as a zero count. Preserve other grep failures.
Proposed fix
- local_geneve_ports=$(grep -F "local_ip=\"${expected_ip}\"" "${show_file}" | wc -l | tr -d ' ') + if local_geneve_ports=$(grep -Fc -- "local_ip=\"${expected_ip}\"" "${show_file}"); then + : + else + grep_status=$? + if [[ "${grep_status}" -eq 1 ]]; then + local_geneve_ports=0 + else + echo "unable to count local Geneve ports for node ${node_name}" >&2 + validation_failed=1 + continue + fi + fiRehearse a case with no matching
local_ipentry. The script should report the node mismatch and continue with the remaining mappings.🤖 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/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh` around lines 101 - 105, Update the local_geneve_ports count operation in the validation loop to treat grep’s status 1 (no matching local_ip) as a zero count, allowing the existing mismatch reporting and remaining-node validation to continue. Preserve propagation of other grep failures while retaining the current count behavior for matches.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In
`@ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh`:
- Around line 101-105: Update the local_geneve_ports count operation in the
validation loop to treat grep’s status 1 (no matching local_ip) as a zero count,
allowing the existing mismatch reporting and remaining-node validation to
continue. Preserve propagation of other grep failures while retaining the
current count behavior for matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 11723d1e-0863-46f1-9d95-38441cc0c4fc
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/cluster-network-operator/openshift-cluster-network-operator-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (14)
ci-operator/config/openshift/cluster-network-operator/openshift-cluster-network-operator-master.yamlci-operator/step-registry/cluster-network-operator/OWNERSci-operator/step-registry/cluster-network-operator/ovn/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yamlci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-commands.shci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.yamlci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.shci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.yaml
- ci-operator/config/openshift/cluster-network-operator/openshift-cluster-network-operator-master.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.metadata.json
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.metadata.json
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.metadata.json
- ci-operator/step-registry/cluster-network-operator/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-commands.sh
|
/pj-rehearse pull-ci-openshift-cluster-network-operator-master-e2e-metal-ipi-ovn-secondary-encap |
|
@SchSeba: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Create a baremetal cluster-network-operator workflow that provisions a secondary interface, injects node-specific OVN_ENCAP_IP overrides, and prints the resolved override mapping before validating OVS state. Signed-off-by: Sebastian Sch <sebassch@gmail.com>
0f0798d to
ec8c86d
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 pull-ci-openshift-cluster-network-operator-master-e2e-metal-ipi-ovn-secondary-encap |
|
@SchSeba: 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: |
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/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh`:
- Line 101: Update the local_geneve_ports count logic in the validation script
to treat grep’s status 1 (no matching interfaces) as a zero count without
triggering pipefail. Preserve handling of successful matches, but detect and
report or propagate other grep failures separately before continuing to the
validation_failed logic.
🪄 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: f451a91c-162e-474f-9818-cbe19c73d105
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/cluster-network-operator/openshift-cluster-network-operator-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (14)
ci-operator/config/openshift/cluster-network-operator/openshift-cluster-network-operator-master.yamlci-operator/step-registry/cluster-network-operator/OWNERSci-operator/step-registry/cluster-network-operator/ovn/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yamlci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-commands.shci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.yamlci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/OWNERSci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.shci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.metadata.jsonci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (13)
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.metadata.json
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-ref.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/OWNERS
- ci-operator/config/openshift/cluster-network-operator/openshift-cluster-network-operator-master.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.yaml
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/cluster-network-operator-ovn-secondary-encap-workflow.metadata.json
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-ref.metadata.json
- ci-operator/step-registry/cluster-network-operator/OWNERS
- ci-operator/step-registry/cluster-network-operator/ovn/secondary-encap/pre/cluster-network-operator-ovn-secondary-encap-pre-commands.sh
| fi | ||
| done < "${mapping_file}" | ||
|
|
||
| local_geneve_ports=$(grep -F "local_ip=\"${expected_ip}\"" "${show_file}" | wc -l | tr -d ' ') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle a zero-match count without exiting the script.
If no interface has the expected local_ip, grep returns 1. With pipefail enabled, Line 101 exits before Lines 102-105 set validation_failed, so later nodes are not validated. Treat exit status 1 as a count of zero. Handle other grep failures separately.
Proposed fix
- local_geneve_ports=$(grep -F "local_ip=\"${expected_ip}\"" "${show_file}" | wc -l | tr -d ' ')
+ if local_geneve_ports=$(grep -Fc "local_ip=\"${expected_ip}\"" "${show_file}"); then
+ :
+ else
+ grep_status=$?
+ if [[ "${grep_status}" -eq 1 ]]; then
+ local_geneve_ports=0
+ else
+ echo "unable to count Geneve ports for node ${node_name}" >&2
+ validation_failed=1
+ continue
+ fi
+ fi📝 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.
| local_geneve_ports=$(grep -F "local_ip=\"${expected_ip}\"" "${show_file}" | wc -l | tr -d ' ') | |
| if local_geneve_ports=$(grep -Fc "local_ip=\"${expected_ip}\"" "${show_file}"); then | |
| : | |
| else | |
| grep_status=$? | |
| if [[ "${grep_status}" -eq 1 ]]; then | |
| local_geneve_ports=0 | |
| else | |
| echo "unable to count Geneve ports for node ${node_name}" >&2 | |
| validation_failed=1 | |
| continue | |
| fi | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[style] 101-101: Consider using 'grep -c' instead of 'grep|wc -l'.
(SC2126)
🤖 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/cluster-network-operator/ovn/secondary-encap/validate/cluster-network-operator-ovn-secondary-encap-validate-commands.sh`
at line 101, Update the local_geneve_ports count logic in the validation script
to treat grep’s status 1 (no matching interfaces) as a zero count without
triggering pipefail. Preserve handling of successful matches, but detect and
report or propagate other grep failures separately before continuing to the
validation_failed logic.
Source: Linters/SAST tools
|
@SchSeba: The following test failed, say
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
Create a baremetal cluster-network-operator workflow that provisions a secondary interface, injects node-specific OVN_ENCAP_IP overrides, and prints the resolved override mapping before validating OVS state.
Summary by CodeRabbit
This PR adds a bare-metal
cluster-network-operatorCI lane for OVN secondary encapsulation onequinix-ocp-metal.The workflow provisions a secondary interface, derives each node’s secondary IP, injects node-specific
OVN_ENCAP_IPvalues through anenv-overridesConfigMap, and restartsovnkube-node.The validation step checks each node’s
ovn-encap-ip, Geneve ports, and peer remote IPs against the expected secondary-interface mapping. It also captures OVS state and runs the standard bare-metal end-to-end tests with OVN debug collection.The CI job runs when selected OVN-Kubernetes manifests, scripts, or networking tests change. Ownership and step-registry metadata support the new workflow.