Repository: metal3-io/baremetal-operator
Pull request #1107 (2022-04-11 05:45:31)

Improve webhook verification
Part of #855 

- Add validations for ImageUrl
- Add validations for Image.DiskFormat

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **dtantsur** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @dtantsur` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dtantsur"]} -->
zhouhao3: /test-v1b1-centos-integration
/test-v1b1-ubuntu-integration
zhouhao3: /test-v1b1-centos-integration
/test-v1b1-ubuntu-integration
zhouhao3: /cc @zaneb @andfasano 
zhouhao3: @elfosardo @dtantsur PTAL
zhouhao3: /test-centos-integration-main
/test-ubuntu-integration-main
zhouhao3: @furkatgofurov7 @fmuyassarov PTAL, thanks!
zhouhao3: @dtantsur @fmuyassarov @elfosardo This PR has been stuck for a long time, please take a look if you have time. Thanks.
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/1107#pullrequestreview-1056796898" title="Approved">fmuyassarov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)~~ [fmuyassarov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
zhouhao3: /test-centos-integration-main
/test-ubuntu-integration-main
metal3-io-bot: @zhouhao3: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
gofmt | c6e327608eb859e07c462d3cfbc7dd5a7c7318d1 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1107/gofmt/1555383444819677184) | true | `/test gofmt`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1107). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
zhouhao3: /test-centos-integration-main
/test-ubuntu-integration-main
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | e4e6ec0a948b2ac09b9e8163cc4c7784724ae404 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1107/unit/1555383937948192768) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1107). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
zhouhao3: /test-centos-integration-main
/test-ubuntu-integration-main
zhouhao3: @honza @furkatgofurov7 PTAL

Patch:
diff --git a/OWNERS b/OWNERS
index d1d22bc0..6fdb1c61 100644
--- a/OWNERS
+++ b/OWNERS
@@ -4,13 +4,13 @@ approvers:
  - andfasano
  - dtantsur
  - fmuyassarov
- - honza
  - zaneb
 
 reviewers:
  - ardaguclu
  - bfournie
  - elfosardo
+ - honza
  - dukov
  - furkatgofurov7
  - kashifest
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index 5a64aa84..fd02cdcc 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -3,6 +3,7 @@ package v1alpha1
 import (
 	"fmt"
 	"net"
+	"net/url"
 	"regexp"
 
 	"github.com/google/uuid"
@@ -43,6 +44,12 @@ func (host *BareMetalHost) validateHost() []error {
 		errs = append(errs, err)
 	}
 
+	if host.Spec.Image != nil {
+		if err := validateImageURL(host.Spec.Image.URL); err != nil {
+			errs = append(errs, err)
+		}
+	}
+
 	return errs
 }
 
@@ -69,6 +76,7 @@ func (host *BareMetalHost) validateChanges(old *BareMetalHost) []error {
 
 func validateBMCAccess(s BareMetalHostSpec, bmcAccess bmc.AccessDetails) []error {
 	var errs []error
+	diskFormat := "live-iso"
 
 	if bmcAccess == nil {
 		return errs
@@ -101,6 +109,10 @@ func validateBMCAccess(s BareMetalHostSpec, bmcAccess bmc.AccessDetails) []error
 		errs = append(errs, fmt.Errorf("BMC driver %s does not support secure boot", bmcAccess.Type()))
 	}
 
+	if s.Image != nil && s.Image.DiskFormat != nil && *s.Image.DiskFormat == diskFormat && !bmcAccess.SupportsISOPreprovisioningImage() {
+		errs = append(errs, fmt.Errorf("BMC driver %s does not support live-iso image", bmcAccess.Type()))
+	}
+
 	return errs
 }
 
@@ -162,3 +174,13 @@ func validateDNSName(hostaddress string) error {
 
 	return nil
 }
+
+func validateImageURL(imageURL string) error {
+
+	_, err := url.ParseRequestURI(imageURL)
+	if err != nil {
+		return fmt.Errorf("Image URL %s is an invalid URL", imageURL)
+	}
+
+	return nil
+}
diff --git a/docs/api.md b/docs/api.md
index a58c557b..5dd84fa8 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -195,19 +195,19 @@ spec:
 
 **NOTE:** Currently the 'raid' field is only supported by ilo5/idrac/irmc.
 
-**NOTE:** Do not try to simultaneously delete hardware RAID volumes and add
-software RAID volumes. Delete hardware RAID first, wait for the host to settle
-down, add software volumes afterwards.
+**NOTE:** Software RAID will always be deleted.
 
 **NOTE:** If you got following error message:
 
 1. raid settings are defined, but the node's driver %s does not support RAID.
 2. node's driver %s does not support hardware RAID.
+3. node's driver %s does not support software RAID.
 
 You can solve it by:
 
 1. Keep raid field is nil.
 2. Keep hardwareRAIDVolumes field is nil.
+3. Keep softwareRAIDVolumes field is nil.
 
 If the error message you get isn't included the above, you may need to check
 whether the BM has a RAID controller and keep the raid field blank.
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 9c575752..aefc8419 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -666,18 +666,6 @@ func (p *ironicProvisioner) tryChangeNodeProvisionState(ironicNode *nodes.Node,
 		"new target", opts.Target,
 	)
 
-	// Changing provision state in maintenance mode will not work.
-	if ironicNode.Fault != "" {
-		p.log.Info("node has a fault, will retry", "fault", ironicNode.Fault, "reason", ironicNode.MaintenanceReason)
-		result, err = retryAfterDelay(provisionRequeueDelay)
-		return
-	}
-	if ironicNode.Maintenance {
-		p.log.Info("trying to change a provision state for a node in maintenance, removing maintenance first", "reason", ironicNode.MaintenanceReason)
-		result, err = p.setMaintenanceFlag(ironicNode, false)
-		return
-	}
-
 	changeResult := nodes.ChangeProvisionState(p.client, ironicNode.UUID, opts)
 	switch changeResult.Err.(type) {
 	case nil:
@@ -1145,11 +1133,6 @@ func (p *ironicProvisioner) Adopt(data provisioner.AdoptData, force bool) (resul
 		return operationFailed(fmt.Sprintf("Host adoption failed: %s",
 			ironicNode.LastError))
 	case nodes.Active:
-		// Empty Fault means that maintenance was set manually, not by Ironic
-		if ironicNode.Maintenance && ironicNode.Fault == "" && data.State != metal3v1alpha1.StateDeleting {
-			p.log.Info("active node was found to be in maintenance, updating", "state", data.State)
-			return p.setMaintenanceFlag(ironicNode, false)
-		}
 	default:
 	}
 	return operationComplete()
@@ -1682,19 +1665,15 @@ func (p *ironicProvisioner) Delete() (result provisioner.Result, err error) {
 		"deploy step", ironicNode.DeployStep,
 	)
 
-	currentProvState := nodes.ProvisionState(ironicNode.ProvisionState)
-	if currentProvState == nodes.Available || currentProvState == nodes.Manageable {
-		// Make sure we don't have a stale instance UUID
-		if ironicNode.InstanceUUID != "" {
-			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)
-			if !success {
-				return result, err
-			}
-		}
-	} else if !ironicNode.Maintenance {
+	if nodes.ProvisionState(ironicNode.ProvisionState) == nodes.Available {
+		// Move back to manageable so we can delete it cleanly.
+		return p.changeNodeProvisionState(
+			ironicNode,
+			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
+		)
+	}
+
+	if !ironicNode.Maintenance {
 		// If we see an active node and the controller doesn't think
 		// we need to deprovision it, that means the node was
 		// ExternallyProvisioned and we should remove it from Ironic
diff --git a/pkg/provisioner/ironic/raid.go b/pkg/provisioner/ironic/raid.go
index f6dbacbb..3c1e66d6 100644
--- a/pkg/provisioner/ironic/raid.go
+++ b/pkg/provisioner/ironic/raid.go
@@ -19,7 +19,7 @@ const (
 
 // setTargetRAIDCfg set the RAID settings to the ironic Node for RAID configuration steps
 func setTargetRAIDCfg(p *ironicProvisioner, raidInterface string, ironicNode *nodes.Node, data provisioner.PrepareData) (provisioner.Result, error) {
-	targetRaidInterface, err := CheckRAIDInterface(raidInterface, data.TargetRAIDConfig, data.ActualRAIDConfig)
+	err := CheckRAIDInterface(raidInterface, data.TargetRAIDConfig)
 	if err != nil {
 		return operationFailed(err.Error())
 	}
@@ -43,13 +43,6 @@ func setTargetRAIDCfg(p *ironicProvisioner, raidInterface string, ironicNode *no
 		p.log.Info("rootDeviceHints is used, the first volume of raid will not be set to root")
 	}
 
-	updater := updateOptsBuilder(p.debugLog)
-	updater.SetTopLevelOpt("raid_interface", targetRaidInterface, ironicNode.RAIDInterface)
-	success, result, err := p.tryUpdateNode(ironicNode, updater)
-	if !success {
-		return result, err
-	}
-
 	// Set target for RAID configuration steps
 	err = nodes.SetRAIDConfig(
 		p.client,
@@ -185,24 +178,28 @@ func buildTargetSoftwareRAIDCfg(volumes []metal3v1alpha1.SoftwareRAIDVolume) (lo
 
 // BuildRAIDCleanSteps build the clean steps for RAID configuration from BaremetalHost spec
 func BuildRAIDCleanSteps(raidInterface string, target *metal3v1alpha1.RAIDConfig, actual *metal3v1alpha1.RAIDConfig) (cleanSteps []nodes.CleanStep, err error) {
-	_, err = CheckRAIDInterface(raidInterface, target, actual)
+	err = CheckRAIDInterface(raidInterface, target)
 	if err != nil {
 		return nil, err
 	}
 
+	// No RAID
+	if raidInterface == noRAIDInterface {
+		return
+	}
+
 	// Software RAID
-	if target != nil && target.SoftwareRAIDVolumes != nil {
+	if raidInterface == softwareRAIDInterface {
 		// Ignore HardwareRAIDVolumes
-		target.HardwareRAIDVolumes = nil
+		if target != nil {
+			target.HardwareRAIDVolumes = nil
+		}
 		if actual != nil {
 			actual.HardwareRAIDVolumes = nil
 		}
 		if reflect.DeepEqual(target, actual) {
 			return
 		}
-		if len(target.SoftwareRAIDVolumes) == 0 && (actual == nil || len(actual.SoftwareRAIDVolumes) == 0) {
-			return
-		}
 
 		cleanSteps = append(
 			cleanSteps,
@@ -219,7 +216,7 @@ func BuildRAIDCleanSteps(raidInterface string, target *metal3v1alpha1.RAIDConfig
 		)
 
 		// If software raid configuration is empty, only need to clear old configuration
-		if len(target.SoftwareRAIDVolumes) == 0 {
+		if target == nil || len(target.SoftwareRAIDVolumes) == 0 {
 			return
 		}
 
@@ -236,7 +233,7 @@ func BuildRAIDCleanSteps(raidInterface string, target *metal3v1alpha1.RAIDConfig
 	// Hardware RAID
 	// If hardware RAID configuration is nil,
 	// keep old hardware RAID configuration
-	if raidInterface == noRAIDInterface || target == nil || target.HardwareRAIDVolumes == nil {
+	if target == nil || target.HardwareRAIDVolumes == nil {
 		return
 	}
 
@@ -277,31 +274,22 @@ func BuildRAIDCleanSteps(raidInterface string, target *metal3v1alpha1.RAIDConfig
 }
 
 // CheckRAIDInterface checks the current RAID interface against the requested configuration
-func CheckRAIDInterface(raidInterface string, target, actual *metal3v1alpha1.RAIDConfig) (string, error) {
-	if target == nil {
-		return raidInterface, nil
-	}
-
-	if raidInterface == noRAIDInterface && len(target.HardwareRAIDVolumes) != 0 {
-		return "", fmt.Errorf("RAID settings are defined, but the node's driver %s does not support RAID", raidInterface)
-	}
-
-	// This is not a real case since no BMC driver defaults to agent, but we add it for consistency
-	if raidInterface == softwareRAIDInterface && len(target.HardwareRAIDVolumes) != 0 {
-		return "", fmt.Errorf("hardware RAID settings are defined, but the node's driver %s only supports software RAID", raidInterface)
-	}
-
-	// If software RAID is requested, change the RAID interface.
-	// FIXME(dtantsur): if a user tries to simultaneously remove hardware RAID volumes and add software RAID volumes, it will not work.
-	// The matter is complicated because Ironic cannot change the RAID interface in the middle of cleaning. We better just document it.
-	if len(target.SoftwareRAIDVolumes) != 0 {
-		return softwareRAIDInterface, nil
-	}
-
-	// If software RAID is being deleted, also change the interface.
-	if actual != nil && len(actual.SoftwareRAIDVolumes) != 0 {
-		return softwareRAIDInterface, nil
+func CheckRAIDInterface(raidInterface string, target *metal3v1alpha1.RAIDConfig) error {
+	// FIXME(dtantsur): the software RAID logic is completely broken: no driver sets raidInterface to "agent".
+	// This value must be used when software RAID volumes are requested or deleted.
+	switch raidInterface {
+	case noRAIDInterface:
+		if target != nil && (len(target.HardwareRAIDVolumes) != 0 || len(target.SoftwareRAIDVolumes) != 0) {
+			return fmt.Errorf("target settings are defined, but the node's driver %s does not support RAID", raidInterface)
+		}
+	case softwareRAIDInterface:
+		if target != nil && len(target.HardwareRAIDVolumes) != 0 {
+			return fmt.Errorf("node's driver %s does not support hardware RAID", raidInterface)
+		}
+	default:
+		if target != nil && len(target.HardwareRAIDVolumes) == 0 && len(target.SoftwareRAIDVolumes) != 0 {
+			return fmt.Errorf("node's driver %s does not support software RAID", raidInterface)
+		}
 	}
-
-	return raidInterface, nil
+	return nil
 }


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index 8b26d1b7..7ef3fccb 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -46,6 +46,7 @@ func TestValidateCreate(t *testing.T) {
 
 	// for RAID validation test cases
 	numberOfPhysicalDisks := 3
+	diskFormat := "live-iso"
 
 	tests := []struct {
 		name      string
@@ -437,6 +438,43 @@ func TestValidateCreate(t *testing.T) {
 			oldBMH:    nil,
 			wantedErr: "BMO validation: failed to parse BMC address information: BMC address hostname/IP : [fe80::fc33:62ff:fe33:8xff] is invalid",
 		},
+		{
+			name: "invalidImageURL",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "idrac://127.0.0.1",
+						CredentialsName: "test1",
+					},
+					Image: &Image{
+						URL: "test1",
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "Image URL test1 is an invalid URL",
+		},
+		{
+			name: "liveISOImageWithUnsupportedBMC",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "idrac://127.0.0.1",
+						CredentialsName: "test1",
+					},
+					Image: &Image{
+						URL:        "http://127.0.0.1",
+						DiskFormat: &diskFormat,
+					},
+				}, // end of BMH spec
+			},
+			oldBMH:    nil,
+			wantedErr: "BMC driver idrac does not support live-iso image",
+		},
 	}
 
 	for _, tt := range tests {
diff --git a/pkg/provisioner/ironic/adopt_test.go b/pkg/provisioner/ironic/adopt_test.go
index 53716c9b..fbd8334f 100644
--- a/pkg/provisioner/ironic/adopt_test.go
+++ b/pkg/provisioner/ironic/adopt_test.go
@@ -87,38 +87,6 @@ func TestAdopt(t *testing.T) {
 			expectedRequestAfter: 10,
 			force:                true,
 		},
-		{
-			name: "node-in-Active",
-			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
-				ProvisionState: string(nodes.Active),
-				UUID:           nodeUUID,
-			}),
-
-			expectedDirty: false,
-		},
-		{
-			name: "node-in-Maintenance",
-			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
-				ProvisionState: string(nodes.Active),
-				UUID:           nodeUUID,
-				Maintenance:    true,
-			}),
-
-			expectedDirty:        false,
-			expectedRequestAfter: 0,
-			expectedError:        true,
-		},
-		{
-			name: "node-in-Fault",
-			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
-				ProvisionState: string(nodes.Active),
-				UUID:           nodeUUID,
-				Maintenance:    true,
-				Fault:          "power fault",
-			}),
-
-			expectedDirty: false,
-		},
 	}
 
 	for _, tc := range cases {
diff --git a/pkg/provisioner/ironic/delete_test.go b/pkg/provisioner/ironic/delete_test.go
index 3c7eba10..1475bec9 100644
--- a/pkg/provisioner/ironic/delete_test.go
+++ b/pkg/provisioner/ironic/delete_test.go
@@ -101,33 +101,14 @@ func deleteTest(t *testing.T, detach bool) {
 		},
 		{
 			name: "available-node",
-			ironic: testserver.NewIronic(t).Node(
-				nodes.Node{
-					UUID:           nodeUUID,
-					ProvisionState: "available",
-					Maintenance:    false,
-				},
-			).Delete(nodeUUID),
-			expectedDirty:        true,
-			expectedRequestAfter: 0,
-		},
-		{
-			name: "stale-instance-uuid-update",
-			ironic: testserver.NewIronic(t).Node(
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(
 				nodes.Node{
 					UUID:           nodeUUID,
 					ProvisionState: "available",
-					InstanceUUID:   nodeUUID,
 				},
-			).NodeUpdate(nodes.Node{
-				UUID: nodeUUID,
-			}).Delete(nodeUUID),
+			),
 			expectedDirty:        true,
-			expectedRequestAfter: 0,
-			expectedUpdate: &nodes.UpdateOperation{
-				Op:   nodes.RemoveOp,
-				Path: "/instance_uuid",
-			},
+			expectedRequestAfter: provisionRequeueDelay,
 		},
 		{
 			name: "not-in-maintenance-update-fail",
diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go
index 13c63ab9..9330510c 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -88,27 +88,6 @@ func TestProvision(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
-		{
-			name: "fault state",
-			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				ProvisionState: string(nodes.Manageable),
-				UUID:           nodeUUID,
-				Fault:          "power fault",
-				Maintenance:    true,
-			}),
-			expectedRequestAfter: 10,
-			expectedDirty:        true,
-		},
-		{
-			name: "maintenance mode",
-			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				ProvisionState: string(nodes.Manageable),
-				UUID:           nodeUUID,
-				Maintenance:    true,
-			}),
-			expectedRequestAfter: 0,
-			expectedDirty:        true,
-		},
 	}
 
 	for _, tc := range cases {
diff --git a/pkg/provisioner/ironic/raid_test.go b/pkg/provisioner/ironic/raid_test.go
index f7671218..88c28f41 100644
--- a/pkg/provisioner/ironic/raid_test.go
+++ b/pkg/provisioner/ironic/raid_test.go
@@ -6,7 +6,6 @@ import (
 	"testing"
 
 	"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
-	"github.com/stretchr/testify/assert"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
@@ -260,6 +259,11 @@ func TestBuildRAIDCleanSteps(t *testing.T) {
 						Level: "1",
 					},
 				},
+				SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{
+					{
+						Level: "1",
+					},
+				},
 			},
 			expected: []nodes.CleanStep{
 				{
@@ -282,6 +286,11 @@ func TestBuildRAIDCleanSteps(t *testing.T) {
 						Level: "1",
 					},
 				},
+				SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{
+					{
+						Level: "1",
+					},
+				},
 			},
 			actual: &metal3v1alpha1.RAIDConfig{
 				HardwareRAIDVolumes: []metal3v1alpha1.HardwareRAIDVolume{
@@ -356,21 +365,6 @@ func TestBuildRAIDCleanSteps(t *testing.T) {
 				},
 			},
 		},
-		{
-			name:          "keep missing software RAID",
-			raidInterface: "agent",
-			target: &metal3v1alpha1.RAIDConfig{
-				SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{},
-			},
-		},
-		{
-			name:          "keep empty software RAID",
-			raidInterface: "agent",
-			target: &metal3v1alpha1.RAIDConfig{
-				SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{},
-			},
-			actual: &metal3v1alpha1.RAIDConfig{},
-		},
 		{
 			name:          "clear software RAID",
 			raidInterface: "agent",
@@ -412,11 +406,9 @@ func TestBuildRAIDCleanSteps(t *testing.T) {
 
 func TestCheckRAIDConfigure(t *testing.T) {
 	cases := []struct {
-		raidInterface        string
-		RAID                 *metal3v1alpha1.RAIDConfig
-		expectedError        bool
-		expectedNewInterface string
-		currentRAID          *metal3v1alpha1.RAIDConfig
+		raidInterface string
+		RAID          *metal3v1alpha1.RAIDConfig
+		expectedError bool
 	}{
 		{
 			raidInterface: "no-raid",
@@ -437,17 +429,6 @@ func TestCheckRAIDConfigure(t *testing.T) {
 			},
 			expectedError: true,
 		},
-		{
-			raidInterface: "no-raid",
-			RAID: &metal3v1alpha1.RAIDConfig{
-				SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{
-					{
-						Level: "1",
-					},
-				},
-			},
-			expectedNewInterface: "agent",
-		},
 		{
 			raidInterface: "agent",
 		},
@@ -504,37 +485,16 @@ func TestCheckRAIDConfigure(t *testing.T) {
 					},
 				},
 			},
-			expectedNewInterface: "agent",
-		},
-		{
-			raidInterface: "hardware",
-			RAID: &metal3v1alpha1.RAIDConfig{
-				SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{},
-			},
-			currentRAID: &metal3v1alpha1.RAIDConfig{
-				SoftwareRAIDVolumes: []metal3v1alpha1.SoftwareRAIDVolume{
-					{
-						Level: "1",
-					},
-				},
-			},
-			expectedNewInterface: "agent",
+			expectedError: true,
 		},
 	}
 
 	for _, c := range cases {
 		t.Run(c.raidInterface, func(t *testing.T) {
-			newInterface, err := CheckRAIDInterface(c.raidInterface, c.RAID, c.currentRAID)
+			err := CheckRAIDInterface(c.raidInterface, c.RAID)
 			if (err != nil) != c.expectedError {
 				t.Errorf("Got unexpected error: %v", err)
 			}
-			if !c.expectedError {
-				if c.expectedNewInterface != "" {
-					assert.Equal(t, c.expectedNewInterface, newInterface)
-				} else {
-					assert.Equal(t, c.raidInterface, newInterface)
-				}
-			}
 		})
 	}
 }