From b2e8a1a42c95a3338c9c83a4781ba4744da5ff6a Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <dtantsur@protonmail.com>
Date: Tue, 24 Jun 2025 18:53:42 +0200
Subject: [PATCH 4/5] Refactor setting various Ironic properties

Currently, Ironic instance_info and properties fields are populated at
random either in most states or before deployment. While potentially
convenient, it makes it very hard to reason about the code.

Now, the logic is split into two parts:
1. configureNode (renamed from configureImages) writes fields that are
   considered properties of the node itself: CPU architecture, deploy
   images, capabilities, etc.
2. getInstanceUpdateOpts (merge of getImageUpdateOptsForNode and
   getUpdateOptsForNode) writes fields that are required for deployment
   and thus are properties of instance. This includes images, checksums,
   runtime capabilities. As an exception, root device hints fall under
   this category and thus are now set in instance_info, not properties.

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
(cherry picked from commit 0c70cba38c926c474f4fa129a7e99ef9827d6ce9)
---
 .../metal3.io/baremetalhost_controller.go     |  2 +-
 pkg/provisioner/ironic/ironic.go              | 49 +++++-------
 pkg/provisioner/ironic/provision_test.go      | 27 +++----
 pkg/provisioner/ironic/register.go            |  3 +-
 pkg/provisioner/ironic/register_test.go       | 78 +------------------
 pkg/provisioner/provisioner.go                |  2 +-
 6 files changed, 40 insertions(+), 121 deletions(-)

diff --git a/internal/controller/metal3.io/baremetalhost_controller.go b/internal/controller/metal3.io/baremetalhost_controller.go
index 1998627e..0d0c9562 100644
--- a/internal/controller/metal3.io/baremetalhost_controller.go
+++ b/internal/controller/metal3.io/baremetalhost_controller.go
@@ -848,6 +848,7 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
 			PreprovisioningNetworkData: preprovisioningNetworkData,
 			HasCustomDeploy:            hasCustomDeploy(info.host),
 			DisablePowerOff:            info.host.Spec.DisablePowerOff,
+			CPUArchitecture:            getHostArchitecture(info.host),
 		},
 		credsChanged,
 		info.host.Status.ErrorType == metal3api.RegistrationError)
@@ -1271,7 +1272,6 @@ func (r *BareMetalHostReconciler) actionProvisioning(prov provisioner.Provisione
 		BootMode:        info.host.Status.Provisioning.BootMode,
 		HardwareProfile: hwProf,
 		RootDeviceHints: info.host.Status.Provisioning.RootDeviceHints.DeepCopy(),
-		CPUArchitecture: getHostArchitecture(info.host),
 	}, forceReboot)
 	if err != nil {
 		return actionError{errors.Wrap(err, "failed to provision")}
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 48db865a..b8e6d72b 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -311,20 +311,24 @@ func (p *ironicProvisioner) createPXEEnabledNodePort(uuid, macAddress string) er
 	return nil
 }
 
-func (p *ironicProvisioner) configureImages(data provisioner.ManagementAccessData, ironicNode *nodes.Node, bmcAccess bmc.AccessDetails) (result provisioner.Result, err error) {
+func (p *ironicProvisioner) configureNode(data provisioner.ManagementAccessData, ironicNode *nodes.Node, bmcAccess bmc.AccessDetails) (result provisioner.Result, err error) {
 	updater := clients.UpdateOptsBuilder(p.log)
 
 	deployImageInfo := setDeployImage(p.config, bmcAccess, data.PreprovisioningImage)
 	updater.SetDriverInfoOpts(deployImageInfo, ironicNode)
 
-	// NOTE(dtantsur): It is risky to update image information for active nodes since it may affect the ability to clean up.
-	if (data.CurrentImage != nil || data.HasCustomDeploy) && ironicNode.ProvisionState != string(nodes.Active) {
-		p.getImageUpdateOptsForNode(ironicNode, data.CurrentImage, data.BootMode, data.HasCustomDeploy, updater)
-	}
 	updater.SetTopLevelOpt("automated_clean",
 		data.AutomatedCleaningMode != metal3api.CleaningModeDisabled,
 		ironicNode.AutomatedClean)
 
+	opts := clients.UpdateOptsData{
+		"capabilities": buildCapabilitiesValue(ironicNode, data.BootMode),
+	}
+	if data.CPUArchitecture != "" {
+		opts["cpu_arch"] = data.CPUArchitecture
+	}
+	updater.SetPropertiesOpts(opts, ironicNode)
+
 	_, success, result, err := p.tryUpdateNode(ironicNode, updater)
 	if !success {
 		return result, err
@@ -656,40 +660,29 @@ func (p *ironicProvisioner) setCustomDeployUpdateOptsForNode(ironicNode *nodes.N
 		SetTopLevelOpt("deploy_interface", "custom-agent", ironicNode.DeployInterface)
 }
 
-func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3api.Image, bootMode metal3api.BootMode, hasCustomDeploy bool, updater *clients.NodeUpdater) {
+func (p *ironicProvisioner) getInstanceUpdateOpts(ironicNode *nodes.Node, data provisioner.ProvisionData) *clients.NodeUpdater {
+	updater := clients.UpdateOptsBuilder(p.log)
+
+	hasCustomDeploy := data.CustomDeploy != nil && data.CustomDeploy.Method != ""
+
 	// instance_uuid
 	updater.SetTopLevelOpt("instance_uuid", string(p.objectMeta.UID), ironicNode.InstanceUUID)
 
 	updater.SetInstanceInfoOpts(clients.UpdateOptsData{
-		"capabilities": buildInstanceInfoCapabilities(bootMode),
+		"capabilities": buildInstanceInfoCapabilities(data.BootMode),
+		"root_device":  devicehints.MakeHintMap(data.RootDeviceHints),
 	}, ironicNode)
 
 	if hasCustomDeploy {
 		// Custom deploy process
-		p.setCustomDeployUpdateOptsForNode(ironicNode, imageData, updater)
-	} else if imageData.IsLiveISO() {
+		p.setCustomDeployUpdateOptsForNode(ironicNode, &data.Image, updater)
+	} else if data.Image.IsLiveISO() {
 		// Set live-iso format options
-		p.setLiveIsoUpdateOptsForNode(ironicNode, imageData, updater)
+		p.setLiveIsoUpdateOptsForNode(ironicNode, &data.Image, updater)
 	} else {
 		// Set deploy_interface direct options when not booting a live-iso
-		p.setDirectDeployUpdateOptsForNode(ironicNode, imageData, updater)
+		p.setDirectDeployUpdateOptsForNode(ironicNode, &data.Image, updater)
 	}
-}
-
-func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node, data provisioner.ProvisionData) *clients.NodeUpdater {
-	updater := clients.UpdateOptsBuilder(p.log)
-
-	hasCustomDeploy := data.CustomDeploy != nil && data.CustomDeploy.Method != ""
-	p.getImageUpdateOptsForNode(ironicNode, &data.Image, data.BootMode, hasCustomDeploy, updater)
-
-	opts := clients.UpdateOptsData{
-		"root_device":  devicehints.MakeHintMap(data.RootDeviceHints),
-		"capabilities": buildCapabilitiesValue(ironicNode, data.BootMode),
-	}
-	if data.CPUArchitecture != "" {
-		opts["cpu_arch"] = data.CPUArchitecture
-	}
-	updater.SetPropertiesOpts(opts, ironicNode)
 
 	return updater
 }
@@ -792,7 +785,7 @@ func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, data pr
 	p.log.Info("starting provisioning", "node properties", ironicNode.Properties)
 
 	ironicNode, success, result, err := p.tryUpdateNode(ironicNode,
-		p.getUpdateOptsForNode(ironicNode, data))
+		p.getInstanceUpdateOpts(ironicNode, data))
 	if !success {
 		return result, err
 	}
diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go
index 72ee57b7..40c714e9 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -713,7 +713,7 @@ func TestGetUpdateOptsForNodeWithRootHints(t *testing.T) {
 		BootMode:        metal3api.DefaultBootMode,
 		RootDeviceHints: host.Status.Provisioning.RootDeviceHints,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -723,7 +723,7 @@ func TestGetUpdateOptsForNodeWithRootHints(t *testing.T) {
 		Value interface{}       // the value being passed to ironic (or value associated with the key)
 	}{
 		{
-			Path:  "/properties/root_device",
+			Path:  "/instance_info/root_device",
 			Value: "userdefined_devicename",
 			Map: map[string]string{
 				"name":                 "s== userd_devicename",
@@ -807,7 +807,7 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 		BootMode:        metal3api.DefaultBootMode,
 		HardwareProfile: hwProf,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -903,9 +903,8 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 		Image:           *host.Spec.Image,
 		BootMode:        metal3api.DefaultBootMode,
 		HardwareProfile: hwProf,
-		CPUArchitecture: "x86_64",
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -930,10 +929,6 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 			Path:  "/instance_uuid",
 			Value: "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
 		},
-		{
-			Path:  "/properties/cpu_arch",
-			Value: "x86_64",
-		},
 	}
 
 	for _, e := range expected {
@@ -971,7 +966,7 @@ func TestGetUpdateOptsForNodeLiveIso(t *testing.T) {
 		Image:    *host.Spec.Image,
 		BootMode: metal3api.DefaultBootMode,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -1038,7 +1033,7 @@ func TestGetUpdateOptsForNodeImageToLiveIso(t *testing.T) {
 		Image:    *host.Spec.Image,
 		BootMode: metal3api.DefaultBootMode,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -1116,7 +1111,7 @@ func TestGetUpdateOptsForNodeLiveIsoToImage(t *testing.T) {
 		Image:    *host.Spec.Image,
 		BootMode: metal3api.DefaultBootMode,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -1188,7 +1183,7 @@ func TestGetUpdateOptsForNodeCustomDeploy(t *testing.T) {
 		BootMode:     metal3api.DefaultBootMode,
 		CustomDeploy: host.Spec.CustomDeploy,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -1245,7 +1240,7 @@ func TestGetUpdateOptsForNodeCustomDeployWithImage(t *testing.T) {
 		BootMode:     metal3api.DefaultBootMode,
 		CustomDeploy: host.Spec.CustomDeploy,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -1312,7 +1307,7 @@ func TestGetUpdateOptsForNodeImageToCustomDeploy(t *testing.T) {
 		BootMode:     metal3api.DefaultBootMode,
 		CustomDeploy: host.Spec.CustomDeploy,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
@@ -1405,7 +1400,7 @@ func TestGetUpdateOptsForNodeSecureBoot(t *testing.T) {
 		BootMode:        metal3api.UEFISecureBoot,
 		HardwareProfile: hwProf,
 	}
-	patches := prov.getUpdateOptsForNode(ironicNode, provData).Updates
+	patches := prov.getInstanceUpdateOpts(ironicNode, provData).Updates
 
 	t.Logf("patches: %v", patches)
 
diff --git a/pkg/provisioner/ironic/register.go b/pkg/provisioner/ironic/register.go
index 390e463f..9a600189 100644
--- a/pkg/provisioner/ironic/register.go
+++ b/pkg/provisioner/ironic/register.go
@@ -220,7 +220,7 @@ func (p *ironicProvisioner) Register(data provisioner.ManagementAccessData, cred
 		fallthrough
 
 	default:
-		result, err = p.configureImages(data, ironicNode, bmcAccess)
+		result, err = p.configureNode(data, ironicNode, bmcAccess)
 		return result, provID, err
 	}
 }
@@ -246,6 +246,7 @@ func (p *ironicProvisioner) enrollNode(data provisioner.ManagementAccessData, bm
 		DisablePowerOff:     &data.DisablePowerOff,
 		Properties: map[string]interface{}{
 			"capabilities": buildCapabilitiesValue(nil, data.BootMode),
+			"cpu_arch":     data.CPUArchitecture,
 		},
 	}
 
diff --git a/pkg/provisioner/ironic/register_test.go b/pkg/provisioner/ironic/register_test.go
index e6c302b5..8e524dad 100644
--- a/pkg/provisioner/ironic/register_test.go
+++ b/pkg/provisioner/ironic/register_test.go
@@ -72,7 +72,7 @@ func TestRegisterMACOptional(t *testing.T) {
 	assert.Equal(t, "", result.ErrorMessage)
 }
 
-func TestRegisterCreateNodeNoImage(t *testing.T) {
+func TestRegisterCreateNode(t *testing.T) {
 	// Create a host without a bootMACAddress and with a BMC that
 	// does not require one.
 	host := makeHost()
@@ -146,79 +146,6 @@ func TestRegisterCreateNodeOldInspection(t *testing.T) {
 	assert.Equal(t, "inspector", createdNode.InspectInterface)
 }
 
-func TestRegisterCreateWithImage(t *testing.T) {
-	// Create a host with Image specified in the Spec
-	host := makeHost()
-	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
-	host.Spec.Image.URL = "theimagefoo"
-	host.Spec.Image.Checksum = "thechecksumxyz"
-	host.Spec.Image.ChecksumType = "auto"
-
-	var createdNode *nodes.Node
-
-	createCallback := func(node nodes.Node) {
-		createdNode = &node
-	}
-
-	ironic := testserver.NewIronic(t).WithDrivers().CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
-	ironic.AddDefaultResponse("/v1/nodes/node-0", "PATCH", http.StatusOK, "{}")
-	ironic.Start()
-	defer ironic.Stop()
-
-	auth := clients.AuthConfig{Type: clients.NoAuth}
-	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher, ironic.Endpoint(), auth)
-	if err != nil {
-		t.Fatalf("could not create provisioner: %s", err)
-	}
-
-	result, provID, err := prov.Register(provisioner.ManagementAccessData{CurrentImage: host.Spec.Image.DeepCopy()}, false, false)
-	if err != nil {
-		t.Fatalf("error from Register: %s", err)
-	}
-	assert.Equal(t, "", result.ErrorMessage)
-	assert.Equal(t, createdNode.UUID, provID)
-	assert.Equal(t, "", createdNode.DeployInterface)
-	updates, _ := ironic.GetLastRequestFor("/v1/nodes/node-0", http.MethodPatch)
-	assert.Contains(t, updates, "/instance_info/image_source")
-	assert.Contains(t, updates, host.Spec.Image.URL)
-	assert.Contains(t, updates, "/instance_info/image_checksum")
-	assert.Contains(t, updates, host.Spec.Image.Checksum)
-}
-
-func TestRegisterCreateWithLiveIso(t *testing.T) {
-	// Create a host with Image specified in the Spec
-	host := makeHostLiveIso()
-	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
-
-	var createdNode *nodes.Node
-
-	createCallback := func(node nodes.Node) {
-		createdNode = &node
-	}
-
-	ironic := testserver.NewIronic(t).WithDrivers().CreateNodes(createCallback).NoNode(host.Namespace + nameSeparator + host.Name).NoNode(host.Name)
-	ironic.AddDefaultResponse("/v1/nodes/node-0", "PATCH", http.StatusOK, "{}")
-	ironic.Start()
-	defer ironic.Stop()
-
-	auth := clients.AuthConfig{Type: clients.NoAuth}
-	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher, ironic.Endpoint(), auth)
-	if err != nil {
-		t.Fatalf("could not create provisioner: %s", err)
-	}
-
-	result, provID, err := prov.Register(provisioner.ManagementAccessData{CurrentImage: host.Spec.Image.DeepCopy()}, false, false)
-	if err != nil {
-		t.Fatalf("error from Register: %s", err)
-	}
-	assert.Equal(t, "", result.ErrorMessage)
-	assert.Equal(t, createdNode.UUID, provID)
-	assert.Equal(t, "ramdisk", createdNode.DeployInterface)
-	updates, _ := ironic.GetLastRequestFor("/v1/nodes/node-0", http.MethodPatch)
-	assert.Contains(t, updates, "/instance_info/boot_iso")
-	assert.Contains(t, updates, host.Spec.Image.URL)
-}
-
 func TestRegisterExistingNode(t *testing.T) {
 	// Create a host without a bootMACAddress and with a BMC that
 	// does not require one.
@@ -342,6 +269,7 @@ func TestRegisterExistingNodeContinue(t *testing.T) {
 					"test_password":  "******", // ironic returns a placeholder
 					"test_port":      "42",
 				},
+				Properties: map[string]interface{}{"capabilities": ""},
 			}).NodeUpdate(nodes.Node{
 				UUID: "uuid",
 			})
@@ -521,6 +449,7 @@ func TestRegisterExistingSteadyStateNoUpdate(t *testing.T) {
 				DeployInterface: imageType.DeployInterface,
 				InstanceInfo:    imageType.InstanceInfo,
 				DriverInfo:      imageType.DriverInfo,
+				Properties:      map[string]interface{}{"capabilities": ""},
 			}).NodeUpdate(nodes.Node{
 				UUID: "uuid",
 			})
@@ -577,6 +506,7 @@ func TestRegisterExistingNodeWaiting(t *testing.T) {
 					"test_password":  "******", // ironic returns a placeholder
 					"test_port":      "42",
 				},
+				Properties: map[string]interface{}{"capabilities": ""},
 			}
 			ironic := testserver.NewIronic(t).CreateNodes(createCallback).Node(node).NodeUpdate(nodes.Node{
 				UUID: "uuid",
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index faddd0fd..e2018e63 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -82,6 +82,7 @@ type ManagementAccessData struct {
 	PreprovisioningNetworkData string
 	HasCustomDeploy            bool
 	DisablePowerOff            bool
+	CPUArchitecture            string
 }
 
 type AdoptData struct {
@@ -122,7 +123,6 @@ type ProvisionData struct {
 	HardwareProfile profile.Profile
 	RootDeviceHints *metal3api.RootDeviceHints
 	CustomDeploy    *metal3api.CustomDeploy
-	CPUArchitecture string
 }
 
 type HTTPHeaders []map[string]string
-- 
2.50.1

