From 36e3cc1f8df4e2c5d1c0d45340ccaa9538dfa0a8 Mon Sep 17 00:00:00 2001 From: Arik Hadas Date: Thu, 2 Nov 2023 22:08:38 +0200 Subject: [PATCH] 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{}) {