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 buildextend-installer command to build installer ISO #306

Merged
merged 1 commit into from
Feb 11, 2019
Merged

Add buildextend-installer command to build installer ISO #306

merged 1 commit into from
Feb 11, 2019

Conversation

dustymabe
Copy link
Member

No description provided.

@dustymabe dustymabe added the WIP PR still being worked on label Feb 1, 2019
@dustymabe
Copy link
Member Author

This is a complete WIP PR. It currently does not attempt to integrate with other outstanding PRs that may be needed to structurally build this the correct way in COSA.

  • Right now you can generate an installer with coreos-assembler buildextend-installer.
  • It expects the installer dracut module to be embedded in the ostree (currently not upstream)
  • It expects to find isolinux configuration files in /src/config/isolinux/. For example I have boot.msg and isolinux.cfg inside of that directory on my local system where I've been hacking on this.

@dustymabe
Copy link
Member Author

It currently does not attempt to integrate with other outstanding PRs that may be needed to structurally build this the correct way in COSA.

this is a way of saying, let's not get too caught up on the structure just yet.

src/cmd-buildextend-installer Outdated Show resolved Hide resolved
src/cmd-buildextend-installer Outdated Show resolved Hide resolved
src/cmd-buildextend-installer Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member Author

dustymabe commented Feb 2, 2019

at least those few comments should be addressed. I've got to take a look at the other PRs and see where this all fits in. Also I just created the github.com/coreos/coreos-installer repo where I'll be posting up the installer code for this.

@cgwalters
Copy link
Member

I've got to take a look at the other PRs and see where this all fits in

Luckily given we didn't explicitly coordinate, I believe this is actually nicely complementary to my work in
#305

This is about a PXE path (right)? Which will end up dd-ing an image to disk. I assume you've been testing with the "base image" which today is cloud oriented. My PR is about having a base disk image designed for metal.

@dustymabe
Copy link
Member Author

I've got to take a look at the other PRs and see where this all fits in

Luckily given we didn't explicitly coordinate, I believe this is actually nicely complementary to my work in
#305

Nice. Sorry I haven't looked at it yet. I'm going to get the bits up at github.com/coreos/coreos-installer today and then have a look at the outstanding PRs you have open here.

This is about a PXE path (right)? Which will end up dd-ing an image to disk. I assume you've been testing with the "base image" which today is cloud oriented. My PR is about having a base disk image designed for metal.

Yeah I've been just using the disk image that we've been building (converting it to raw first).

@cgwalters
Copy link
Member

So this ISO will boot into the initramfs; in order to do an install one will need to change the kernel command line to point to the target disk image, right?

(Soon we're going to need to cross the "documentation for FCOS" issue)

@cgwalters
Copy link
Member

Code seems sane to me!

@dustymabe
Copy link
Member Author

OK I've done a little knocking down some of these roadblocks:

Right now you can generate an installer with coreos-assembler buildextend-installer.

This is this PR. You'll need to pull down this PR to use it.

It expects the installer dracut module to be embedded in the ostree (currently not upstream)

I've upstreamed my work to https://github.com/coreos/coreos-installer/tree/master

It expects to find isolinux configuration files in /src/config/isolinux/. For example I have boot.msg and isolinux.cfg inside of that directory on my local system where I've been hacking on this.

Opened a PR here for this: coreos/fedora-coreos-config#47. As part of that PR I also have a section that includes the dracut files directly into the OSTree for fast development (see coreos/fedora-coreos-config@a8577b8).

You should be able to:

$ cd $COREOS_ASSEMBLER_CONFIG_GIT
$ git fetch origin pull/47/head:PR47; git checkout PR47
$ git clone https://github.com/coreos/coreos-installer
$ cd -
$ coreos-assembler build
$ coreos-assembler buildextend-installer

You'll then need to copy the generated ISO somewhere and boot it. You can provide command line args: coreos.inst.image_url coreos.inst.install_dev coreos.inst.ignition_url.

This is still very rough around the edges so I only suggest trying this if you like pain and can't wait :)

Also noteworthy is that ignition fails creating the coreos user when trying to actually boot the generated FCOS qcow with the installer dracut module included. Haven't had a chance to debug that one yet.

os.chmod(dst, mode)

# Generate the ISO image
run_verbose(['/usr/bin/genisoimage', '-b', 'isolinux.bin', '-c', 'boot.cat',
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing the genisoimage is pulled in as a dependency of another package. It may be worth adding it to the deps.txt.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 - fixed

@ashcrow
Copy link
Member

ashcrow commented Feb 5, 2019

/cc @mrguitar

@dustymabe
Copy link
Member Author

fixed code review comments, rebased to latest upstream master, and also added an exception to cosa compress to not compress ISO images.

I'm going to lift WIP for now. The other work is still in PRs elsewhere but I think we can go ahead and merge this without bothering anything else.

@dustymabe dustymabe removed the WIP PR still being worked on label Feb 8, 2019
@dustymabe dustymabe changed the title WIP: add command to build installer ISO add command to build installer ISO Feb 8, 2019
Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

LGTM

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.

How about a commit message like:

buildextend-installer: New command

This will allow people to use the new installer
https://github.com/coreos/coreos-installer/
to e.g. do PXE installs to bare metal.

'-no-emul-boot', '-boot-load-size', '4', '-boot-info-table',
'-rock', '-J', '--verbose', '-o', tmpisofile, tmpisoroot])
checksum = sha256sum_file(tmpisofile)
buildmeta['images']['iso'] = {
Copy link
Member

Choose a reason for hiding this comment

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

Eventually we'll want to land a live-iso or so right? How about installer-iso?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're still working out those details as we may just decide to ship one ISO that can do both. Still some details to work out there. Let's cross that in a followup if you don't mind.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM!

src/cmd-buildextend-installer Show resolved Hide resolved
@dustymabe
Copy link
Member Author

How about a commit message like:

buildextend-installer: New command

This will allow people to use the new installer
https://github.com/coreos/coreos-installer/
to e.g. do PXE installs to bare metal.

done. though I s/PXE/ISO/ for now since I haven't specifically addressed that use case yet

@dustymabe
Copy link
Member Author

added fixups from CR and rebased on latest master

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Just some minor nits. But LGTM as is!

src/cmd-buildextend-installer Outdated Show resolved Hide resolved
src/cmd-buildextend-installer Outdated Show resolved Hide resolved
This will allow people to use the new installer
https://github.com/coreos/coreos-installer/
to e.g. do ISO installs to bare metal.
@dustymabe
Copy link
Member Author

fixed CR comments from @jlebon

@jlebon jlebon changed the title add command to build installer ISO Add buildextend-installer command to build installer ISO Feb 11, 2019
@jlebon
Copy link
Member

jlebon commented Feb 11, 2019

LGTM.
I just tweaked the PR title to include the new command name to make it easier to find in the future.

@dustymabe
Copy link
Member Author

I just tweaked the PR title to include the new command name to make it easier to find in the future.

👍 - anyone opposed to merging?

Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

No objections to merge.

@jlebon
Copy link
Member

jlebon commented Feb 11, 2019

Will merge before end of day today if there are no other objections.
Awesome work on this BTW! 👍

@jlebon jlebon merged commit 6c20f93 into coreos:master Feb 11, 2019
@mrguitar
Copy link

Yeah nice work on this. Only issue I see is I don't think this will work w/ UEFI support, but that should be pretty simple to add later.

jlebon added a commit to jlebon/fedora-coreos-pipeline that referenced this pull request Feb 12, 2019
Start using the new `buildextend-installer` command from:
coreos/coreos-assembler#306
@dustymabe dustymabe deleted the installer branch February 15, 2019 20:48
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.

5 participants