machine-drivers / docker-machine-driver-xhyve

docker-machine/minikube/minishift driver plugin for xhyve/hyperkit (native macOS hypervisor.framework)
https://godoc.org/github.com/machine-drivers/docker-machine-driver-xhyve
BSD 3-Clause "New" or "Revised" License
888 stars 74 forks source link

Fix #146 Make /boot configurable to allow different live iso to work with driver #147

Closed praveenkumar closed 7 years ago

praveenkumar commented 7 years ago

This patch will create a command line option to specify location for kernel and initrd files and set default boot so that it will not break any existing functionality.

Use case: Now to use a centos live iso (which have docker configured) can consume it with docker machine.

docker-machine -D create centos -d xhyve --xhyve-boot2docker-url=file:/Users/prkumar/live-centos.iso --xhyve-boot-cmd "loglevel=3 root=live:CDLABEL=live-centos user=docker console=ttyS0 console=tty0 noembed nomodeset norestore waitusb=10 base host=boot2docker" --xhyve-boot-location "isolinux"
zchee commented 7 years ago

@praveenkumar Thanks pull request! Good catch, and yes, hardcoded boot is bad.

It seems to works fine on docker-machine and minikube(minikube using my driver for the xhyve backend). Just in case, /cc @dlorenc

hferentschik commented 7 years ago

Would it not be better to do something similar to issue #135 where in the end several default kernel names are checked? Basically instead of having '--xhyve-boot-location', the driver internally checks a known list of locations. I am just afraid that the user gets overpowered by very low level command line switches.

Maybe a mixed approach is best, where the options does exist, but there is also a list of locations which get checked.

zchee commented 7 years ago

@hferentschik Hmm, You have a point. Thanks. @praveenkumar What do you think?

And Originally, We wanted to provide the pre-compiled binary for minikube users ASAP. Because of gettimeofday ABI problem. https://github.com/zchee/docker-machine-driver-xhyve/issues/137 So it had become sloppy implementation such as the hard-coded boot.

Maybe a mixed approach is best, where the options does exist, but there is also a list of locations which get checked.

SGTM. ...BTW, Sorry I want to ask the question because of you work on Red Hat. I think at least know than me. and I saw the linux source the first time at last month for kexec issue, So I do not yet read all specification and do not know linux boot process...

But is there uniformity specifications of kernel and initrd filenames and boot directory path? If there, I want to write a parser(or file|dir name list) that conforms to specifications. Do you know about it?

Edit: I know that is different by the distributions. I mean, whether there the uniformity specification.

hferentschik commented 7 years ago

But is there any specifications of kernel and initrd filenames and boot directory path?

I would love to know this as well :-) My understanding is that there is at best some best practices or some more used alternatives. However, in the end you can call this whatever you like and place it pretty much anywhere. It is the bootloader (for example Grub) which will in the case of an actual boot of the ISO handle the kernel load.

If there, I want to write a parser(or file|dir name list) that conforms to specifications. Do you know about it?

No. One might just have to compile a list of most used kernel names and locations and try to locate them.

Another approach could be to look for the boot loader configuration and extract the kernel location from there. Obviously one needs to deal with multiple bootloaders here as well (Syslinux for Boot2Docker ISO or Grub2 for the CentOS image for which this issue was created), but maybe the choices are more limited and the result is more deterministic.

praveenkumar commented 7 years ago

@praveenkumar What do you think?

Yes we can have mix-match approach which consist default option as list and then also user have capability to add something if structure is different.

I know that is different by the distributions. I mean, whether there the uniformity specification.

AFAIK, no each distribution use different tools to create their images and it's completely depend on distribution community how would they structure it. I did some check on different distro like ubuntu, suse, fedora/centos ..etc. and got different path for kernel + initrd file.

zchee commented 7 years ago

@hferentschik @praveenkumar Thanks for the polite reply :)

That was very helpful for me, and I understand your said meaning. But this topic is my knowledge are not enough... I'll research it. Please give me some time for detailed reply.

This is just a quick reply,

Another approach could be to look for the boot loader configuration and extract the kernel location from there. Obviously one needs to deal with multiple bootloaders here as well (Syslinux for Boot2Docker ISO or Grub2 for the CentOS image for which this issue was created), but maybe the choices are more limited and the result is more deterministic.

That's a good alternative approach. But how can we find the boot loader configuration? I tried to think of it, can find use https://github.com/zchee/linux-documentation/blob/master/x86/boot.md#type_of_loader parameter, but this my idea was needed kernel file path before to parse it...

Yes we can have mix-match approach which consist default option as list and then also user have capability to add something if structure is different.

Got it. I'll research it, but do you have a representative location list?

LalatenduMohanty commented 7 years ago

@hferentschik @zchee Isolinux is the boot loader for live images and the the path for Vmlinuz and initrd should be mentioned in isolinux.cfg [2].

[1] http://www.syslinux.org/wiki/index.php?title=ISOLINUX [2] https://github.com/boot2docker/boot2docker/blob/master/rootfs/isolinux/isolinux.cfg

zchee commented 7 years ago

@LalatenduMohanty Thanks for the advice! also thanks again Red Hat team 😭 I understand one by one...

Yes, minikube-iso and centos iso also have isolinux.cfg. So search isolinux dir with recursive and parse isolinux.cfg file, It will be we can get the kernel and initrd filename from it(right?)

I'll more research and thinking logic...

hferentschik commented 7 years ago

But this topic is my knowledge are not enough... I'll research it. Please give me some time for detailed reply.

+1

But how can we find the boot loader configuration?

I think you would need to look for some known config files, for example the isolinux, grub. The advantage is that if you find a known bootloader config you know the right kernel, initd locations. I think this approach is potentially better. Another advantage could be that you are able to extract the kernel boot parameter this way as well and make them a bit more dynamic. The boot parameters are another thing we are having issues with the CentOS image we are trying to work with.

praveenkumar commented 7 years ago

@zchee @hferentschik @LalatenduMohanty I invested quality time to understand if we go with isolinux.cfg or grub.cfg parsing path, can we able to handle most (all of) linux distro, with my experiment we can't because some distro like Arch even don't have grub.cfg and isolinux.cfg doesn't contain kernel + ramdisk path. Even Ubuntu doesn't have any details about those files in isolinux.cfg. So I will prefer either have it in option as we already have in PR or have a list which consist known location and if a distribution doesn't fall under that then either user add required details as PR or use bootLocation option. I did created a gist about what all distro I tried to check https://gist.github.com/praveenkumar/19f23d7c3fd8dcd744b2b54534d13a56

Another problem we have about kernel option which we can overcome with using available option as of now.

WDYT?

Edit: Another alternative which I found is to do a recursive search for kernel and Ramdisk and return absolute path of those files which can be used directly. I put some code to tryout and tested with boot2docker/centos/ubuntu/fedora and results are as expected so I think this will little bit better for us.

$ cat bootlocation.go 
package main

import (
    "flag"
    "fmt"
    "os"
    "path/filepath"
    "strings"
)

func getKernelRamdiskPath(root string) []string {
    kernelRamdiskPath := []string{}
    err := filepath.Walk(root, func(path string, f os.FileInfo, err error) error {
        if strings.Contains(path, "vmlinu") || strings.Contains(path, "initrd") {
            kernelRamdiskPath = append(kernelRamdiskPath, path)
        }
        return nil
    })
    if err != nil {
        fmt.Println("Something Wrong")
    }
    return kernelRamdiskPath
}

func main() {
    flag.Parse()
    root := flag.Arg(0)
    kernelRamdiskPath := getKernelRamdiskPath(root)
    fmt.Println(kernelRamdiskPath)
}

14:37 $ ./bootlocation /Volumes/Ubuntu\ 16.04.1\ L/
[/Volumes/Ubuntu 16.04.1 L/casper/initrd.lz /Volumes/Ubuntu 16.04.1 L/casper/vmlinuz.efi]

14:38 $ ./bootlocation /Volumes/b2d-v1.12.2/
[/Volumes/b2d-v1.12.2/boot/initrd.img /Volumes/b2d-v1.12.2/boot/vmlinuz64]

14:39 $ ./bootlocation /Volumes/live-centos/
[/Volumes/live-centos/isolinux/initrd0.img /Volumes/live-centos/isolinux/vmlinuz0]
LalatenduMohanty commented 7 years ago

@praveenkumar It is interesting that Ubuntu and Arch do not use ISOLinux. I am curious to know which boot loader thy use. But I agree that if distributions are using such a verity of locations then we should go with the parameter option which will be simple as compared to parsing config files and figuring out the path.

hferentschik commented 7 years ago

Talking more about providing additional (power user) options, I think it makes sense to actualy expose flags for the actual things which are relevant for the driver. If one has --xhyve-boot-location, we internally still need to have some heuristics to determine the potential kernel and initrd name.

So if options, I think it should be more along the lines:

AFAIU, this three options should pretty much cover all/most of the potential dynamic information we the driver needs.

That said, I think it would be nice if the current implementation could take care out of the box for the cases we already know about now - Boot2Docker, CoreOS, CentOS. This avoids users to deal with these types of options as much as possible. If then somehow some other VM comes along which does not match the implemented heuristics, then the above mentioned options can be used to start with.

praveenkumar commented 7 years ago

@hferentschik @zchee I made required changes as per discussion, can you please try this patch and let me know if now it feasible?

I am not sure why CI failed this PR as per logs nothing shown.

zchee commented 7 years ago

@praveenkumar @hferentschik @LalatenduMohanty Thanks reply :) Sorry, I'm going to do the conference talk on vimconf2016 at tomorrow. So I must write a slide... lol I will more reply that by tomorrow or day after tomorrow 🙇

I am not sure why CI failed this PR as per logs nothing shown.

np, It seems to TravisCI causes. I have restarted the job, passes the test. But there are some points which concern me. I'll review later.

hferentschik commented 7 years ago

I'm going to do the conference talk on vimconf2016 at tomorrow.

Nice one. So you are a hard core vim user ;-)

zchee commented 7 years ago

@hferentschik LOL I'll talk about deoplete-go(jedi|clang) and nvim-go :)


@praveenkumar Sorry, I had planned to code review but found a bug on https://github.com/zchee/docker-machine-driver-xhyve/pull/143. That's related to defer func() and (err error) logic.

defer func() {
    log.Debugf("Unmounting %s", isoFilename)
    err = hdiutil("detach", volumeRootDir)
}()

It always err to nil, so can't handle error on extractKernelImages function. /cc @dlorenc

For now, Instead of review comment, I made a fix-147 branch. It's just for reference. Please check it. I'll delete this branch after merged your PR. Note that very rough coding and maybe have another bug because I have no time today...(write slide lol)

Now xhyve/xhyve.go#L630 is comment out. So, should fail. But If patched below diff to fix-147 branch, can't display error message and not failed.

diff --git a/xhyve/xhyve.go b/xhyve/xhyve.go
index 8c44d14..429fb57 100644
--- a/xhyve/xhyve.go
+++ b/xhyve/xhyve.go
@@ -610,18 +610,18 @@ func (d *Driver) publicSSHKeyPath() string {
    return d.GetSSHKeyPath() + ".pub"
 }

-func (d *Driver) extractKernelImages() error {
+func (d *Driver) extractKernelImages() (err error) {
    log.Debugf("Mounting %s", isoFilename)

    volumeRootDir := d.ResolveStorePath(isoMountPath)
-   err := hdiutil("attach", d.ResolveStorePath(isoFilename), "-mountpoint", volumeRootDir)
+   err = hdiutil("attach", d.ResolveStorePath(isoFilename), "-mountpoint", volumeRootDir)
    if err != nil {
        return err
    }

-   defer func() error {
+   defer func() {
        log.Debugf("Unmounting %s", isoFilename)
-       return hdiutil("detach", volumeRootDir)
+       err = hdiutil("detach", volumeRootDir)
    }()

    if d.BootKernel == "" && d.BootInitrd == "" {

And, I wanted to say for your PR:

Thanks! I'll more comment tomorrow :)

praveenkumar commented 7 years ago

But If patched below diff to fix-147 branch, can't display error message and not failed.

@zchee I updated the patch and also applied diff to test and it did work without any issue. I did test with boot2docker, centos and minicube iso. After this patch merge then defer func() patch also go in otherwise it will break.

zchee commented 7 years ago

@praveenkumar Thanks! I'll merge after test and check :)

zchee commented 7 years ago

Merged. Thanks!

@hferentschik

So if options, I think it should be more along the lines:

  • --xhyve-boot-kernel - Full path to kernel
  • --xhyve-boot-initrd - Full path to initrd
  • --xhyve-boot-options - Kernel boot options

--xhyve-boot-options is already supported --xhyve-boot-cmd, Did I solve your concern?

coodoo commented 7 years ago

Out of curiosity, with this new release, could it run ubuntu or debian iso instead of boot2docker? Any suggested commands? Thanks.

praveenkumar commented 7 years ago

@coodoo it can run boot2docker type of iso which can be based on ubuntu debian etc. boot2docker type iso means that distro specific iso have docker installed and when system boots it should be run as init process and also when we use docker-machine it also required around 20GB partition mounted which keeps docker specific data on each reboot. So if you want to run plain ubuntu/debain iso even after this patch will not going to work and fail during provision step from docker-machine.

coodoo commented 7 years ago

Thanks for the details, have you noticed any boot2docker-style iso based on ubuntu/debian? The reason behind is I really miss using apt-getto provision the vm, ie: install vim, git, dotfiles and stuff, how do people normally deal with this with plain boot2docker iso?