MGMT-16273: Allow installing on iSCSI disks on OCI #5728

avishayt · 2023-11-22T16:09:33Z

Allow booting from iSCSI for x86_64 OpenShift versions at least 4.15.0 on the OCI platform.

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 · 2023-11-22T16:09:37Z

avishayt · 2023-11-22T16:09:40Z

/hold WIP

codecov · 2023-11-22T16:23:46Z

Codecov Report

Merging #5728 (200b930) into master (86e56d9) will increase coverage by 0.09%.
Report is 3 commits behind head on master.
The diff coverage is 91.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5728      +/-   ##
==========================================
+ Coverage   67.93%   68.02%   +0.09%     
==========================================
  Files         233      233              
  Lines       34391    34338      -53     
==========================================
- Hits        23362    23357       -5     
+ Misses       8954     8915      -39     
+ Partials     2075     2066       -9

Files	Coverage Δ
internal/host/hostcommands/install_cmd.go	`83.91% <100.00%> (+0.21%)`	⬆️
internal/host/validator.go	`81.95% <100.00%> (ø)`
internal/hardware/validator.go	`76.73% <88.23%> (+0.23%)`	⬆️

... and 17 files with indirect coverage changes

internal/host/hostcommands/install_cmd.go

jlebon · 2023-11-23T16:28:46Z

internal/host/hostcommands/install_cmd.go

+			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")


Since this PR looks like it's adding generic support for iSCSI on bare metal, I will mention here that auto-configuration via iBFT (used by rd.iscsi.firmware) is not always a given. The other way iSCSI configuration is provided is via netroot=iscsi:... (see dracut.cmdline(7)).

If we want to provide a nice UX for it, we could provide separate fields for e.g. server name, target name, initiator name, etc... and assemble the netroot karg ourselves. This is similar to what blivet (Anaconda storage backend) does: https://github.com/storaged-project/blivet/blob/57f1382f19a1390dd2176e4f4772bab89380cfdf/blivet/devices/disk.py#L379-L394.

Alternatively, if the UX allows users to provide custom kargs, then we could skip this for now and users can always directly provide the netroot karg themselves as an escape hatch (and having rd.iscsi.firmware=1 still hardcoded shouldn't hurt, though this code could also auto-detect if one of the custom kargs is netroot and skip it I guess).

Users can provides custom kargs to the installer though the API (https://github.com/openshift/assisted-service/blob/master/docs/user-guide/install-customization.md#set-the-installer-params). Maybe we can add a word about customizing iSCSI settings in our doc in the future.

That is a good point about generic support. I'll make this PR focused on OCI. We can explore generic support in the future if there is demand for it.

adriengentil · 2023-11-23T17:13:36Z

/test edge-e2e-oci-assisted
The issue should be fixed by now

adriengentil · 2023-11-24T07:48:15Z

/test edge-e2e-metal-assisted edge-e2e-oci-assisted

openshift-ci-robot · 2023-11-26T08:58:52Z

Allow booting from iSCSI for x86_64 OpenShift versions at least 4.15.0 on the OCI platform.

adriengentil

Just one nit, otherwise it looks good

adriengentil · 2023-11-27T11:13:31Z

internal/hardware/validator.go

@@ -147,8 +157,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, openshiftVersion string, ociPlatformType bool) bool {


Suggested change

func (v *validator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, openshiftVersion string, ociPlatformType bool) bool {

func (v *validator) IsValidStorageDeviceType(disk *models.Disk, hostArchitecture string, openshiftVersion string, isISCSIEnabled bool) bool {

nit: Even though this change is specific to oci, I would prefer to make the intention of this parameter clearer for this function and in getValidDeviceStorageTypes. WDYT?

I think it's OK for now. The next big change to this code will probably be when iSCSI support will be generic, and this parameter will be removed.

openshift-ci · 2023-11-27T14:21:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil, avishayt

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~~ [adriengentil,avishayt]

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

avishayt · 2023-11-27T15:31:43Z

/unhold

openshift-ci-robot · 2023-11-27T18:05:51Z

/retest-required

Remaining retests: 0 against base HEAD 25d0ae7 and 2 for PR HEAD 200b930 in total

avishayt · 2023-11-27T21:12:17Z

/test edge-e2e-metal-assisted

avishayt · 2023-11-28T06:29:33Z

/test edge-e2e-metal-assisted

openshift-ci · 2023-11-28T08:11:50Z

@avishayt: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2023-11-28T10:32:56Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-agent-installer-api-server-container-v4.15.0-202311280949.p0.gc6ba398.assembly.stream for distgit ose-agent-installer-api-server.
All builds following this will include this PR.

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

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 22, 2023

openshift-ci bot requested review from adriengentil and javipolo November 22, 2023 16:19

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2023

jlebon reviewed Nov 22, 2023

View reviewed changes

internal/host/hostcommands/install_cmd.go Show resolved Hide resolved

avishayt force-pushed the avishay/MGMT-16273 branch from 381e8c8 to 92ccb40 Compare November 23, 2023 08:33

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 Nov 23, 2023

avishayt force-pushed the avishay/MGMT-16273 branch from 92ccb40 to 15dbe56 Compare November 23, 2023 10:16

adriengentil approved these changes Nov 23, 2023

View reviewed changes

openshift-ci bot assigned adriengentil Nov 23, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2023

avishayt force-pushed the avishay/MGMT-16273 branch from 15dbe56 to 22b19a6 Compare November 23, 2023 16:27

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2023

jlebon reviewed Nov 23, 2023

View reviewed changes

adriengentil approved these changes Nov 25, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2023

avishayt force-pushed the avishay/MGMT-16273 branch from 22b19a6 to 1f120d2 Compare November 26, 2023 08:58

openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2023

avishayt changed the title ~~MGMT-16273: Allow installing on iSCSI disks~~ MGMT-16273: Allow installing on iSCSI disks on OCI Nov 26, 2023

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 26, 2023

avishayt force-pushed the avishay/MGMT-16273 branch from 1f120d2 to 4fb6988 Compare November 26, 2023 19:22

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 Nov 26, 2023

MGMT-16273: Allow installing on iSCSI disks on OCI

200b930

Allow booting from iSCSI for x86_64 OpenShift versions at least 4.15.0 on the OCI platform.

avishayt force-pushed the avishay/MGMT-16273 branch from 4fb6988 to 200b930 Compare November 27, 2023 07:38

adriengentil reviewed Nov 27, 2023

View reviewed changes

adriengentil approved these changes Nov 27, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2023

openshift-merge-bot bot merged commit c6ba398 into openshift:master Nov 28, 2023
14 checks passed

adriengentil mentioned this pull request Dec 1, 2023

test iscsi without ip=ibft #5758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-16273: Allow installing on iSCSI disks on OCI #5728

MGMT-16273: Allow installing on iSCSI disks on OCI #5728

avishayt commented Nov 22, 2023 •

edited

openshift-ci-robot commented Nov 22, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

avishayt commented Nov 22, 2023

codecov bot commented Nov 22, 2023 •

edited

jlebon Nov 23, 2023

adriengentil Nov 25, 2023

avishayt Nov 26, 2023

adriengentil commented Nov 23, 2023

adriengentil commented Nov 24, 2023

openshift-ci-robot commented Nov 26, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

adriengentil left a comment

adriengentil Nov 27, 2023

avishayt Nov 27, 2023

openshift-ci bot commented Nov 27, 2023

avishayt commented Nov 27, 2023

openshift-ci-robot commented Nov 27, 2023

avishayt commented Nov 27, 2023

avishayt commented Nov 28, 2023

openshift-ci bot commented Nov 28, 2023

openshift-bot commented Nov 28, 2023

	func (v validator) IsValidStorageDeviceType(disk models.Disk, hostArchitecture string, openshiftVersion string, ociPlatformType bool) bool {
	func (v validator) IsValidStorageDeviceType(disk models.Disk, hostArchitecture string, openshiftVersion string, isISCSIEnabled bool) bool {

MGMT-16273: Allow installing on iSCSI disks on OCI #5728

MGMT-16273: Allow installing on iSCSI disks on OCI #5728

Conversation

avishayt commented Nov 22, 2023 • edited

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 Nov 22, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

avishayt commented Nov 22, 2023

codecov bot commented Nov 22, 2023 • edited

Codecov Report

jlebon Nov 23, 2023

Choose a reason for hiding this comment

adriengentil Nov 25, 2023

Choose a reason for hiding this comment

avishayt Nov 26, 2023

Choose a reason for hiding this comment

adriengentil commented Nov 23, 2023

adriengentil commented Nov 24, 2023

openshift-ci-robot commented Nov 26, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

adriengentil left a comment

Choose a reason for hiding this comment

adriengentil Nov 27, 2023

Choose a reason for hiding this comment

avishayt Nov 27, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Nov 27, 2023

avishayt commented Nov 27, 2023

openshift-ci-robot commented Nov 27, 2023

avishayt commented Nov 27, 2023

avishayt commented Nov 28, 2023

openshift-ci bot commented Nov 28, 2023

openshift-bot commented Nov 28, 2023

avishayt commented Nov 22, 2023 •

edited

openshift-ci-robot commented Nov 22, 2023 •

edited by openshift-ci bot

codecov bot commented Nov 22, 2023 •

edited

openshift-ci-robot commented Nov 26, 2023 •

edited by openshift-ci bot