Repository: metal3-io/baremetal-operator
Pull request #1207 (2023-01-27 15:58:42)

🌱 Separate updating images from ValidateManagementAccess
Contrary to its name, ValidateManagementAccess also enrolls the node and
configures various deploy-related parameters. This change splits
cleaning, image, custom deploy and ramdisk settings to a new function
with a goal to eventually delineate them.

Fix tryUpdateNode to return the updated node.



Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index a4a73f9c..9b760a95 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -355,7 +355,6 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 
 	driverInfo := bmcAccess.DriverInfo(p.bmcCreds)
 	driverInfo = setExternalURL(p, driverInfo)
-	deployImageInfo := setDeployImage(driverInfo, p.config, bmcAccess, data.PreprovisioningImage)
 
 	// If we have not found a node yet, we need to create one
 	if ironicNode == nil {
@@ -453,23 +452,14 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 		// otherwise we will be writing on every call to this function.
 		if credentialsChanged {
 			updater.SetTopLevelOpt("driver_info", driverInfo, ironicNode.DriverInfo)
-		} else {
-			updater.SetDriverInfoOpts(deployImageInfo, ironicNode)
 		}
 
 		// We don't return here because we also have to set the
 		// target provision state to manageable, which happens
 		// below.
 	}
-	if data.CurrentImage != nil || data.HasCustomDeploy {
-		p.getImageUpdateOptsForNode(ironicNode, data.CurrentImage, data.BootMode, data.HasCustomDeploy, updater)
-	}
-	updater.SetTopLevelOpt("automated_clean",
-		data.AutomatedCleaningMode != metal3v1alpha1.CleaningModeDisabled,
-		ironicNode.AutomatedClean)
 
-	var success bool
-	success, result, err = p.tryUpdateNode(ironicNode, updater)
+	ironicNode, success, result, err := p.tryUpdateNode(ironicNode, updater)
 	if !success {
 		return
 	}
@@ -483,8 +473,6 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 		"target", ironicNode.TargetProvisionState,
 	)
 
-	result, err = operationComplete()
-
 	// Ensure the node is marked manageable.
 	switch nodes.ProvisionState(ironicNode.ProvisionState) {
 
@@ -530,33 +518,59 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 		fallthrough
 
 	default:
-		switch data.State {
-		case metal3v1alpha1.StateProvisioning,
-			metal3v1alpha1.StateDeprovisioning:
-			if data.State == metal3v1alpha1.StateProvisioning {
-				if data.CurrentImage.IsLiveISO() {
-					// Live ISO doesn't need pre-provisioning image
-					return
-				}
-			} else {
-				if data.AutomatedCleaningMode == metal3v1alpha1.CleaningModeDisabled {
-					// No need for pre-provisioning image if cleaning disabled
-					return
-				}
+		result, err = p.configureImages(data, ironicNode, bmcAccess)
+		return
+	}
+}
+
+func (p *ironicProvisioner) configureImages(data provisioner.ManagementAccessData, ironicNode *nodes.Node, bmcAccess bmc.AccessDetails) (result provisioner.Result, err error) {
+	updater := updateOptsBuilder(p.debugLog)
+
+	deployImageInfo := setDeployImage(p.config, bmcAccess, data.PreprovisioningImage)
+	updater.SetDriverInfoOpts(deployImageInfo, ironicNode)
+
+	if data.CurrentImage != nil || data.HasCustomDeploy {
+		p.getImageUpdateOptsForNode(ironicNode, data.CurrentImage, data.BootMode, data.HasCustomDeploy, updater)
+	}
+	updater.SetTopLevelOpt("automated_clean",
+		data.AutomatedCleaningMode != metal3v1alpha1.CleaningModeDisabled,
+		ironicNode.AutomatedClean)
+
+	_, success, result, err := p.tryUpdateNode(ironicNode, updater)
+	if !success {
+		return
+	}
+
+	result, err = operationComplete()
+
+	switch data.State {
+	case metal3v1alpha1.StateProvisioning,
+		metal3v1alpha1.StateDeprovisioning:
+		if data.State == metal3v1alpha1.StateProvisioning {
+			if data.CurrentImage.IsLiveISO() {
+				// Live ISO doesn't need pre-provisioning image
+				return
 			}
-			fallthrough
-		case metal3v1alpha1.StateInspecting,
-			metal3v1alpha1.StatePreparing:
-			if deployImageInfo == nil {
-				if p.config.havePreprovImgBuilder {
-					result, err = transientError(provisioner.ErrNeedsPreprovisioningImage)
-				} else {
-					result, err = operationFailed("no preprovisioning image available")
-				}
+		} else {
+			if data.AutomatedCleaningMode == metal3v1alpha1.CleaningModeDisabled {
+				// No need for pre-provisioning image if cleaning disabled
+				return
 			}
 		}
-		return
+		fallthrough
+	case metal3v1alpha1.StateInspecting,
+		metal3v1alpha1.StatePreparing:
+		if deployImageInfo == nil {
+			if p.config.havePreprovImgBuilder {
+				result, err = transientError(provisioner.ErrNeedsPreprovisioningImage)
+			} else {
+				result, err = operationFailed("no preprovisioning image available")
+			}
+			return
+		}
 	}
+
+	return
 }
 
 // PreprovisioningImageFormats returns a list of acceptable formats for a
@@ -623,7 +637,7 @@ func setExternalURL(p *ironicProvisioner, driverInfo map[string]interface{}) map
 	return driverInfo
 }
 
-func setDeployImage(driverInfo map[string]interface{}, config ironicConfig, accessDetails bmc.AccessDetails, hostImage *provisioner.PreprovisioningImage) optionsData {
+func setDeployImage(config ironicConfig, accessDetails bmc.AccessDetails, hostImage *provisioner.PreprovisioningImage) optionsData {
 	deployImageInfo := optionsData{
 		deployKernelKey:  nil,
 		deployRamdiskKey: nil,
@@ -631,16 +645,6 @@ func setDeployImage(driverInfo map[string]interface{}, config ironicConfig, acce
 		kernelParamsKey:  nil,
 	}
 
-	defer func() {
-		// Copy into driverInfo so that if we end up creating a new Node, the
-		// info will be there from the outset.
-		for k, v := range deployImageInfo {
-			if v != nil {
-				driverInfo[k] = v
-			}
-		}
-	}()
-
 	allowISO := accessDetails.SupportsISOPreprovisioningImage()
 
 	if hostImage != nil {
@@ -682,14 +686,15 @@ func setDeployImage(driverInfo map[string]interface{}, config ironicConfig, acce
 	return nil
 }
 
-func (p *ironicProvisioner) tryUpdateNode(ironicNode *nodes.Node, updater *nodeUpdater) (success bool, result provisioner.Result, err error) {
+func (p *ironicProvisioner) tryUpdateNode(ironicNode *nodes.Node, updater *nodeUpdater) (updatedNode *nodes.Node, success bool, result provisioner.Result, err error) {
 	if len(updater.Updates) == 0 {
+		updatedNode = ironicNode
 		success = true
 		return
 	}
 
 	p.log.Info("updating node settings in ironic")
-	_, err = nodes.Update(p.client, ironicNode.UUID, updater.Updates).Extract()
+	updatedNode, err = nodes.Update(p.client, ironicNode.UUID, updater.Updates).Extract()
 	switch err.(type) {
 	case nil:
 		success = true
@@ -699,6 +704,7 @@ func (p *ironicProvisioner) tryUpdateNode(ironicNode *nodes.Node, updater *nodeU
 	default:
 		result, err = transientError(errors.Wrap(err, "failed to update host settings in ironic"))
 	}
+
 	return
 }
 
@@ -783,7 +789,7 @@ func (p *ironicProvisioner) InspectHardware(data provisioner.InspectData, force,
 				}
 				fallthrough
 			default:
-				started, result, err = p.tryUpdateNode(
+				_, started, result, err = p.tryUpdateNode(
 					ironicNode,
 					updateOptsBuilder(p.debugLog).
 						SetPropertiesOpts(optionsData{
@@ -1098,7 +1104,7 @@ func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, data pr
 
 	p.log.Info("starting provisioning", "node properties", ironicNode.Properties)
 
-	success, result, err := p.tryUpdateNode(ironicNode,
+	ironicNode, success, result, err := p.tryUpdateNode(ironicNode,
 		p.getUpdateOptsForNode(ironicNode, data))
 	if !success {
 		return
@@ -1595,7 +1601,7 @@ func (p *ironicProvisioner) Provision(data provisioner.ProvisionData) (result pr
 }
 
 func (p *ironicProvisioner) setMaintenanceFlag(ironicNode *nodes.Node, value bool) (result provisioner.Result, err error) {
-	success, result, err := p.tryUpdateNode(ironicNode,
+	_, success, result, err := p.tryUpdateNode(ironicNode,
 		updateOptsBuilder(p.log).SetTopLevelOpt("maintenance", value, nil))
 	if err != nil {
 		err = fmt.Errorf("failed to set host maintenance flag to %v (%w)", value, err)
@@ -1732,7 +1738,7 @@ func (p *ironicProvisioner) Delete() (result provisioner.Result, err error) {
 			p.log.Info("removing stale instance UUID before deletion", "instanceUUID", ironicNode.InstanceUUID)
 			updater := updateOptsBuilder(p.debugLog)
 			updater.SetTopLevelOpt("instance_uuid", nil, ironicNode.InstanceUUID)
-			success, result, err := p.tryUpdateNode(ironicNode, updater)
+			_, success, result, err := p.tryUpdateNode(ironicNode, updater)
 			if !success {
 				return result, err
 			}
diff --git a/pkg/provisioner/ironic/raid.go b/pkg/provisioner/ironic/raid.go
index d3300100..868a3e68 100644
--- a/pkg/provisioner/ironic/raid.go
+++ b/pkg/provisioner/ironic/raid.go
@@ -45,7 +45,7 @@ func setTargetRAIDCfg(p *ironicProvisioner, raidInterface string, ironicNode *no
 
 	updater := updateOptsBuilder(p.debugLog)
 	updater.SetTopLevelOpt("raid_interface", targetRaidInterface, ironicNode.RAIDInterface)
-	success, result, err := p.tryUpdateNode(ironicNode, updater)
+	ironicNode, success, result, err := p.tryUpdateNode(ironicNode, updater)
 	if !success {
 		return result, err
 	}


Test patch:
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index 5eb202a7..ca840731 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -458,6 +458,7 @@ func TestValidateManagementAccessExistingNodeWaiting(t *testing.T) {
 	statuses := []nodes.ProvisionState{
 		nodes.Enroll,
 		nodes.Verifying,
+		nodes.Inspecting,
 	}
 
 	for _, status := range statuses {
@@ -504,12 +505,16 @@ func TestValidateManagementAccessExistingNodeWaiting(t *testing.T) {
 				t.Fatalf("error from ValidateManagementAccess: %s", err)
 			}
 			assert.Equal(t, "", result.ErrorMessage)
-			assert.Equal(t, true, result.Dirty)
 
-			updates := ironic.GetLastNodeUpdateRequestFor("uuid")
-			assert.Len(t, updates, 1)
-			assert.Equal(t, "/automated_clean", updates[0].Path)
-			assert.Equal(t, true, updates[0].Value)
+			if status == nodes.Inspecting {
+				assert.False(t, result.Dirty)
+				updates := ironic.GetLastNodeUpdateRequestFor("uuid")
+				assert.Len(t, updates, 1)
+				assert.Equal(t, "/automated_clean", updates[0].Path)
+				assert.Equal(t, true, updates[0].Value)
+			} else {
+				assert.True(t, result.Dirty)
+			}
 		})
 	}
 }
@@ -524,13 +529,15 @@ func TestValidateManagementAccessNewCredentials(t *testing.T) {
 	ironic := testserver.NewIronic(t).
 		Node(
 			nodes.Node{
-				Name: host.Namespace + nameSeparator + host.Name,
-				UUID: "uuid",
+				Name:           host.Namespace + nameSeparator + host.Name,
+				ProvisionState: string(nodes.Verifying),
+				UUID:           "uuid",
 			}).
 		NodeUpdate(
 			nodes.Node{
-				Name: host.Namespace + nameSeparator + host.Name,
-				UUID: "uuid",
+				Name:           host.Namespace + nameSeparator + host.Name,
+				ProvisionState: string(nodes.Verifying),
+				UUID:           "uuid",
 				DriverInfo: map[string]interface{}{
 					"test_address": "test.bmc",
 				},
@@ -579,7 +586,7 @@ func TestValidateManagementAccessLinkExistingIronicNodeByMAC(t *testing.T) {
 	ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(existingNode).Port(existingNodePort)
 	ironic.AddDefaultResponse("/v1/nodes/myns"+nameSeparator+"myhost", "GET", http.StatusNotFound, "")
 	ironic.AddDefaultResponse("/v1/nodes/myhost", "GET", http.StatusNotFound, "")
-	ironic.AddDefaultResponse("/v1/nodes/"+existingNode.UUID, "PATCH", http.StatusOK, "{}")
+	ironic.AddDefaultResponse("/v1/nodes/"+existingNode.UUID, "PATCH", http.StatusOK, "{\"uuid\": \"33ce8659-7400-4c68-9535-d10766f07a58\"}")
 	ironic.Start()
 	defer ironic.Stop()
 
@@ -1097,53 +1104,38 @@ func TestSetDeployImage(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.Scenario, func(t *testing.T) {
-			driverInfo := make(map[string]interface{}, 0)
-			opts := setDeployImage(driverInfo, tc.Config, tc.Driver, tc.Image)
+			opts := setDeployImage(tc.Config, tc.Driver, tc.Image)
 
 			switch {
 			case tc.ExpectISO:
 				if tc.ExpectBuild {
 					assert.Equal(t, buildIso, opts["deploy_iso"])
-					assert.Equal(t, buildIso, driverInfo["deploy_iso"])
 				} else {
 					assert.Equal(t, localIso, opts["deploy_iso"])
-					assert.Equal(t, localIso, driverInfo["deploy_iso"])
 				}
 				assert.Nil(t, opts["deploy_kernel"])
 				assert.Nil(t, opts["deploy_ramdisk"])
-				assert.Nil(t, driverInfo["deploy_kernel"])
-				assert.Nil(t, driverInfo["deploy_ramdisk"])
 				assert.Nil(t, opts["kernel_append_params"])
-				assert.Nil(t, driverInfo["kernel_append_params"])
 			case tc.ExpectPXE:
 				assert.Nil(t, opts["deploy_iso"])
-				assert.Nil(t, driverInfo["deploy_iso"])
 				if tc.ExpectBuild {
 					if tc.ExpectNewKernel {
 						assert.Equal(t, buildKernel, opts["deploy_kernel"])
-						assert.Equal(t, buildKernel, driverInfo["deploy_kernel"])
 					} else {
 						assert.Equal(t, localKernel, opts["deploy_kernel"])
-						assert.Equal(t, localKernel, driverInfo["deploy_kernel"])
 					}
 					assert.Equal(t, buildRamdisk, opts["deploy_ramdisk"])
-					assert.Equal(t, buildRamdisk, driverInfo["deploy_ramdisk"])
 				} else {
 					assert.Equal(t, localKernel, opts["deploy_kernel"])
 					assert.Equal(t, localRamdisk, opts["deploy_ramdisk"])
-					assert.Equal(t, localKernel, driverInfo["deploy_kernel"])
-					assert.Equal(t, localRamdisk, driverInfo["deploy_ramdisk"])
 				}
 				if tc.ExpectKernelParams {
 					assert.Equal(t, expectedKernelParams, opts["kernel_append_params"])
-					assert.Equal(t, expectedKernelParams, driverInfo["kernel_append_params"])
 				} else {
 					assert.Nil(t, opts["kernel_append_params"])
-					assert.Nil(t, driverInfo["kernel_append_params"])
 				}
 			default:
 				assert.Nil(t, opts)
-				assert.Empty(t, driverInfo)
 			}
 		})
 	}