Repository: metal3-io/baremetal-operator
Pull request #1428 (2023-11-03 12:00:48)

🐛 Do not update instance_info and deploy_interface for active nodes
Doing so may interfere with cleaning. Updates to deploy_interface will
be rejected by Ironic, causing BMO to enter infinite reconcile retries.


Patch:
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 8d4f952a..2dc7e6d1 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -546,7 +546,8 @@ func (p *ironicProvisioner) configureImages(data provisioner.ManagementAccessDat
 	deployImageInfo := setDeployImage(p.config, bmcAccess, data.PreprovisioningImage)
 	updater.SetDriverInfoOpts(deployImageInfo, ironicNode)
 
-	if data.CurrentImage != nil || data.HasCustomDeploy {
+	// 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",


Test patch:
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index bb914877..819f88ec 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -330,8 +330,10 @@ func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) {
 	imageTypes := []struct {
 		DeployInterface string
 		Image           *metal3api.Image
+		HasCustomDeploy bool
 		InstanceInfo    map[string]interface{}
 		DriverInfo      map[string]interface{}
+		ProvisionState  string
 	}{
 		{
 			DeployInterface: "",
@@ -389,6 +391,47 @@ func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) {
 				"test_port":                    "42",
 			},
 		},
+		{
+			DeployInterface: "custom-agent",
+			HasCustomDeploy: true,
+			InstanceInfo: map[string]interface{}{
+				"capabilities": map[string]interface{}{},
+			},
+			DriverInfo: map[string]interface{}{
+				"force_persistent_boot_device": "Default",
+				"deploy_kernel":                "http://deploy.test/ipa.kernel",
+				"deploy_ramdisk":               "http://deploy.test/ipa.initramfs",
+				"test_address":                 "test.bmc",
+				"test_username":                "",
+				"test_password":                "******", // ironic returns a placeholder
+				"test_port":                    "42",
+			},
+		},
+		// NOTE(dtantsur): This is a corner case. If the node is active, do not change any instance information until it's deprovisioned.
+		// Otherwise, clean up may not work correctly (and updates to deploy_interface will be rejected anyway).
+		{
+			ProvisionState: string(nodes.Active),
+			Image: &metal3api.Image{
+				URL:        "theimage",
+				DiskFormat: &liveFormat,
+			},
+			InstanceInfo: map[string]interface{}{
+				"image_source":        "theimage",
+				"image_os_hash_algo":  "md5",
+				"image_os_hash_value": "thechecksum",
+				"image_checksum":      "thechecksum",
+				"capabilities":        map[string]interface{}{},
+			},
+			DriverInfo: map[string]interface{}{
+				"force_persistent_boot_device": "Default",
+				"deploy_kernel":                "http://deploy.test/ipa.kernel",
+				"deploy_ramdisk":               "http://deploy.test/ipa.initramfs",
+				"test_address":                 "test.bmc",
+				"test_username":                "",
+				"test_password":                "******", // ironic returns a placeholder
+				"test_port":                    "42",
+			},
+		},
 	}
 	clean := true
 
@@ -404,10 +447,14 @@ func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) {
 				t.Fatal("create callback should not be invoked for existing node")
 			}
 
+			provisionState := imageType.ProvisionState
+			if provisionState == "" {
+				provisionState = string(nodes.Manageable)
+			}
 			ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).Node(nodes.Node{
 				Name:            host.Namespace + nameSeparator + host.Name,
 				UUID:            "uuid", // to match status in host
-				ProvisionState:  string(nodes.Manageable),
+				ProvisionState:  provisionState,
 				AutomatedClean:  &clean,
 				InstanceUUID:    string(host.UID),
 				DeployInterface: imageType.DeployInterface,
@@ -425,7 +472,8 @@ func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) {
 				t.Fatalf("could not create provisioner: %s", err)
 			}
 
-			result, _, err := prov.ValidateManagementAccess(provisioner.ManagementAccessData{CurrentImage: imageType.Image}, false, false)
+			data := provisioner.ManagementAccessData{CurrentImage: imageType.Image, HasCustomDeploy: imageType.HasCustomDeploy}
+			result, _, err := prov.ValidateManagementAccess(data, false, false)
 			if err != nil {
 				t.Fatalf("error from ValidateManagementAccess: %s", err)
 			}