Skip to content

Commit

Permalink
Enable multipath + iSCSI as installation disk
Browse files Browse the repository at this point in the history 
This commit removes the disablement of multipath+iSCSI, updates the required kernel arguments, and extends the `nicReapplyManifest` workaround previously added for iSCSI to also support multipath+iSCSI.
  • Loading branch information
@linoyaslan
linoyaslan committed Jan 29, 2025
1 parent 7490e1a commit 1eaabc5
Show file tree
Hide file tree
Showing 6 changed files with 208 additions and 92 deletions.
35 changes: 4 additions & 31 deletions internal/hardware/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ const (
tooSmallDiskTemplate = "Disk is too small (disk only has %s, but %s are required)"
wrongDriveTypeTemplate = "Drive type is %s, it must be one of %s."
wrongMultipathTypeTemplate = "Multipath device has path of type %s, it must be %s"
iSCSIWithMultipathHolder = "iSCSI disk with a multipath holder is not eligible"
wrongISCSINetworkTemplate = "iSCSI host IP %s is the same as host IP, they must be different"
)

Expand Down Expand Up @@ -147,25 +146,20 @@ func (v *validator) DiskIsEligible(ctx context.Context, disk *models.Disk, infra
fmt.Sprintf(wrongDriveTypeTemplate, disk.DriveType, strings.Join(v.getValidDeviceStorageTypes(hostArchitecture, clusterVersion), ", ")))
}

// We only allow multipath if all paths are FC
if disk.DriveType == models.DriveTypeMultipath {
for _, inventoryDisk := range inventory.Disks {
if lo.Contains(strings.Split(inventoryDisk.Holders, ","), disk.Name) {
if inventoryDisk.DriveType != models.DriveTypeFC {
if strings.Contains(inventoryDisk.Holders, disk.Name) {
// We only allow multipath if all paths are FC/ iSCSI
if inventoryDisk.DriveType != models.DriveTypeFC && inventoryDisk.DriveType != models.DriveTypeISCSI {
notEligibleReasons = append(notEligibleReasons,
fmt.Sprintf(wrongMultipathTypeTemplate, inventoryDisk.DriveType, string(models.DriveTypeFC)))
fmt.Sprintf(wrongMultipathTypeTemplate, inventoryDisk.DriveType, strings.Join([]string{string(models.DriveTypeFC), string(models.DriveTypeISCSI)}, ", ")))
break
}
}
}
}

if disk.DriveType == models.DriveTypeISCSI {
err := areISCSIHoldersValid(disk, inventory)
if err != nil {
notEligibleReasons = append(notEligibleReasons, err.Error())
}

// Check if network is configured properly to install on iSCSI boot drive
err = isISCSINetworkingValid(disk, inventory)
if err != nil {
Expand All @@ -176,27 +170,6 @@ func (v *validator) DiskIsEligible(ctx context.Context, disk *models.Disk, infra
return notEligibleReasons, nil
}

// Validate holders of iSCSI disk. We do not allow iSCSI disk with multipath holder.
func areISCSIHoldersValid(disk *models.Disk, inventory *models.Inventory) error {
multipathDiskNamesMap := make(map[string]struct{})
for _, inventoryDisk := range inventory.Disks {
if inventoryDisk.DriveType == models.DriveTypeMultipath {
multipathDiskNamesMap[inventoryDisk.Name] = struct{}{}
}
}

// Check if the iSCSI disk has any holders that are multipath disks
holders := strings.Split(disk.Holders, ",")
for _, holder := range holders {
if _, exists := multipathDiskNamesMap[holder]; exists {
return fmt.Errorf(iSCSIWithMultipathHolder)
}
}

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.
Expand Down
50 changes: 15 additions & 35 deletions internal/hardware/validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"encoding/json"
"errors"
"fmt"
"math"
"os"
"testing"
Expand Down Expand Up @@ -235,6 +234,17 @@ var _ = Describe("Disk eligibility", func() {
notEligibleReasons, err = hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, nil, &host, inventory)
Expect(err).ToNot(HaveOccurred())
Expect(notEligibleReasons).To(BeEmpty())

By("Check that iSCSI is 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

notEligibleReasons, err := hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, &cluster, &host, inventory)

Expect(err).ToNot(HaveOccurred())
Expect(notEligibleReasons).To(BeEmpty())
})
})

It("Check if RAID is eligible", func() {
Expand Down Expand Up @@ -278,7 +288,7 @@ var _ = Describe("Disk eligibility", func() {
Expect(notEligibleReasons).To(BeEmpty())
})

It("Check that iSCSI multipath is not eligible", func() {
It("Check that Multipath iSCSI is eligible", func() {
testDisk.Name = "dm-0"
testDisk.DriveType = models.DriveTypeMultipath
allDisks := []*models.Disk{&testDisk, {Name: "sda", DriveType: models.DriveTypeISCSI, Holders: "dm-0"}, {Name: "sdb", DriveType: models.DriveTypeISCSI, Holders: "dm-0"}}
Expand All @@ -287,44 +297,14 @@ var _ = Describe("Disk eligibility", func() {
notEligibleReasons, err := hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, &cluster, &host, inventory)

Expect(err).ToNot(HaveOccurred())
Expect(notEligibleReasons).ToNot(BeEmpty())
Expect(notEligibleReasons).To(BeEmpty())

By("Check infra env iSCSI multipath is not eligible")
By("Check infra env Multipath iSCSI is eligible")
notEligibleReasons, err = hwvalidator.DiskIsEligible(ctx, &testDisk, infraEnv, nil, &host, inventory)

Expect(err).ToNot(HaveOccurred())
Expect(notEligibleReasons).ToNot(BeEmpty())
})

It("Check that iSCSI is not eligible when its holder is Multipath.", func() {
testDisk.Name = "sdb"
testDisk.Holders = "dm-0"
cluster.OpenshiftVersion = "4.15.0"
testDisk.DriveType = models.DriveTypeISCSI
allDisks := []*models.Disk{&testDisk, {Name: "sdc", DriveType: models.DriveTypeISCSI, 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[0]).To(Equal(iSCSIWithMultipathHolder))
Expect(notEligibleReasons).ToNot(BeEmpty())
})
It("Check that iSCSI is not eligible on older version when its holder is Multipath.", func() {
testDisk.Name = "sdb"
testDisk.Holders = "dm-0"
cluster.OpenshiftVersion = "4.14.1"
testDisk.DriveType = models.DriveTypeISCSI
allDisks := []*models.Disk{&testDisk, {Name: "sdc", DriveType: models.DriveTypeISCSI, 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[0]).To(ContainSubstring(fmt.Sprintf("Drive type is %s, it must be one of", models.DriveTypeISCSI)))
Expect(notEligibleReasons).ToNot(BeEmpty())
Expect(notEligibleReasons).To(BeEmpty())
})

It("Check if FC is not eligible on non-s390x/x86_64", func() {
testDisk.DriveType = models.DriveTypeFC
inventory.CPU.Architecture = models.ClusterCPUArchitectureArm64
Expand Down
32 changes: 26 additions & 6 deletions internal/host/hostcommands/install_cmd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,10 @@ func constructHostInstallerArgs(cluster *common.Cluster, host *models.Host, inve
// append kargs depending on installation drive type
installationDisk := hostutil.GetDiskByInstallationPath(inventory.Disks, hostutil.GetHostInstallationPath(host))
if installationDisk != nil {
installerArgs = appendMultipathArgs(installerArgs, installationDisk)
installerArgs, err = appendMultipathArgs(installerArgs, installationDisk, inventory, hasUserConfiguredIP)
if err != nil {
return "", err
}
installerArgs, err = appendISCSIArgs(installerArgs, installationDisk, inventory, hasUserConfiguredIP)
if err != nil {
return "", err
Expand Down Expand Up @@ -380,7 +383,9 @@ func appendISCSIArgs(installerArgs []string, installationDisk *models.Disk, inve
}

// enable iSCSI on boot
installerArgs = append(installerArgs, "--append-karg", "rd.iscsi.firmware=1")
if !lo.Contains(installerArgs, "rd.iscsi.firmware=1") {
installerArgs = append(installerArgs, "--append-karg", "rd.iscsi.firmware=1")
}

if hasUserConfiguredIP {
return installerArgs, nil
Expand All @@ -401,16 +406,31 @@ func appendISCSIArgs(installerArgs []string, installationDisk *models.Disk, inve
if iSCSIHostIP.Is6() {
dhcp = "dhcp6"
}
installerArgs = append(installerArgs, "--append-karg", fmt.Sprintf("ip=%s:%s", nic.Name, dhcp))

netArgs := fmt.Sprintf("ip=%s:%s", nic.Name, dhcp)
if !lo.Contains(installerArgs, fmt.Sprintf("ip=%s:%s", nic.Name, dhcp)) {
installerArgs = append(installerArgs, "--append-karg", netArgs)
}

return installerArgs, nil
}

func appendMultipathArgs(installerArgs []string, installationDisk *models.Disk) []string {
func appendMultipathArgs(installerArgs []string, installationDisk *models.Disk, inventory *models.Inventory, hasUserConfiguredIP bool) ([]string, error) {
if installationDisk.DriveType != models.DriveTypeMultipath {
return installerArgs
return installerArgs, nil
}
return append(installerArgs, "--append-karg", "root=/dev/disk/by-label/dm-mpath-root", "--append-karg", "rw", "--append-karg", "rd.multipath=default")

installerArgs = append(installerArgs, "--append-karg", "root=/dev/disk/by-label/dm-mpath-root", "--append-karg", "rw", "--append-karg", "rd.multipath=default")
var err error
for _, disk := range inventory.Disks {
if disk.DriveType == models.DriveTypeISCSI && strings.Contains(disk.Holders, installationDisk.Name) {
installerArgs, err = appendISCSIArgs(installerArgs, disk, inventory, hasUserConfiguredIP)
if err != nil {
return nil, err
}
}
}
return installerArgs, nil
}

func appends390xArgs(inventory *models.Inventory, installerArgs []string, log logrus.FieldLogger) ([]string, bool) {
Expand Down
80 changes: 80 additions & 0 deletions internal/host/hostcommands/install_cmd_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,86 @@ var _ = Describe("construct host install arguments", func() {
Expect(err).NotTo(HaveOccurred())
Expect(args).To(Equal(`["--append-karg","root=/dev/disk/by-label/dm-mpath-root","--append-karg","rw","--append-karg","rd.multipath=default"]`))
})
It("multipath 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",
"name": "dm-0"
},
{
"id": "other-id",
"drive_type": "%s",
"iscsi": {
"host_ip_address": "10.56.20.80"
},
"holders": "dm-0"
},
{
"id": "other-id-2",
"drive_type": "%s",
"iscsi": {
"host_ip_address": "10.56.20.80"
},
"holders": "dm-0"
}
],
"interfaces":[
{
"name": "eth1",
"ipv4_addresses":["10.56.20.80/25"]
}
]
}`, models.DriveTypeMultipath, models.DriveTypeISCSI, models.DriveTypeISCSI)
inventory, _ := common.UnmarshalInventory(host.Inventory)
args, err := constructHostInstallerArgs(cluster, host, inventory, infraEnv, log)
Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Debug info: Unmarshalled Inventory: %+v, Host's Inventory: %s", inventory, host.Inventory))
Expect(args).To(Equal(`["--append-karg","root=/dev/disk/by-label/dm-mpath-root","--append-karg","rw","--append-karg","rd.multipath=default","--append-karg","rd.iscsi.firmware=1","--append-karg","ip=eth1:dhcp"]`), fmt.Sprintf("Debug info: Actual args returned: %s", args))
})
It("multipath iSCSI installation disk with 2 different nics", func() {
cluster.MachineNetworks = []*models.MachineNetwork{{Cidr: "192.186.10.0/24"}}
host.Inventory = fmt.Sprintf(`{
"disks":[
{
"id": "install-id",
"drive_type": "%s",
"name": "dm-0"
},
{
"id": "other-id",
"drive_type": "%s",
"iscsi": {
"host_ip_address": "10.56.20.80"
},
"holders": "dm-0"
},
{
"id": "other-id-2",
"drive_type": "%s",
"iscsi": {
"host_ip_address": "10.56.20.81"
},
"holders": "dm-0"
}
],
"interfaces":[
{
"name": "eth0",
"ipv4_addresses":["10.56.20.80/25"]
},
{
"name": "eth1",
"ipv4_addresses":["10.56.20.81/25"]
}
]
}`, models.DriveTypeMultipath, models.DriveTypeISCSI, models.DriveTypeISCSI)
inventory, _ := common.UnmarshalInventory(host.Inventory)
args, err := constructHostInstallerArgs(cluster, host, inventory, infraEnv, log)
Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Debug info: Unmarshalled Inventory: %+v, Host's Inventory: %s", inventory, host.Inventory))
Expect(args).To(Equal(`["--append-karg","root=/dev/disk/by-label/dm-mpath-root","--append-karg","rw","--append-karg","rd.multipath=default","--append-karg","rd.iscsi.firmware=1","--append-karg","ip=eth0:dhcp","--append-karg","ip=eth1:dhcp"]`), fmt.Sprintf("Debug info: Actual args returned: %s", args))
})
It("non-multipath installation disk", func() {
cluster.MachineNetworks = []*models.MachineNetwork{{Cidr: "192.186.10.0/24"}}
host.Inventory = fmt.Sprintf(`{
Expand Down
28 changes: 20 additions & 8 deletions internal/network/manifests_generator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,12 +555,16 @@ spec:
[Service]
Type=oneshot
ExecStart=-/bin/sh -c ' \
lsblk -o NAME,TRAN,MOUNTPOINTS --json | jq -e \'.blockdevices[] | select(.tran == "iscsi") | select(.children) | .children[].mountpoints | select(.) | index("/sysroot") | select(.)\' > /dev/null; \
if [ $? = 0 ]; \
then \
echo "iSCSI boot volume detected, force network reconfiguration..."; \
nmcli -t -f DEVICE device status | xargs -l nmcli device reapply; \
ExecStart=-/bin/sh -c ' set -x; \
# Query to detect multipath iSCSI boot volumes \
MULTIPATH=".blockdevices[] | select(.tran == \\\"iscsi\\\") | select(.children) | .children[] | select(.children) | .children[].mountpoints | select(.) | index(\\\"/sysroot\\\")"; \
# Query to detect standalone iSCSI boot volumes \
ISCSI_ALONE=".blockdevices[] | select(.tran == \\\"iscsi\\\") | select(.children) | .children[].mountpoints | select(.) | index(\\\"/sysroot\\\")"; \
(lsblk -o NAME,TRAN,MOUNTPOINTS --json | jq -e "$MULTIPATH" > /dev/null) || (lsblk -o NAME,TRAN,MOUNTPOINTS --json | jq -e "$ISCSI_ALONE" > /dev/null); \
if [ $? = 0 ]; \
then \
echo "iSCSI OR multipath iSCSI boot volume detected, force network reconfiguration..."; \
nmcli -t -f DEVICE device status | xargs -l nmcli device reapply; \
fi'
ExecStartPost=-systemctl disable iscsi-nic-reapply.service
Expand All @@ -569,12 +573,20 @@ spec:
`

func (m *ManifestsGenerator) AddNicReapply(ctx context.Context, log logrus.FieldLogger, c *common.Cluster) error {
// Add this manifest only is one of the host is installting on an iSCSI boot drive
// Add this manifest only if one of the host is installing on an iSCSI / multiapth + iSCSI boot drive
_, isUsingISCSIBootDrive := lo.Find(c.Cluster.Hosts, func(h *models.Host) bool {
installationDisk, err := hostutil.GetHostInstallationDisk(h)
inventory, err := common.UnmarshalInventory(h.Inventory)
if err != nil {
return false
}
installationDisk := hostutil.GetDiskByInstallationPath(inventory.Disks, hostutil.GetHostInstallationPath(h))
if installationDisk.DriveType == models.DriveTypeMultipath {
for _, disk := range inventory.Disks {
if disk.DriveType == models.DriveTypeISCSI && strings.Contains(disk.Holders, installationDisk.Name) {
return true
}
}
}
return installationDisk.DriveType == models.DriveTypeISCSI
})

Expand Down
Loading

0 comments on commit 1eaabc5

Please sign in to comment.