From 92ccb40593af6457ae5233fad58040efd84dc2be Mon Sep 17 00:00:00 2001 From: Avishay Traeger Date: Wed, 22 Nov 2023 18:04:53 +0200 Subject: [PATCH] MGMT-16273: Allow installing on iSCSI disks Allow booting from iSCSI for x86_64 OpenShift versions at least 4.15.0. Multipath is not supported at this time. --- internal/hardware/mock_validator.go | 8 +-- internal/hardware/validator.go | 27 +++++++--- internal/hardware/validator_test.go | 15 ++++++ internal/host/hostcommands/install_cmd.go | 12 ++++- .../host/hostcommands/install_cmd_test.go | 51 +++++++++++++++++++ internal/host/transition_test.go | 2 +- internal/host/validations_test.go | 2 +- internal/host/validator.go | 4 +- 8 files changed, 106 insertions(+), 15 deletions(-) diff --git a/internal/hardware/mock_validator.go b/internal/hardware/mock_validator.go index 817e08ae4e3..fb0edabbd57 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 string) bool { +func (m *MockValidator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, cluster *common.Cluster) bool { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsValidStorageDeviceType", disk, hostArchitecture) + ret := m.ctrl.Call(m, "IsValidStorageDeviceType", disk, hostArchitecture, cluster) ret0, _ := ret[0].(bool) return ret0 } // IsValidStorageDeviceType indicates an expected call of IsValidStorageDeviceType. -func (mr *MockValidatorMockRecorder) IsValidStorageDeviceType(disk, hostArchitecture interface{}) *gomock.Call { +func (mr *MockValidatorMockRecorder) IsValidStorageDeviceType(disk, hostArchitecture, cluster interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsValidStorageDeviceType", reflect.TypeOf((*MockValidator)(nil).IsValidStorageDeviceType), disk, hostArchitecture) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsValidStorageDeviceType", reflect.TypeOf((*MockValidator)(nil).IsValidStorageDeviceType), disk, hostArchitecture, cluster) } // ListEligibleDisks mocks base method. diff --git a/internal/hardware/validator.go b/internal/hardware/validator.go index a388df47145..be5052dfc80 100644 --- a/internal/hardware/validator.go +++ b/internal/hardware/validator.go @@ -37,7 +37,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) bool + IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, cluster *common.Cluster) 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. @@ -126,9 +126,9 @@ func (v *validator) DiskIsEligible(ctx context.Context, disk *models.Disk, infra humanize.Bytes(uint64(disk.SizeBytes)), humanize.Bytes(uint64(minSizeBytes)))) } - if !v.IsValidStorageDeviceType(disk, hostArchitecture) { + if !v.IsValidStorageDeviceType(disk, hostArchitecture, cluster) { notEligibleReasons = append(notEligibleReasons, - fmt.Sprintf(wrongDriveTypeTemplate, disk.DriveType, strings.Join(v.getValidDeviceStorageTypes(hostArchitecture), ", "))) + fmt.Sprintf(wrongDriveTypeTemplate, disk.DriveType, strings.Join(v.getValidDeviceStorageTypes(hostArchitecture, cluster), ", "))) } // We only allow multipath if all paths are FC @@ -147,8 +147,8 @@ func (v *validator) DiskIsEligible(ctx context.Context, disk *models.Disk, infra return notEligibleReasons, nil } -func (v *validator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string) bool { - return funk.ContainsString(v.getValidDeviceStorageTypes(hostArchitecture), string(disk.DriveType)) +func (v *validator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, cluster *common.Cluster) bool { + return funk.ContainsString(v.getValidDeviceStorageTypes(hostArchitecture, cluster), string(disk.DriveType)) } func (v *validator) purgeServiceReasons(reasons []string) []string { @@ -424,11 +424,26 @@ func (v *validator) getOCPRequirementsForVersion(openshiftVersion string) (*mode return v.VersionedRequirements.GetVersionedHostRequirements(openshiftVersion) } -func (v *validator) getValidDeviceStorageTypes(hostArchitecture string) []string { +func (v *validator) getValidDeviceStorageTypes(hostArchitecture string, cluster *common.Cluster) []string { validTypes := []string{string(models.DriveTypeHDD), string(models.DriveTypeSSD), string(models.DriveTypeMultipath)} + + if hostArchitecture == models.ClusterCPUArchitectureX8664 { + iSCSISupported := true + if cluster != nil { + isGreater, _ := common.BaseVersionGreaterOrEqual("4.15.0", cluster.OpenshiftVersion) + if !isGreater { + iSCSISupported = false + } + } + if iSCSISupported { + validTypes = append(validTypes, string(models.DriveTypeISCSI)) + } + } + if hostArchitecture == models.ClusterCPUArchitectureS390x { validTypes = append(validTypes, string(models.DriveTypeFC), string(models.DriveTypeECKDESE), string(models.DriveTypeECKD), string(models.DriveTypeFBA)) } + return validTypes } diff --git a/internal/hardware/validator_test.go b/internal/hardware/validator_test.go index 639c5b9d331..6e547c8be80 100644 --- a/internal/hardware/validator_test.go +++ b/internal/hardware/validator_test.go @@ -157,6 +157,21 @@ var _ = Describe("Disk eligibility", func() { Expect(eligible).To(BeEmpty()) }) + It("Check if iSCSI is eligible", func() { + testDisk.DriveType = models.DriveTypeISCSI + + eligible, err := hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, &cluster, &host, models.ClusterCPUArchitectureX8664, []*models.Disk{&testDisk}) + + Expect(err).ToNot(HaveOccurred()) + Expect(eligible).To(BeEmpty()) + + By("Check infra env iSCSI is eligible") + eligible, err = hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, nil, &host, models.ClusterCPUArchitectureX8664, []*models.Disk{&testDisk}) + + Expect(err).ToNot(HaveOccurred()) + Expect(eligible).To(BeEmpty()) + }) + It("Check that FC multipath is eligible", func() { testDisk.Name = "dm-0" testDisk.DriveType = models.DriveTypeMultipath diff --git a/internal/host/hostcommands/install_cmd.go b/internal/host/hostcommands/install_cmd.go index 09cafe26242..049c030c44f 100644 --- a/internal/host/hostcommands/install_cmd.go +++ b/internal/host/hostcommands/install_cmd.go @@ -303,8 +303,16 @@ func constructHostInstallerArgs(cluster *common.Cluster, host *models.Host, inve } for _, disk := range inventory.Disks { - if disk.DriveType == models.DriveTypeMultipath && disk.ID == host.InstallationDiskID { - installerArgs = append(installerArgs, "--append-karg", "root=/dev/disk/by-label/dm-mpath-root", "--append-karg", "rw", "--append-karg", "rd.multipath=default") + if disk.ID == host.InstallationDiskID { + if disk.DriveType == models.DriveTypeMultipath { + 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 { + installerArgs = append(installerArgs, "--append-karg", "rd.iscsi.firmware=1") + if common.PlatformTypeValue(cluster.Platform.Type) == models.PlatformTypeOci { + // ip=ibft is needed for Oracle Cloud to work around NICs taking too long to get DHCP + installerArgs = append(installerArgs, "--append-karg", "ip=ibft") + } + } } } diff --git a/internal/host/hostcommands/install_cmd_test.go b/internal/host/hostcommands/install_cmd_test.go index e32453bd9c3..e349f986236 100644 --- a/internal/host/hostcommands/install_cmd_test.go +++ b/internal/host/hostcommands/install_cmd_test.go @@ -694,6 +694,57 @@ var _ = Describe("construct host install arguments", func() { Expect(err).NotTo(HaveOccurred()) Expect(args).To(BeEmpty()) }) + It("iSCSI installation disk", func() { + cluster.MachineNetworks = []*models.MachineNetwork{{Cidr: "192.186.10.0/24"}} + host.Inventory = fmt.Sprintf(`{ + "disks":[ + { + "id": "install-id", + "drive_type": "%s" + }, + { + "id": "other-id", + "drive_type": "%s" + } + ], + "interfaces":[ + { + "name": "eth1", + "ipv4_addresses":["10.56.20.80/25"] + } + ] + }`, models.DriveTypeISCSI, models.DriveTypeSSD) + 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"]`)) + }) + It("iSCSI installation disk on OCI", func() { + cluster.MachineNetworks = []*models.MachineNetwork{{Cidr: "192.186.10.0/24"}} + cluster.Platform = &models.Platform{Type: common.PlatformTypePtr(models.PlatformTypeOci)} + host.Inventory = fmt.Sprintf(`{ + "disks":[ + { + "id": "install-id", + "drive_type": "%s" + }, + { + "id": "other-id", + "drive_type": "%s" + } + ], + "interfaces":[ + { + "name": "eth1", + "ipv4_addresses":["10.56.20.80/25"] + } + ] + }`, models.DriveTypeISCSI, models.DriveTypeSSD) + 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"]`)) + }) It("ip=:dhcp6 added when machine CIDR is IPv6", func() { cluster.MachineNetworks = []*models.MachineNetwork{{Cidr: "2001:db8::/64"}} host.Inventory = `{ diff --git a/internal/host/transition_test.go b/internal/host/transition_test.go index 193dc2d639c..66be5458cd3 100644 --- a/internal/host/transition_test.go +++ b/internal/host/transition_test.go @@ -1577,7 +1577,7 @@ var _ = Describe("Refresh Host", func() { )).AnyTimes() mockDefaultClusterHostRequirements(mockHwValidator) - mockHwValidator.EXPECT().IsValidStorageDeviceType(gomock.Any(), gomock.Any()).Return(true).AnyTimes() + mockHwValidator.EXPECT().IsValidStorageDeviceType(gomock.Any(), gomock.Any(), nil).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 2bd25c64176..8924b84a6a5 100644 --- a/internal/host/validations_test.go +++ b/internal/host/validations_test.go @@ -82,7 +82,7 @@ var _ = Describe("Validations test", func() { mockAndRefreshStatusWithoutEvents := func(h *models.Host) { mockDefaultClusterHostRequirements(mockHwValidator) - mockHwValidator.EXPECT().IsValidStorageDeviceType(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{ diff --git a/internal/host/validator.go b/internal/host/validator.go index b717fb849ee..2ea039033af 100644 --- a/internal/host/validator.go +++ b/internal/host/validator.go @@ -1703,7 +1703,9 @@ 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) && !disk.HasUUID { + if v.hwValidator.IsValidStorageDeviceType(disk, c.inventory.CPU.Architecture, nil) && + funk.ContainsString([]string{string(models.DriveTypeHDD), string(models.DriveTypeSSD)}, string(disk.DriveType)) && // skip network disks + !disk.HasUUID { return ValidationFailure, "VSphere disk.EnableUUID isn't enabled for this virtual machine, it's necessary for disks to be mounted properly" } }