Repository: metal3-io/baremetal-operator
Pull request #2229 (2025-01-31 16:07:41)

✨ Add support for Disablepoweroff
**What this PR does / why we need it**:
Add support for disable_power_off
as described in https://github.com/metal3-io/metal3-docs/pull/478

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

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please assign [zaneb](https://github.com/zaneb) for approval. For more information see [the Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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/main/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"]} -->
derekhiggins: /retest
metal3-io-bot: PR needs rebase.

<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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>

Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index f7c7c68d..ba1b4333 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -477,6 +477,11 @@ type BareMetalHostSpec struct {
 	// eventually populated by inspection.
 	// +optional
 	Architecture string `json:"architecture,omitempty"`
+
+	// When set to true, power off of the node will be disabled,
+	// instead, a reboot will be used in place of power on/off
+	// +optional
+	DisablePowerOff bool `json:"disablePowerOff,omitempty"`
 }
 
 // AutomatedCleaningMode is the interface to enable/disable automated cleaning
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index 13da49ec..48cfbc2f 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -67,6 +67,10 @@ func (host *BareMetalHost) validateHost() []error {
 		errs = append(errs, annotationErrors...)
 	}
 
+	if err := validatePowerStatus(host); err != nil {
+		errs = append(errs, err)
+	}
+
 	return errs
 }
 
@@ -350,3 +354,10 @@ func (host *BareMetalHost) validateCrossNamespaceSecretReferences() []error {
 	}
 	return errs
 }
+
+func validatePowerStatus(host *BareMetalHost) error {
+	if host.Spec.DisablePowerOff && !host.Spec.Online {
+		return fmt.Errorf("node can't simultaneously have online set to false and have power off disabled")
+	}
+	return nil
+}
diff --git a/config/base/crds/bases/metal3.io_baremetalhosts.yaml b/config/base/crds/bases/metal3.io_baremetalhosts.yaml
index 636f1bcf..cc8ccc52 100644
--- a/config/base/crds/bases/metal3.io_baremetalhosts.yaml
+++ b/config/base/crds/bases/metal3.io_baremetalhosts.yaml
@@ -199,6 +199,11 @@ spec:
                 description: Description is a human-entered text used to help identify
                   the host.
                 type: string
+              disablePowerOff:
+                description: |-
+                  When set to true, power off of the node will be disabled,
+                  instead, a reboot will be used in place of power on/off
+                type: boolean
               externallyProvisioned:
                 description: |-
                   ExternallyProvisioned means something else has provisioned the
diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml
index d857a6a0..42283193 100644
--- a/config/render/capm3.yaml
+++ b/config/render/capm3.yaml
@@ -199,6 +199,11 @@ spec:
                 description: Description is a human-entered text used to help identify
                   the host.
                 type: string
+              disablePowerOff:
+                description: |-
+                  When set to true, power off of the node will be disabled,
+                  instead, a reboot will be used in place of power on/off
+                type: boolean
               externallyProvisioned:
                 description: |-
                   ExternallyProvisioned means something else has provisioned the
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 6058437d..7532d0d3 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -496,6 +496,11 @@ func setErrorMessage(host *metal3api.BareMetalHost, errType metal3api.ErrorType,
 }
 
 func (r *BareMetalHostReconciler) actionPowerOffBeforeDeleting(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
+	if info.host.Spec.DisablePowerOff {
+		info.log.Info("Skipping host powered off as Power Off has been disabled")
+		return actionComplete{}
+	}
+
 	info.log.Info("host ready to be powered off")
 	provResult, err := prov.PowerOff(
 		metal3api.RebootModeHard,
@@ -840,6 +845,7 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
 			PreprovisioningImage:       preprovImg,
 			PreprovisioningNetworkData: preprovisioningNetworkData,
 			HasCustomDeploy:            hasCustomDeploy(info.host),
+			DisablePowerOff:            info.host.Spec.DisablePowerOff,
 		},
 		credsChanged,
 		info.host.Status.ErrorType == metal3api.RegistrationError)
@@ -1588,6 +1594,18 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 		return actionError{errors.Wrap(err, "failed to manage power state of host")}
 	}
 
+	// If DisablePowerOff was enabled then prov.PowerOff above will have rebooted instead of powering off, in this case
+	// the operation is complete (no need to power on) and any reboot annotation can be removed
+	if info.host.Spec.DisablePowerOff {
+		if _, suffixlessAnnotationExists := info.host.Annotations[metal3api.RebootAnnotationPrefix]; suffixlessAnnotationExists {
+			delete(info.host.Annotations, metal3api.RebootAnnotationPrefix)
+			if err = r.Update(info.ctx, info.host); err != nil {
+				return actionError{errors.Wrap(err, "failed to remove reboot annotation from host")}
+			}
+			return actionContinue{}
+		}
+	}
+
 	if provResult.ErrorMessage != "" {
 		if !desiredPowerOnState && desiredRebootMode == metal3api.RebootModeSoft &&
 			info.host.Status.ErrorType != metal3api.PowerManagementError {
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index 51fa446e..f9bac981 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -73,12 +73,16 @@ type Fixture struct {
 	BecomeReadyCounter int
 	// state to manage deletion
 	Deleted bool
+	// state to manage DisablePowerOff
+	DisablePowerOff bool
+	// state to manage power
+	PoweredOn bool
+	// Has reboot been called
+	RebootCalled bool
 	// state to manage the two-step adopt process
 	adopted bool
 	// state to manage provisioning
 	image metal3api.Image
-	// state to manage power
-	poweredOn bool
 	// state to manage inspection
 	inspectionStarted bool
 
@@ -211,7 +215,7 @@ func (p *fixtureProvisioner) InspectHardware(_ provisioner.InspectData, _, _, _
 // is expected to do this in the least expensive way possible, such as
 // reading from a cache.
 func (p *fixtureProvisioner) UpdateHardwareState() (hwState provisioner.HardwareState, err error) {
-	hwState.PoweredOn = &p.state.poweredOn
+	hwState.PoweredOn = &p.state.PoweredOn
 	p.log.Info("updating hardware state")
 	return
 }
@@ -334,10 +338,10 @@ func (p *fixtureProvisioner) Detach() (result provisioner.Result, err error) {
 func (p *fixtureProvisioner) PowerOn(_ bool) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered on")
 
-	if !p.state.poweredOn {
+	if !p.state.PoweredOn {
 		p.publisher("PowerOn", "Host powered on")
 		p.log.Info("changing status")
-		p.state.poweredOn = true
+		p.state.PoweredOn = true
 		result.Dirty = true
 		return result, nil
 	}
@@ -350,10 +354,16 @@ func (p *fixtureProvisioner) PowerOn(_ bool) (result provisioner.Result, err err
 func (p *fixtureProvisioner) PowerOff(_ metal3api.RebootMode, _ bool) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered off")
 
-	if p.state.poweredOn {
+	if p.state.DisablePowerOff {
+		p.state.RebootCalled = true
+		result.Dirty = true
+		return result, nil
+	}
+
+	if p.state.PoweredOn {
 		p.publisher("PowerOff", "Host powered off")
 		p.log.Info("changing status")
-		p.state.poweredOn = false
+		p.state.PoweredOn = false
 		result.Dirty = true
 		return result, nil
 	}
diff --git a/pkg/provisioner/ironic/clients/features.go b/pkg/provisioner/ironic/clients/features.go
index 6998d5f9..982e5bfc 100644
--- a/pkg/provisioner/ironic/clients/features.go
+++ b/pkg/provisioner/ironic/clients/features.go
@@ -58,7 +58,15 @@ func (af AvailableFeatures) HasDataImage() bool {
 	return af.MaxVersion >= 89
 }
 
+func (af AvailableFeatures) HasDisablePowerOff() bool {
+	return af.MaxVersion >= 95
+}
+
 func (af AvailableFeatures) ChooseMicroversion() string {
+	if af.HasDisablePowerOff() {
+		return "1.95"
+	}
+
 	if af.HasDataImage() {
 		return "1.89"
 	}
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 18f08ced..13844cce 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1642,13 +1642,21 @@ func (p *ironicProvisioner) PowerOff(rebootMode metal3api.RebootMode, force bool
 		}
 
 		if rebootMode == metal3api.RebootModeSoft && !force {
-			result, err = p.changePower(ironicNode, nodes.SoftPowerOff)
+			powerTarget := nodes.SoftPowerOff
+			if ironicNode.DisablePowerOff {
+				powerTarget = nodes.SoftRebooting
+			}
+			result, err = p.changePower(ironicNode, powerTarget)
 			if !errors.As(err, &softPowerOffUnsupportedError{}) {
 				return result, err
 			}
 		}
 		// Reboot mode is hard, force flag is set, or soft power off is not supported
-		return p.changePower(ironicNode, nodes.PowerOff)
+		powerTarget := nodes.PowerOff
+		if ironicNode.DisablePowerOff {
+			powerTarget = nodes.Rebooting
+		}
+		return p.changePower(ironicNode, powerTarget)
 	}
 
 	return operationComplete()
diff --git a/pkg/provisioner/ironic/register.go b/pkg/provisioner/ironic/register.go
index 55601171..dee78dc8 100644
--- a/pkg/provisioner/ironic/register.go
+++ b/pkg/provisioner/ironic/register.go
@@ -55,6 +55,15 @@ func (p *ironicProvisioner) Register(data provisioner.ManagementAccessData, cred
 		return result, "", err
 	}
 
+	// Refuse to manage a node that has Disabled Power off if not supported by ironic,
+	// accidentally powering it off would require a arctic expedition to the data center
+	if data.DisablePowerOff && !p.availableFeatures.HasDisablePowerOff() {
+		msg := "current ironic version does not support DisablePowerOff, refusing to manage node"
+		p.log.Info(msg)
+		result, err = operationFailed(msg)
+		return result, "", err
+	}
+
 	var ironicNode *nodes.Node
 	updater := clients.UpdateOptsBuilder(p.log)
 
@@ -129,6 +138,9 @@ func (p *ironicProvisioner) Register(data provisioner.ManagementAccessData, cred
 			updater.SetTopLevelOpt("driver_info", driverInfo, ironicNode.DriverInfo)
 		}
 
+		// The updater only updates disable_power_off if it has changed
+		updater.SetTopLevelOpt("disable_power_off", data.DisablePowerOff, ironicNode.DisablePowerOff)
+
 		// We don't return here because we also have to set the
 		// target provision state to manageable, which happens
 		// below.
@@ -232,6 +244,7 @@ func (p *ironicProvisioner) enrollNode(data provisioner.ManagementAccessData, bm
 		PowerInterface:      bmcAccess.PowerInterface(),
 		RAIDInterface:       bmcAccess.RAIDInterface(),
 		VendorInterface:     bmcAccess.VendorInterface(),
+		DisablePowerOff:     &data.DisablePowerOff,
 		Properties: map[string]interface{}{
 			"capabilities": buildCapabilitiesValue(nil, data.BootMode),
 		},
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index c19f970c..313e58d7 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -81,6 +81,7 @@ type ManagementAccessData struct {
 	PreprovisioningImage       *PreprovisioningImage
 	PreprovisioningNetworkData string
 	HasCustomDeploy            bool
+	DisablePowerOff            bool
 }
 
 type AdoptData struct {


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index 2ee11ed8..86cd6734 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -770,6 +770,26 @@ func TestValidateCreate(t *testing.T) {
 			oldBMH:    nil,
 			wantedErr: "", // Should be valid
 		},
+		{
+			name: "disablePowerOff",
+			newBMH: &BareMetalHost{
+				Spec: BareMetalHostSpec{
+					DisablePowerOff: true,
+					Online:          true,
+				},
+			},
+			wantedErr: "",
+		},
+		{
+			name: "disablePowerOffErr",
+			newBMH: &BareMetalHost{
+				Spec: BareMetalHostSpec{
+					DisablePowerOff: true,
+					Online:          false,
+				},
+			},
+			wantedErr: "node can't simultaneously have online set to false and have power off disabled",
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 4d683afc..b5f2f736 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -797,6 +797,48 @@ func TestRebootWithSuffixedAnnotation(t *testing.T) {
 	)
 }
 
+// TestRebootWithSuffixedAnnotation tests a full reboot cycle, with suffixed annotation
+// to verify that controller holds power off until annotation removal.
+func TestRebootWithSuffixedAnnotationPowerOffDisabled(t *testing.T) {
+	host := newDefaultHost(t)
+	host.Status.PoweredOn = true
+	host.Status.Provisioning.State = metal3api.StateProvisioned
+	host.Spec.Online = true
+	host.Spec.Image = &metal3api.Image{URL: "foo", Checksum: "123"}
+	host.Spec.Image.URL = "foo"
+	host.Status.Provisioning.Image.URL = "foo"
+	host.Spec.DisablePowerOff = true
+
+	fix := fixture.Fixture{DisablePowerOff: true, PoweredOn: true}
+	r := newTestReconcilerWithFixture(&fix, host)
+
+	// bmh reconcils until credentials are verified as valid
+	tryReconcile(t, r, host,
+		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
+			return host.Status.GoodCredentials.Reference != nil
+		},
+	)
+
+	assert.True(t, host.Status.PoweredOn)
+	assert.Equal(t, fix.RebootCalled, false)
+
+	// Add the reboot annotation
+	host.Annotations = make(map[string]string)
+	host.Annotations[metal3api.RebootAnnotationPrefix] = "{\"mode\":\"soft\"}"
+	err := r.Update(context.TODO(), host)
+	assert.NoError(t, err)
+
+	tryReconcile(t, r, host,
+		func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
+			_, ok := host.Annotations[metal3api.RebootAnnotationPrefix]
+			return !ok
+		},
+	)
+
+	assert.True(t, host.Status.PoweredOn)
+	assert.Equal(t, fix.RebootCalled, true)
+}
+
 // TestRebootWithServicing tests full reboot cycle with suffixless
 // annotation and servicing.
 func TestRebootWithServicing(t *testing.T) {
diff --git a/pkg/provisioner/ironic/clients/features_test.go b/pkg/provisioner/ironic/clients/features_test.go
index 517dc2ab..ed5d1f3b 100644
--- a/pkg/provisioner/ironic/clients/features_test.go
+++ b/pkg/provisioner/ironic/clients/features_test.go
@@ -6,7 +6,7 @@ import (
 )
 
 func TestAvailableFeatures_ChooseMicroversion(t *testing.T) {
-	microVersion := "1.89"
+	microVersion := "1.95"
 	type fields struct {
 		MaxVersion int
 	}
@@ -25,7 +25,7 @@ func TestAvailableFeatures_ChooseMicroversion(t *testing.T) {
 		{
 			name: fmt.Sprintf("MaxVersion = %d return %s", 89, microVersion),
 			feature: fields{
-				MaxVersion: 89,
+				MaxVersion: 95,
 			},
 			want: microVersion,
 		},
diff --git a/pkg/provisioner/ironic/register_test.go b/pkg/provisioner/ironic/register_test.go
index ae0252c8..307a52f8 100644
--- a/pkg/provisioner/ironic/register_test.go
+++ b/pkg/provisioner/ironic/register_test.go
@@ -1417,3 +1417,60 @@ func TestSetExternalURLRemoving(t *testing.T) {
 
 	assert.Equal(t, nil, updatedDriverInfo["external_http_url"])
 }
+
+func TestRegisterDisablePowerOff(t *testing.T) {
+	// Create a host with disable power off enabled
+	host := makeHost()
+	host.Spec.DisablePowerOff = true
+
+	// Set up ironic server to return the node
+	ironic := testserver.NewIronic(t).
+		Node(nodes.Node{
+			UUID: host.Status.Provisioning.ID,
+		}).NodeUpdate(nodes.Node{
+		UUID: host.Status.Provisioning.ID,
+	})
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil, ironic.Endpoint(), auth)
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	prov.TryInit()
+	result, _, err := prov.Register(provisioner.ManagementAccessData{DisablePowerOff: true}, false, false)
+	if err != nil {
+		t.Fatalf("error from Register: %s", err)
+	}
+	assert.Equal(t, "", result.ErrorMessage)
+}
+
+func TestRegisterDisablePowerOffNotAvail(t *testing.T) {
+	// Create a host with disable power off enabled
+	host := makeHost()
+
+	// Set up ironic server to return the node
+	ironic := testserver.NewIronic(t).WithVersion("1.87").
+		Node(nodes.Node{
+			UUID: host.Status.Provisioning.ID,
+		}).NodeUpdate(nodes.Node{
+		UUID: host.Status.Provisioning.ID,
+	})
+	ironic.Start()
+	defer ironic.Stop()
+
+	auth := clients.AuthConfig{Type: clients.NoAuth}
+	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nil, ironic.Endpoint(), auth)
+	if err != nil {
+		t.Fatalf("could not create provisioner: %s", err)
+	}
+
+	prov.TryInit()
+	result, _, err := prov.Register(provisioner.ManagementAccessData{DisablePowerOff: true}, false, false)
+	if err != nil {
+		t.Fatalf("error from Register: %s", err)
+	}
+	assert.Equal(t, "current ironic version does not support DisablePowerOff, refusing to manage node", result.ErrorMessage)
+}
diff --git a/pkg/provisioner/ironic/testserver/ironic.go b/pkg/provisioner/ironic/testserver/ironic.go
index e56182b0..d9864e52 100644
--- a/pkg/provisioner/ironic/testserver/ironic.go
+++ b/pkg/provisioner/ironic/testserver/ironic.go
@@ -23,7 +23,7 @@ type IronicMock struct {
 // NewIronic builds an ironic mock server.
 func NewIronic(t *testing.T) *IronicMock {
 	t.Helper()
-	server := New(t, "ironic").AddDefaultResponse("/v1/?", "", http.StatusOK, versionedRootResult)
+	server := New(t, "ironic").AddDefaultResponse("/v1/?", "", http.StatusOK, fmt.Sprintf(versionedRootResult, "1.95"))
 	return &IronicMock{
 		MockServer:   server,
 		CreatedNodes: 0,
@@ -42,10 +42,17 @@ const versionedRootResult = `
     "links": [ { "href": "/v1/", "rel": "self" } ],
     "status": "CURRENT",
     "min_version": "1.1",
-    "version": "1.87"
+    "version": "%s"
   }
 }`
 
+// If your calling this, call it before you add any other Responses.
+func (m *IronicMock) WithVersion(v string) *IronicMock {
+	m.ClearDatabase()
+	m.AddDefaultResponse("/v1/?", "", http.StatusOK, fmt.Sprintf(versionedRootResult, v))
+	return m
+}
+
 // WithDefaultResponses sets a valid answer for all the API calls.
 func (m *IronicMock) WithDefaultResponses() *IronicMock {
 	m.AddDefaultResponseJSON("/v1/nodes/{id}", "", http.StatusOK, nodes.Node{