Repository: openshift/assisted-service (language: python)

Issue:
MGMT-17558: Allow installation on iSCSI volume
Open installation of OCP on iSCSI boot volume to all platforms, as it was
allowed only on OCI.

Remove `ip=ibft` from the kernel arguments as it prevent proper network
configuration when several NICs are in use. RHCOS team recommend to
leave DHCP configuration.



Discussion / hints:

Fix patch:
diff --git a/internal/hardware/mock_validator.go b/internal/hardware/mock_validator.go
index 083de79a49c..286294ee27a 100644
--- a/internal/hardware/mock_validator.go
+++ b/internal/hardware/mock_validator.go
@@ -156,17 +156,17 @@ func (mr *MockValidatorMockRecorder) GetPreflightInfraEnvHardwareRequirements(ct
 }
 
 // IsValidStorageDeviceType mocks base method.
-func (m *MockValidator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture, openshiftVersion string, ociPlatformType bool) bool {
+func (m *MockValidator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture, openshiftVersion string) bool {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "IsValidStorageDeviceType", disk, hostArchitecture, openshiftVersion, ociPlatformType)
+	ret := m.ctrl.Call(m, "IsValidStorageDeviceType", disk, hostArchitecture, openshiftVersion)
 	ret0, _ := ret[0].(bool)
 	return ret0
 }
 
 // IsValidStorageDeviceType indicates an expected call of IsValidStorageDeviceType.
-func (mr *MockValidatorMockRecorder) IsValidStorageDeviceType(disk, hostArchitecture, openshiftVersion, ociPlatformType interface{}) *gomock.Call {
+func (mr *MockValidatorMockRecorder) IsValidStorageDeviceType(disk, hostArchitecture, openshiftVersion interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsValidStorageDeviceType", reflect.TypeOf((*MockValidator)(nil).IsValidStorageDeviceType), disk, hostArchitecture, openshiftVersion, ociPlatformType)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsValidStorageDeviceType", reflect.TypeOf((*MockValidator)(nil).IsValidStorageDeviceType), disk, hostArchitecture, openshiftVersion)
 }
 
 // ListEligibleDisks mocks base method.
diff --git a/internal/hardware/validator.go b/internal/hardware/validator.go
index 16315b2815d..41136213ebf 100644
--- a/internal/hardware/validator.go
+++ b/internal/hardware/validator.go
@@ -16,7 +16,6 @@ import (
 	"github.com/openshift/assisted-service/internal/feature"
 	"github.com/openshift/assisted-service/internal/host/hostutil"
 	"github.com/openshift/assisted-service/internal/operators"
-	"github.com/openshift/assisted-service/internal/provider/external"
 	"github.com/openshift/assisted-service/internal/provider/registry"
 	"github.com/openshift/assisted-service/models"
 	"github.com/openshift/assisted-service/pkg/conversions"
@@ -39,7 +38,7 @@ type Validator interface {
 	GetInfraEnvHostRequirements(ctx context.Context, infraEnv *common.InfraEnv) (*models.ClusterHostRequirements, error)
 	DiskIsEligible(ctx context.Context, disk *models.Disk, infraEnv *common.InfraEnv, cluster *common.Cluster, host *models.Host, hostArchitecture string, allDisks []*models.Disk) ([]string, error)
 	ListEligibleDisks(inventory *models.Inventory) []*models.Disk
-	IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, openshiftVersion string, ociPlatformType bool) bool
+	IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, openshiftVersion string) bool
 	GetInstallationDiskSpeedThresholdMs(ctx context.Context, cluster *common.Cluster, host *models.Host) (int64, error)
 	// GetPreflightHardwareRequirements provides hardware (host) requirements that can be calculated only using cluster information.
 	// Returned information describe requirements coming from OCP and OLM operators.
@@ -133,13 +132,9 @@ func (v *validator) DiskIsEligible(ctx context.Context, disk *models.Disk, infra
 				humanize.Bytes(uint64(disk.SizeBytes)), humanize.Bytes(uint64(minSizeBytes))))
 	}
 
-	ociPlatformType, err := external.IsOciHost(host)
-	if err != nil {
-		return nil, err
-	}
-	if !v.IsValidStorageDeviceType(disk, hostArchitecture, clusterVersion, ociPlatformType) {
+	if !v.IsValidStorageDeviceType(disk, hostArchitecture, clusterVersion) {
 		notEligibleReasons = append(notEligibleReasons,
-			fmt.Sprintf(wrongDriveTypeTemplate, disk.DriveType, strings.Join(v.getValidDeviceStorageTypes(hostArchitecture, clusterVersion, ociPlatformType), ", ")))
+			fmt.Sprintf(wrongDriveTypeTemplate, disk.DriveType, strings.Join(v.getValidDeviceStorageTypes(hostArchitecture, clusterVersion), ", ")))
 	}
 
 	// We only allow multipath if all paths are FC
@@ -158,8 +153,8 @@ func (v *validator) DiskIsEligible(ctx context.Context, disk *models.Disk, infra
 	return notEligibleReasons, nil
 }
 
-func (v *validator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, openshiftVersion string, ociPlatformType bool) bool {
-	return funk.ContainsString(v.getValidDeviceStorageTypes(hostArchitecture, openshiftVersion, ociPlatformType), string(disk.DriveType))
+func (v *validator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, openshiftVersion string) bool {
+	return funk.ContainsString(v.getValidDeviceStorageTypes(hostArchitecture, openshiftVersion), string(disk.DriveType))
 }
 
 func (v *validator) purgeServiceReasons(reasons []string) []string {
@@ -435,14 +430,12 @@ func (v *validator) getOCPRequirementsForVersion(openshiftVersion string) (*mode
 	return v.VersionedRequirements.GetVersionedHostRequirements(openshiftVersion)
 }
 
-func (v *validator) getValidDeviceStorageTypes(hostArchitecture string, openshiftVersion string, ociPlatformType bool) []string {
+func (v *validator) getValidDeviceStorageTypes(hostArchitecture string, openshiftVersion string) []string {
 	validTypes := []string{string(models.DriveTypeHDD), string(models.DriveTypeSSD), string(models.DriveTypeMultipath)}
 
-	if ociPlatformType {
-		isGreater, err := common.BaseVersionGreaterOrEqual("4.15.0", openshiftVersion)
-		if err == nil && isGreater {
-			validTypes = append(validTypes, string(models.DriveTypeISCSI))
-		}
+	isGreater, err := common.BaseVersionGreaterOrEqual("4.15.0", openshiftVersion)
+	if err == nil && isGreater {
+		validTypes = append(validTypes, string(models.DriveTypeISCSI))
 	}
 
 	if hostArchitecture == models.ClusterCPUArchitectureS390x {
diff --git a/internal/host/hostcommands/install_cmd.go b/internal/host/hostcommands/install_cmd.go
index b849303891a..557cbd1b338 100644
--- a/internal/host/hostcommands/install_cmd.go
+++ b/internal/host/hostcommands/install_cmd.go
@@ -312,7 +312,7 @@ func constructHostInstallerArgs(cluster *common.Cluster, host *models.Host, inve
 				installerArgs = append(installerArgs, "--append-karg", "root=/dev/disk/by-label/dm-mpath-root", "--append-karg", "rw", "--append-karg", "rd.multipath=default")
 			} else if disk.DriveType == models.DriveTypeISCSI {
 				// Currently only allowed on the OCI platform
-				installerArgs = append(installerArgs, "--append-karg", "rd.iscsi.firmware=1", "--append-karg", "ip=ibft")
+				installerArgs = append(installerArgs, "--append-karg", "rd.iscsi.firmware=1")
 			}
 		}
 	}
diff --git a/internal/host/validator.go b/internal/host/validator.go
index 8e82c6cd8ed..e7de81a2190 100644
--- a/internal/host/validator.go
+++ b/internal/host/validator.go
@@ -1678,7 +1678,7 @@ func (v *validator) isVSphereDiskUUIDEnabled(c *validationContext) (ValidationSt
 		// if any of them doesn't have that flag, it's likely because the user has forgotten to
 		// enable `disk.EnableUUID` for this virtual machine
 		// See https://access.redhat.com/solutions/4606201
-		if v.hwValidator.IsValidStorageDeviceType(disk, c.inventory.CPU.Architecture, "", false) && !disk.HasUUID {
+		if v.hwValidator.IsValidStorageDeviceType(disk, c.inventory.CPU.Architecture, "") && !disk.HasUUID {
 			return ValidationFailure, "VSphere disk.EnableUUID isn't enabled for this virtual machine, it's necessary for disks to be mounted properly"
 		}
 	}


Test patch:
diff --git a/internal/hardware/validator_test.go b/internal/hardware/validator_test.go
index dbf89771c11..42cd5f7d6dd 100644
--- a/internal/hardware/validator_test.go
+++ b/internal/hardware/validator_test.go
@@ -160,11 +160,10 @@ var _ = Describe("Disk eligibility", func() {
 		Expect(eligible).To(BeEmpty())
 	})
 
-	It("Check if iSCSI is eligible only on newer OCI clusters", func() {
+	It("Check if iSCSI is eligible", func() {
 		testDisk.DriveType = models.DriveTypeISCSI
 		cluster.OpenshiftVersion = "4.15.0"
 		hostInventory, _ := common.UnmarshalInventory(host.Inventory)
-		hostInventory.SystemVendor.Manufacturer = "OracleCloud.com"
 		hw, _ := json.Marshal(&hostInventory)
 		host.Inventory = string(hw)
 		eligible, err := hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, &cluster, &host, models.ClusterCPUArchitectureX8664, []*models.Disk{&testDisk})
diff --git a/internal/host/host_test.go b/internal/host/host_test.go
index 066c7dc3687..b08a1c0a126 100644
--- a/internal/host/host_test.go
+++ b/internal/host/host_test.go
@@ -4987,7 +4987,7 @@ var _ = Describe("disconnection - soft timeouts", func() {
 			},
 		})
 		mockHwValidator.EXPECT().GetHostInstallationPath(gomock.Any()).Return("/dev/sda").AnyTimes()
-		mockHwValidator.EXPECT().IsValidStorageDeviceType(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(true).AnyTimes()
+		mockHwValidator.EXPECT().IsValidStorageDeviceType(gomock.Any(), gomock.Any(), gomock.Any()).Return(true).AnyTimes()
 		mockProviderRegistry = registry.NewMockProviderRegistry(ctrl)
 		mockProviderRegistry.EXPECT().IsHostSupported(gomock.Any(), gomock.Any()).Return(true, nil).AnyTimes()
 		mockOperatorManager = operators.NewMockAPI(ctrl)
diff --git a/internal/host/hostcommands/install_cmd_test.go b/internal/host/hostcommands/install_cmd_test.go
index c13e42b9e68..e195416f8de 100644
--- a/internal/host/hostcommands/install_cmd_test.go
+++ b/internal/host/hostcommands/install_cmd_test.go
@@ -701,15 +701,8 @@ var _ = Describe("construct host install arguments", func() {
 		Expect(err).NotTo(HaveOccurred())
 		Expect(args).To(BeEmpty())
 	})
-	It("iSCSI installation disk on OCI", func() {
+	It("iSCSI installation disk", func() {
 		cluster.MachineNetworks = []*models.MachineNetwork{{Cidr: "192.186.10.0/24"}}
-		cluster.Platform = &models.Platform{
-			Type: common.PlatformTypePtr(models.PlatformTypeExternal),
-			External: &models.PlatformExternal{
-				PlatformName:           swag.String(common.ExternalPlatformNameOci),
-				CloudControllerManager: swag.String(models.PlatformExternalCloudControllerManagerExternal),
-			},
-		}
 		host.Inventory = fmt.Sprintf(`{
 			"disks":[
 				{
@@ -731,7 +724,7 @@ var _ = Describe("construct host install arguments", func() {
 		inventory, _ := common.UnmarshalInventory(host.Inventory)
 		args, err := constructHostInstallerArgs(cluster, host, inventory, infraEnv, log)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(args).To(Equal(`["--append-karg","rd.iscsi.firmware=1","--append-karg","ip=ibft"]`))
+		Expect(args).To(Equal(`["--append-karg","rd.iscsi.firmware=1"]`))
 	})
 	It("ip=<nic>:dhcp6 added when machine CIDR is IPv6", func() {
 		cluster.MachineNetworks = []*models.MachineNetwork{{Cidr: "2001:db8::/64"}}
diff --git a/internal/host/transition_test.go b/internal/host/transition_test.go
index 6aa8e775294..696157be7d4 100644
--- a/internal/host/transition_test.go
+++ b/internal/host/transition_test.go
@@ -1578,7 +1578,7 @@ var _ = Describe("Refresh Host", func() {
 				)).AnyTimes()
 
 				mockDefaultClusterHostRequirements(mockHwValidator)
-				mockHwValidator.EXPECT().IsValidStorageDeviceType(gomock.Any(), gomock.Any(), "", false).Return(true).AnyTimes()
+				mockHwValidator.EXPECT().IsValidStorageDeviceType(gomock.Any(), gomock.Any(), "").Return(true).AnyTimes()
 				mockHwValidator.EXPECT().ListEligibleDisks(gomock.Any()).Return([]*models.Disk{}).AnyTimes()
 				mockHwValidator.EXPECT().GetHostInstallationPath(gomock.Any()).Return("/dev/sda").AnyTimes()
 			}
diff --git a/internal/host/validations_test.go b/internal/host/validations_test.go
index cda6e509cbe..ba64c423cbf 100644
--- a/internal/host/validations_test.go
+++ b/internal/host/validations_test.go
@@ -84,7 +84,7 @@ var _ = Describe("Validations test", func() {
 
 	mockAndRefreshStatusWithoutEvents := func(h *models.Host) {
 		mockDefaultClusterHostRequirements(mockHwValidator)
-		mockHwValidator.EXPECT().IsValidStorageDeviceType(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(true).AnyTimes()
+		mockHwValidator.EXPECT().IsValidStorageDeviceType(gomock.Any(), gomock.Any(), gomock.Any()).Return(true).AnyTimes()
 		mockHwValidator.EXPECT().ListEligibleDisks(gomock.Any()).Return([]*models.Disk{}).AnyTimes()
 		mockHwValidator.EXPECT().GetHostInstallationPath(gomock.Any()).Return("/dev/sda").AnyTimes()
 		mockOperators.EXPECT().ValidateHost(gomock.Any(), gomock.Any(), gomock.Any()).Return([]api.ValidationResult{
@@ -1615,7 +1615,7 @@ var _ = Describe("Validations test", func() {
 			updateClusterPlatform()
 			host = hostutil.GetHostFromDB(*host.ID, host.InfraEnvID, db).Host
 
-			mockHwValidator.EXPECT().IsValidStorageDeviceType(CDROM, models.ClusterCPUArchitectureX8664, "", false).Return(false).Times(1)
+			mockHwValidator.EXPECT().IsValidStorageDeviceType(CDROM, models.ClusterCPUArchitectureX8664, "").Return(false).Times(1)
 			mockAndRefreshStatus(&host)
 			host = hostutil.GetHostFromDB(*host.ID, host.InfraEnvID, db).Host
 			status, _, _ := getValidationResult(host.ValidationsInfo, hostUUIDValidation)