Skip to content

Fix fallback for ironic drivers that don't support soft power off #985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
metal3-io-bot merged 6 commits into from
Sep 28, 2021
Merged

Fix fallback for ironic drivers that don't support soft power off #985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ba2fd2
ironic: Refactor PowerOff()
zaneb Sep 20, 2021
f75c6b7
ironic: Restore check for unsupported soft power off
zaneb Sep 20, 2021
33fb45a
ironic: Refactor changePower() to publish events directly
zaneb Sep 20, 2021
e6ddc9f
ironic: Don't export SoftPowerOffUnsupportedError
zaneb Sep 20, 2021
ca66184
ironic: Use error wrapping for softPowerOffUnsupported
zaneb Sep 20, 2021
9a63628
ironic: Use gophercloud constants for power states
zaneb Sep 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions pkg/provisioner/ironic/errors.go
View file
Open in desktop

This file was deleted.

125 changes: 43 additions & 82 deletions pkg/provisioner/ironic/ironic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ var (

const (
// See nodes.Node.PowerState for details
powerOn = "power on"
powerOff = "power off"
softPowerOff = "soft power off"
powerOn = string(nodes.PowerOn)
powerOff = string(nodes.PowerOff)
softPowerOff = string(nodes.SoftPowerOff)
powerNone = "None"
nameSeparator = "~"
customDeployPriority = 80
Expand Down Expand Up @@ -1487,6 +1487,20 @@ func (p *ironicProvisioner) Detach() (result provisioner.Result, err error) {
return p.Delete()
}

// softPowerOffUnsupportedError is returned when the BMC does not
// support soft power off.
type softPowerOffUnsupportedError struct {
cause error
}

func (e softPowerOffUnsupportedError) Unwrap() error {
return e.cause
}

func (e softPowerOffUnsupportedError) Error() string {
return "soft power off is unsupported on BMC"
}

func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.TargetPowerState) (result provisioner.Result, err error) {
p.log.Info("changing power state")

Expand All @@ -1501,7 +1515,7 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar
powerStateOpts := nodes.PowerStateOpts{
Target: target,
}
if target == softPowerOff {
if target == nodes.SoftPowerOff {
powerStateOpts.Timeout = int(softPowerOffTimeout.Seconds())
}

Expand All @@ -1513,19 +1527,25 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar
switch changeResult.Err.(type) {
case nil:
p.log.Info("power change OK")
event := map[nodes.TargetPowerState]struct{ Event, Reason string }{
nodes.PowerOn: {Event: "PowerOn", Reason: "Host powered on"},
nodes.PowerOff: {Event: "PowerOff", Reason: "Host powered off"},
nodes.SoftPowerOff: {Event: "PowerOff", Reason: "Host soft powered off"},
}[target]
p.publisher(event.Event, event.Reason)
return operationContinuing(0)
case gophercloud.ErrDefault409:
p.log.Info("host is locked, trying again after delay", "delay", powerRequeueDelay)
result, _ = retryAfterDelay(powerRequeueDelay)
return result, HostLockedError{}
return retryAfterDelay(powerRequeueDelay)
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 {
changeResult.Err = softPowerOffUnsupportedError{changeResult.Err}
}
}
p.log.Info("power change error", "message", changeResult.Err)
return transientError(errors.Wrap(changeResult.Err,
fmt.Sprintf("failed to %s node", target)))
}

// PowerOn ensures the server is powered on independently of any image
Expand All @@ -1551,16 +1571,7 @@ func (p *ironicProvisioner) PowerOn(force bool) (result provisioner.Result, err
return operationFailed(fmt.Sprintf("PowerOn operation failed: %s",
ironicNode.LastError))
}
if result, err = p.changePower(ironicNode, nodes.PowerOn); err == nil {
p.publisher("PowerOn", "Host powered on")
return result, nil
}
switch err.(type) {
case HostLockedError:
return retryAfterDelay(powerRequeueDelay)
default:
return transientError(errors.Wrap(err, "failed to PowerOn node"))
}
return p.changePower(ironicNode, nodes.PowerOn)
}
return result, nil
}
Expand All @@ -1570,55 +1581,6 @@ 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 {
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"))
}
}
p.publisher("PowerOff", "Host powered off")
return result, err
}

return operationComplete()
}

// 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) {
p.log.Info("ensuring host is powered off by \"soft power off\" command")

ironicNode, err := p.getNode()
if err != nil {
return transientError(err)
Expand All @@ -1632,24 +1594,23 @@ func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error
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)
// 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)
}
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"))

if rebootMode == metal3v1alpha1.RebootModeSoft && !force {
result, err = p.changePower(ironicNode, nodes.SoftPowerOff)
if !errors.As(err, &softPowerOffUnsupportedError{}) {
return result, err
}
}
p.publisher("PowerOff", "Host soft powered off")
// Reboot mode is hard, force flag is set, or soft power off is not supported
return p.changePower(ironicNode, nodes.PowerOff)
}

return result, nil
return operationComplete()
}

func ironicNodeName(objMeta metav1.ObjectMeta) string {
Expand Down
88 changes: 68 additions & 20 deletions pkg/provisioner/ironic/power_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ironic

import (
"errors"
"net/http"
"testing"
"time"
Expand Down Expand Up @@ -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
Expand All @@ -155,6 +157,7 @@ func TestPowerOff(t *testing.T) {
expectedError bool
expectedRequestAfter int
expectedErrorResult bool
expectedReason string
rebootMode metal3v1alpha1.RebootMode
}{
{
Expand All @@ -178,29 +181,28 @@ 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,
}),
Expand All @@ -211,7 +213,6 @@ func TestPowerOff(t *testing.T) {
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),
Expand All @@ -222,19 +223,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",
Expand All @@ -247,14 +247,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,
},
}

Expand All @@ -273,7 +273,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,
Expand All @@ -295,7 +298,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)
Copy link
Copy Markdown
Member

@andfasano andfasano Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the current mock server does not allow to specify multiple, different responses for the same API call, so I don't think there is an easy way currently to test such scenario without enhancing properly the mock (right now, when a response call is configured, like here in .WithNodeStatesPowerUpdate(nodeUUID, http.StatusBadRequest) for example, the mock server will always provide the same answer for all the requests).
I think we could discuss this change in a separate PR as it may be useful also for other tests.

Copy link
Copy Markdown
Member Author

@zaneb zaneb Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree that would be a helpful feature to have in the mocks. If that were possible then this could have been implemented in TestPowerOff (and the regression would not have happened) without having to write this separate TestSoftPowerOffFallback test with its own logic.

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