Repository: metal3-io/baremetal-operator
Pull request #795 (2021-02-15 13:05:19)

Implement explicit reboot mode options
The default reboot-interface behaviour is to attempt a soft power
off, and if this fails, revert to a hard power off (PR #294). For high
availability use-cases we require the ability to immediately power-off
a node. This PR attempts to address that requirement and is a WIP given
that other changes are going to be required to the provisioner API and
the CAPBM actuator to actually enact these changes.

Also see: https://bugzilla.redhat.com/show_bug.cgi?id=1927678
And design doc: metal3-io/metal3-docs#164

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/795#" title="Author self-approved">rdoxenham</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **zaneb**
You can assign the PR to them by writing `/assign @zaneb` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["zaneb"]} -->
metal3-io-bot: Hi @rdoxenham. Thanks for your PR.

I'm waiting for a [metal3-io](https://github.com/orgs/metal3-io/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/metal3-io/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

dhellmann: This looks really close. The commit messages don't match the code (there's no isHardReboot now) so fixing that up should make this ready to go, as soon as the design is approved.

Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index be2bef7d..498f64b0 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -483,6 +483,18 @@ type CredentialsStatus struct {
 	Version   string                  `json:"credentialsVersion,omitempty"`
 }
 
+// RebootMode defines known variations of reboot modes
+type RebootMode string
+
+const (
+	RebootModeHard RebootMode = "hard"
+	RebootModeSoft RebootMode = "soft"
+)
+
+type RebootAnnotationArguments struct {
+	Mode RebootMode `json:"mode"`
+}
+
 // Match compares the saved status information with the name and
 // content of a secret object.
 func (cs CredentialsStatus) Match(secret corev1.Secret) bool {
diff --git a/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go b/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go
index 928d7920..499b596b 100644
--- a/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go
@@ -393,6 +393,21 @@ func (in *ProvisionStatus) DeepCopy() *ProvisionStatus {
 	return out
 }
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *RebootAnnotationArguments) DeepCopyInto(out *RebootAnnotationArguments) {
+	*out = *in
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RebootAnnotationArguments.
+func (in *RebootAnnotationArguments) DeepCopy() *RebootAnnotationArguments {
+	if in == nil {
+		return nil
+	}
+	out := new(RebootAnnotationArguments)
+	in.DeepCopyInto(out)
+	return out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *RootDeviceHints) DeepCopyInto(out *RootDeviceHints) {
 	*out = *in
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index d6ebdf55..891f7fe3 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -339,13 +339,40 @@ func (r *BareMetalHostReconciler) credentialsErrorResult(err error, request ctrl
 }
 
 // hasRebootAnnotation checks for existence of reboot annotations and returns true if at least one exist
-func hasRebootAnnotation(host *metal3v1alpha1.BareMetalHost) bool {
-	for annotation := range host.Annotations {
+func hasRebootAnnotation(info *reconcileInfo) (hasReboot bool, rebootMode metal3v1alpha1.RebootMode) {
+	rebootMode = metal3v1alpha1.RebootModeSoft
+
+	for annotation, value := range info.host.GetAnnotations() {
 		if isRebootAnnotation(annotation) {
-			return true
+			hasReboot = true
+			newRebootMode := getRebootMode(value, info)
+			// If any annotation has asked for a hard reboot, that
+			// mode takes precedence.
+			if newRebootMode == metal3v1alpha1.RebootModeHard {
+				rebootMode = newRebootMode
+			}
+			// Don't use a break here as we may have multiple clients setting
+			// reboot annotations and we always want hard requests honoured
 		}
 	}
-	return false
+	return
+}
+
+func getRebootMode(annotation string, info *reconcileInfo) metal3v1alpha1.RebootMode {
+
+	if annotation == "" {
+		info.log.Info("No reboot annotation value specified, assuming soft-reboot.")
+		return metal3v1alpha1.RebootModeSoft
+	}
+
+	annotations := metal3v1alpha1.RebootAnnotationArguments{}
+	err := json.Unmarshal([]byte(annotation), &annotations)
+	if err != nil {
+		info.publishEvent("InvalidAnnotationValue", fmt.Sprintf("could not parse reboot annotation (%s) - invalid json, assuming soft-reboot", annotation))
+		info.log.Info(fmt.Sprintf("Could not parse reboot annotation (%q) - invalid json, assuming soft-reboot", annotation))
+		return metal3v1alpha1.RebootModeSoft
+	}
+	return annotations.Mode
 }
 
 // isRebootAnnotation returns true if the provided annotation is a reboot annotation (either suffixed or not)
@@ -765,7 +792,9 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 
 	provState := info.host.Status.Provisioning.State
 	isProvisioned := provState == metal3v1alpha1.StateProvisioned || provState == metal3v1alpha1.StateExternallyProvisioned
-	if hasRebootAnnotation(info.host) && isProvisioned {
+
+	desiredReboot, desiredRebootMode := hasRebootAnnotation(info)
+	if desiredReboot && isProvisioned {
 		desiredPowerOnState = false
 	}
 
@@ -780,12 +809,13 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 	info.log.Info("power state change needed",
 		"expected", desiredPowerOnState,
 		"actual", info.host.Status.PoweredOn,
+		"reboot mode", desiredRebootMode,
 		"reboot process", desiredPowerOnState != info.host.Spec.Online)
 
 	if desiredPowerOnState {
 		provResult, err = prov.PowerOn()
 	} else {
-		provResult, err = prov.PowerOff()
+		provResult, err = prov.PowerOff(desiredRebootMode)
 	}
 	if err != nil {
 		return actionError{errors.Wrap(err, "failed to manage power state of host")}
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index 44477802..f6bd2614 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -316,7 +316,7 @@ func (p *demoProvisioner) PowerOn() (result provisioner.Result, err error) {
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *demoProvisioner) PowerOff() (result provisioner.Result, err error) {
+func (p *demoProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
 
 	hostName := p.host.ObjectMeta.Name
 	switch hostName {
diff --git a/pkg/provisioner/empty/empty.go b/pkg/provisioner/empty/empty.go
index ee108067..19fc939b 100644
--- a/pkg/provisioner/empty/empty.go
+++ b/pkg/provisioner/empty/empty.go
@@ -80,7 +80,7 @@ func (p *emptyProvisioner) PowerOn() (provisioner.Result, error) {
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *emptyProvisioner) PowerOff() (provisioner.Result, error) {
+func (p *emptyProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (provisioner.Result, error) {
 	return provisioner.Result{}, nil
 }
 
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index 18609465..35894999 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -271,7 +271,7 @@ func (p *fixtureProvisioner) PowerOn() (result provisioner.Result, err error) {
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *fixtureProvisioner) PowerOff() (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered off")
 
 	if p.state.poweredOn {
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 8a2910b6..d73b63d0 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1705,10 +1705,14 @@ func (p *ironicProvisioner) PowerOn() (result provisioner.Result, err error) {
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *ironicProvisioner) PowerOff() (result provisioner.Result, err error) {
-	p.log.Info("ensuring host is powered off")
+func (p *ironicProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
+	p.log.Info(fmt.Sprintf("ensuring host is powered off (mode: %s)", rebootMode))
 
-	result, err = p.softPowerOff()
+	if rebootMode == metal3v1alpha1.RebootModeHard {
+		result, err = p.hardPowerOff()
+	} else {
+		result, err = p.softPowerOff()
+	}
 	if err != nil {
 		switch err.(type) {
 		// In case of soft power off is unsupported or has failed,
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index 85d77360..f516fbd6 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -84,8 +84,9 @@ type Provisioner interface {
 	PowerOn() (result Result, err error)
 
 	// PowerOff ensures the server is powered off independently of any image
-	// provisioning operation.
-	PowerOff() (result Result, err error)
+	// provisioning operation. The boolean argument may be used to specify
+	// if a hard reboot (force power off) is required - true if so.
+	PowerOff(rebootMode metal3v1alpha1.RebootMode) (result Result, err error)
 
 	// IsReady checks if the provisioning backend is available to accept
 	// all the incoming requests.


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 55a5eca9..b41dbb33 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -20,6 +20,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/event"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
@@ -359,43 +360,96 @@ func TestInspectionDisabledAnnotation(t *testing.T) {
 	assert.True(t, inspectionDisabled(host))
 }
 
+func makeReconcileInfo(host *metal3v1alpha1.BareMetalHost) *reconcileInfo {
+	return &reconcileInfo{
+		log:  logf.Log.WithName("controllers").WithName("BareMetalHost").WithName("baremetal_controller"),
+		host: host,
+	}
+}
+
 func TestHasRebootAnnotation(t *testing.T) {
 	host := newDefaultHost(t)
+	info := makeReconcileInfo(host)
 	host.Annotations = make(map[string]string)
 
-	if hasRebootAnnotation(host) {
-		t.Fail()
-	}
+	hasReboot, rebootMode := hasRebootAnnotation(info)
+	assert.False(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
 
 	host.Annotations = make(map[string]string)
 	suffixedAnnotation := rebootAnnotationPrefix + "/foo"
 	host.Annotations[suffixedAnnotation] = ""
 
-	if !hasRebootAnnotation(host) {
-		t.Fail()
-	}
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
 
 	delete(host.Annotations, suffixedAnnotation)
 	host.Annotations[rebootAnnotationPrefix] = ""
 
-	if !hasRebootAnnotation(host) {
-		t.Fail()
-	}
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
 
 	host.Annotations[suffixedAnnotation] = ""
 
-	if !hasRebootAnnotation(host) {
-		t.Fail()
-	}
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
 
 	//two suffixed annotations to simulate multiple clients
 
 	host.Annotations[suffixedAnnotation+"bar"] = ""
 
-	if !hasRebootAnnotation(host) {
-		t.Fail()
-	}
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
+}
+
+func TestHasHardRebootAnnotation(t *testing.T) {
+	host := newDefaultHost(t)
+	info := makeReconcileInfo(host)
+	host.Annotations = make(map[string]string)
+
+	hasReboot, rebootMode := hasRebootAnnotation(info)
+	assert.False(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
+
+	host.Annotations = make(map[string]string)
+	suffixedAnnotation := rebootAnnotationPrefix + "/foo/"
+	host.Annotations[suffixedAnnotation] = "{\"mode\": \"hard\"}"
+
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeHard, rebootMode)
+
+	delete(host.Annotations, suffixedAnnotation)
+	host.Annotations[rebootAnnotationPrefix] = "{\"mode\": \"soft\"}"
+
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
+
+	delete(host.Annotations, suffixedAnnotation)
+	host.Annotations[rebootAnnotationPrefix] = "{\"bad\"= \"json\"]"
+
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
+
+	host.Annotations[suffixedAnnotation] = ""
+
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeSoft, rebootMode)
+
+	//two suffixed annotations to simulate multiple clients
+
+	host.Annotations[suffixedAnnotation+"bar"] = "{\"mode\": \"hard\"}"
 
+	hasReboot, rebootMode = hasRebootAnnotation(info)
+	assert.True(t, hasReboot)
+	assert.Equal(t, metal3v1alpha1.RebootModeHard, rebootMode)
 }
 
 // TestRebootWithSuffixlessAnnotation tests full reboot cycle with suffixless
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 162cf689..a2dc7d1f 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -651,7 +651,7 @@ func (m *mockProvisioner) PowerOn() (result provisioner.Result, err error) {
 	return m.getNextResultByMethod("PowerOn"), err
 }
 
-func (m *mockProvisioner) PowerOff() (result provisioner.Result, err error) {
+func (m *mockProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
 	return m.getNextResultByMethod("PowerOff"), err
 }
 
diff --git a/pkg/provisioner/ironic/power_test.go b/pkg/provisioner/ironic/power_test.go
index 8aeda724..02ff3694 100644
--- a/pkg/provisioner/ironic/power_test.go
+++ b/pkg/provisioner/ironic/power_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection"
 	"github.com/stretchr/testify/assert"
 
+	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/testserver"
@@ -123,6 +124,7 @@ func TestPowerOff(t *testing.T) {
 		expectedDirty        bool
 		expectedError        bool
 		expectedRequestAfter int
+		rebootMode           metal3v1alpha1.RebootMode
 	}{
 		{
 			name: "node-already-power-off",
@@ -150,6 +152,18 @@ func TestPowerOff(t *testing.T) {
 				UUID:                 nodeUUID,
 			}),
 			expectedDirty: true,
+			rebootMode:    metal3v1alpha1.RebootModeSoft,
+		},
+		{
+			name: "power-off hard",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				PowerState:           powerOn,
+				TargetPowerState:     powerOn,
+				TargetProvisionState: "",
+				UUID:                 nodeUUID,
+			}),
+			expectedDirty: true,
+			rebootMode:    metal3v1alpha1.RebootModeHard,
 		},
 		{
 			name: "power-off wait for Provisioning state",
@@ -199,7 +213,8 @@ func TestPowerOff(t *testing.T) {
 			}
 
 			prov.status.ID = nodeUUID
-			result, err := prov.PowerOff()
+			// We pass the RebootMode type here to define the reboot action
+			result, err := prov.PowerOff(tc.rebootMode)
 
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)