-
Notifications
You must be signed in to change notification settings - Fork 52
CNV-62749: Add support for storage migration across multiple namespaces #3059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
CNV-62749: Add support for storage migration across multiple namespaces #3059
Conversation
|
@pcbailey: This pull request references CNV-62749 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
...rtualmachines/actions/components/VirtualMachineMigration/hooks/useMigrationNamespacesPVCs.ts
Outdated
Show resolved
Hide resolved
...ws/virtualmachines/actions/components/VirtualMachineMigration/hooks/useCreateEmptyMigPlan.ts
Outdated
Show resolved
Hide resolved
4db6f0b to
ff403cf
Compare
rszwajko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to clarify the behavior for the remote clusters.
.../virtualmachines/actions/components/VirtualMachineMigration/VirtualMachineMigrationModal.tsx
Show resolved
Hide resolved
| const { t } = useKubevirtTranslation(); | ||
| const vmNamespaces = removeDuplicates(vms?.map((vm) => getNamespace(vm))); | ||
|
|
||
| const migrationNamespace = getNamespace(vms?.[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accessReview for the bulk action is still based on the namespace from the first VM only - we should make sure that all VMs are from the same namespace OR check all of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked, but don't see a way to do multiple access review checks. I want to make sure I'm not missing something before creating our own hook to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know a bulk access check hook - maybe @upalatucci can help. However I can share how this problem is solved for LazyMenuActions. The PR #3077 needs another iteration to fix tests but the concept is not new - the original code in the Console is working the same way:
- lazy pre-load access checks as soon as possible - the action is triggered by onHover event. The calls are memoized (lodash).
- the check is repeated when creating the menu items
In both cases the utility methods ignores failures - the assumption is that the backend will re-check anyway.
The performance impact due to lazy loading is small - it's less then 10 calls usually. In case of namespaces most of them should be memoized already.
| } | ||
|
|
||
| if (namespaces.size === 1 && mtcInstalled) { | ||
| if (mtcInstalled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the check is for the local cluster - how about the remote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we should test this in the destination cluster, and that vms come all from the same cluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@upalatucci How do I check for flags on a different cluster?
.../virtualmachines/actions/components/VirtualMachineMigration/VirtualMachineMigrationModal.tsx
Outdated
Show resolved
Hide resolved
...tualmachines/actions/components/VirtualMachineMigration/hooks/useExistingMigPlanConflicts.ts
Show resolved
Hide resolved
src/views/virtualmachines/actions/components/VirtualMachineMigration/utils/utils.ts
Show resolved
Hide resolved
| useEffect(() => { | ||
| Promise.all( | ||
| memoizedNamespaces?.map((ns) => | ||
| fleetK8sList<IoK8sApiCoreV1PersistentVolumeClaim>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in src/multicluster/k8sRequests.ts we have all the methods for GET POST and so on. It would be awesome to have kubevirtK8sList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the types for FleetK8sListOptions. It doesn't have a cluster property.
ff403cf to
9079d77
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request refactors virtual machine migration from single-namespace to multi-namespace support. It updates locale translations to reflect plural and parameterized MigPlan messages, extends MigPlan type with a namespaces array field, introduces new hooks for multi-namespace PVC fetching and conflict detection, renames components, and adds cluster-aware logic to migration action conditions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Modal as VirtualMachineMigrationModal
participant Hooks as Migration Hooks
participant K8s as Kubernetes API
participant Alert as VMMigrationNamespaceConflictsAlert
User->>Modal: Initiate multi-VM migration
Note over Modal: Collect VM namespaces & clusters
Modal->>Hooks: useExistingMigPlanConflicts(vmNamespaces)
Hooks->>K8s: Fetch MigPlans
K8s-->>Hooks: MigPlan list
Hooks->>Hooks: Compute namespace conflicts
Hooks-->>Modal: {migPlansLoaded, namespaceConflicts}
Modal->>Hooks: useMigrationNamespacesPVCs(vmNamespaces)
Hooks->>K8s: Fetch PVCs per namespace (parallel)
K8s-->>Hooks: PVCs
Hooks-->>Modal: pvcsInNamespaces
alt Conflicts detected
Modal->>Alert: Render with namespaceConflicts
Alert-->>User: Show conflict alert
else No conflicts
Modal->>Alert: Render details tab with PVCs
Alert-->>User: Show migration review form
end
User->>Modal: Submit migration
Modal->>Hooks: useCreateEmptyMigPlan(vmNamespaces)
Hooks-->>Modal: MigPlan with namespaces array
Modal->>K8s: Create MigPlan
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
@pcbailey: This pull request references CNV-62749 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
9079d77 to
f46df28
Compare
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pcbailey, upalatucci The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/remove-lgtm |

📝 Description
This PR adds support for storage migration for multiple VMs across multiple namespaces. The alert in the modal has been adjusted to list the namespaces that conflict when more than one exists.
Jira: https://issues.redhat.com/browse/CNV-62749
🎥 Screenshot
After
Summary by CodeRabbit
New Features
Localization