Skip to content

Commit

Permalink
MGMT-17558: Allow installation on iSCSI volume
Browse files Browse the repository at this point in the history
Open installation of OCP on iSCSI boot volume to all platforms.

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.
  • Loading branch information
adriengentil committed Apr 18, 2024
1 parent dffc2f9 commit 8ce7a08
Show file tree
Hide file tree
Showing 9 changed files with 22 additions and 37 deletions.
8 changes: 4 additions & 4 deletions internal/hardware/mock_validator.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 9 additions & 16 deletions internal/hardware/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions internal/hardware/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down
2 changes: 1 addition & 1 deletion internal/host/host_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion internal/host/hostcommands/install_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
}
Expand Down
11 changes: 2 additions & 9 deletions internal/host/hostcommands/install_cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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":[
{
Expand All @@ -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"}}
Expand Down
2 changes: 1 addition & 1 deletion internal/host/transition_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
4 changes: 2 additions & 2 deletions internal/host/validations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion internal/host/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
Expand Down

0 comments on commit 8ce7a08

Please sign in to comment.