From fd603c715b05b481b53929ef8de6b0ac51b17e57 Mon Sep 17 00:00:00 2001 From: Riccardo Pittau Date: Tue, 16 Dec 2025 16:03:02 +0100 Subject: [PATCH] Require PreprovisioningImage for deprovisioning when cleaning is enabled When deprovisioning a host with automated cleaning enabled, BMO now: 1. Checks if a PreprovisioningImage is available before proceeding (in the provisioner's configureNode function) 2. Treats a PPI with a DeletionTimestamp as unavailable (in the controller's getPreprovImage function) Previously, the StateDeprovisioning case was not handled, and PPIs being deleted could still be used. This caused Ironic to fall back to building its own ISO and fail with a misleading "uefi_esp.img not found" error when the PPI was unavailable or being deleted. Assisted-By: Claude Sonnet 4.5 (commercial license) Signed-off-by: Riccardo Pittau (cherry picked from commit c4631b50abc5082b9e279549ae0007a52aed0ef8) --- .../metal3.io/baremetalhost_controller.go | 6 ++ .../baremetalhost_controller_test.go | 42 +++++++++++ pkg/provisioner/ironic/ironic.go | 3 +- pkg/provisioner/ironic/register_test.go | 74 +++++++++++++++++++ 4 files changed, 124 insertions(+), 1 deletion(-) diff --git a/internal/controller/metal3.io/baremetalhost_controller.go b/internal/controller/metal3.io/baremetalhost_controller.go index 28337b011c..3eaecb8a8f 100644 --- a/internal/controller/metal3.io/baremetalhost_controller.go +++ b/internal/controller/metal3.io/baremetalhost_controller.go @@ -775,6 +775,12 @@ func (r *BareMetalHostReconciler) getPreprovImage(info *reconcileInfo, formats [ return nil, fmt.Errorf("failed to retrieve pre-provisioning image data: %w", err) } + // If the PreprovisioningImage is being deleted, treat it as unavailable + if !preprovImage.DeletionTimestamp.IsZero() { + info.log.Info("PreprovisioningImage is being deleted, waiting for new one") + return nil, nil //nolint:nilnil + } + needsUpdate := false if preprovImage.Labels == nil && len(info.host.Labels) > 0 { preprovImage.Labels = make(map[string]string, len(info.host.Labels)) diff --git a/internal/controller/metal3.io/baremetalhost_controller_test.go b/internal/controller/metal3.io/baremetalhost_controller_test.go index 1b7e45316a..f6110ca4d4 100644 --- a/internal/controller/metal3.io/baremetalhost_controller_test.go +++ b/internal/controller/metal3.io/baremetalhost_controller_test.go @@ -2646,6 +2646,48 @@ func TestGetPreprovImageNotCurrent(t *testing.T) { assert.Nil(t, imgData) } +func TestGetPreprovImageBeingDeleted(t *testing.T) { + host := newDefaultHost(t) + imageURL := "http://example.test/image.iso" + acceptFormats := []metal3api.ImageFormat{metal3api.ImageFormatISO, metal3api.ImageFormatInitRD} + now := metav1.Now() + image := &metal3api.PreprovisioningImage{ + ObjectMeta: metav1.ObjectMeta{ + Name: host.Name, + Namespace: namespace, + DeletionTimestamp: &now, // PPI is being deleted + Finalizers: []string{"test-finalizer"}, + }, + Spec: metal3api.PreprovisioningImageSpec{ + Architecture: "x86_64", + AcceptFormats: acceptFormats, + }, + Status: metal3api.PreprovisioningImageStatus{ + Architecture: "x86_64", + Format: metal3api.ImageFormatISO, + ImageUrl: imageURL, + Conditions: []metav1.Condition{ + { + Type: string(metal3api.ConditionImageReady), + Status: metav1.ConditionTrue, + }, + { + Type: string(metal3api.ConditionImageError), + Status: metav1.ConditionFalse, + }, + }, + }, + } + r := newTestReconciler(t, host, image) + i := makeReconcileInfo(host) + + // Even though the image is ready, it should be treated as unavailable + // because it has a DeletionTimestamp + imgData, err := r.getPreprovImage(i, acceptFormats) + require.NoError(t, err) + assert.Nil(t, imgData) +} + func TestPreprovImageAvailable(t *testing.T) { host := newDefaultHost(t) r := newTestReconciler(t, host, newSecret("network_secret_1", nil)) diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index c99bf04e7f..819fcd377f 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -364,7 +364,8 @@ func (p *ironicProvisioner) configureNode(data provisioner.ManagementAccessData, } switch data.State { - case metal3api.StateInspecting, + case metal3api.StateDeprovisioning, + metal3api.StateInspecting, metal3api.StatePreparing: if deployImageInfo == nil && p.config.havePreprovImgBuilder { result, err = transientError(provisioner.ErrNeedsPreprovisioningImage) diff --git a/pkg/provisioner/ironic/register_test.go b/pkg/provisioner/ironic/register_test.go index ee05abc6d3..29d11ff942 100644 --- a/pkg/provisioner/ironic/register_test.go +++ b/pkg/provisioner/ironic/register_test.go @@ -1392,3 +1392,77 @@ func TestRegisterDisablePowerOffNotAvail(t *testing.T) { } assert.Equal(t, "current ironic version does not support DisablePowerOff, refusing to manage node", result.ErrorMessage) } + +func TestRegisterDeprovisioningNeedsPreprovisioningImage(t *testing.T) { + // Test that when deprovisioning with cleaning enabled and no + // PreprovisioningImage available, ErrNeedsPreprovisioningImage is returned + host := makeHost() + host.Status.Provisioning.ID = "uuid" + + ironic := testserver.NewIronic(t). + WithDrivers(). + Node(nodes.Node{ + Name: host.Namespace + nameSeparator + host.Name, + UUID: host.Status.Provisioning.ID, + ProvisionState: string(nodes.Manageable), + }).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) + } + // Enable preprov image builder + prov.config.havePreprovImgBuilder = true + + // Test StateDeprovisioning with cleaning enabled (not disabled) and no PreprovisioningImage + _, _, err = prov.Register(provisioner.ManagementAccessData{ + State: metal3api.StateDeprovisioning, + AutomatedCleaningMode: metal3api.CleaningModeMetadata, // Cleaning enabled + // No PreprovisioningImage provided + }, false, false) + + require.Error(t, err) + assert.ErrorIs(t, err, provisioner.ErrNeedsPreprovisioningImage) +} + +func TestRegisterDeprovisioningCleaningDisabledNoPreprovisioningImage(t *testing.T) { + // Test that when deprovisioning with cleaning disabled, + // no PreprovisioningImage is required + host := makeHost() + host.Status.Provisioning.ID = "uuid" + + ironic := testserver.NewIronic(t). + WithDrivers(). + Node(nodes.Node{ + Name: host.Namespace + nameSeparator + host.Name, + UUID: host.Status.Provisioning.ID, + ProvisionState: string(nodes.Manageable), + }).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) + } + // Enable preprov image builder + prov.config.havePreprovImgBuilder = true + + // Test StateDeprovisioning with cleaning disabled - should NOT require PreprovisioningImage + result, _, err := prov.Register(provisioner.ManagementAccessData{ + State: metal3api.StateDeprovisioning, + AutomatedCleaningMode: metal3api.CleaningModeDisabled, // Cleaning disabled + // No PreprovisioningImage provided + }, false, false) + + require.NoError(t, err) + assert.Empty(t, result.ErrorMessage) +}