From b75f9abba8b5193da199b0fdc76783196f101690 Mon Sep 17 00:00:00 2001 From: Martin Necas Date: Mon, 18 Nov 2024 13:47:38 +0100 Subject: [PATCH] MTV-1656 | Disable snapshot consolidation Issue: When deleting the snapshot the users with slow storage could fail to get data corruption due to the consolidation process. The consolidation writes the snapshot overlay to the disk and if the user has slow storage it can take a long time. However, the Forklift Controller does not check if the consolidation is finished and creates a new snapshot. This new snapshot could be created in the middle of the writing process and it would not see the changes in the underlying image from the consolidation. Wrong order 1. Delete snapshot 2. Create snapshot 3. Consolidate Correct order 1. Delete snapshot 2. Consolidate 3. Create snapshot Fix: Disable consolidation in the middle of migration and consolidate in RemoveFinalSnapshot phase. This will make the warm migration faster as we don't need to wait for the consolidation but at the same time we will consolidate at the end of migration so we won't cause any issues on the users env. Ref: https://issues.redhat.com/browse/MTV-1656 Signed-off-by: Martin Necas --- pkg/controller/plan/adapter/base/doc.go | 2 +- pkg/controller/plan/adapter/ocp/client.go | 2 +- pkg/controller/plan/adapter/openstack/client.go | 2 +- pkg/controller/plan/adapter/ova/client.go | 2 +- pkg/controller/plan/adapter/ovirt/client.go | 2 +- pkg/controller/plan/adapter/vsphere/client.go | 8 ++++---- pkg/controller/plan/migration.go | 8 ++++++-- 7 files changed, 15 insertions(+), 11 deletions(-) diff --git a/pkg/controller/plan/adapter/base/doc.go b/pkg/controller/plan/adapter/base/doc.go index 0900f3397..0803f1abf 100644 --- a/pkg/controller/plan/adapter/base/doc.go +++ b/pkg/controller/plan/adapter/base/doc.go @@ -106,7 +106,7 @@ type Client interface { // Create a snapshot of the source VM. CreateSnapshot(vmRef ref.Ref, hostsFunc util.HostsFunc) (string, error) // Remove a snapshot. - RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc) error + RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc, consolidate bool) error // Check if a snapshot is ready to transfer. CheckSnapshotReady(vmRef ref.Ref, snapshot string) (ready bool, err error) // Set DataVolume checkpoints. diff --git a/pkg/controller/plan/adapter/ocp/client.go b/pkg/controller/plan/adapter/ocp/client.go index c17cf49a8..445820d1a 100644 --- a/pkg/controller/plan/adapter/ocp/client.go +++ b/pkg/controller/plan/adapter/ocp/client.go @@ -40,7 +40,7 @@ func (r *Client) CreateSnapshot(vmRef ref.Ref, hostsFunc util.HostsFunc) (string } // Remove a VM snapshot. No-op for this provider. -func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc) (err error) { +func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc, consolidate bool) (err error) { return } diff --git a/pkg/controller/plan/adapter/openstack/client.go b/pkg/controller/plan/adapter/openstack/client.go index e8e7a89da..1af3d4f31 100644 --- a/pkg/controller/plan/adapter/openstack/client.go +++ b/pkg/controller/plan/adapter/openstack/client.go @@ -125,7 +125,7 @@ func (r *Client) SetCheckpoints(vmRef ref.Ref, precopies []planapi.Precopy, data } // Remove a VM snapshot. No-op for this provider. -func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc) (err error) { +func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc, consolidate bool) (err error) { return } diff --git a/pkg/controller/plan/adapter/ova/client.go b/pkg/controller/plan/adapter/ova/client.go index c1ef2838d..fefbf994a 100644 --- a/pkg/controller/plan/adapter/ova/client.go +++ b/pkg/controller/plan/adapter/ova/client.go @@ -51,7 +51,7 @@ func (r *Client) CreateSnapshot(vmRef ref.Ref, hostsFunc util.HostsFunc) (snapsh } // Remove a VM snapshot. No-op for this provider. -func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc) (err error) { +func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc, consolidate bool) (err error) { return } diff --git a/pkg/controller/plan/adapter/ovirt/client.go b/pkg/controller/plan/adapter/ovirt/client.go index 4a7a7fec5..9fb106875 100644 --- a/pkg/controller/plan/adapter/ovirt/client.go +++ b/pkg/controller/plan/adapter/ovirt/client.go @@ -105,7 +105,7 @@ func (r *Client) CheckSnapshotReady(vmRef ref.Ref, snapshot string) (ready bool, } // Remove a VM snapshot. No-op for this provider. -func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc) (err error) { +func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hostsFunc util.HostsFunc, consolidate bool) (err error) { return } diff --git a/pkg/controller/plan/adapter/vsphere/client.go b/pkg/controller/plan/adapter/vsphere/client.go index a48af1af5..47cb7745a 100644 --- a/pkg/controller/plan/adapter/vsphere/client.go +++ b/pkg/controller/plan/adapter/vsphere/client.go @@ -67,11 +67,11 @@ func (r *Client) CheckSnapshotReady(vmRef ref.Ref, snapshot string) (ready bool, } // Remove a VM snapshot. -func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hosts util.HostsFunc) (err error) { +func (r *Client) RemoveSnapshot(vmRef ref.Ref, snapshot string, hosts util.HostsFunc, consolidate bool) (err error) { r.Log.V(1).Info("RemoveSnapshot", "vmRef", vmRef, "snapshot", snapshot) - err = r.removeSnapshot(vmRef, snapshot, false, hosts) + err = r.removeSnapshot(vmRef, snapshot, false, hosts, consolidate) return } @@ -371,7 +371,7 @@ func nullableHosts() (hosts map[string]*v1beta1.Host, err error) { } // Remove a VM snapshot and optionally its children. -func (r *Client) removeSnapshot(vmRef ref.Ref, snapshot string, children bool, hosts util.HostsFunc) (err error) { +func (r *Client) removeSnapshot(vmRef ref.Ref, snapshot string, children bool, hosts util.HostsFunc, consolidate bool) (err error) { r.Log.Info("Removing snapshot", "vmRef", vmRef, "snapshot", snapshot, @@ -381,7 +381,7 @@ func (r *Client) removeSnapshot(vmRef ref.Ref, snapshot string, children bool, h if err != nil { return } - _, err = vm.RemoveSnapshot(context.TODO(), snapshot, children, nil) + _, err = vm.RemoveSnapshot(context.TODO(), snapshot, children, &consolidate) if err != nil { err = liberr.Wrap(err) return diff --git a/pkg/controller/plan/migration.go b/pkg/controller/plan/migration.go index 5d06b92ec..971be28b9 100644 --- a/pkg/controller/plan/migration.go +++ b/pkg/controller/plan/migration.go @@ -517,7 +517,7 @@ func (r *Migration) removeLastWarmSnapshot(vm *plan.VMStatus) { return } snapshot := vm.Warm.Precopies[n-1].Snapshot - if err := r.provider.RemoveSnapshot(vm.Ref, snapshot, r.kubevirt.loadHosts); err != nil { + if err := r.provider.RemoveSnapshot(vm.Ref, snapshot, r.kubevirt.loadHosts, true); err != nil { r.Log.Error( err, "Failed to clean up warm migration snapshots.", @@ -995,7 +995,11 @@ func (r *Migration) execute(vm *plan.VMStatus) (err error) { break } n := len(vm.Warm.Precopies) - err = r.provider.RemoveSnapshot(vm.Ref, vm.Warm.Precopies[n-1].Snapshot, r.kubevirt.loadHosts) + consolidate := false + if vm.Phase == RemoveFinalSnapshot { + consolidate = true + } + err = r.provider.RemoveSnapshot(vm.Ref, vm.Warm.Precopies[n-1].Snapshot, r.kubevirt.loadHosts, consolidate) if err != nil { step.AddError(err.Error()) err = nil