[WIP] spoke to spoke bi-direction vm migration - #82731
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughAdds an OCP 4.22 AWS interoperability job for ACM, MTV, ODF, Submariner, and bidirectional VM migration. Adds Submariner remediation and diagnostics, MTV return maps, migration chains, return-leg execution, reporting, and cleanup. ChangesACM P2P bidirectional migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InteropJob
participant SubmarinerSteps
participant MTVMapStep
participant ACMHub
participant SpokeClusters
participant MTVReturnStep
InteropJob->>SubmarinerSteps: apply SNAT fix and collect diagnostics
SubmarinerSteps->>SpokeClusters: update Submariner and run diagnostics
InteropJob->>MTVMapStep: create return NetworkMap and StorageMap
MTVMapStep->>ACMHub: apply maps and wait for Ready
InteropJob->>MTVReturnStep: execute return migration
MTVReturnStep->>ACMHub: apply Plan and Migration
MTVReturnStep->>SpokeClusters: validate synchronization and verify destination VMI
MTVReturnStep-->>InteropJob: emit status, diagnostics, and JUnit results
Possibly related PRs
Suggested labels: 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shakyav 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 periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: 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: 8
🧹 Nitpick comments (3)
ci-operator/step-registry/p2p/mtv-create-return-migration-maps/p2p-mtv-create-return-migration-maps-commands.sh (1)
22-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider reusing the forward map script instead of copying it.
Lines 22-126 are identical to
ci-operator/step-registry/p2p/mtv-create-migration-maps/p2p-mtv-create-migration-maps-commands.sh. Only the header comment and the>>redirect differ. Every value that distinguishes the return leg already comes fromenvin the ref. Two options remove the duplicate logic. Reuse the forward ref twice with differentenvvalues in the chain. Or make the artifact redirect mode an env var, for exampleMTV_MAP_STATUS_REDIRECT, and keep one script. Duplicated shell drifts when only one copy is fixed later.🤖 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/p2p/mtv-create-return-migration-maps/p2p-mtv-create-return-migration-maps-commands.sh` around lines 22 - 127, Replace the duplicated implementation in the return migration maps command with reuse of the existing forward map script, configuring the return-leg values through the ref’s environment. Preserve the return-specific artifact output behavior by either invoking the shared ref with the appropriate redirect setting or introducing an environment-controlled redirect mode such as MTV_MAP_STATUS_REDIRECT, so only one implementation remains.ci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.sh (2)
21-30: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffVerify remote script execution before piping to
bash.Static analysis flags
curl -Ls https://get.submariner.io | bashas downloading and executing code without an integrity check (CWE-494). This mirrors the existingInstallSubctlpattern already used inacm-interop-p2p-submariner-broker-join-commands.sh, so it is a repository-established pattern rather than new risk introduced by this file. Consider downloading to a file, verifying a checksum or signature, and then executing the verified local copy — for this and the existing sibling implementation.🤖 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/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.sh` around lines 21 - 30, Update InstallSubctl in both the shown command script and its sibling broker-join implementation to download the installer to a local file, verify its checksum or signature against a trusted value, and execute it only after successful verification; preserve the existing subctl installation and early-return behavior.Source: Linters/SAST tools
188-203: 🧹 Nitpick | 🔵 TrivialO(n²) firewall diagnostics scale with spoke count.
The nested loop runs
DiagnoseFirewallInterClustertwice per spoke pair (both directions), so total invocations grow quadratically withspokeCount. This is fine for the default of 2 spokes, but if this bidirectional-migration cohort later supports more spokes, the fixed 30-minutetimeoutin the ref.yaml may become insufficient.🤖 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/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.sh` around lines 188 - 203, The nested loop structure with dual `DiagnoseFirewallInterCluster` calls per spoke pair creates quadratic scaling in execution time relative to spokeCount, but the fixed 30-minute timeout does not account for this growth. Either optimize the firewall diagnostic loop to reduce the number of calls (such as eliminating redundant bidirectional checks if they are not strictly necessary), or coordinate with the corresponding timeout configuration in ref.yaml to ensure the timeout scales appropriately if spokeCount increases beyond the current default of 2 spokes.
🤖 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/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.sh`:
- Around line 136-162: Update GatherSubmariner to write both the gather log and
tarball under "${diagDir}/gather/" so the output matches the documented artifact
contract and uses the directory created by the existing mkdir -p. Keep the
gather naming and archival behavior unchanged, and ensure the gather
subdirectory exists before writing.
- Around line 6-9: Enforce the diagnostic-only exit-0 contract across the
script: guard all failure-prone commands in InstallSubctl, LoadSpokeConfig, the
main precondition checks, and the GatherSubmariner tar operation so their
failures skip or end diagnostics without propagating a nonzero status. Preserve
the existing subctl diagnostic behavior while ensuring the outer script always
exits successfully.
- Line 9: Remove global xtrace from the script’s startup flags and enable it
only for non-sensitive commands that require tracing. Around
BuildMergedKubeconfig and LoadSpokeConfig, explicitly disable tracing with a
clear comment, keeping that scope minimal; preserve errexit, nounset, pipefail,
inherit_errexit, and use SHARED_DIR for cross-step data without logging
kubeconfig contents.
- Around line 127-130: Update the subctl firewall diagnostic invocation to
replace the unsupported --tocontext option with --remotecontext, while
preserving the existing originating --context value and all other command
behavior.
In
`@ci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.sh`:
- Around line 624-628: Rename the remaining forward-leg reporting identifiers in
p2p-mtv-execute-live-migration-return-commands.sh: update the testsuite name and
testcase classname near lines 624-628 to cclm-live-migration-return, change the
diagnostics path near lines 586-587 to mtv-live-migration-return-diagnostics/,
and update the debug warning step name near lines 692-698 to
p2p-mtv-execute-live-migration-return.
- Around line 692-698: Update the debug-mode warning in the cclmStepRc failure
branch to identify the return step as p2p-mtv-execute-live-migration-return,
while preserving the existing return-code and debug-mode behavior.
- Around line 586-587: Update the failure message in the command block around
the JUnit printf to reference the return-step diagnostics directory created at
line 79, mtv-live-migration-return-diagnostics/, instead of the forward-step
mtv-live-migration-diagnostics/ path.
- Around line 686-693: Move the ResolveSpokeKubeconfigs and targetNs assignment
from inside the subshell to the parent-shell setup before the subshell starts,
ensuring sourceKubeconfig and destKubeconfig contain resolved values even when
environment overrides are unset. Keep the existing subshell execution and
OnError diagnostics flow unchanged, and ensure the parent DumpDiagnostics uses
this shared state without overwriting valid artifacts.
---
Nitpick comments:
In
`@ci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.sh`:
- Around line 21-30: Update InstallSubctl in both the shown command script and
its sibling broker-join implementation to download the installer to a local
file, verify its checksum or signature against a trusted value, and execute it
only after successful verification; preserve the existing subctl installation
and early-return behavior.
- Around line 188-203: The nested loop structure with dual
`DiagnoseFirewallInterCluster` calls per spoke pair creates quadratic scaling in
execution time relative to spokeCount, but the fixed 30-minute timeout does not
account for this growth. Either optimize the firewall diagnostic loop to reduce
the number of calls (such as eliminating redundant bidirectional checks if they
are not strictly necessary), or coordinate with the corresponding timeout
configuration in ref.yaml to ensure the timeout scales appropriately if
spokeCount increases beyond the current default of 2 spokes.
In
`@ci-operator/step-registry/p2p/mtv-create-return-migration-maps/p2p-mtv-create-return-migration-maps-commands.sh`:
- Around line 22-127: Replace the duplicated implementation in the return
migration maps command with reuse of the existing forward map script,
configuring the return-leg values through the ref’s environment. Preserve the
return-specific artifact output behavior by either invoking the shared ref with
the appropriate redirect setting or introducing an environment-controlled
redirect mode such as MTV_MAP_STATUS_REDIRECT, so only one implementation
remains.
🪄 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: Pro Plus
Run ID: 5b722c92-693b-429e-805a-bbb90be260d7
⛔ Files ignored due to path filters (2)
ci-operator/jobs/RedHatQE/interop-testing/RedHatQE-interop-testing-master-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/RedHatQE/interop-testing/RedHatQE-interop-testing-master-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (19)
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.22-p2p-bidir.yamlci-operator/step-registry/acm/interop-p2p/submariner-diag/OWNERSci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.shci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-ref.metadata.jsonci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-ref.yamlci-operator/step-registry/p2p/cclm-bidir-migration-test/OWNERSci-operator/step-registry/p2p/cclm-bidir-migration-test/p2p-cclm-bidir-migration-test-chain.metadata.jsonci-operator/step-registry/p2p/cclm-bidir-migration-test/p2p-cclm-bidir-migration-test-chain.yamlci-operator/step-registry/p2p/mtv-bidir-mig-config/OWNERSci-operator/step-registry/p2p/mtv-bidir-mig-config/p2p-mtv-bidir-mig-config-chain.metadata.jsonci-operator/step-registry/p2p/mtv-bidir-mig-config/p2p-mtv-bidir-mig-config-chain.yamlci-operator/step-registry/p2p/mtv-create-return-migration-maps/OWNERSci-operator/step-registry/p2p/mtv-create-return-migration-maps/p2p-mtv-create-return-migration-maps-commands.shci-operator/step-registry/p2p/mtv-create-return-migration-maps/p2p-mtv-create-return-migration-maps-ref.metadata.jsonci-operator/step-registry/p2p/mtv-create-return-migration-maps/p2p-mtv-create-return-migration-maps-ref.yamlci-operator/step-registry/p2p/mtv-execute-live-migration-return/OWNERSci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.shci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-ref.metadata.jsonci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-ref.yaml
| # GatherSubmariner — subctl gather for deep diagnostics; archives tarball to diagDir. | ||
| GatherSubmariner() { | ||
| typeset kc="${1:?}"; (($#)) && shift | ||
| typeset name="${1:?}"; (($#)) && shift | ||
|
|
||
| typeset gatherWorkDir | ||
| gatherWorkDir="$(mktemp -d /tmp/subctl-gather-XXXXXX)" | ||
|
|
||
| ( | ||
| cd "${gatherWorkDir}" | ||
| KUBECONFIG="${kc}" "${subctlBin}" gather > "${diagDir}/gather-${name}.log" 2>&1 || true | ||
|
|
||
| # Archive whatever subctl gather produced | ||
| typeset tarball="" | ||
| typeset gatherDir="" | ||
| for d in submariner-*/; do | ||
| [[ -d "${d}" ]] && gatherDir="${d}" && break | ||
| done | ||
|
|
||
| if [[ -n "${gatherDir}" ]]; then | ||
| tar czf "${diagDir}/gather-${name}.tar.gz" "${gatherDir}" | ||
| rm -rf "${gatherDir}" | ||
| fi | ||
| ) | ||
| rm -rf "${gatherWorkDir}" | ||
| true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Gather output path does not match the documented contract.
GatherSubmariner writes gather-${name}.log and gather-${name}.tar.gz directly under ${diagDir} (Lines 146 and 156), but acm-interop-p2p-submariner-diag-ref.yaml documents that subctl gather output is archived under ${ARTIFACT_DIR}/submariner-diag/gather/-style path (see ref.yaml Lines 28-29). The mkdir -p "${diagDir}/gather" on Line 172 creates that subdirectory, but it is never used — no code writes into it.
Either move the gather output into ${diagDir}/gather/ to match the documented contract, or update the ref.yaml documentation and drop the unused mkdir.
Also applies to: 172-172
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 149-149: tarball appears unused. Verify use (or export if used externally).
(SC2034)
🤖 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/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.sh`
around lines 136 - 162, Update GatherSubmariner to write both the gather log and
tarball under "${diagDir}/gather/" so the output matches the documented artifact
contract and uses the directory created by the existing mkdir -p. Keep the
gather naming and archival behavior unchanged, and ensure the gather
subdirectory exists before writing.
| ) || cclmStepRc=$? | ||
|
|
||
| # Always write JUnit XML — both on success and on failure, so Prow/Firewatch | ||
| # always has a report regardless of which step caused the subshell to exit. | ||
| WriteJunit | ||
|
|
||
| if (( cclmStepRc != 0 )); then | ||
| DumpDiagnostics |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The parent DumpDiagnostics call overwrites good diagnostics with empty output.
ResolveSpokeKubeconfigs and the targetNs assignment run at lines 650-651, inside the subshell. Those assignments do not reach the parent shell. In the parent, sourceKubeconfig and destKubeconfig still hold the values from lines 33-34, which are empty when the ref leaves MTV_SOURCE_SPOKE_KUBECONFIG and MTV_DEST_SPOKE_KUBECONFIG unset.
OnError already writes complete diagnostics inside the subshell. Line 693 then calls DumpDiagnostics again and truncates each file with >. Every SourceOc and DestOc command runs as oc --kubeconfig="", fails, and || true hides the failure. The spoke VM, VMI, VMIM, storage, and virt-controller artifacts are replaced with error text on exactly the failure path that needs them.
Resolve the kubeconfigs and targetNs in the parent shell before the subshell starts.
🐛 Proposed fix: resolve shared state before the subshell
typeset -i cclmStepRc=0
+
+# Resolve before the subshell so the parent-level DumpDiagnostics at the end
+# sees the same kubeconfigs and target namespace.
+ResolveSpokeKubeconfigs
+targetNs="${targetNs:-${MTV_TEST_VM_NAMESPACE}}"
+
(
trap OnError ERR
- ResolveSpokeKubeconfigs
- targetNs="${targetNs:-${MTV_TEST_VM_NAMESPACE}}"
-
[[ "${MTV_PLAN_TYPE}" == "live" || "${MTV_PLAN_TYPE}" == "cold" ]]📝 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.
| ) || cclmStepRc=$? | |
| # Always write JUnit XML — both on success and on failure, so Prow/Firewatch | |
| # always has a report regardless of which step caused the subshell to exit. | |
| WriteJunit | |
| if (( cclmStepRc != 0 )); then | |
| DumpDiagnostics | |
| typeset -i cclmStepRc=0 | |
| # Resolve before the subshell so the parent-level DumpDiagnostics at the end | |
| # sees the same kubeconfigs and target namespace. | |
| ResolveSpokeKubeconfigs | |
| targetNs="${targetNs:-${MTV_TEST_VM_NAMESPACE}}" | |
| ( | |
| trap OnError ERR | |
| [[ "${MTV_PLAN_TYPE}" == "live" || "${MTV_PLAN_TYPE}" == "cold" ]] |
🤖 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/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.sh`
around lines 686 - 693, Move the ResolveSpokeKubeconfigs and targetNs assignment
from inside the subshell to the parent-shell setup before the subshell starts,
ensuring sourceKubeconfig and destKubeconfig contain resolved values even when
environment overrides are unset. Keep the existing subshell execution and
OnError diagnostics flow unchanged, and ensure the parent DumpDiagnostics uses
this shared state without overwriting valid artifacts.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-commands.sh`:
- Around line 76-84: Update the rollout checks for
daemonsets/submariner-routeagent and daemonsets/ovnkube-node to distinguish a
timeout from other non-zero oc rollout status failures. Capture and inspect the
command’s exit status or verify each DaemonSet exists first; continue only for
an actual timeout, while propagating or explicitly reporting missing resources,
RBAC errors, and API failures.
In
`@ci-operator/step-registry/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-ref.yaml`:
- Line 9: The step timeout in acm-interop-p2p-submariner-snat-fix-ref.yaml is
fixed at 40 minutes and does not account for sequential processing of multiple
spokes. Update the ACM_SPOKE_CLUSTER_COUNT configuration documentation to state
the supported maximum or constraint implied by this timeout, or adjust timeout
sizing to cover the maximum expected spoke count while preserving per-spoke
failure reporting.
🪄 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: Pro Plus
Run ID: cf89ac54-109e-4df7-87d3-7762a335cf16
📒 Files selected for processing (5)
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.22-p2p-bidir.yamlci-operator/step-registry/acm/interop-p2p/submariner-snat-fix/OWNERSci-operator/step-registry/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-commands.shci-operator/step-registry/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-ref.metadata.jsonci-operator/step-registry/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.22-p2p-bidir.yaml
| KUBECONFIG="${kubeconfig}" oc rollout status daemonset/submariner-routeagent \ | ||
| -n submariner-operator \ | ||
| --timeout=5m || \ | ||
| : "routeagent rollout on '${spokeName}': timed out — continuing" | ||
|
|
||
| KUBECONFIG="${kubeconfig}" oc rollout status daemonset/ovnkube-node \ | ||
| -n openshift-ovn-kubernetes \ | ||
| --timeout=10m || \ | ||
| : "ovnkube-node rollout on '${spokeName}': timed out — continuing" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Distinguish real rollout failures from timeouts.
The || : fallback runs on any non-zero exit of oc rollout status, not only on timeout. A missing DaemonSet, an RBAC error, or an API server error is all logged as "timed out — continuing." The workaround can silently fail to apply, and the job proceeds into migration steps with an unfixed environment, causing a confusing downstream failure.
Capture the actual exit reason, or verify the DaemonSet exists first, so genuine failures are distinguishable from a timeout.
🔧 Proposed fix to distinguish timeout from genuine failure
KUBECONFIG="${kubeconfig}" oc rollout status daemonset/submariner-routeagent \
-n submariner-operator \
- --timeout=5m || \
- : "routeagent rollout on '${spokeName}': timed out — continuing"
+ --timeout=5m; typeset -i rcRoute=$?
+ if ((rcRoute != 0)); then
+ KUBECONFIG="${kubeconfig}" oc get daemonset/submariner-routeagent -n submariner-operator \
+ || : "routeagent rollout on '${spokeName}': daemonset missing — continuing"
+ : "routeagent rollout on '${spokeName}': non-zero exit ${rcRoute} — continuing"
+ fi🤖 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/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-commands.sh`
around lines 76 - 84, Update the rollout checks for
daemonsets/submariner-routeagent and daemonsets/ovnkube-node to distinguish a
timeout from other non-zero oc rollout status failures. Capture and inspect the
command’s exit status or verify each DaemonSet exists first; continue only for
an actual timeout, while propagating or explicitly reporting missing resources,
RBAC errors, and API failures.
| requests: | ||
| cpu: 100m | ||
| memory: 250Mi | ||
| timeout: 40m0s |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Step timeout does not scale with ACM_SPOKE_CLUSTER_COUNT.
Each spoke can consume up to roughly 15.5 minutes in the worst case (5m + 10m rollout timeouts + 30s settle) in acm-interop-p2p-submariner-snat-fix-commands.sh, applied sequentially per spoke. With the default ACM_SPOKE_CLUSTER_COUNT of "2," the worst case stays under the 40m step timeout. With 3 or more spokes, the worst case exceeds 40m and the step itself times out before ApplySnatFix can report per-spoke failures.
Document this constraint in ACM_SPOKE_CLUSTER_COUNT, or size timeout relative to the maximum expected spoke count.
Also applies to: 12-13
🤖 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/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-ref.yaml`
at line 9, The step timeout in acm-interop-p2p-submariner-snat-fix-ref.yaml is
fixed at 40 minutes and does not account for sequential processing of multiple
spokes. Update the ACM_SPOKE_CLUSTER_COUNT configuration documentation to state
the supported maximum or constraint implied by this timeout, or adjust timeout
sizing to cover the maximum expected spoke count while preserving per-spoke
failure reporting.
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: 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: 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/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.sh`:
- Around line 43-45: Replace the non-emitting `:` diagnostics in
acm-interop-p2p-submariner-diag-commands.sh at lines 43-45 and 175-196 with
`printf '%s\n' ... >&2`, preserving each warning and skip message content so
missing spoke files and setup failures are visibly reported on stderr.
- Around line 23-31: Update InstallSubctl to remove the remote installer pipe
and obtain subctl from a pinned Submariner release artifact. Verify the artifact
checksum or signature before extracting it, then install the verified binary at
subctlBin while preserving the existing non-fatal return-status behavior.
In
`@ci-operator/step-registry/p2p/mtv-execute-live-migration/p2p-mtv-execute-live-migration-commands.sh`:
- Around line 325-328: Update GetSourceVirtLauncherPod() so both the label query
and fallback query propagate SourceOc failures and jq parse errors instead of
masking them with “|| true”. Preserve successful empty-list handling by allowing
jq’s “// empty” result to produce an empty pod name without failing.
🪄 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: Pro Plus
Run ID: bdf0978f-bb3d-48ff-9ad0-9fbb387b00b9
📒 Files selected for processing (5)
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.22-p2p-bidir.yamlci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.shci-operator/step-registry/acm/interop-p2p/submariner-install/acm-interop-p2p-submariner-install-chain.yamlci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.shci-operator/step-registry/p2p/mtv-execute-live-migration/p2p-mtv-execute-live-migration-commands.sh
💤 Files with no reviewable changes (1)
- ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.22-p2p-bidir.yaml
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: 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/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.sh`:
- Around line 329-336: Update both pod-name jq queries in
GetSourceVirtLauncherPod to filter for pods whose status.phase is Running and
metadata.deletionTimestamp is null before selecting the first name. Preserve the
existing label-based primary lookup and virt-launcher name-prefix fallback,
ensuring ProbeCclmSyncPortFromPod receives only a healthy, non-terminating pod.
🪄 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: Pro Plus
Run ID: 91a1ef4e-a96e-488f-83c9-a58e9d0c81f8
📒 Files selected for processing (7)
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.22-p2p-bidir.yamlci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-commands.shci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-ref.yamlci-operator/step-registry/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-commands.shci-operator/step-registry/acm/interop-p2p/submariner-snat-fix/acm-interop-p2p-submariner-snat-fix-ref.yamlci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.shci-operator/step-registry/p2p/mtv-execute-live-migration/p2p-mtv-execute-live-migration-commands.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- ci-operator/step-registry/acm/interop-p2p/submariner-diag/acm-interop-p2p-submariner-diag-ref.yaml
- ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.22-p2p-bidir.yaml
- ci-operator/step-registry/p2p/mtv-execute-live-migration/p2p-mtv-execute-live-migration-commands.sh
| podName="$(SourceOc get pods -n "${MTV_TEST_VM_NAMESPACE}" \ | ||
| -l "kubevirt.io=virt-launcher,kubevirt.io/domain=${MTV_TEST_VM_NAME}" \ | ||
| -o json | jq -r 'first(.items[].metadata.name) // empty')" | ||
| [[ -n "${podName}" ]] && printf '%s' "${podName}" && return 0 | ||
|
|
||
| podName="$(SourceOc get pods -n "${MTV_TEST_VM_NAMESPACE}" -o json \ | ||
| | jq -r --arg name "${MTV_TEST_VM_NAME}" \ | ||
| '[.items[].metadata.name | select(startswith("virt-launcher-" + $name))] | first // ""')" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)p2p-mtv-execute-live-migration-return-commands\.sh$' || true
echo "== relevant lines =="
sed -n '300,355p' ci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.sh
echo "== usages around GetSourceVirtLauncherPod / podName =="
rg -n 'GetSourceVirtLauncherPod|virt-launcher|PodIsRunning|Connectivity|PodName|podName|deletionTimestamp|status\.phase' ci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.shRepository: openshift/release
Length of output: 4069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PreflightCclmSyncConnectivity implementation =="
sed -n '321,390p' ci-operator/step-registry/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.sh
echo "== Read-only jq behavior for duplicate virt-launcher pods with statuses/deletionTimestamp =="
python3 - <<'PY'
import json, subprocess, textwrap
samples = {
"ordered by api": {
"items": [
{"metadata": {"name": "virt-launcher-vm1-waiting"}, "status": {"phase": "Pending"}},
{"metadata": {"name": "virt-launcher-vm1-running"}, "status": {"phase": "Running"}},
]
},
"named query current": {
"items": [
{"metadata": {"name": "virt-launcher-vm1-waiting"}, "status": {"phase": "Pending"}},
{"metadata": {"name": "virt-launcher-vm1-running"}, "status": {"phase": "Running"}},
]
},
"terminating then running": {
"items": [
{"metadata": {"name": "virt-launcher-vm1-terminating", "deletionTimestamp": "2026-08-01T00:00:00Z"}, "status": {"phase": "Running"}},
{"metadata": {"name": "virt-launcher-vm1-running"}, "status": {"phase": "Running"}},
]
},
}
# These exactly echo the jq expressions in GetSourceVirtLauncherPod.
queries = {
"labeled current": "first(.items[].metadata.name) // empty",
"named current": '[.items[].metadata.name | select(startswith("virt-launcher-" + name))] | first // "",',
}
jq = subprocess.run(["jq", "--version"], text=True, capture_output=True)
print("jq_version", jq.stdout.strip())
for title, data in samples.items():
for qtitle, expr in queries.items():
if title == "named query current":
input_data = json.dumps({"items": data["items"], "name": "mtv-vm1"})
else:
input_data = json.dumps(data)
r = subprocess.run(["jq", "-r", expr], text=True, stdin=input_data, capture_output=True)
print(f"## {title} / {qtitle}")
print(f"in_status_phases={','.join(i['status']['phase'] for i in data['items'])}")
print(f"deletionTimestamps={','.join(i.get('metadata',{}).get('deletionTimestamp') or '' for i in data['items'])}")
print(f"return={r.stdout.strip()}")
print(f"stderr={r.stderr.strip() or '(none)'}")
print(f"code={r.returncode}")
PYRepository: openshift/release
Length of output: 3695
Select a running, non-terminating virt-launcher pod.
GetSourceVirtLauncherPod() selects the first matching pod by JSON array order instead of filtering for a healthy pod. If a Pending or terminating replacement pod exists alongside a Running virt-launcher pod, the function can pass the wrong pod name to ProbeCclmSyncPortFromPod().
Both pod-name queries should require status.phase == "Running" and metadata.deletionTimestamp == null.
🤖 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/p2p/mtv-execute-live-migration-return/p2p-mtv-execute-live-migration-return-commands.sh`
around lines 329 - 336, Update both pod-name jq queries in
GetSourceVirtLauncherPod to filter for pods whose status.phase is Running and
metadata.deletionTimestamp is null before selecting the first name. Preserve the
existing label-based primary lookup and virt-launcher name-prefix fallback,
ensuring ProbeCclmSyncPortFromPod receives only a healthy, non-terminating pod.
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: 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/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-commands.sh`:
- Around line 390-393: Update the timeout invocation for subctl verify in the
SUBMARINER_VERIFY_DEBUG_MODE flow to include a short forced-kill grace period
using timeout’s --kill-after option. Keep the existing 35-minute limit and
command behavior unchanged, ensuring unresponsive processes are terminated
before the step timeout.
🪄 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: Pro Plus
Run ID: 2370d7bb-db3d-40cf-8862-393a9619712d
📒 Files selected for processing (1)
ci-operator/step-registry/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-commands.sh
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@shakyav: 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. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.22-p2p-bidir-cclm-bidir-mig-lp-interop-aws |
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
1 participant
Summary by CodeRabbit
subctl verify.