[WIP] hub spoke bi-directional vm migration#82106
Draft
shakyav wants to merge 7 commits into
Draft
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
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.23 scheduled ACM hub-spoke CCLM interop workflow covering ACM-managed Submariner, hub CNV and VM provisioning, bidirectional MTV maps and migrations, verification, diagnostics, and CI registration. ChangesHub-Spoke CCLM Interop
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Prow
participant ACM
participant Hub
participant Spoke
participant MTV
Prow->>ACM: Install Submariner and hub CNV
ACM->>Hub: Configure hub services
Prow->>Hub: Create hub VMs
Prow->>Spoke: Create spoke VMs
Prow->>MTV: Create bidirectional maps
MTV-->>Prow: Report maps Ready
Prow->>MTV: Execute hub-to-spoke migration
MTV->>Spoke: Validate destination VMIs
Prow->>MTV: Execute spoke-to-hub migration
MTV->>Hub: Validate destination VMIs
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Contributor
|
[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
Contributor
Author
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.23-p2p-hub-spoke-cclm-hub-spoke-aws-p2p-lp-interop |
Contributor
|
@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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 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/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.23-p2p-hub-spoke.yaml`:
- Line 87: Update FIREWATCH_DEFAULT_JIRA_ADDITIONAL_LABELS for the
acm-virt-ocp4.23-p2p-hub-spoke job to replace the incorrect 4.22-p2p label with
the corresponding 4.23-p2p label, preserving all other labels unchanged.
In
`@ci-operator/step-registry/acm/interop-p2p/submariner-addon/acm-interop-p2p-submariner-addon-commands.sh`:
- Around line 27-31: Replace the mutable remote eval in the EnsureReqs setup
with a trusted, immutable helper source: pin EnsureReqs.sh to a specific commit
or use a packaged copy in the test image, and verify its integrity before
execution. Preserve the subsequent EnsureReqs jq invocation while ensuring no
unverified network response is passed directly to eval.
- Around line 208-215: Require the Globalnet-state read in the Submariner addon
assertion to succeed and accept only an exact false value; remove the suppressed
failure path and do not treat an empty result as disabled. In
ci-operator/step-registry/acm/interop-p2p/submariner-addon/acm-interop-p2p-submariner-addon-commands.sh
lines 208-215, update the globalnetEnabled check accordingly. In
ci-operator/step-registry/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-commands.sh
lines 283-305, propagate any oc or jq failure before evaluating the returned
subnet list.
In
`@ci-operator/step-registry/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-commands.sh`:
- Around line 507-517: Update the hub-to-spoke loop invoking VerifyCclmSyncPath
so each spoke uses a unique probe namespace, or ensure VerifyCclmSyncPath waits
for submariner-cclm-probe deletion to complete before returning. Preserve the
existing per-spoke verification behavior while preventing subsequent iterations
from reusing a namespace still terminating.
In
`@ci-operator/step-registry/p2p/create-hub-spoke-test-vms/p2p-create-hub-spoke-test-vms-commands.sh`:
- Around line 112-115: Update the PVC selection expression in the command
substitution feeding the read loop to avoid the malformed regex and select PVCs
structurally from metadata: include the PVC whose name matches dvName and
associated CDI prime PVCs. Remove the unmatched-parenthesis pattern so jq
succeeds and the cleanup loop does not silently retain stale PVCs.
- Around line 219-227: Export VM_NAME before each yq invocation and replace
every strenv(vmName) reference with strenv(VM_NAME) so VM names and labels are
populated. Apply this in
ci-operator/step-registry/p2p/create-hub-spoke-test-vms/p2p-create-hub-spoke-test-vms-commands.sh
at lines 219-227 and 285-296 for both VM definitions, and correct the CirrOS and
RHEL DataVolume labels at lines 129-135 and 175-184 respectively.
In
`@ci-operator/step-registry/p2p/install-cnv-hub/p2p-install-cnv-hub-commands.sh`:
- Around line 23-27: Remove the remote eval-based EnsureReqs loading in both
p2p-install-cnv-hub-commands.sh lines 23-27 and
p2p-create-hub-spoke-test-vms-commands.sh lines 17-21. Replace it with a
checked-in helper or a checksum-verified download, while preserving the
EnsureReqs jq setup without executing unpinned remote code.
- Line 21: Remove the global -x tracing option from the shell setup in
ci-operator/step-registry/p2p/install-cnv-hub/p2p-install-cnv-hub-commands.sh:21
and
ci-operator/step-registry/p2p/create-hub-spoke-test-vms/p2p-create-hub-spoke-test-vms-commands.sh:15,
leaving set -euo pipefail and inherit_errexit behavior intact; preserve the
existing narrow password-protection scope in the VM script.
In
`@ci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-commands.sh`:
- Around line 19-30: Update both
ci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-commands.sh
lines 19-30 and
ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh
lines 21-32 to default to set -euo pipefail, source proxy-conf.sh before the
EnsureReqs network bootstrap, and keep tracing disabled while loading proxy
credentials; enable tracing afterward only if required.
In
`@ci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-ref.yaml`:
- Line 5: Increase the step timeout in the p2p-mtv-create-hub-spoke-maps ref so
it covers the sequential readiness waits for two providers and four maps,
allowing up to 90 minutes instead of terminating after 30 minutes.
In
`@ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh`:
- Around line 216-224: Update PreflightSubmarinerNoGlobalnet to distinguish a
missing submariner-globalnet daemonset from other oc/API failures: capture the
oc get result, return success only when the resource is explicitly NotFound, and
propagate or fail on authorization, connectivity, and other API errors. Preserve
the existing kubeconfig and namespace checks.
- Around line 356-381: Update GetVmRootDiskStorageClass so it never falls back
to an unrelated PVC’s storage class. Remove the namespace-wide
first(.items[].spec.storageClassName) fallback and return an empty result when
the PVC identified by pvcName cannot be resolved.
- Around line 157-163: The KubeVirt patch in the migration command must not
replace HCO-managed feature gates. Remove the direct mutation of the managed
KubeVirt CR, and instead validate that HCO has propagated
DecentralizedLiveMigration; if it has not, fail the flow before or within
WaitForDecentralizedLiveMigrationGate rather than modifying the featureGates
array.
- Around line 626-638: Update the migration subshell invoking JStep so its exit
status is captured directly in cclmStepRc, preserving the first failed preflight
or migration step. Remove the trailing true command that masks earlier failures,
while keeping the existing JStep execution and diagnostics behavior unchanged.
In
`@ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-ref.yaml`:
- Line 5: Increase the outer timeout in
ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-ref.yaml
at line 5 to approximately 8 hours, covering two two-hour migrations and
sequential preflights. Also increase the timeout in
ci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-ref.yaml
at line 5 to approximately 2 hours, covering six sequential 15-minute readiness
waits.
🪄 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: d9ff1eb6-c625-4fd2-88c3-f01a357df843
⛔ 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 (30)
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.23-p2p-hub-spoke.yamlci-operator/step-registry/acm/interop-p2p/submariner-addon/OWNERSci-operator/step-registry/acm/interop-p2p/submariner-addon/acm-interop-p2p-submariner-addon-commands.shci-operator/step-registry/acm/interop-p2p/submariner-addon/acm-interop-p2p-submariner-addon-ref.metadata.jsonci-operator/step-registry/acm/interop-p2p/submariner-addon/acm-interop-p2p-submariner-addon-ref.yamlci-operator/step-registry/acm/interop-p2p/submariner-install/acm-interop-p2p-submariner-install-chain.yamlci-operator/step-registry/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-commands.shci-operator/step-registry/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-ref.yamlci-operator/step-registry/p2p/cclm-hub-spoke-migration-test/OWNERSci-operator/step-registry/p2p/cclm-hub-spoke-migration-test/p2p-cclm-hub-spoke-migration-test-chain.metadata.jsonci-operator/step-registry/p2p/cclm-hub-spoke-migration-test/p2p-cclm-hub-spoke-migration-test-chain.yamlci-operator/step-registry/p2p/cclm-hub-spoke/OWNERSci-operator/step-registry/p2p/cclm-hub-spoke/p2p-cclm-hub-spoke-workflow.metadata.jsonci-operator/step-registry/p2p/cclm-hub-spoke/p2p-cclm-hub-spoke-workflow.yamlci-operator/step-registry/p2p/create-hub-spoke-test-vms/OWNERSci-operator/step-registry/p2p/create-hub-spoke-test-vms/p2p-create-hub-spoke-test-vms-commands.shci-operator/step-registry/p2p/create-hub-spoke-test-vms/p2p-create-hub-spoke-test-vms-ref.metadata.jsonci-operator/step-registry/p2p/create-hub-spoke-test-vms/p2p-create-hub-spoke-test-vms-ref.yamlci-operator/step-registry/p2p/install-cnv-hub/OWNERSci-operator/step-registry/p2p/install-cnv-hub/p2p-install-cnv-hub-commands.shci-operator/step-registry/p2p/install-cnv-hub/p2p-install-cnv-hub-ref.metadata.jsonci-operator/step-registry/p2p/install-cnv-hub/p2p-install-cnv-hub-ref.yamlci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/OWNERSci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-commands.shci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-ref.metadata.jsonci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-ref.yamlci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/OWNERSci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.shci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-ref.metadata.jsonci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-ref.yaml
Comment on lines
+27
to
+31
| eval "$( | ||
| typeset -a _fURL=() | ||
| type -t wget 1>/dev/null && _fURL=(wget -nv -O-) || _fURL=(curl -fsSL) | ||
| "${_fURL[@]}" https://raw.githubusercontent.com/RedHatQE/OpenShift-LP-QE--Tools/refs/heads/main/libs/bash/common/EnsureReqs.sh | ||
| )"; EnsureReqs jq |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not execute mutable remote code with cluster credentials.
This downloads EnsureReqs.sh from main and passes its contents directly to eval. A compromised upstream branch or transport response gains arbitrary code execution in a cluster-admin CI step. Pin and verify an immutable revision, or package the helper in the test image.
🤖 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-addon/acm-interop-p2p-submariner-addon-commands.sh`
around lines 27 - 31, Replace the mutable remote eval in the EnsureReqs setup
with a trusted, immutable helper source: pin EnsureReqs.sh to a specific commit
or use a packaged copy in the test image, and verify its integrity before
execution. Preserve the subsequent EnsureReqs jq invocation while ensuring no
unverified network response is passed directly to eval.
Source: Linters/SAST tools
Comment on lines
+208
to
+215
| typeset globalnetEnabled | ||
| globalnetEnabled="$(oc get submarinerconfig/submariner -n "${ns}" \ | ||
| -o jsonpath='{.spec.globalnetEnabled}' || true)" | ||
| if [[ "${globalnetEnabled}" == "true" ]]; then | ||
| : "ERROR: SubmarinerConfig globalnetEnabled=true on '${clusterName}' — incompatible with CCLM" >&2 | ||
| false | ||
| fi | ||
| : "Globalnet disabled on '${clusterName}' (globalnetEnabled=${globalnetEnabled:-false})" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate failed Globalnet-state queries instead of reporting success. Both assertions suppress read errors and interpret the resulting empty value as proof that Globalnet is disabled.
ci-operator/step-registry/acm/interop-p2p/submariner-addon/acm-interop-p2p-submariner-addon-commands.sh#L208-L215: require a successful read whose value is exactlyfalse.ci-operator/step-registry/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-commands.sh#L283-L305: propagateoc/jqfailures before checking the returned subnet list.
📍 Affects 2 files
ci-operator/step-registry/acm/interop-p2p/submariner-addon/acm-interop-p2p-submariner-addon-commands.sh#L208-L215(this comment)ci-operator/step-registry/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-commands.sh#L283-L305
🤖 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-addon/acm-interop-p2p-submariner-addon-commands.sh`
around lines 208 - 215, Require the Globalnet-state read in the Submariner addon
assertion to succeed and accept only an exact false value; remove the suppressed
failure path and do not treat an empty result as disabled. In
ci-operator/step-registry/acm/interop-p2p/submariner-addon/acm-interop-p2p-submariner-addon-commands.sh
lines 208-215, update the globalnetEnabled check accordingly. In
ci-operator/step-registry/acm/interop-p2p/submariner-verify/acm-interop-p2p-submariner-verify-commands.sh
lines 283-305, propagate any oc or jq failure before evaluating the returned
subnet list.
Comment on lines
+507
to
+517
| # Hub↔spoke CCLM sync TCP probes — when ACM hub participates in Submariner mesh. | ||
| # Required for hub↔spoke CCLM migration. Set SUBMARINER_VERIFY_HUB_SPOKE=true | ||
| # when acm-interop-p2p-submariner-addon enrolled local-cluster. | ||
| if [[ "${verifyHubSpoke}" == "true" && -r "${KUBECONFIG}" ]]; then | ||
| for ((i = 0; i < spokeCount; i++)); do | ||
| VerifyCclmSyncPath \ | ||
| "${KUBECONFIG}" \ | ||
| "${spokeKubeconfigsArr[i]}" \ | ||
| "hub" \ | ||
| "${spokeNamesArr[i]}" | ||
| done |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid reusing an asynchronously deleted probe namespace across spokes.
Each VerifyCclmSyncPath deletes submariner-cclm-probe with --wait=false. With multiple spokes, the next loop iteration can attempt to recreate resources while the hub namespace is still Terminating, failing verification. Use a unique namespace per probe or wait for deletion before returning.
🤖 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-verify/acm-interop-p2p-submariner-verify-commands.sh`
around lines 507 - 517, Update the hub-to-spoke loop invoking VerifyCclmSyncPath
so each spoke uses a unique probe namespace, or ensure VerifyCclmSyncPath waits
for submariner-cclm-probe deletion to complete before returning. Preserve the
existing per-spoke verification behavior while preventing subsequent iterations
from reusing a namespace still terminating.
Comment on lines
+112
to
+115
| done < <(oc --kubeconfig="${kc}" get pvc -n "${ns}" -o json \ | ||
| | jq -r --arg dv "${dvName}" \ | ||
| '.items[].metadata.name | select(test("^(" + $dv + "|prime-))"))' \ | ||
| || true) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix the malformed PVC-selection expression.
The resulting regex contains an unmatched closing parenthesis, so jq fails and || true silently leaves stale PVCs behind. Select the intended DataVolume PVC and associated CDI prime PVCs using structured metadata rather than this regex.
🤖 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/create-hub-spoke-test-vms/p2p-create-hub-spoke-test-vms-commands.sh`
around lines 112 - 115, Update the PVC selection expression in the command
substitution feeding the read loop to avoid the malformed regex and select PVCs
structurally from metadata: include the PVC whose name matches dvName and
associated CDI prime PVCs. Remove the unmatched-parenthesis pattern so jq
succeeds and the cleanup loop does not silently retain stale PVCs.
Comment on lines
+157
to
+163
| oc --kubeconfig="${kc}" patch kubevirt "${MTV_HS_KUBEVIRT_NAME}" \ | ||
| -n "${MTV_HS_CNV_NAMESPACE}" \ | ||
| --type merge \ | ||
| -p '{"spec":{"configuration":{"developerConfiguration":{"featureGates":["DecentralizedLiveMigration"]}}}}' | ||
|
|
||
| WaitForDecentralizedLiveMigrationGate "${kc}" | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not replace HCO-managed KubeVirt feature gates.
The merge patch replaces the entire featureGates array with only DecentralizedLiveMigration, potentially disabling existing required gates. If HCO does not propagate its setting, fail instead of directly mutating its managed KubeVirt CR.
Proposed fix
- oc --kubeconfig="${kc}" patch kubevirt "${MTV_HS_KUBEVIRT_NAME}" \
- -n "${MTV_HS_CNV_NAMESPACE}" \
- --type merge \
- -p '{"spec":{"configuration":{"developerConfiguration":{"featureGates":["DecentralizedLiveMigration"]}}}}'
-
- WaitForDecentralizedLiveMigrationGate "${kc}"
+ printf 'HCO did not propagate DecentralizedLiveMigration on %s\n' \
+ "${clusterLabel}" >&2
+ return 1🤖 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-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh`
around lines 157 - 163, The KubeVirt patch in the migration command must not
replace HCO-managed feature gates. Remove the direct mutation of the managed
KubeVirt CR, and instead validate that HCO has propagated
DecentralizedLiveMigration; if it has not, fail the flow before or within
WaitForDecentralizedLiveMigrationGate rather than modifying the featureGates
array.
Comment on lines
+216
to
+224
| # PreflightSubmarinerNoGlobalnet — Globalnet breaks raw pod-IP sync routing. | ||
| PreflightSubmarinerNoGlobalnet() { | ||
| typeset kc="${1:?}" | ||
| typeset clusterLabel="${2:?}" | ||
|
|
||
| # Submariner Globalnet daemonset should NOT exist; ! inverts exit code. | ||
| ! oc --kubeconfig="${kc}" get daemonset submariner-globalnet \ | ||
| -n submariner-operator 1>/dev/null | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Distinguish “Globalnet absent” from API failures.
! oc get passes for authorization, connectivity, and API errors as well as NotFound, allowing the CCLM preflight to continue without verifying the required topology.
Proposed fix
PreflightSubmarinerNoGlobalnet() {
typeset kc="${1:?}"
typeset clusterLabel="${2:?}"
+ typeset globalnet
- # Submariner Globalnet daemonset should NOT exist; ! inverts exit code.
- ! oc --kubeconfig="${kc}" get daemonset submariner-globalnet \
- -n submariner-operator 1>/dev/null
+ globalnet="$(oc --kubeconfig="${kc}" get daemonset submariner-globalnet \
+ -n submariner-operator --ignore-not-found -o name)"
+ [[ -z "${globalnet}" ]] || {
+ printf 'Globalnet is enabled on %s\n' "${clusterLabel}" >&2
+ return 1
+ }
}📝 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.
Suggested change
| # PreflightSubmarinerNoGlobalnet — Globalnet breaks raw pod-IP sync routing. | |
| PreflightSubmarinerNoGlobalnet() { | |
| typeset kc="${1:?}" | |
| typeset clusterLabel="${2:?}" | |
| # Submariner Globalnet daemonset should NOT exist; ! inverts exit code. | |
| ! oc --kubeconfig="${kc}" get daemonset submariner-globalnet \ | |
| -n submariner-operator 1>/dev/null | |
| } | |
| # PreflightSubmarinerNoGlobalnet — Globalnet breaks raw pod-IP sync routing. | |
| PreflightSubmarinerNoGlobalnet() { | |
| typeset kc="${1:?}" | |
| typeset clusterLabel="${2:?}" | |
| typeset globalnet | |
| globalnet="$(oc --kubeconfig="${kc}" get daemonset submariner-globalnet \ | |
| -n submariner-operator --ignore-not-found -o name)" | |
| [[ -z "${globalnet}" ]] || { | |
| printf 'Globalnet is enabled on %s\n' "${clusterLabel}" >&2 | |
| return 1 | |
| } | |
| } |
🤖 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-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh`
around lines 216 - 224, Update PreflightSubmarinerNoGlobalnet to distinguish a
missing submariner-globalnet daemonset from other oc/API failures: capture the
oc get result, return success only when the resource is explicitly NotFound, and
propagate or fail on authorization, connectivity, and other API errors. Preserve
the existing kubeconfig and namespace checks.
Comment on lines
+356
to
+381
| # GetVmRootDiskStorageClass — resolve root disk StorageClass from first source VM. | ||
| GetVmRootDiskStorageClass() { | ||
| typeset kc="${1:?}" | ||
| typeset vmName="${2:?}" | ||
| typeset vmNs="${3:?}" | ||
| typeset dvName pvcName scName | ||
|
|
||
| dvName="$(oc --kubeconfig="${kc}" get "virtualmachine/${vmName}" -n "${vmNs}" -o json \ | ||
| | jq -r 'first(.spec.template.spec.volumes[]?.dataVolume.name // empty) // ""')" | ||
| [[ -z "${dvName}" ]] && dvName="${vmName}-rootdisk" | ||
|
|
||
| pvcName="$(oc --kubeconfig="${kc}" get "datavolume/${dvName}" -n "${vmNs}" \ | ||
| -o jsonpath='{.status.claimName}' || true)" | ||
| [[ -z "${pvcName}" ]] && pvcName="${dvName}" | ||
|
|
||
| scName="$(oc --kubeconfig="${kc}" get "persistentvolumeclaim/${pvcName}" -n "${vmNs}" \ | ||
| -o jsonpath='{.spec.storageClassName}' || true)" | ||
| [[ -n "${scName}" ]] && printf '%s' "${scName}" && return 0 | ||
|
|
||
| oc --kubeconfig="${kc}" get pvc -n "${vmNs}" -o json \ | ||
| | jq -r --arg pvc "${pvcName}" ' | ||
| (first(.items[] | select(.metadata.name == $pvc) | .spec.storageClassName) // | ||
| first(.items[].spec.storageClassName) // | ||
| "") // "" | ||
| ' | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not validate against an unrelated namespace PVC.
If the root-disk PVC cannot be resolved, the fallback returns the first storage class in the namespace. That can falsely pass the StorageMap preflight for a different VM.
Proposed fix
scName="$(oc --kubeconfig="${kc}" get "persistentvolumeclaim/${pvcName}" -n "${vmNs}" \
-o jsonpath='{.spec.storageClassName}' || true)"
[[ -n "${scName}" ]] && printf '%s' "${scName}" && return 0
- oc --kubeconfig="${kc}" get pvc -n "${vmNs}" -o json \
- | jq -r --arg pvc "${pvcName}" '
- (first(.items[] | select(.metadata.name == $pvc) | .spec.storageClassName) //
- first(.items[].spec.storageClassName) //
- "") // ""
- '
+ printf 'Unable to resolve root-disk StorageClass for %s/%s\n' \
+ "${vmNs}" "${vmName}" >&2
+ return 1
}📝 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.
Suggested change
| # GetVmRootDiskStorageClass — resolve root disk StorageClass from first source VM. | |
| GetVmRootDiskStorageClass() { | |
| typeset kc="${1:?}" | |
| typeset vmName="${2:?}" | |
| typeset vmNs="${3:?}" | |
| typeset dvName pvcName scName | |
| dvName="$(oc --kubeconfig="${kc}" get "virtualmachine/${vmName}" -n "${vmNs}" -o json \ | |
| | jq -r 'first(.spec.template.spec.volumes[]?.dataVolume.name // empty) // ""')" | |
| [[ -z "${dvName}" ]] && dvName="${vmName}-rootdisk" | |
| pvcName="$(oc --kubeconfig="${kc}" get "datavolume/${dvName}" -n "${vmNs}" \ | |
| -o jsonpath='{.status.claimName}' || true)" | |
| [[ -z "${pvcName}" ]] && pvcName="${dvName}" | |
| scName="$(oc --kubeconfig="${kc}" get "persistentvolumeclaim/${pvcName}" -n "${vmNs}" \ | |
| -o jsonpath='{.spec.storageClassName}' || true)" | |
| [[ -n "${scName}" ]] && printf '%s' "${scName}" && return 0 | |
| oc --kubeconfig="${kc}" get pvc -n "${vmNs}" -o json \ | |
| | jq -r --arg pvc "${pvcName}" ' | |
| (first(.items[] | select(.metadata.name == $pvc) | .spec.storageClassName) // | |
| first(.items[].spec.storageClassName) // | |
| "") // "" | |
| ' | |
| } | |
| # GetVmRootDiskStorageClass — resolve root disk StorageClass from first source VM. | |
| GetVmRootDiskStorageClass() { | |
| typeset kc="${1:?}" | |
| typeset vmName="${2:?}" | |
| typeset vmNs="${3:?}" | |
| typeset dvName pvcName scName | |
| dvName="$(oc --kubeconfig="${kc}" get "virtualmachine/${vmName}" -n "${vmNs}" -o json \ | |
| | jq -r 'first(.spec.template.spec.volumes[]?.dataVolume.name // empty) // ""')" | |
| [[ -z "${dvName}" ]] && dvName="${vmName}-rootdisk" | |
| pvcName="$(oc --kubeconfig="${kc}" get "datavolume/${dvName}" -n "${vmNs}" \ | |
| -o jsonpath='{.status.claimName}' || true)" | |
| [[ -z "${pvcName}" ]] && pvcName="${dvName}" | |
| scName="$(oc --kubeconfig="${kc}" get "persistentvolumeclaim/${pvcName}" -n "${vmNs}" \ | |
| -o jsonpath='{.spec.storageClassName}' || true)" | |
| [[ -n "${scName}" ]] && printf '%s' "${scName}" && return 0 | |
| printf 'Unable to resolve root-disk StorageClass for %s/%s\n' \ | |
| "${vmNs}" "${vmName}" >&2 | |
| return 1 | |
| } |
🤖 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-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh`
around lines 356 - 381, Update GetVmRootDiskStorageClass so it never falls back
to an unrelated PVC’s storage class. Remove the namespace-wide
first(.items[].spec.storageClassName) fallback and return an empty result when
the PVC identified by pvcName cannot be resolved.
Comment on lines
+626
to
+638
| JStep() { | ||
| typeset name="${1:?}"; shift | ||
| typeset -i t0=$SECONDS rc=0 | ||
| "$@" || rc=$? | ||
| typeset -i elapsed=$(( SECONDS - t0 )) | ||
| if (( rc == 0 )); then | ||
| printf 'PASS\t%s\t%d\t\n' "${name}" "${elapsed}" >> "${junitFile}" | ||
| else | ||
| printf 'FAIL\t%s\t%d\tFailed (rc=%d); see diagnostics in hub-spoke-migration-diagnostics/\n' \ | ||
| "${name}" "${elapsed}" "${rc}" >> "${junitFile}" | ||
| fi | ||
| return "${rc}" | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
rc=0
(
false
echo "failure was ignored"
true
) || rc=$?
printf 'captured rc=%d\n' "${rc}" # Demonstrates the current pattern returns 0.Repository: openshift/release
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh'
# Map the file structure and inspect the relevant function ranges.
wc -l "$file"
sed -n '560,700p' "$file"
# Find the outer subshell / cclmStepRc logic referenced in the comment.
rg -n 'cclmStepRc|JStep\(|trap OnError ERR|set -eE|true\)' "$file"Repository: openshift/release
Length of output: 6407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh'
# Read the exact lines around JStep and the surrounding flow.
sed -n '620,690p' "$file"Repository: openshift/release
Length of output: 2547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh'
# Inspect the broader block that uses cclmStepRc so the control flow can be reasoned about.
sed -n '820,900p' "$file"Repository: openshift/release
Length of output: 2980
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh'
wc -l "$file"
sed -n '610,690p' "$file"
sed -n '840,900p' "$file"
rg -n 'cclmStepRc|JStep\(|trap OnError ERR|set -eE|true\)' "$file"Repository: openshift/release
Length of output: 6214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh'
# Show the exact JStep definition and the call site around cclmStepRc.
sed -n '620,690p' "$file"
echo '---'
sed -n '840,890p' "$file"Repository: openshift/release
Length of output: 4465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh'
# Find all JStep call sites to see whether the wrapper itself can mask failures.
rg -n '\bJStep\b' "$file"Repository: openshift/release
Length of output: 1133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh'
sed -n '770,815p' "$file"Repository: openshift/release
Length of output: 2462
Preserve the first failure from the migration block. The subshell ends with true, so (...) || cclmStepRc=$? can still record 0 after an earlier JStep failure. Capture the subshell status directly and remove the trailing true so any failed preflight or migration step fails the job.
🤖 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-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-commands.sh`
around lines 626 - 638, Update the migration subshell invoking JStep so its exit
status is captured directly in cclmStepRc, preserving the first failed preflight
or migration step. Remove the trailing true command that masks earlier failures,
while keeping the existing JStep execution and diagnostics behavior unchanged.
| as: p2p-mtv-execute-hub-spoke-migration | ||
| from: cli | ||
| commands: p2p-mtv-execute-hub-spoke-migration-commands.sh | ||
| timeout: 6h0m0s |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Align outer step timeouts with their cumulative internal waits.
ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-ref.yaml#L5-L5: provide enough time for two two-hour migrations plus sequential preflights; approximately eight hours.ci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-ref.yaml#L5-L5: cover the six sequential 15-minute readiness waits; approximately two hours.
📍 Affects 2 files
ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-ref.yaml#L5-L5(this comment)ci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-ref.yaml#L5-L5
🤖 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-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-ref.yaml`
at line 5, Increase the outer timeout in
ci-operator/step-registry/p2p/mtv-execute-hub-spoke-migration/p2p-mtv-execute-hub-spoke-migration-ref.yaml
at line 5 to approximately 8 hours, covering two two-hour migrations and
sequential preflights. Also increase the timeout in
ci-operator/step-registry/p2p/mtv-create-hub-spoke-maps/p2p-mtv-create-hub-spoke-maps-ref.yaml
at line 5 to approximately 2 hours, covering six sequential 15-minute readiness
waits.
Contributor
Author
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.23-p2p-hub-spoke-cclm-hub-spoke-aws-p2p-lp-interop |
Contributor
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
shakyav
force-pushed
the
hub_spoke_virt_vm_migration
branch
from
July 17, 2026 23:12
54ece45 to
b7e5241
Compare
Contributor
Author
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.23-p2p-hub-spoke-cclm-hub-spoke-aws-p2p-lp-interop |
Contributor
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Contributor
Author
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.23-p2p-hub-spoke-cclm-hub-spoke-aws-p2p-lp-interop |
Contributor
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Contributor
Author
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.23-p2p-hub-spoke-cclm-hub-spoke-aws-p2p-lp-interop |
Contributor
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci-operator/step-registry/p2p/create-spoke-imageset/p2p-create-spoke-imageset-commands.sh (1)
26-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass JSON directly to
oc applyto avoid an unnecessaryyqdependency.
oc applynatively supports JSON input, so converting the JSON object to YAML usingyqis unnecessary. Removingyqsimplifies the pipeline and avoids potential failures ifyqis missing from thecliimage.♻️ Proposed refactor
# Create (or idempotently update) the ClusterImageSet using jq data marshalling # to avoid injecting shell variables directly into YAML. jq -cn \ --arg name "${imageSetName}" \ --arg image "${RELEASE_IMAGE_LATEST}" \ '{ "apiVersion": "hive.openshift.io/v1", "kind": "ClusterImageSet", "metadata": {"name": $name}, "spec": {"releaseImage": $image} - }' | -yq -p json -o yaml eval . | -oc apply -f - + }' | oc apply -f -🤖 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/create-spoke-imageset/p2p-create-spoke-imageset-commands.sh` around lines 26 - 38, Update the ClusterImageSet apply pipeline in the create/update command to pass the JSON produced by jq directly to oc apply, removing the yq conversion step and its dependency while preserving the existing resource fields and idempotent apply behavior.
🤖 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.
Nitpick comments:
In
`@ci-operator/step-registry/p2p/create-spoke-imageset/p2p-create-spoke-imageset-commands.sh`:
- Around line 26-38: Update the ClusterImageSet apply pipeline in the
create/update command to pass the JSON produced by jq directly to oc apply,
removing the yq conversion step and its dependency while preserving the existing
resource fields and idempotent apply behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 02e87bb3-2900-4496-9b6a-b830ab9ab8cf
⛔ 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 (7)
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.23-p2p-hub-spoke.yamlci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.23-p2p.yamlci-operator/step-registry/p2p/cclm-hub-spoke/p2p-cclm-hub-spoke-workflow.yamlci-operator/step-registry/p2p/create-spoke-imageset/OWNERSci-operator/step-registry/p2p/create-spoke-imageset/p2p-create-spoke-imageset-commands.shci-operator/step-registry/p2p/create-spoke-imageset/p2p-create-spoke-imageset-ref.metadata.jsonci-operator/step-registry/p2p/create-spoke-imageset/p2p-create-spoke-imageset-ref.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/p2p/cclm-hub-spoke/p2p-cclm-hub-spoke-workflow.yaml
- ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__acm-virt-ocp4.23-p2p-hub-spoke.yaml
Contributor
Author
|
/pj-rehearse periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.23-p2p-hub-spoke-cclm-hub-spoke-aws-p2p-lp-interop |
Contributor
|
@shakyav: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Contributor
|
@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. |
Contributor
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
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 by CodeRabbit
This PR extends OpenShift CI’s P2P interop testing for ACM hub↔spoke VM live migration by adding a scheduled
acm-virt-ocp4.23-p2p-hub-spokeRedHatQE job variant and implementing a new end-to-end bi-directional CCLM (hub↔spoke) workflow that provisions VMs, configures MTV, and runs live migrations.Wires the new 4.23 hub↔spoke periodic job + variant
periodic-ci-RedHatQE-interop-testing-master-acm-virt-ocp4.23-p2p-hub-spoke-cclm-hub-spoke-aws-p2p-lp-interopexercises the new scenario (including fail classification via the job’sFIREWATCH_CONFIG).Adds/updates the
acm-virt-ocp4.23-p2p-hub-spokeinterop-testing configurationIntroduces a new bi-directional migration workflow
ci-operator/step-registry/p2p/cclm-hub-spoke/p2p-cclm-hub-spoke-workflow.yamlwhich orchestrates:ManagedClusterAddon/SubmarinerConfig(ACM-native path),Adds core P2P step implementations used by the workflow
Adds the hub-side 4.23 imageset creation step (for pre-GA spoke provisioning)
ci-operator/step-registry/p2p/create-spoke-imageset/*:ClusterImageSeton the hub namedimg<tag>-x86-64,<tag>fromRELEASE_IMAGE_LATEST,img${ACM_SPOKE_CLUSTER_INITIAL_VERSION}.prefix,Refines Submariner verification behavior
subctlis fully opt-in, and adds optional hub↔spoke connectivity verification (including CCLM TCP sync probing) to better validate the migration prerequisites.