Add a CNO secondary encap CI lane - #81614
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 (14)
🚧 Files skipped from review as they are similar to previous changes (13)
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 (1 error, 1 warning)
✅ 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 |
openshift-ci
Bot
added
lgtm
c32399f to
d73c04d
Compare
openshift-ci
Bot
removed
lgtm
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
|
/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. |
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. |
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
|
Hi @tssurya let me know if we can merge this one to test it on the open PR |
| capabilities: | ||
| - intranet | ||
| optional: true | ||
| run_if_changed: ^bindata\/network\/ovn-kubernetes\/common\/008-script-lib\.yaml$|^bindata\/network\/ovn-kubernetes\/(managed|self-hosted)\/ovnkube-node\.yaml$|^pkg\/network\/ovn_kubernetes_test\.go$ |
There was a problem hiding this comment.
so if any of these files changed in a PR it would auto-run? do we care about unit tests files
There was a problem hiding this comment.
not critical as we don't really run the tests on this lane removing
| run_if_changed: ^bindata\/network\/ovn-kubernetes\/common\/008-script-lib\.yaml$|^bindata\/network\/ovn-kubernetes\/(managed|self-hosted)\/ovnkube-node\.yaml$|^pkg\/network\/ovn_kubernetes_test\.go$ | ||
| steps: | ||
| cluster_profile: equinix-ocp-metal | ||
| workflow: cluster-network-operator-ovn-secondary-encap |
There was a problem hiding this comment.
I think we discussed this before? what's the test chain we are running in this lane?
is it chain: baremetalds-ipi-test?
There was a problem hiding this comment.
yep that should run the e2e test after we finish the cluster creation and the encap interface move validation
There was a problem hiding this comment.
I guess we don't need to restart control plane since the config isn't used there at all?
There was a problem hiding this comment.
right follows the same proceduce that the user will need to make, and there is no need to restart the control-plane
There was a problem hiding this comment.
AI suggested:
Line 61 — trailing empty element in readarray: The jsonpath produces a trailing \n, so readarray may capture an empty last element. This would cause the loop at line 69 to process an empty pod_name, which would fail noisily (probably caught by errexit), but it's sloppy. Should use readarray -t ovnkube_pods < <(... | grep -v '^$') or trim.
I'm not bash expert so take it with pinch of salt
There was a problem hiding this comment.
if there is an error the lane will failed and it will be easy to see the error I don't think we need to complicate this with a grep into it
There was a problem hiding this comment.
again AI suggested:
Line 72 — errexit won't catch function failure in a variable assignment: With set -o errexit, secondary_record=$(get_secondary_interface_record ...) will not trigger an exit if the oc exec inside the function fails and the function returns non-zero. This is a well-known bash gotcha: command substitution in an assignment (var=$(cmd)) does not propagate failure under errexit. The explicit -z check on line 74 mitigates this for the "empty output" case, but if oc exec fails with an error message printed to stdout (e.g., "error: pod not found"), that error text would be treated as valid data — secondary_record would be non-empty, and parsing on lines 79-81 would produce garbage values silently.
take with pinch of salt
There was a problem hiding this comment.
not exactly right but I still apply a check for robustness
There was a problem hiding this comment.
AI generated:
Line 71 — same errexit gotcha for get_ovnkube_container: If no matching container is found, the function writes to stderr and returns 1, but container_name=$(get_ovnkube_container ...) won't exit the script. The variable would be empty, and the subsequent oc exec with an empty -c "" would fail — but the error message would be confusing rather than the clear one from the function.
take with pinch of salt
There was a problem hiding this comment.
not exactly right but I still apply a check for robustness
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>
ec8c86d to
78a0640
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: |
| capabilities: | ||
| - intranet | ||
| optional: true | ||
| run_if_changed: ^bindata\/network\/ovn-kubernetes\/common\/008-script-lib\.yaml$|^bindata\/network\/ovn-kubernetes\/(managed|self-hosted)\/ovnkube-node\.yaml$ |
There was a problem hiding this comment.
nit: I don't think we have changes in ovnkube-node.yml in your PR but its fine, approving this
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SchSeba, tssurya, zeeke The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/pj-rehearse ack |
|
@SchSeba: 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
|
@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.