-
Notifications
You must be signed in to change notification settings - Fork 72
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
Fix #146 Make /boot configurable to allow different live iso to work with driver #147
Conversation
@praveenkumar Thanks pull request! It seems to works fine on |
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. |
@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
SGTM. But is there uniformity specifications of kernel and initrd filenames and boot directory path? Edit: I know that is different by the distributions. I mean, whether there the uniformity specification. |
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.
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. |
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.
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. |
@hferentschik @praveenkumar Thanks for the polite reply :) That was very helpful for me, and I understand your said meaning. This is just a quick reply,
That's a good alternative approach.
Got it. I'll research it, but do you have a representative location list? |
@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 |
@LalatenduMohanty Thanks for the advice! also thanks again Red Hat team 😭 Yes, minikube-iso and centos iso also have isolinux.cfg. So search I'll more research and thinking logic... |
+1
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. |
@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 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.
|
@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. |
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. |
@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. |
@praveenkumar @hferentschik @LalatenduMohanty Thanks reply :)
np, It seems to TravisCI causes. I have restarted the job, passes the test. |
Nice one. So you are a hard core vim user ;-) |
Signed-off-by: Koichi Shiraishi <[email protected]>
Signed-off-by: Koichi Shiraishi <[email protected]>
Signed-off-by: Koichi Shiraishi <[email protected]>
@hferentschik LOL @praveenkumar Sorry, I had planned to code review but found a bug on #143. defer func() {
log.Debugf("Unmounting %s", isoFilename)
err = hdiutil("detach", volumeRootDir)
}() It always 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. Now xhyve/xhyve.go#L630 is comment out. So, should fail. 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 :) |
…ve iso to work with driver
@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 |
@praveenkumar Thanks! I'll merge after test and check :) |
Merged. Thanks!
|
Out of curiosity, with this new release, could it run ubuntu or debian iso instead of boot2docker? Any suggested commands? Thanks. |
@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 |
Thanks for the details, have you noticed any boot2docker-style iso based on ubuntu/debian? The reason behind is I really miss using |
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.