[release-4.9] Bug 2009850: Fix fallback for ironic drivers that don't support soft power off

pkg/provisioner/ironic/ironic.go

@@ -1519,12 +1519,13 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar
 		return result, HostLockedError{}
 	case gophercloud.ErrDefault400:
 		// Error 400 Bad Request means target power state is not supported by vendor driver
-		p.log.Info("power change error", "message", changeResult.Err)
-		return result, SoftPowerOffUnsupportedError{}
-	default:
-		p.log.Info("power change error", "message", changeResult.Err)
-		return transientError(errors.Wrap(changeResult.Err, "failed to change power state"))
+		if target == nodes.SoftPowerOff {
+			p.log.Info("power change error", "message", changeResult.Err)
+			return result, SoftPowerOffUnsupportedError{}
+		}
 	}
+	p.log.Info("power change error", "message", changeResult.Err)
+	return transientError(errors.Wrap(changeResult.Err, "failed to change power state"))
 }
 
 // PowerOn ensures the server is powered on independently of any image
@@ -1569,85 +1570,73 @@ func (p *ironicProvisioner) PowerOn(force bool) (result provisioner.Result, err
 func (p *ironicProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
 	p.log.Info(fmt.Sprintf("ensuring host is powered off (mode: %s)", rebootMode))
 
-	if rebootMode == metal3v1alpha1.RebootModeSoft {
-		return p.softPowerOff()
-	}
-	// Reboot mode is hard or force flag is set
-	return p.hardPowerOff(force)
-}
-
-// hardPowerOff sends 'power off' request to BM node and waits for the result
-func (p *ironicProvisioner) hardPowerOff(force bool) (result provisioner.Result, err error) {
-	p.log.Info("ensuring host is powered off by \"hard power off\" command")
-
 	ironicNode, err := p.getNode()
 	if err != nil {
 		return transientError(err)
 	}
 
 	if ironicNode.PowerState != powerOff {
-		if ironicNode.LastError != "" && !force {
-			p.log.Info("hard power off error", "msg", ironicNode.LastError)
-			return operationFailed(ironicNode.LastError)
-		}
-		if ironicNode.TargetPowerState == powerOff {
+		targetState := ironicNode.TargetPowerState
+		// If the target state is either powerOff or softPowerOff, then we should wait
+		if targetState == powerOff || targetState == softPowerOff {
 			p.log.Info("waiting for power status to change")
 			return operationContinuing(powerRequeueDelay)
 		}
-		result, err = p.changePower(ironicNode, nodes.PowerOff)
-		if err != nil {
-			switch err.(type) {
-			case HostLockedError:
-				return retryAfterDelay(powerRequeueDelay)
-			default:
-				return transientError(errors.Wrap(err, "failed to power off host"))
+		// If the target state is unset while the last error is set,
+		// then the last execution of power off has failed.
+		if targetState == "" && ironicNode.LastError != "" && !force {
+			p.log.Info("power off error", "msg", ironicNode.LastError)
+			return operationFailed(ironicNode.LastError)
+		}
+
+		if rebootMode == metal3v1alpha1.RebootModeSoft && !force {
+			result, err := p.softPowerOff(ironicNode)
+			if !errors.As(err, &SoftPowerOffUnsupportedError{}) {
+				return result, err
 			}
 		}
-		p.publisher("PowerOff", "Host powered off")
-		return result, err
+		// Reboot mode is hard, force flag is set, or soft power off is not supported
+		return p.hardPowerOff(ironicNode)
 	}
 
 	return operationComplete()
 }
 
+// hardPowerOff sends 'power off' request to BM node and waits for the result
+func (p *ironicProvisioner) hardPowerOff(ironicNode *nodes.Node) (result provisioner.Result, err error) {
+	p.log.Info("ensuring host is powered off by \"hard power off\" command")
+
+	result, err = p.changePower(ironicNode, nodes.PowerOff)
+	if err != nil {
+		switch err.(type) {
+		case HostLockedError:
+			return retryAfterDelay(powerRequeueDelay)
+		default:
+			return transientError(errors.Wrap(err, "failed to power off host"))
+		}
+	}
+	p.publisher("PowerOff", "Host powered off")
+	return result, err
+}
+
 // softPowerOff sends 'soft power off' request to BM node.
 // If soft power off is not supported, the request ends with an error.
 // Otherwise the request ends with no error and the result should be
 // checked later via node fields "power_state", "target_power_state"
 // and "last_error".
-func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error) {
+func (p *ironicProvisioner) softPowerOff(ironicNode *nodes.Node) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered off by \"soft power off\" command")
 
-	ironicNode, err := p.getNode()
+	result, err = p.changePower(ironicNode, nodes.SoftPowerOff)
 	if err != nil {
-		return transientError(err)
-	}
-
-	if ironicNode.PowerState != powerOff {
-		targetState := ironicNode.TargetPowerState
-		// If the target state is either powerOff or softPowerOff, then we should wait
-		if targetState == powerOff || targetState == softPowerOff {
-			p.log.Info("waiting for power status to change")
-			return operationContinuing(powerRequeueDelay)
-		}
-		// If the target state is unset while the last error is set,
-		// then the last execution of soft power off has failed.
-		if targetState == "" && ironicNode.LastError != "" {
-			p.log.Info("soft power off error", "msg", ironicNode.LastError)
-			return operationFailed(ironicNode.LastError)
-		}
-		result, err = p.changePower(ironicNode, nodes.SoftPowerOff)
-		if err != nil {
-			switch err.(type) {
-			case HostLockedError:
-				return retryAfterDelay(powerRequeueDelay)
-			default:
-				return transientError(errors.Wrap(err, "failed to power off host"))
-			}
+		switch err.(type) {
+		case HostLockedError:
+			return retryAfterDelay(powerRequeueDelay)
+		default:
+			return transientError(errors.Wrap(err, "failed to power off host"))
 		}
-		p.publisher("PowerOff", "Host soft powered off")
 	}
-
+	p.publisher("PowerOff", "Host soft powered off")
 	return result, nil
 }
 

pkg/provisioner/ironic/power_test.go

@@ -1,6 +1,7 @@
 package ironic
 
 import (
+	"errors"
 	"net/http"
 	"testing"
 	"time"
@@ -144,7 +145,8 @@ func TestPowerOn(t *testing.T) {
 }
 
 func TestPowerOff(t *testing.T) {
-
+	hardPowerOffReason := "Host powered off"
+	softPowerOffReason := "Host soft powered off"
 	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
 	cases := []struct {
 		name   string
@@ -155,6 +157,7 @@ func TestPowerOff(t *testing.T) {
 		expectedError        bool
 		expectedRequestAfter int
 		expectedErrorResult  bool
+		expectedReason       string
 		rebootMode           metal3v1alpha1.RebootMode
 	}{
 		{
@@ -178,40 +181,39 @@ func TestPowerOff(t *testing.T) {
 			name: "power-off normal",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 			}),
-			expectedDirty: true,
-			rebootMode:    metal3v1alpha1.RebootModeSoft,
+			rebootMode:     metal3v1alpha1.RebootModeSoft,
+			expectedDirty:  true,
+			expectedReason: softPowerOffReason,
 		},
 		{
 			name: "power-off hard",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 			}),
-			expectedDirty: true,
-			rebootMode:    metal3v1alpha1.RebootModeHard,
+			expectedDirty:  true,
+			rebootMode:     metal3v1alpha1.RebootModeHard,
+			expectedReason: hardPowerOffReason,
 		},
 		{
 			name: "power-off wait for Provisioning state",
 			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
 				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
 				TargetProvisionState: string(nodes.TargetDeleted),
 				UUID:                 nodeUUID,
 			}),
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
+			expectedReason:       hardPowerOffReason,
 		},
 		{
 			name: "power-off wait for locked host",
 			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
 				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 			}).WithNodeStatesPower(nodeUUID, http.StatusConflict).WithNodeStatesPowerUpdate(nodeUUID, http.StatusConflict),
@@ -222,19 +224,18 @@ func TestPowerOff(t *testing.T) {
 			name: "power-off soft with force",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 			}),
-			rebootMode:    metal3v1alpha1.RebootModeSoft,
-			force:         true,
-			expectedDirty: true,
+			rebootMode:     metal3v1alpha1.RebootModeSoft,
+			force:          true,
+			expectedDirty:  true,
+			expectedReason: hardPowerOffReason,
 		},
 		{
 			name: "power-off hard with LastError",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 				LastError:            "hard power off failed",
@@ -247,14 +248,14 @@ func TestPowerOff(t *testing.T) {
 			name: "power-off hard with force",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
 				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
 				TargetProvisionState: "",
 				UUID:                 nodeUUID,
 				LastError:            "hard power off failed",
 			}),
-			rebootMode:    metal3v1alpha1.RebootModeHard,
-			force:         true,
-			expectedDirty: true,
+			rebootMode:     metal3v1alpha1.RebootModeHard,
+			force:          true,
+			expectedDirty:  true,
+			expectedReason: hardPowerOffReason,
 		},
 	}
 
@@ -273,7 +274,10 @@ func TestPowerOff(t *testing.T) {
 
 			host := makeHost()
 			host.Status.Provisioning.ID = nodeUUID
-			publisher := func(reason, message string) {}
+			var eventReasons []string
+			publisher := func(reason, message string) {
+				eventReasons = append(eventReasons, message)
+			}
 			auth := clients.AuthConfig{Type: clients.NoAuth}
 			prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, publisher,
 				tc.ironic.Endpoint(), auth, inspector.Endpoint(), auth,
@@ -295,7 +299,52 @@ func TestPowerOff(t *testing.T) {
 			if tc.expectedErrorResult {
 				assert.Contains(t, result.ErrorMessage, "hard power off failed")
 			}
-
+			if tc.expectedReason != "" {
+				assert.Len(t, eventReasons, 1)
+				assert.Contains(t, eventReasons, tc.expectedReason)
+			} else {
+				assert.Empty(t, eventReasons)
+			}
 		})
 	}
 }
+
+func TestSoftPowerOffFallback(t *testing.T) {
+	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
+	node := nodes.Node{
+		PowerState: powerOn,
+		UUID:       nodeUUID,
+	}
+	ironic := testserver.NewIronic(t).Ready().Node(node).WithNodeStatesPowerUpdate(nodeUUID, http.StatusBadRequest)
+	ironic.Start()
+	defer ironic.Stop()
+	inspector := testserver.NewInspector(t).Ready()
+	inspector.Start()
+	defer inspector.Stop()
+
+	host := makeHost()
+	host.Status.Provisioning.ID = nodeUUID
+	var eventReasons []string
+	publisher := func(reason, message string) {
+		eventReasons = append(eventReasons, message)
+	}
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, publisher,
+		ironic.Endpoint(), auth, inspector.Endpoint(), auth,
+	)
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	_, err = prov.PowerOff(metal3v1alpha1.RebootModeSoft, false)
+	assert.Error(t, err)
+	assert.False(t, errors.As(err, &SoftPowerOffUnsupportedError{}))
+
+	_, err = prov.changePower(&node, nodes.PowerOff)
+	assert.Error(t, err)
+	assert.False(t, errors.As(err, &SoftPowerOffUnsupportedError{}))
+
+	_, err = prov.changePower(&node, nodes.SoftPowerOff)
+	assert.Error(t, err)
+	assert.True(t, errors.As(err, &SoftPowerOffUnsupportedError{}))
+}