Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmdlib: only use index=N for supermin root #1398

Merged
merged 2 commits into from
Apr 28, 2020

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Apr 24, 2020

For #1244, I want to use serial=osmet on the disk to mark which one is
the source disk containing the metal image instead of relying on
index=N. However, to use serial=osmet, I have to use a separate
-device arg instead of the implied one from if=virtio, and it seems
like doing that causes qemu to give those disks higher priority such
that e.g. it gets /dev/vda, which throws off supermin.

Let's switch over all our disks to use -drive ... -device .... While
we're here, only rely on index=N for the supermin root itself, since
we can't use udev symlinks in the kernel cmdline. The cache disk too
doesn't need an index, we use the LABEL instead already.

@jlebon
Copy link
Member Author

jlebon commented Apr 24, 2020

@Prashanth684 Could you sanity-check that a cosa build works with this on s390x? It should, since it uses the same approach as coreos/ignition#905, but just to be sure since it does touch a bunch of arch-specific paths.

@cgwalters
Copy link
Member

since we can't use udev symlinks in the kernel cmdline.

Sure we can with an initramfs which is what we have now. It's just the custom supermin initramfs which is limited.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

LGTM!

@openshift-ci-robot
Copy link

@jlebon: PR needs rebase.

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.

Copy link
Contributor

@darkmuggle darkmuggle left a comment

Choose a reason for hiding this comment

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

LGTM as well.

jlebon added 2 commits April 28, 2020 09:32
For coreos#1244, I want to use `serial=osmet` on the disk to mark which one is
the source disk containing the metal image instead of relying on
`index=N`. However, to use `serial=osmet`, I have to use a separate
`-device` arg instead of the implied one from `if=virtio`, and it seems
like doing that causes qemu to give those disks higher priority such
that e.g. it gets `/dev/vda`, which throws off supermin.

Let's switch over all our disks to use `-drive ... -device ...`. While
we're here, only rely on `index=N` for the supermin root itself, since
we can't use udev symlinks in the kernel cmdline. The cache disk too
doesn't need an index, we use the LABEL instead already.
Instead of using SCSI, use virtio-blk's `serial=` option so allow us to
identify the disk inside `create_disk.sh`. And since we already have
udev running there now, we don't have to walk sysfs anymore looking for
our block device.
@jlebon jlebon force-pushed the pr/more-osmet-prep branch from 3da5c18 to e24956a Compare April 28, 2020 13:32
@jlebon
Copy link
Member Author

jlebon commented Apr 28, 2020

Rebased! Was hoping for a sanity-check for multi-arch, though maybe let's just get this in? I'd like to get #1244 in for coreos/coreos-installer#187.

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, darkmuggle, jlebon

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 [cgwalters,darkmuggle,jlebon]

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

@jlebon
Copy link
Member Author

jlebon commented Apr 28, 2020

/refresh

@jlebon
Copy link
Member Author

jlebon commented Apr 28, 2020

/refresh

@openshift-merge-robot openshift-merge-robot merged commit a5de52d into coreos:master Apr 28, 2020
@jlebon jlebon deleted the pr/more-osmet-prep branch July 6, 2020 20:32
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