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

Add metal-bios and metal-uefi targets #305

Merged
merged 4 commits into from
Feb 6, 2019
Merged

Conversation

cgwalters
Copy link
Member

Split out the "net.ifnames" stuff into a separate kickstart, and
generate a raw disk image metal-bios if specified.

Note to really be useful this requires Ignition to load /boot/config.ign
or so.

@cgwalters
Copy link
Member Author

Depends #302

@cgwalters
Copy link
Member Author

cgwalters commented Feb 1, 2019

An architectural question here is whether we should follow the "buildextend" model instead. The issue with that is that the "cloud/metal" split introduced here has changes around the kernel cmdline/networking that would be tricky to do as a "transformation" of a cloud image (or vice versa).

One thing that might be nice is if we actually pushed handling the early networking logic to Ignition; e.g. it could detect the oemid and e.g. tell dracut to request networking, and also configure the network adapter. That way the disk image would look a lot closer between the two cases - we'd be doing things dynamically in Ignition.

@cgwalters
Copy link
Member Author

@ajeddeloh re my random network/Ignition musings ⬆️

@cgwalters cgwalters changed the title WIP: Add metal-bios target Add metal-bios target Feb 6, 2019
@cgwalters
Copy link
Member Author

OK rebased 🏄‍♂️ and lifting WIP. This still requires Ignition to read /boot or something equivalent to be ergonomic though.

@cgwalters cgwalters changed the title Add metal-bios target Add metal-bios and metal-uefi targets Feb 6, 2019
@cgwalters
Copy link
Member Author

OK and now with GPT partitioning by default and a metal-uefi target.

@cgwalters
Copy link
Member Author

and a metal-uefi target

Which actually boots when I dd it to a spare laptop I have.

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

Changes mostly LGTM some comments scattered.

src/image-metal.ks Show resolved Hide resolved
src/cmd-build Outdated Show resolved Hide resolved
src/cmd-build Show resolved Hide resolved
src/cmd-build Show resolved Hide resolved
@dustymabe
Copy link
Member

Which actually boots when I dd it to a spare laptop I have.

nice! 🥳

@dustymabe
Copy link
Member

One thing I do wonder: If the "no anaconda" work lands soon we will be ripping some of this out. Is that something you'd be OK with ?

@dustymabe
Copy link
Member

cc @ajeddeloh

@cgwalters
Copy link
Member Author

One thing I do wonder: If the "no anaconda" work lands soon we will be ripping some of this out. Is that something you'd be OK with ?

Yeah of course, but the external API should stay the same - e.g. the metal-bios and metal-uefi names. (So if we want to bikeshed those do it now)

@cgwalters
Copy link
Member Author

Squashed in the fixes ⬆️

@dustymabe
Copy link
Member

Yeah of course, but the external API should stay the same - e.g. the metal-bios and metal-uefi names. (So if we want to bikeshed those do it now)

re: metal-bios and metal-uefi. No problem with the names but if we are able to get a combined image then the need for having separate names would go away. So the api would change in that case. Acceptable?

@cgwalters
Copy link
Member Author

No problem with the names but if we are able to get a combined image then the need for having separate names would go away. So the api would change in that case. Acceptable?

I had thought we'd just have them refer to the same image.

@dustymabe
Copy link
Member

I had thought we'd just have them refer to the same image.

I guess we can cross that bridge when we get there, but if we do the work to get a combined image I'd rather deprecate metal-bios metal-uefi and expose it as metal in the future.

@cgwalters
Copy link
Member Author

but if we do the work to get a combined image I'd rather deprecate metal-bios metal-uefi and expose it as metal in the future.

shrug - we're talking about a few lines of JSON. I don't think it's worth worrying over too much.

@dustymabe
Copy link
Member

but if we do the work to get a combined image I'd rather deprecate metal-bios metal-uefi and expose it as metal in the future.

shrug - we're talking about a few lines of JSON. I don't think it's worth worrying over too much.

agree. just wanted to set expectations accordingly

@dustymabe
Copy link
Member

ok relooked over this.. only outstanding comment is #305 (comment) I think

@dustymabe
Copy link
Member

/me running through a test now

Split out the "net.ifnames" stuff into a separate kickstart, and
generate a raw disk image `metal-bios` if specified.

Note to really be useful this requires Ignition to load `/boot/config.ign`
or so.
Let's be clear these two things are tightly coupled.  Also
virt-install is already in Python and it makes sense to do
more that way than via bash.
With a `/boot` partition.  But today Anaconda doesn't expose an
API to explicitly label it `/boot` (or even change the filesystem
type).  We'll live with that for now.

Prep for UEFI.
This builds but I haven't tested running it yet.
@cgwalters
Copy link
Member Author

And updated again to fix ShellCheck.

@dustymabe
Copy link
Member

ran a cosa build metal-bios after running a cosa build and I get No apparent changes since previous commit; use --force-nocache to override . Should we add any handling for cases like that?

@cgwalters
Copy link
Member Author

cgwalters commented Feb 6, 2019

Should we add any handling for cases like that?

Yes but let's not block merging this on that.

  • It's a developer issue and we need to ship content
  • This code needs to land so multi-arch work doesn't conflict and I've already rebased a few times
  • Really cmd-build needs a rewrite; doing things like that requires parsing the JSON which sucks in bash

@dustymabe
Copy link
Member

Yes but let's not block merging this on that.

OK

Finally got through with testing. I was able to take the output fedora-coreos-29.4-metal-bios.raw convert it to gz and then use the installer ISO from my other work and perform an install.

@cgwalters
Copy link
Member Author

This was deployed in FCOS in coreos/fedora-coreos-pipeline#43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants