MGMT-19670, MGMT-19484: Enable multipath + iSCSI as installation disk #7192

linoyaslan · 2025-01-15T10:21:03Z

Building on the work done to enable iSCSI as an installation disk, this PR extends support to enable multipath+iSCSI as an installation disk.

/cc @adriengentil

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2025-01-15T10:21:07Z

openshift-ci · 2025-01-15T10:21:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: linoyaslan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [linoyaslan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2025-01-15T10:22:44Z

linoyaslan · 2025-01-15T10:26:43Z

/cc @adriengentil

openshift-ci-robot · 2025-01-15T10:53:15Z

internal/host/hostcommands/install_cmd.go

adriengentil · 2025-01-15T10:48:52Z

internal/host/hostcommands/install_cmd.go

+				dhcp = "dhcp6"
+			}
+			installerArgs = append(installerArgs, "--append-karg", fmt.Sprintf("ip=%s:%s", nic.Name, dhcp))
+			break


we may have several disks connected through different interfaces, I think we should not break

adriengentil · 2025-01-15T10:50:17Z

internal/network/manifests_generator.go

@@ -556,7 +556,7 @@ 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; \
+            (lsblk -o NAME,TRAN,MOUNTPOINTS --json | jq -e ".blockdevices[] | select(.tran == \"iscsi\") | select(.children) | .children[] | select(.children) | .children[].mountpoints | select(.) | index(\"/sysroot\")" > /dev/null) || (lsblk -o NAME,TRAN,MOUNTPOINTS --json | jq -e ".blockdevices[] | select(.tran == \"iscsi\") | select(.children) | .children[].mountpoints | select(.) | index(\"/sysroot\")" > /dev/null); \


can you add an example? or maybe split in 2 conditions, in order to echo the right message below, depending if it's iSCSI boot or multipath+iscsi boot

What do you think of using a more generic message, such as "iSCSI / multipath+iSCSI boot volume detected, forcing network reconfiguration..." instead of splitting it into two separate conditions?

yeah, it would be fine. My main comment on this is to make the parsing of lsblk's output a bit clearer.

internal/network/manifests_generator.go

internal/hardware/validator.go

openshift-ci-robot · 2025-01-16T06:45:40Z

openshift-ci-robot · 2025-01-16T06:45:45Z

openshift-ci-robot · 2025-01-16T06:46:03Z

openshift-ci-robot · 2025-01-16T06:46:03Z

openshift-ci-robot · 2025-01-16T06:46:17Z

openshift-ci-robot · 2025-01-16T06:46:29Z

openshift-ci-robot · 2025-01-16T06:46:34Z

adriengentil · 2025-01-16T09:40:55Z

internal/host/hostcommands/install_cmd.go

@@ -391,16 +397,32 @@ 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))
+
+	if !slices.Contains(installerArgs, fmt.Sprintf("ip=%s:%s", nic.Name, dhcp)) {


use lo instead, it's already imported in this file: https://github.com/samber/lo?tab=readme-ov-file#contains

adriengentil · 2025-01-16T09:43:16Z

internal/hardware/validator.go

+	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"
+	errsInIscsiDisableMultipathInstallation = "Installation in multipath is not possible due to errors in at least one iSCSI disk."


Suggested change

errsInIscsiDisableMultipathInstallation = "Installation in multipath is not possible due to errors in at least one iSCSI disk."

errsInIscsiDisableMultipathInstallation = "Installation on multipath device is not possible due to errors in at least one iSCSI disk"

nit: or even
I think when describing the contents of a disk, it's more correct to say 'on' than 'in'. Data is 'on' a disk or 'in' memory as an example.

"Installation on multipath device is not possible due to errors on at least one iSCSI disk"

adriengentil · 2025-01-16T09:49:53Z

internal/hardware/validator.go

 					notEligibleReasons = append(notEligibleReasons,
-						fmt.Sprintf(wrongMultipathTypeTemplate, inventoryDisk.DriveType, string(models.DriveTypeFC)))
+						fmt.Sprintf(wrongMultipathTypeTemplate, inventoryDisk.DriveType, fmt.Sprintf("%s or %s", string(models.DriveTypeFC), string(models.DriveTypeISCSI))))


I think the message should be be consistent with

assisted-service/internal/hardware/validator.go

Line 147 in 2e03b51

fmt.Sprintf(wrongDriveTypeTemplate, disk.DriveType, strings.Join(v.getValidDeviceStorageTypes(hostArchitecture, clusterVersion), ", ")))

adriengentil · 2025-01-16T10:01:10Z

internal/host/hostcommands/install_cmd.go

+
+	for _, disk := range inventory.Disks {
+		if disk.DriveType == models.DriveTypeISCSI && strings.Contains(disk.Holders, installationDisk.Name) {
+			iSCSIInstallerArgs, err := appendISCSIArgs(installerArgs, disk, inventory, hasUserConfiguredIP)


Suggested change

iSCSIInstallerArgs, err := appendISCSIArgs(installerArgs, disk, inventory, hasUserConfiguredIP)

installerArgs, err := appendISCSIArgs(installerArgs, disk, inventory, hasUserConfiguredIP)

what is returned are all args that will be passed to the installer, not only the iSCSI ones

but if I remove the break, each iteration needs to take the returned arguments and append them to the installerArgs. Maybe the name isn’t accurate

appendISCSIArgs function already append items to installerArgs

adriengentil · 2025-01-16T10:04:59Z

internal/host/hostcommands/install_cmd.go

+			if err != nil {
+				return nil, err
+			}
+			installerArgs = append(installerArgs, iSCSIInstallerArgs...)


installerArgs should not be used at all in this flow if an error is raised, so I don't think you need a temp variable here (iSCSIInstallerArgs)

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.

adriengentil · 2025-01-16T16:36:20Z

internal/network/manifests_generator.go

+			if err != nil {
+				return false
+			}


paul-maidment · 2025-01-16T16:36:24Z

internal/network/manifests_generator.go

-                nmcli -t -f DEVICE device status | xargs -l nmcli device reapply;        \
+            ExecStart=-/bin/sh -c ' set -x; \
+            MULTIPATH=".blockdevices[] | select(.tran == \\\"iscsi\\\") | select(.children) | .children[] | select(.children) | .children[].mountpoints | select(.) | index(\\\"/sysroot\\\")"; \
+            ISCSI_ALONE=".blockdevices[] | select(.tran == \\\"iscsi\\\") | select(.children) | .children[].mountpoints | select(.) | index(\\\"/sysroot\\\")"; \


Maybe add a comment here to highlight what we are trying to do.
It's quite a hard read and could use a small explanation.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 15, 2025

linoyaslan marked this pull request as draft January 15, 2025 10:21

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2025

openshift-ci bot requested review from paul-maidment and rccrdpccl January 15, 2025 10:21

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2025

openshift-ci bot requested a review from adriengentil January 15, 2025 10:27

linoyaslan changed the title ~~MGMT-19670, MGMT-19484: Enable multipath as installation disk~~ MGMT-19670, MGMT-19484: Enable multipath + iSCSI as installation disk Jan 15, 2025

linoyaslan force-pushed the MGMT-19670.enable_multipath branch from e9a87b7 to cea6d01 Compare January 15, 2025 10:57

adriengentil reviewed Jan 15, 2025

View reviewed changes

linoyaslan force-pushed the MGMT-19670.enable_multipath branch from cea6d01 to 564e22b Compare January 15, 2025 18:17

adriengentil reviewed Jan 16, 2025

View reviewed changes

Enable multipath + iSCSI as installation disk

c6e41ad

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.

linoyaslan force-pushed the MGMT-19670.enable_multipath branch from 564e22b to c6e41ad Compare January 16, 2025 11:08

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 16, 2025

adriengentil reviewed Jan 16, 2025

View reviewed changes

paul-maidment reviewed Jan 16, 2025

View reviewed changes

	errsInIscsiDisableMultipathInstallation = "Installation in multipath is not possible due to errors in at least one iSCSI disk."
	errsInIscsiDisableMultipathInstallation = "Installation on multipath device is not possible due to errors in at least one iSCSI disk"

	iSCSIInstallerArgs, err := appendISCSIArgs(installerArgs, disk, inventory, hasUserConfiguredIP)
	installerArgs, err := appendISCSIArgs(installerArgs, disk, inventory, hasUserConfiguredIP)

MGMT-19670, MGMT-19484: Enable multipath + iSCSI as installation disk #7192

Are you sure you want to change the base?

MGMT-19670, MGMT-19484: Enable multipath + iSCSI as installation disk #7192

Conversation

linoyaslan commented Jan 15, 2025 • edited Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 15, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jan 15, 2025

openshift-ci-robot commented Jan 15, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

linoyaslan commented Jan 15, 2025

openshift-ci-robot commented Jan 15, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 16, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 16, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 16, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 16, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 16, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 16, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jan 16, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

adriengentil Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-maidment Jan 16, 2025 • edited Loading

linoyaslan commented Jan 15, 2025 •

edited

Loading

openshift-ci-robot commented Jan 15, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 15, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 15, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 16, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 16, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 16, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 16, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 16, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 16, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 16, 2025 •

edited by openshift-ci bot

Loading

adriengentil Jan 16, 2025 •

edited

Loading

paul-maidment Jan 16, 2025 •

edited

Loading