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 <[email protected]>

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"`

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.

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

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
 }
 

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
 }
 

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,19 +141,19 @@ 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
 	}
 	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
 }
 

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
 }

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)

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{}) {