Allow exiting the externally provisioned state

This change unblocks setting externallyProvisioned to false to allow
a host to follow the regular lifecycle.

During the transition, image and customDeploy are copied over to the
status to make sure the host does not get unconditionally deprovisioned.
Hosts without image and customDeploy will still be deprovisioned.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>

Author: dtantsur

internal/controller/metal3.io/baremetalhost_controller_test.go

@@ -1581,7 +1581,7 @@ func TestExternallyProvisionedTransitions(t *testing.T) {
 		waitForProvisioningState(t, r, host, metal3api.StateExternallyProvisioned)
 	})
 
-	t.Run("externally provisioned to inspecting", func(t *testing.T) {
+	t.Run("externally provisioned to provisioned", func(t *testing.T) {
 		host := newDefaultHost(t)
 		host.Spec.Online = true
 		host.Spec.ExternallyProvisioned = true
@@ -1596,7 +1596,49 @@ func TestExternallyProvisionedTransitions(t *testing.T) {
 		}
 		t.Log("set externally provisioned to false")
 
-		waitForProvisioningState(t, r, host, metal3api.StateInspecting)
+		waitForProvisioningState(t, r, host, metal3api.StateProvisioned)
+	})
+
+	t.Run("externally provisioned to provisioned copies image to status", func(t *testing.T) {
+		host := newDefaultHost(t)
+		host.Spec.Online = true
+		host.Spec.ExternallyProvisioned = true
+		host.Spec.Image = &metal3api.Image{URL: "foo", Checksum: "123"}
+		r := newTestReconciler(t, host)
+
+		waitForProvisioningState(t, r, host, metal3api.StateExternallyProvisioned)
+
+		host.Spec.ExternallyProvisioned = false
+		err := r.Update(t.Context(), host)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		tryReconcile(t, r, host,
+			func(host *metal3api.BareMetalHost, result reconcile.Result) bool {
+				return host.Status.Provisioning.State == metal3api.StateProvisioned &&
+					host.Status.Provisioning.Image.URL == "foo"
+			},
+		)
+		assert.Equal(t, "foo", host.Status.Provisioning.Image.URL)
+		assert.Equal(t, "123", host.Status.Provisioning.Image.Checksum)
+	})
+
+	t.Run("externally provisioned without image deprovisions", func(t *testing.T) {
+		host := newDefaultHost(t)
+		host.Spec.Online = true
+		host.Spec.ExternallyProvisioned = true
+		r := newTestReconciler(t, host)
+
+		waitForProvisioningState(t, r, host, metal3api.StateExternallyProvisioned)
+
+		host.Spec.ExternallyProvisioned = false
+		err := r.Update(t.Context(), host)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		waitForProvisioningState(t, r, host, metal3api.StateDeprovisioning)
 	})
 
 	t.Run("preparing to externally provisioned", func(t *testing.T) {

internal/controller/metal3.io/host_state_machine.go

@@ -464,11 +464,17 @@ func (hsm *hostStateMachine) handleExternallyProvisioned(ctx context.Context, in
 		return hsm.Reconciler.actionManageSteadyState(ctx, hsm.Provisioner, info)
 	}
 
-	if hsm.Host.NeedsHardwareInspection() {
-		hsm.NextState = metal3api.StateInspecting
-	} else {
-		hsm.NextState = metal3api.StatePreparing
+	// The host is exiting externally provisioned at this point.
+	// Set image and customDeploy are set in status to prevent unconditional deprovisioning.
+	if hsm.Host.Spec.Image != nil {
+		hsm.Host.Status.Provisioning.Image = *hsm.Host.Spec.Image
 	}
+	if hsm.Host.Spec.CustomDeploy != nil {
+		hsm.Host.Status.Provisioning.CustomDeploy = hsm.Host.Spec.CustomDeploy.DeepCopy()
+	}
+
+	// Move to Provisioned. If image and customDeploy are not set, deprovisioning will start on the next reconciliation.
+	hsm.NextState = metal3api.StateProvisioned
 	return actionComplete{}
 }
 

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

@@ -113,11 +113,6 @@ func (webhook *BareMetalHost) validateChanges(oldObj *metal3api.BareMetalHost, n
 		errs = append(errs, errors.New("bootMACAddress can not be changed once it is set"))
 	}
 
-	// Disallow disabling externallyProvisioned.
-	if oldObj.Spec.ExternallyProvisioned && !newObj.Spec.ExternallyProvisioned {
-		errs = append(errs, errors.New("externallyProvisioned can not be changed from true to false"))
-	}
-
 	// Only allow enabling externallyProvisioned from Available state.
 	if !oldObj.Spec.ExternallyProvisioned && newObj.Spec.ExternallyProvisioned &&
 		newObj.Status.Provisioning.State != metal3api.StateAvailable {

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go

@@ -1139,12 +1139,12 @@ func TestValidateUpdate(t *testing.T) {
 			wantedErr: "address invalid-mac: invalid MAC address",
 		},
 		{
-			name: "rejectDisablingExternallyProvisioned",
+			name: "allowDisablingExternallyProvisioned",
 			newBMH: &metal3api.BareMetalHost{
 				TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{ExternallyProvisioned: false}},
 			oldBMH: &metal3api.BareMetalHost{
 				TypeMeta: tm, ObjectMeta: om, Spec: metal3api.BareMetalHostSpec{ExternallyProvisioned: true}},
-			wantedErr: "externallyProvisioned can not be changed from true to false",
+			wantedErr: "",
 		},
 		{
 			name: "rejectEnablingExternallyProvisionedWhenNotInAvailable",

test/e2e/externally_provisioned_test.go

@@ -6,7 +6,6 @@ package e2e
 import (
 	"context"
 	"path"
-	"time"
 
 	metal3api "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	. "github.com/onsi/ginkgo/v2"
@@ -38,7 +37,7 @@ var _ = Describe("Create as externally provisioned, deprovision", Label("require
 			})
 		})
 
-		It("provisions a BMH as externally provisioned, validates state immutability, then deprovisions", func() {
+		It("provisions a BMH as externally provisioned, then removes the flag to deprovision", func() {
 			testSecretName := secretName + "-external"
 			By("Creating a secret with BMH credentials")
 			bmcCredentialsData := map[string]string{
@@ -85,7 +84,7 @@ var _ = Describe("Create as externally provisioned, deprovision", Label("require
 			Expect(bmh.Status.OperationHistory.Inspect.Start.IsZero()).To(BeTrue())
 			Expect(bmh.Status.OperationHistory.Provision.Start.IsZero()).To(BeTrue())
 
-			By("Attempting to set ExternallyProvisioned to false (should be blocked by webhook)")
+			By("Setting ExternallyProvisioned to false to trigger deprovisioning")
 			err = clusterProxy.GetClient().Get(ctx, types.NamespacedName{
 				Name:      bmh.Name,
 				Namespace: bmh.Namespace,
@@ -95,22 +94,21 @@ var _ = Describe("Create as externally provisioned, deprovision", Label("require
 			patch := client.MergeFrom(bmh.DeepCopy())
 			bmh.Spec.ExternallyProvisioned = false
 			err = clusterProxy.GetClient().Patch(ctx, &bmh, patch)
-			Expect(err).To(HaveOccurred(), "Webhook should reject transition from true to false")
-			Expect(err.Error()).To(ContainSubstring("externallyProvisioned can not be changed from true to false"))
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Waiting for the BMH to reach Available state after deprovisioning")
+			WaitForBmhInProvisioningState(ctx, WaitForBmhInProvisioningStateInput{
+				Client: clusterProxy.GetClient(),
+				Bmh:    bmh,
+				State:  metal3api.StateAvailable,
+			}, e2eConfig.GetIntervals(specName, "wait-available")...)
 
-			By("Verifying BMH remains in ExternallyProvisioned state")
+			By("Cleaning up the BMH")
 			err = clusterProxy.GetClient().Get(ctx, types.NamespacedName{
 				Name:      bmh.Name,
 				Namespace: bmh.Namespace,
 			}, &bmh)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(bmh.Spec.ExternallyProvisioned).To(BeTrue(), "ExternallyProvisioned should still be true")
-			Expect(bmh.Status.Provisioning.State).To(Equal(metal3api.StateExternallyProvisioned))
-
-			By("Deleting the BMH")
-			// Wait for 2 seconds to allow time to confirm annotation is set
-			// TODO: fix this so we do not need the sleep
-			time.Sleep(2 * time.Second)
 
 			err = clusterProxy.GetClient().Delete(ctx, &bmh)
 			Expect(err).NotTo(HaveOccurred())
@@ -120,12 +118,6 @@ var _ = Describe("Create as externally provisioned, deprovision", Label("require
 				Client:    clusterProxy.GetClient(),
 				BmhName:   bmh.Name,
 				Namespace: bmh.Namespace,
-				UndesiredStates: []metal3api.ProvisioningState{
-					metal3api.StateProvisioning,
-					metal3api.StateRegistering,
-					metal3api.StateDeprovisioning,
-					metal3api.StateInspecting,
-				},
 			}, e2eConfig.GetIntervals(specName, "wait-bmh-deleted")...)
 
 			By("Waiting for the secret to be deleted")