search

kubevirt / community

Community content
https://kubevirt.io
50 stars 105 forks source link

Proposal for local storage migration #240

Closed alicefr closed 1 year ago

alicefr commented 1 year ago

Reference PR for migrating local storage in KubeVirt: https://github.com/kubevirt/kubevirt/pull/10305

kubevirt-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign fabiand for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubevirt/community/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
alicefr commented 1 year ago

In this proposal we require the user to explicitly create the target PVC which is fine, but it would be nice in a follow up proposal to allow the user to omit that, and have the controller create a compatible target PVC of the exact same size, that would make it more like seamless live migration.

I'm not against this but just not so sure. This should be thought of like a CSIClone where you create a new PVC from another, but the copy is done with the workload migration. If you want to have a controller, then you need a new CRD that abstracts the PVC. Do we really want this? It is an open question, I don't have an opinion.

awels commented 1 year ago

Right that is why I said for a follow up. The idea is that if we are cordoning a node or something, we would want the system to take care of moving the VMs instead of us manually live migrating the VMs beforehand. But that is definitely a more advanced use case vs simply moving from one storage class to another.

alicefr commented 1 year ago

I had some offline discussions on this topic, and to keep everyone on the same page, I'll summarize here the highlights:

  1. The scope of this proposal is too broad and we should reduce a single use case (migrating local storage with the same PVC geometry and storage class)
  2. Initially, we should avoid any APIs and KubeVirt will take care of the destination PVC creation. Later, once we gain more experience then we could introduce a new CRD where users can customize the PVC destination preferences
  3. The migrate disks handling needs to be kept separated from the VirtualMachineInstanceMigration CRD, instead a new CRD can be added and then the migration controller could simply pick the information from there

After this feedback, I think the PoC PR is still valid, but I need to introduce the part that handles the destination PVC. Once, I finish, I'll update accordingly this proposal

@vladikr @fabiand @davidvossel I hope I have captured the essence of our discussion. Please, correct if there is anything wrong

alicefr commented 1 year ago

I'll close this proposal as we'll address separately every use-cases. I'll open a new one for the Storage Class migration