Skip to content

Conversation

@Prashanth684
Copy link
Contributor

For s390x in pXE mode, the network device has the bootindex 1. Add a disk
without specifying it as a primary disk with bootindex.

Had to open this anew because #1336 couldn't be reopened because i rebased it .

Size: "12G", // Arbitrary
primaryDiskAdded := false
if system.RpmArch() == "s390x" {
// For s390x PXE installs the network device has the bootindex of 1.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...like this is something hardcoded into qemu or the firmware or something?

I am trying to get my head around how this works; is it basically that the s390x firmware has builtin logic around "use the disk first if it's bootable, otherwise network" and our adjusting the bootorder breaks it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am sure it is something like that..i try without the bootindex and it looks for a bootable disk. I have been tried to get a hold of someone from the s390x world to confirm but no one seems to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For s390x in pXE mode, the network device has the bootindex 1. Add a disk
without specifying it as a primary disk with bootindex.
@cgwalters
Copy link
Member

/test sanity
/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, Prashanth684

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-merge-robot openshift-merge-robot merged commit 2a8ca39 into coreos:master Apr 15, 2020
@jlebon
Copy link
Member

jlebon commented Apr 15, 2020

AddPrimaryDisk doesn't just affect the bootindex though. There's a lot of different paths that this is now missing out on, i.e. Ignition config injection and setting the disk serial.

@cgwalters
Copy link
Member

There's a lot of different paths that this is now missing out on, i.e. Ignition config injection and setting the disk serial.

First, let's keep in mind this is only changing s390x, and only for tests, and we have plans for a better fix.

That said...I don't understand your comment. We are injecting ignition still with coreos-installer right? And what do you mean here by "disk serial"?

@jlebon
Copy link
Member

jlebon commented Apr 16, 2020

Yeah, this came up first in my backlog, so it didn't connect that it'd get revamped by #1348. :)

That said...I don't understand your comment. We are injecting ignition still with coreos-installer right? And what do you mean here by "disk serial"?

Just overall, I think having the concept of a primary disk also makes sense for the installer case; it's the disk we're installing to (e.g. if we later add multiple disks to test separate /var or a moved /sysroot or something we'd have to discern between them). The primary disk gets serial=primary-disk, which we could use to find it instead of hardcoding vda (which would break on e.g. NVMe or SCSI). You're right though the Ignition injection bits aren't needed here.

Anyway, I think using QMP as in #1348 is definitely the right path, but just wanted to flag that it's not just about the bootindex.

@cgwalters
Copy link
Member

Oh yes...we should probably use the same wwn approach here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants