Merge pull request #337 from honza/backport-4.13-instance-info

[release-4.13] OCPBUGS-30629: Do not update instance_info and deploy_interface for active nodes
openshift · Mar 12, 2024 · f6db335 · f6db335
2 parents ecd1733 + daf234b
commit f6db335
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 3 deletions.
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
@@ -530,7 +530,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",

diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -348,6 +348,8 @@ func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) {
 		Image           *metal3v1alpha1.Image
 		InstanceInfo    map[string]interface{}
 		DriverInfo      map[string]interface{}
+		ProvisionState  string
+		HasCustomDeploy bool
 	}{
 		{
 			DeployInterface: "",
@@ -405,6 +407,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: &metal3v1alpha1.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
 
@@ -420,10 +463,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,
@@ -443,7 +490,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)
 			}