From 3c1f1216c18116d38cdd874ad8c8185f85068321 Mon Sep 17 00:00:00 2001 From: Arik Hadas Date: Sun, 29 Oct 2023 17:55:51 +0200 Subject: [PATCH 1/2] fix #437: Post VMs with desired 'Running' state Long time ago, we posted VMs at the beginning of the migration and then started and stopped them during the migration and therefore in order to restore their original power state, we had to initiate another call to patch the VM with its desired power state. However, now that we post the VMs at the very last steps of the migration, we can set their desired power state in their specification and avoid another call to the target environment. Signed-off-by: Arik Hadas --- pkg/controller/plan/kubevirt.go | 15 ++---------- pkg/controller/plan/migration.go | 41 -------------------------------- 2 files changed, 2 insertions(+), 54 deletions(-) diff --git a/pkg/controller/plan/kubevirt.go b/pkg/controller/plan/kubevirt.go index 79f314040..cf267a337 100644 --- a/pkg/controller/plan/kubevirt.go +++ b/pkg/controller/plan/kubevirt.go @@ -427,18 +427,6 @@ func (r *KubeVirt) DeleteVM(vm *plan.VMStatus) (err error) { return } -// Set the Running state on a Kubevirt VirtualMachine. -func (r *KubeVirt) SetRunning(vmCr *VirtualMachine, running bool) (err error) { - vmCopy := vmCr.VirtualMachine.DeepCopy() - vmCr.VirtualMachine.Spec.Running = &running - patch := client.MergeFrom(vmCopy) - err = r.Destination.Client.Patch(context.TODO(), vmCr.VirtualMachine, patch) - if err != nil { - err = liberr.Wrap(err) - } - return -} - func (r *KubeVirt) DataVolumes(vm *plan.VMStatus) (dataVolumes []cdi.DataVolume, err error) { secret, err := r.ensureSecret(vm.Ref, r.secretDataSetterForCDI(vm.Ref)) if err != nil { @@ -1006,7 +994,8 @@ func (r *KubeVirt) virtualMachine(vm *plan.VMStatus) (object *cnv.VirtualMachine object.ObjectMeta.Annotations = annotations } - running := false + // Power on the destination VM if the source VM was originally powered on. + running := vm.RestorePowerState == On object.Spec.Running = &running err = r.Builder.VirtualMachine(vm.Ref, &object.Spec, pvcs) diff --git a/pkg/controller/plan/migration.go b/pkg/controller/plan/migration.go index 7f7d46a12..055ca3949 100644 --- a/pkg/controller/plan/migration.go +++ b/pkg/controller/plan/migration.go @@ -1052,15 +1052,6 @@ func (r *Migration) execute(vm *plan.VMStatus) (err error) { Durable: true, }) - // Power on the destination VM if the source VM was originally powered on. - err = r.setRunning(vm, vm.RestorePowerState == On) - if err != nil { - r.Log.Error(err, - "Could not power on destination VM.", - "vm", - vm.String()) - err = nil - } } else if vm.Error != nil { vm.Phase = Completed vm.SetCondition( @@ -1317,38 +1308,6 @@ func (r *Migration) ensureGuestConversionPod(vm *plan.VMStatus) (err error) { return } -// Set the running state of the kubevirt VM. -func (r *Migration) setRunning(vm *plan.VMStatus, running bool) (err error) { - if r.vmMap == nil { - r.vmMap, err = r.kubevirt.VirtualMachineMap() - if err != nil { - return - } - } - var vmCr VirtualMachine - found := false - if vmCr, found = r.vmMap[vm.ID]; !found { - // Recreate the map and check again, the map may be stale - r.vmMap, err = r.kubevirt.VirtualMachineMap() - if err != nil { - return - } - - if vmCr, found = r.vmMap[vm.ID]; !found { - msg := "VirtualMachine CR not found." - vm.AddError(msg) - return - } - } - - if vmCr.Spec.Running != nil && *vmCr.Spec.Running == running { - return - } - - err = r.kubevirt.SetRunning(&vmCr, running) - return -} - // Update the progress of the appropriate disk copy step. (DiskTransfer, Cutover) func (r *Migration) updateCopyProgress(vm *plan.VMStatus, step *plan.Step) (err error) { var pendingReason string From 1e9d314d529be028070ca8cddbe2f260ef3241de Mon Sep 17 00:00:00 2001 From: Arik Hadas Date: Thu, 2 Nov 2023 22:08:38 +0200 Subject: [PATCH 2/2] fix power state handling from OpenStack The function Client#PowerState returned the power state as a string under the assumption that its values would be On, Off, or Unknown. However, that was not enforced and it was easy to get confuse and return different values. The client for OpenStack did just that and returned the status that is received from OpenStack, for instance ACTIVE, and that led to have different power state than On when a VM was running on OpenStack at the time the migration was triggered, resulting in not setting the migrated VM with Running = true and therefore the migrated VM was stopped in KubeVirt even though it was supposed to start. Therefore, we change the return argument for the power state in the Client#PowerState function to have its own type (reverting b4ff20b), VMPowerState, and change this function in the client for OpenStack to return "On" for VMs that were running in OpenStack. Signed-off-by: Arik Hadas --- pkg/apis/forklift/v1beta1/plan/vm.go | 13 +++++++++++-- pkg/controller/plan/adapter/base/doc.go | 2 +- pkg/controller/plan/adapter/ocp/client.go | 8 ++++---- pkg/controller/plan/adapter/openstack/client.go | 16 +++++++++++++--- pkg/controller/plan/adapter/ova/client.go | 2 +- pkg/controller/plan/adapter/ovirt/client.go | 17 +++++------------ pkg/controller/plan/adapter/vsphere/client.go | 15 ++++----------- pkg/controller/plan/kubevirt.go | 2 +- pkg/controller/plan/migration.go | 9 ++------- 9 files changed, 42 insertions(+), 42 deletions(-) diff --git a/pkg/apis/forklift/v1beta1/plan/vm.go b/pkg/apis/forklift/v1beta1/plan/vm.go index 2f1515a37..2244dd552 100644 --- a/pkg/apis/forklift/v1beta1/plan/vm.go +++ b/pkg/apis/forklift/v1beta1/plan/vm.go @@ -2,11 +2,12 @@ package plan import ( "fmt" + "path" + "github.com/konveyor/forklift-controller/pkg/apis/forklift/v1beta1/ref" libcnd "github.com/konveyor/forklift-controller/pkg/lib/condition" core "k8s.io/api/core/v1" meta "k8s.io/apimachinery/pkg/apis/meta/v1" - "path" ) // Plan hook. @@ -57,7 +58,7 @@ type VMStatus struct { // Warm migration status Warm *Warm `json:"warm,omitempty"` // Source VM power state before migration. - RestorePowerState string `json:"restorePowerState,omitempty"` + RestorePowerState VMPowerState `json:"restorePowerState,omitempty"` // Conditions. libcnd.Conditions `json:",inline"` @@ -72,6 +73,14 @@ type Warm struct { Precopies []Precopy `json:"precopies,omitempty"` } +type VMPowerState string + +const ( + VMPowerStateOn VMPowerState = "On" + VMPowerStateOff VMPowerState = "Off" + VMPowerStateUnknown VMPowerState = "Unknown" +) + // Precopy durations type Precopy struct { Start *meta.Time `json:"start,omitempty"` diff --git a/pkg/controller/plan/adapter/base/doc.go b/pkg/controller/plan/adapter/base/doc.go index 6246cf82b..b3c9c30f5 100644 --- a/pkg/controller/plan/adapter/base/doc.go +++ b/pkg/controller/plan/adapter/base/doc.go @@ -80,7 +80,7 @@ type Client interface { // Power off the source VM. PowerOff(vmRef ref.Ref) error // Return the source VM's power state. - PowerState(vmRef ref.Ref) (string, error) + PowerState(vmRef ref.Ref) (planapi.VMPowerState, error) // Return whether the source VM is powered off. PoweredOff(vmRef ref.Ref) (bool, error) // Create a snapshot of the source VM. diff --git a/pkg/controller/plan/adapter/ocp/client.go b/pkg/controller/plan/adapter/ocp/client.go index 0e1ebd5c5..8ac57eb3f 100644 --- a/pkg/controller/plan/adapter/ocp/client.go +++ b/pkg/controller/plan/adapter/ocp/client.go @@ -91,19 +91,19 @@ func (r *Client) PowerOn(vmRef ref.Ref) error { } // PowerState implements base.Client -func (r *Client) PowerState(vmRef ref.Ref) (string, error) { +func (r *Client) PowerState(vmRef ref.Ref) (planapi.VMPowerState, error) { vm := cnv.VirtualMachine{} err := r.sourceClient.Get(context.TODO(), k8sclient.ObjectKey{Namespace: vmRef.Namespace, Name: vmRef.Name}, &vm) if err != nil { err = liberr.Wrap(err) - return "", err + return planapi.VMPowerStateUnknown, err } if vm.Spec.Running != nil && *vm.Spec.Running { - return "On", nil + return planapi.VMPowerStateOn, nil } - return "Off", nil + return planapi.VMPowerStateOff, nil } // PoweredOff implements base.Client diff --git a/pkg/controller/plan/adapter/openstack/client.go b/pkg/controller/plan/adapter/openstack/client.go index e21a1a55e..80e52f0bf 100644 --- a/pkg/controller/plan/adapter/openstack/client.go +++ b/pkg/controller/plan/adapter/openstack/client.go @@ -76,10 +76,20 @@ func (r *Client) PowerOff(vmRef ref.Ref) (err error) { } // Return the source VM's power state. -func (r *Client) PowerState(vmRef ref.Ref) (state string, err error) { - state, err = r.VMStatus(vmRef.ID) +func (r *Client) PowerState(vmRef ref.Ref) (state planapi.VMPowerState, err error) { + status, err := r.VMStatus(vmRef.ID) if err != nil { err = liberr.Wrap(err) + state = planapi.VMPowerStateUnknown + return + } + switch status { + case libclient.VmStatusActive: + state = planapi.VMPowerStateOn + case libclient.VmStatusShutoff: + state = planapi.VMPowerStateOff + default: + state = planapi.VMPowerStateUnknown } return } @@ -91,7 +101,7 @@ func (r *Client) PoweredOff(vmRef ref.Ref) (off bool, err error) { err = liberr.Wrap(err) return } - off = state == libclient.VmStatusShutoff + off = state == planapi.VMPowerStateOff return } diff --git a/pkg/controller/plan/adapter/ova/client.go b/pkg/controller/plan/adapter/ova/client.go index 3327e3f3e..7a1df17cd 100644 --- a/pkg/controller/plan/adapter/ova/client.go +++ b/pkg/controller/plan/adapter/ova/client.go @@ -65,7 +65,7 @@ func (r *Client) SetCheckpoints(vmRef ref.Ref, precopies []planapi.Precopy, data } // Get the power state of the VM. -func (r *Client) PowerState(vmRef ref.Ref) (state string, err error) { +func (r *Client) PowerState(vmRef ref.Ref) (state planapi.VMPowerState, err error) { return } diff --git a/pkg/controller/plan/adapter/ovirt/client.go b/pkg/controller/plan/adapter/ovirt/client.go index 774c58ae2..37f630820 100644 --- a/pkg/controller/plan/adapter/ovirt/client.go +++ b/pkg/controller/plan/adapter/ovirt/client.go @@ -24,13 +24,6 @@ const ( snapshotDesc = "Forklift Operator warm migration precopy" ) -// VM power states -const ( - powerOn = "On" - powerOff = "Off" - powerUnknown = "Unknown" -) - // Snapshot event codes const ( SNAPSHOT_FINISHED_SUCCESS int64 = 68 @@ -148,7 +141,7 @@ func (r *Client) SetCheckpoints(vmRef ref.Ref, precopies []planapi.Precopy, data } // Get the power state of the VM. -func (r *Client) PowerState(vmRef ref.Ref) (state string, err error) { +func (r *Client) PowerState(vmRef ref.Ref) (state planapi.VMPowerState, err error) { vm, _, err := r.getVM(vmRef) if err != nil { return @@ -156,11 +149,11 @@ func (r *Client) PowerState(vmRef ref.Ref) (state string, err error) { status, _ := vm.Status() switch status { case ovirtsdk.VMSTATUS_DOWN: - state = powerOff + state = planapi.VMPowerStateOff case ovirtsdk.VMSTATUS_UP, ovirtsdk.VMSTATUS_POWERING_UP: - state = powerOn + state = planapi.VMPowerStateOn default: - state = powerUnknown + state = planapi.VMPowerStateUnknown } return } @@ -203,7 +196,7 @@ func (r *Client) PoweredOff(vmRef ref.Ref) (poweredOff bool, err error) { if err != nil { return } - poweredOff = powerState == powerOff + poweredOff = powerState == planapi.VMPowerStateOff return } diff --git a/pkg/controller/plan/adapter/vsphere/client.go b/pkg/controller/plan/adapter/vsphere/client.go index d6b6a46e1..1480b7a36 100644 --- a/pkg/controller/plan/adapter/vsphere/client.go +++ b/pkg/controller/plan/adapter/vsphere/client.go @@ -26,13 +26,6 @@ const ( snapshotDesc = "Forklift Operator warm migration precopy" ) -// VM power states -const ( - powerOn = "On" - powerOff = "Off" - powerUnknown = "Unknown" -) - // vSphere VM Client type Client struct { *plancontext.Context @@ -138,7 +131,7 @@ func (r *Client) SetCheckpoints(vmRef ref.Ref, precopies []planapi.Precopy, data } // Get the power state of the VM. -func (r *Client) PowerState(vmRef ref.Ref) (state string, err error) { +func (r *Client) PowerState(vmRef ref.Ref) (state planapi.VMPowerState, err error) { vm, err := r.getVM(vmRef) if err != nil { return @@ -150,11 +143,11 @@ func (r *Client) PowerState(vmRef ref.Ref) (state string, err error) { } switch powerState { case types.VirtualMachinePowerStatePoweredOn: - state = powerOn + state = planapi.VMPowerStateOn case types.VirtualMachinePowerStatePoweredOff: - state = powerOff + state = planapi.VMPowerStateOff default: - state = powerUnknown + state = planapi.VMPowerStateUnknown } return } diff --git a/pkg/controller/plan/kubevirt.go b/pkg/controller/plan/kubevirt.go index cf267a337..c71b2e463 100644 --- a/pkg/controller/plan/kubevirt.go +++ b/pkg/controller/plan/kubevirt.go @@ -995,7 +995,7 @@ func (r *KubeVirt) virtualMachine(vm *plan.VMStatus) (object *cnv.VirtualMachine } // Power on the destination VM if the source VM was originally powered on. - running := vm.RestorePowerState == On + running := vm.RestorePowerState == plan.VMPowerStateOn object.Spec.Running = &running err = r.Builder.VirtualMachine(vm.Ref, &object.Spec, pvcs) diff --git a/pkg/controller/plan/migration.go b/pkg/controller/plan/migration.go index 055ca3949..7fc33572f 100644 --- a/pkg/controller/plan/migration.go +++ b/pkg/controller/plan/migration.go @@ -83,11 +83,6 @@ const ( Unknown = "Unknown" ) -// Power states. -const ( - On = "On" -) - const ( TransferCompleted = "Transfer completed." ) @@ -401,7 +396,7 @@ func (r *Migration) Cancel() (err error) { vm.String()) err = nil } - if vm.RestorePowerState == On { + if vm.RestorePowerState == plan.VMPowerStateOn { err = r.provider.PowerOn(vm.Ref) if err != nil { r.Log.Error(err, @@ -905,7 +900,7 @@ func (r *Migration) execute(vm *plan.VMStatus) (err error) { vm.AddError(fmt.Sprintf("Step '%s' not found", r.step(vm))) break } - var state string + var state plan.VMPowerState state, err = r.provider.PowerState(vm.Ref) if err != nil { if !errors.As(err, &web.ProviderNotReadyError{}) {