diff --git a/cmd/agentbasedinstaller/host_config.go b/cmd/agentbasedinstaller/host_config.go index 670bbb65072e..14ec91ec100e 100644 --- a/cmd/agentbasedinstaller/host_config.go +++ b/cmd/agentbasedinstaller/host_config.go @@ -138,7 +138,7 @@ func applyRootDeviceHints(log *log.Logger, host *models.Host, inventory *models. diskID := "/dev/not-found-by-hints" if len(acceptableDisks) > 0 { - diskID = hostutil.GetPreferredDiskID(acceptableDisks) + diskID = acceptableDisks[0].ID log.Infof("Selecting disk %s for installation", diskID) } else { log.Info("No disk found matching root device hints") diff --git a/cmd/agentbasedinstaller/host_config_test.go b/cmd/agentbasedinstaller/host_config_test.go deleted file mode 100644 index de2fdeb4c68c..000000000000 --- a/cmd/agentbasedinstaller/host_config_test.go +++ /dev/null @@ -1,112 +0,0 @@ -package agentbasedinstaller - -import ( - bmh_v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" - "github.com/openshift/assisted-service/models" - "github.com/sirupsen/logrus/hooks/test" -) - -var _ = Describe("applyRootDeviceHints", func() { - Context("when multiple disks match WWN hint and one is multipath", func() { - It("should prefer the multipath disk over raw FC paths", func() { - testLogger, _ := test.NewNullLogger() - - wwn := "0x1111111111111111" - rdh := &bmh_v1alpha1.RootDeviceHints{ - WWN: wwn, - } - - // Simulate FC multipath: two raw FC paths and one multipath device, all sharing the same WWN - inventory := &models.Inventory{ - Disks: []*models.Disk{ - { - ID: "/dev/sda", - Path: "/dev/sda", - DriveType: models.DriveTypeFC, - Wwn: wwn, - InstallationEligibility: models.DiskInstallationEligibility{ - Eligible: true, - }, - }, - { - ID: "/dev/sdb", - Path: "/dev/sdb", - DriveType: models.DriveTypeFC, - Wwn: wwn, - InstallationEligibility: models.DiskInstallationEligibility{ - Eligible: true, - }, - }, - { - ID: "/dev/dm-0", - Path: "/dev/dm-0", - DriveType: models.DriveTypeMultipath, - Wwn: wwn, - InstallationEligibility: models.DiskInstallationEligibility{ - Eligible: true, - }, - }, - }, - } - - host := &models.Host{ - InstallationDiskID: "", - } - - updateParams := &models.HostUpdateParams{} - - applied := applyRootDeviceHints(testLogger, host, inventory, rdh, updateParams) - - Expect(applied).To(BeTrue()) - Expect(updateParams.DisksSelectedConfig).To(HaveLen(1)) - Expect(*updateParams.DisksSelectedConfig[0].ID).To(Equal("/dev/dm-0"), "should prefer multipath device over raw FC paths") - }) - - It("should select the first disk when no multipath device exists", func() { - testLogger, _ := test.NewNullLogger() - - wwn := "0x1111111111111111" - rdh := &bmh_v1alpha1.RootDeviceHints{ - WWN: wwn, - } - - // Two FC paths, no multipath device - inventory := &models.Inventory{ - Disks: []*models.Disk{ - { - ID: "/dev/sda", - Path: "/dev/sda", - DriveType: models.DriveTypeFC, - Wwn: wwn, - InstallationEligibility: models.DiskInstallationEligibility{ - Eligible: true, - }, - }, - { - ID: "/dev/sdb", - Path: "/dev/sdb", - DriveType: models.DriveTypeFC, - Wwn: wwn, - InstallationEligibility: models.DiskInstallationEligibility{ - Eligible: true, - }, - }, - }, - } - - host := &models.Host{ - InstallationDiskID: "", - } - - updateParams := &models.HostUpdateParams{} - - applied := applyRootDeviceHints(testLogger, host, inventory, rdh, updateParams) - - Expect(applied).To(BeTrue()) - Expect(updateParams.DisksSelectedConfig).To(HaveLen(1)) - Expect(*updateParams.DisksSelectedConfig[0].ID).To(Equal("/dev/sda"), "should select first disk when no multipath exists") - }) - }) -}) diff --git a/internal/controller/controllers/bmh_agent_controller.go b/internal/controller/controllers/bmh_agent_controller.go index 78c42ee4c91d..5262899a5cfa 100644 --- a/internal/controller/controllers/bmh_agent_controller.go +++ b/internal/controller/controllers/bmh_agent_controller.go @@ -1169,7 +1169,7 @@ func (r *BMACReconciler) findInstallationDiskID(devices []aiv1beta1.HostDisk, hi acceptable := hostutil.GetAcceptableDisksWithHints(disks, hints) if len(acceptable) > 0 { - return hostutil.GetPreferredDiskID(acceptable) + return getDiskID(acceptable) } // If hints are provided but we did not find an eligible disk, we need to raise an error. @@ -1182,6 +1182,18 @@ func (r *BMACReconciler) findInstallationDiskID(devices []aiv1beta1.HostDisk, hi return "/dev/not-found-by-hints" } +// Given a list of acceptable disks, +// find the multipath disk ID, if it exists, +// otherwise return the ID of the first disk in the list. +func getDiskID(disks []*models.Disk) string { + for _, disk := range disks { + if strings.EqualFold(string(disk.DriveType), string(models.DriveTypeMultipath)) { + return disk.ID + } + } + return disks[0].ID +} + // Finds the agents related to this ClusterDeployment // // The ClusterDeployment <-> agent relation is one-to-many. diff --git a/internal/hardware/validator.go b/internal/hardware/validator.go index 87862ed1ff3a..66084bb815f8 100644 --- a/internal/hardware/validator.go +++ b/internal/hardware/validator.go @@ -33,6 +33,7 @@ const ( wrongDriveTypeTemplate = "Drive type is %s, it must be one of %s." wrongMultipathTypeTemplate = "Multipath device has path of unsupported type. It must be %s" mixedTypesInMultipath = "Multipath device has paths of different types, but they must all be the same type" + diskIsMultipathMember = "Disk is a member of multipath device %s and is not eligible for installation. Use the multipath device instead." wrongISCSINetworkTemplate = "iSCSI host IP %s is the same as host IP, they must be different" ErrsInIscsiDisableMultipathInstallation = "Installation on multipath device is not possible due to errors on at least one iSCSI disk" iscsiHostIPNotAvailable = "Host IP address is not available" @@ -62,6 +63,7 @@ func NewValidator(log logrus.FieldLogger, cfg ValidatorCfg, operatorsAPI operato compileDiskReasonTemplate(wrongDriveTypeTemplate, ".*", ".*"), compileDiskReasonTemplate(wrongMultipathTypeTemplate, ".*"), compileDiskReasonTemplate(mixedTypesInMultipath), + compileDiskReasonTemplate(diskIsMultipathMember, ".*"), compileDiskReasonTemplate(wrongISCSINetworkTemplate, ".*"), compileDiskReasonTemplate(ErrsInIscsiDisableMultipathInstallation), compileDiskReasonTemplate(iscsiHostIPNotAvailable), @@ -160,6 +162,14 @@ func (v *validator) DiskIsEligible(ctx context.Context, disk *models.Disk, infra fmt.Sprintf(wrongDriveTypeTemplate, disk.DriveType, strings.Join(v.getValidDeviceStorageTypes(hostArchitecture, clusterVersion), ", "))) } + // We do not allow FC or iSCSI disks that are members of a multipath device + if disk.DriveType == models.DriveTypeFC || disk.DriveType == models.DriveTypeISCSI { + err = hasMultipathHolder(disk, inventory) + if err != nil { + notEligibleReasons = append(notEligibleReasons, err.Error()) + } + } + if disk.DriveType == models.DriveTypeMultipath { fcDisks := hostutil.GetDisksOfHolderByType(inventory.Disks, disk, models.DriveTypeFC) iSCSIDisks := hostutil.GetDisksOfHolderByType(inventory.Disks, disk, models.DriveTypeISCSI) @@ -203,6 +213,29 @@ func (v *validator) DiskIsEligible(ctx context.Context, disk *models.Disk, infra return notEligibleReasons, nil } +// hasMultipathHolder checks if a disk is a member of a multipath volume by +// looking for multipath devices in its holders list. +func hasMultipathHolder(disk *models.Disk, inventory *models.Inventory) error { + if disk.Holders == "" { + return nil + } + + multipathDiskNamesMap := make(map[string]struct{}) + for _, inventoryDisk := range inventory.Disks { + if inventoryDisk.DriveType == models.DriveTypeMultipath { + multipathDiskNamesMap[inventoryDisk.Name] = struct{}{} + } + } + + holders := strings.Split(disk.Holders, ",") + for _, holder := range holders { + if _, exists := multipathDiskNamesMap[holder]; exists { + return fmt.Errorf(diskIsMultipathMember, holder) + } + } + return nil +} + // isISCSINetworkingValid checks if the iSCSI disk is not connected through the // default network interface. The default network interface is the interface // which is used by the default gateway. diff --git a/internal/hardware/validator_test.go b/internal/hardware/validator_test.go index f8d0876163d6..7201376b5d37 100644 --- a/internal/hardware/validator_test.go +++ b/internal/hardware/validator_test.go @@ -255,7 +255,7 @@ var _ = Describe("Disk eligibility", func() { Expect(err).ToNot(HaveOccurred()) Expect(notEligibleReasons).To(BeEmpty()) - By("Check that iSCSI is eligible when its holder is Multipath.", func() { + By("Check that iSCSI is not eligible when its holder is Multipath.", func() { testDisk.Holders = "dm-0" allDisks := []*models.Disk{&testDisk, {Name: "iscsi0", DriveType: models.DriveTypeISCSI, Holders: "dm-0"}, {Name: "dm-0", DriveType: models.DriveTypeMultipath}} inventory.Disks = allDisks @@ -263,7 +263,7 @@ var _ = Describe("Disk eligibility", func() { notEligibleReasons, err := hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, &cluster, &host, inventory) Expect(err).ToNot(HaveOccurred()) - Expect(notEligibleReasons).To(BeEmpty()) + Expect(notEligibleReasons).To(ContainElement(fmt.Sprintf(diskIsMultipathMember, "dm-0"))) }) }) @@ -308,6 +308,18 @@ var _ = Describe("Disk eligibility", func() { Expect(notEligibleReasons).To(BeEmpty()) }) + It("Check that FC disk with multipath holder is not eligible", func() { + testDisk.DriveType = models.DriveTypeFC + testDisk.Holders = "dm-0" + allDisks := []*models.Disk{&testDisk, {Name: "sdb", DriveType: models.DriveTypeFC, Holders: "dm-0"}, {Name: "dm-0", DriveType: models.DriveTypeMultipath}} + inventory.Disks = allDisks + + notEligibleReasons, err := hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, &cluster, &host, inventory) + + Expect(err).ToNot(HaveOccurred()) + Expect(notEligibleReasons).To(ContainElement(fmt.Sprintf(diskIsMultipathMember, "dm-0"))) + }) + It("Check that mixed types under multipath isn't eligible", func() { testDisk.Name = "dm-0" testDisk.DriveType = models.DriveTypeMultipath diff --git a/internal/host/hostutil/host_utils.go b/internal/host/hostutil/host_utils.go index 7d8c096b2a5f..941726483237 100644 --- a/internal/host/hostutil/host_utils.go +++ b/internal/host/hostutil/host_utils.go @@ -399,18 +399,6 @@ func GetIgnitionEndpointAndCert(cluster *common.Cluster, host *models.Host, logg return ignitionEndpointUrl, cert, nil } -// GetPreferredDiskID returns the ID of the preferred installation disk from a list of acceptable disks. -// If any disk is a multipath device, it is preferred over raw paths (e.g. FC or iSCSI). -// Otherwise, the first disk in the list is returned. -func GetPreferredDiskID(disks []*models.Disk) string { - for _, disk := range disks { - if strings.EqualFold(string(disk.DriveType), string(models.DriveTypeMultipath)) { - return disk.ID - } - } - return disks[0].ID -} - func GetDisksOfHolderByType(allDisks []*models.Disk, holderDisk *models.Disk, driveTypeFilter models.DriveType) []*models.Disk { disksOfHolder := GetAllDisksOfHolder(allDisks, holderDisk) return lo.Filter(disksOfHolder, func(d *models.Disk, _ int) bool { return d.DriveType == driveTypeFilter })