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.

Sign up for GitHub

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

Jump to bottom

Add support for 4k metal images #1130

Merged
merged 3 commits into from
Mar 13, 2020
Merged

Add support for 4k metal images #1130

merged 3 commits into from
Mar 13, 2020

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Feb 13, 2020 •
edited
Loading

First, add a new buildextend-metal4k command to create 4k disk images.
Then, teach kola and cosa run to read these images.

To test:

host$ cosa run -I metal4k
...
vm$ sudo fdisk -l /dev/vda
...
Sector size (logical/physical): 4096 bytes / 4096 bytes
...

One potentially controversial bit here is that this requires a newer
libguestfs which isn't in f31 yet, so we pull it from f32 for now.

Closes: coreos/fedora-coreos-tracker#385

@jlebon
Copy link
Member Author

jlebon commented Feb 13, 2020

Let's hold this until #1024 gets in, and I'll rebase on top of that.

@jlebon jlebon mentioned this pull request Feb 13, 2020
Closed
@jlebon
Copy link
Member Author

jlebon commented Feb 13, 2020

Discussions about this in coreos/fedora-coreos-tracker#385.

@darkmuggle
Copy link
Contributor

@jlebon when this is ready, let's merge this. I do not forsee #1024 merging quickly due to the larger question raised. This has user pain now and solving that is more important IMO.

@bcg62
Copy link

bcg62 commented Mar 10, 2020

looks like #1024 was closed, is there anything else holding this up?

@jlebon
Copy link
Member Author

jlebon commented Mar 11, 2020

Got the artifact working, but need to complete wiring it up on the kola side to make it easier to test.

@jlebon jlebon mentioned this pull request Mar 11, 2020
Merged
@jlebon jlebon changed the title WIP: Add metal4k build artifacts Add support for 4k metal images Mar 11, 2020
@jlebon jlebon marked this pull request as ready for review March 11, 2020 16:20
@jlebon
Copy link
Member Author

jlebon commented Mar 11, 2020

OK, dropped WIP on this and marked ready for review!

I updated the first comment with the commit message, but this is worth highlighting again:

One potentially controversial bit here is that this requires a newer
libguestfs which isn't in f31 yet, so we pull it from f32 for now.

I think this is probably fine since f32 is almost out. But given that we extensively use libguestfs, it's possible we'll hit regressions too.

cgwalters
cgwalters reviewed Mar 11, 2020
View reviewed changes
src/cmd-run Show resolved Hide resolved
@cgwalters
Copy link
Member

There's a lot of other stuff to enhance for this, including coreos-installer support and testiso too. Let me keep holding a mutex on testiso stuff for now!

@jlebon
Copy link
Member Author

jlebon commented Mar 11, 2020

Updated!

@cgwalters
Copy link
Member

/approve
Notable change, so I'll defer to a second for lgtm.

@jlebon
create_disk.sh: drop -- handling
50bec70 
We don't pass extra args to some program. So stop trying to handle `--`.
Plus, it wouldn't work anyway since the `*)` preceding it would catch
it.
@jlebon
create_disk.sh: drop unused BIOSPN var
6dbb099
@jlebon
Add support for 4k metal images
3e60ef1 
First, add a new `buildextend-metal4k` command to create 4k disk images.
Then, teach `kola` and `cosa run` to read these images.

To test:

    host$ cosa run -I metal4k
    ...
    vm$ sudo fdisk -l /dev/vda
    ...
    Sector size (logical/physical): 4096 bytes / 4096 bytes
    ...

One potentially controversial bit here is that this requires a newer
libguestfs which isn't in f31 yet, so we pull it from f32 for now.

Closes: coreos/fedora-coreos-tracker#385
@jlebon
Copy link
Member Author

jlebon commented Mar 13, 2020

Any more thoughts on this?

@jlebon
Copy link
Member Author

jlebon commented Mar 13, 2020

With the FCOS release that just went out, this would be a good time to get this in and let it bake and rollback the libguestfs bump if necessary.

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, 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:

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 dd8086c into coreos:master Mar 13, 2020
jcajka pushed a commit to jcajka/coreos-assembler that referenced this pull request Mar 24, 2020
@jlebon
Merge pull request coreos#1130 from cgwalters/qemu-process
a13f9df 
qemu: Clean up argv handling a bit
@jlebon jlebon mentioned this pull request Apr 16, 2020
Closed
@jlebon jlebon deleted the pr/add-4k branch July 6, 2020 20:28
@dustymabe dustymabe mentioned this pull request Nov 22, 2023
Merged
@jlebon jlebon mentioned this pull request Mar 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters left review comments

@bgilbert bgilbert Awaiting requested review from bgilbert

@dustymabe dustymabe Awaiting requested review from dustymabe

Assignees

@cgwalters cgwalters

Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to install on 4k drive
6 participants
@jlebon @darkmuggle @bcg62 @cgwalters @openshift-ci-robot @openshift-merge-robot