support to boot guest with an initrd image #98

bergwolf · 2018-03-22T15:13:39Z

If an initrd image is configured in HypervisorConfig or passed in by annotations, append it to qemu command line arguments so that we boot the guest OS with the specified initrd image rather than a rootfs disk image.

A sample qemu command line:

-name pod-test9 -uuid 9fc4a0e0-52a3-4e1c-b562-14b5c197dd2b -machine pc,accel=kvm,kernel_irqchip,nvdimm -cpu host,pmu=off -qmp unix:/run/virtcontainers/pods/test9/mon-9fc4a0e0-52a3-4e1c-b562-14b5c197dd2b,server,nowait -qmp unix:/run/virtcontainers/pods/test9/ctl-9fc4a0e0-52a3-4e1c-b562-14b5c197dd2b,server,nowait -m 2048M,slots=2,maxmem=4720M -device virtio-serial-pci,disable-modern=true,id=serial0 -device virtconsole,chardev=charconsole0,id=console0 -chardev socket,id=charconsole0,path=/run/virtcontainers/pods/test9/console.sock,server,nowait -device virtio-scsi-pci,id=scsi0,disable-modern=true -device virtserialport,chardev=charch0,id=channel0,name=agent.channel.0 -chardev socket,id=charch0,path=/run/virtcontainers/pods/test9/kata.sock,server,nowait -device virtio-9p-pci,disable-modern=true,fsdev=extra-9p-kataShared,mount_tag=kataShared -fsdev local,id=extra-9p-kataShared,path=/run/kata-containers/shared/pods/test9,security_model=none -rtc base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=discard -vga none -no-user-config -nodefaults -nographic -daemonize -kernel /golang/src/github.com/hyperhq/hyperstart/build/arch/x86_64/kernel -initrd /golang/src/github.com/kata-containers/osbuilder/kata-initrd.img -append tsc=reliable no_timer_check rcupdate.rcu_expedited=1 i8042.direct=1 i8042.dumbkbd=1 i8042.nopnp=1 i8042.noaux=1 noreplace-smp reboot=k console=hvc0 console=hvc1 iommu=off cryptomgr.notests net.ifnames=0 pci=lastbus=0 debug panic=1 initcall_debug ip=::::::test9::off:: agent.log=debug -smp 1,cores=1,threads=1,sockets=1,maxcpus=240

sboeuf · 2018-03-22T15:44:07Z

cli/config.go

 func (h hypervisor) image() (string, error) {
 	p := h.Image

 	if p == "" {
-		p = defaultImagePath


You're modifying the default behavior here. I am not against it, but it is worth being mentioned from the commit message I think.

The default value has to be removed because otherwise we always have ImagePath and InitrdPath set.

Yes that's a good point, there is no reason to maintain default anymore I guess.

Yup, I've added description of the removal in the commit message[1]. Thanks!

[1] a67e506

jodh-intel · 2018-03-22T16:35:37Z

cli/config/configuration.toml.in

@@ -8,6 +8,7 @@
 [hypervisor.qemu]
 path = "@QEMUPATH@"
 kernel = "@KERNELPATH@"
+initrd = "@INITRDPATH@"


Could you update kata-env.go to add a [Initrd] entry for parity with [Kernel] and [Image] (and bump formatVersion to 1.0.10)?

devimc · 2018-03-22T17:33:26Z

fixing @jodh-intel and @sboeuf 's comments, lgtm

sboeuf

A few comments but globally looks good.

sboeuf · 2018-03-22T18:26:33Z

virtcontainers/qemu_amd64.go

@@ -83,12 +86,12 @@ func maxQemuVCPUs() uint32 {
 	return uint32(240)
 }

-func newQemuArch(machineType string) qemuArch {
+func newQemuArch(machineType string, rootImage bool) qemuArch {


Could you modify the function so that we directly pass the hypervisor configuration instead. This will prevent from adding some specific parameters like rootImage bool and the check will be performed from each implementation if needed:

func newQemuArch(config HypervisorConfig) qemuArch { ... if config.ImagePath != "" { ... }

Good point.

sboeuf · 2018-03-22T18:30:09Z

cli/create.go

-			Key:   "systemd.mask",
-			Value: "systemd-networkd.socket",
-		},
+var systemdParam = []vc.Param{


s/systemdParam/systemdKernelParams/

Good catch!

sboeuf · 2018-03-22T18:35:26Z

cli/create.go

 }

 // setKernelParams adds the user-specified kernel parameters (from the
 // configuration file) to the defaults so that the former take priority.
 func setKernelParams(containerID string, runtimeConfig *oci.RuntimeConfig) error {
-	defaultKernelParams := getKernelParamsFunc(containerID)
+	defaultKernelParams := getKernelParamsFunc(containerID, runtimeConfig.HypervisorConfig.ImagePath != "")


Could you please get this check done outside of the function call, for readability purpose.
We should define a function:

func isInitRamFs(config vc.HypervisorConfig) bool { if config.ImagePath == "" && config.InitrdPath != "" { return true } return false }

Will do. Thanks!

bergwolf · 2018-03-23T01:35:23Z

@jodh-intel @sboeuf change applied. PTAL.

sboeuf · 2018-03-23T01:44:00Z

LGTM
@bergwolf thanks for the changes !

bergwolf · 2018-03-23T04:12:59Z

Travis green. It seem jenkins is broken again.

jodh-intel · 2018-03-23T08:39:52Z

Thanks @bergwolf.

lgtm

P.S. I want that qemu command-line printed on a T-shirt! 😄

jodh-intel · 2018-03-23T08:40:43Z

Once this lands, presumably, we'll want some new initrd tests to be created in https://github.com/kata-containers/tests?

bergwolf · 2018-03-23T08:50:16Z

@jodh-intel Yes, we need to add some initrd tests for it.

jodh-intel · 2018-03-27T07:55:57Z

Hi @bergwolf. The CI is failing with a real error:

docker: Error response from daemon: OCI runtime create failed: /usr/share/defaults/kata-containers/configuration.toml: file /usr/share/kata-containers/kata-containers-initrd.img does not exist: unknown.

I think we need to do the following:

Change the Makefile so that by default the config file will contain an empty initrd value (meaning "no initrd will be used"):
```
# Path to initial ramdisk / initramfs image (optional)
initrd = ""
```
Add a check in Makefile so that if the user runs make initrd=yes (or similar) the config file will contain the initrd path:
```
initrd = "/usr/share/kata-containers/kata-containers-initrd.img"
```
Update .travis.yml so that it builds with and without an initrd.

But you'll also need to generate the image for the "with" scenario of course.

To include support for qemu initrd config. Signed-off-by: Peng Tao <bergwolf@gmail.com>

If an initrd image is configured in HypervisorConfig or passed in by annotations, append it to qemu command line arguments. Fixes: kata-containers#97 Signed-off-by: Peng Tao <bergwolf@gmail.com>

Add `initrd=[path]` option to configuration.toml and use it to set the HypervisorConfig.InitrdPath option. The default value of hypervisor image option is removed since we want to allow it to be unset. For the same reason, there is no default value for hypervisor initrd option either. Signed-off-by: Peng Tao <bergwolf@gmail.com>

Show the configured hypervisor initrd setting. Signed-off-by: Peng Tao <bergwolf@gmail.com>

For initrd based boot, we do not need the root parameters. Signed-off-by: Peng Tao <bergwolf@gmail.com>

When we install agent as init process in initrd based boot, there is no systemd to be configured. Signed-off-by: Peng Tao <bergwolf@gmail.com>

When we use initrd based booting, there is no systemd to be configured. Signed-off-by: Peng Tao <bergwolf@gmail.com>

To fix CI complains: virtcontainers/qemu.go:248::warning: cyclomatic complexity 18 of function (*qemu).createPod() is high (> 15) (gocyclo) Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf · 2018-03-27T08:35:56Z

@jodh-intel initrd is just a configuration. I don't think it needs to go into Makefile to set it. The test scripts can be tweaked to test proper targets. kata-containers/tests#172 is to enable the CI tests to do such testing properly.

OTOH, do we have an equivalent way in https://github.com/kata-containers/tests that works similarly in our Jenkins setup to configure env matrix in .travis.yml? E.g., in .travis.yml, I would be able to configure CI with following change:

diff --git a/.travis.yml b/.travis.yml
index 8887def..761453e 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -13,6 +13,10 @@ go_import_path: github.com/kata-containers/runtime
 go:
   - 1.8

+env:
+  - AGENT_INIT=yes TEST_INITRD=yes OSBUILDER_DISTRO=alpine
+  - AGENT_INIT=no TEST_INITRD=no
+
 before_script:
   - ".ci/static-checks.sh"

bergwolf · 2018-03-27T09:01:11Z

hmmm, it can be set in Jenkins's job config. We can do it after this one and kata-containers/tests/pull/173 are both merged.

bergwolf · 2018-03-28T11:04:08Z

Now with kata-containers/tests/pull/173, all checks passed. Merging as it is already approved.

amshinde added the review label Mar 22, 2018

bergwolf force-pushed the initrd branch from 3487064 to 2e31f27 Compare March 22, 2018 15:27

sboeuf reviewed Mar 22, 2018

View reviewed changes

jodh-intel suggested changes Mar 22, 2018

View reviewed changes

sboeuf suggested changes Mar 22, 2018

View reviewed changes

bergwolf force-pushed the initrd branch from 2e31f27 to e1baaa0 Compare March 23, 2018 01:33

sboeuf approved these changes Mar 23, 2018

View reviewed changes

bergwolf force-pushed the initrd branch 2 times, most recently from 6773997 to 98cdfc9 Compare March 23, 2018 03:38

jodh-intel approved these changes Mar 23, 2018

View reviewed changes

bergwolf added 8 commits March 27, 2018 15:58

vendor: update govmm package

991fe81

To include support for qemu initrd config. Signed-off-by: Peng Tao <bergwolf@gmail.com>

hypervisor: add initrd image support

4f57b65

If an initrd image is configured in HypervisorConfig or passed in by annotations, append it to qemu command line arguments. Fixes: kata-containers#97 Signed-off-by: Peng Tao <bergwolf@gmail.com>

cli: add initrd info to kata-env

21343d7

Show the configured hypervisor initrd setting. Signed-off-by: Peng Tao <bergwolf@gmail.com>

qemu: append kernel root parameters iff root image is provided

463e6de

For initrd based boot, we do not need the root parameters. Signed-off-by: Peng Tao <bergwolf@gmail.com>

cli: only set systemd related kernel parameters when needed

38af66c

When we install agent as init process in initrd based boot, there is no systemd to be configured. Signed-off-by: Peng Tao <bergwolf@gmail.com>

qemu: only set systemd related kernel parameters when needed

8c5fb45

When we use initrd based booting, there is no systemd to be configured. Signed-off-by: Peng Tao <bergwolf@gmail.com>

qemu: refector createPod()

423e864

To fix CI complains: virtcontainers/qemu.go:248::warning: cyclomatic complexity 18 of function (*qemu).createPod() is high (> 15) (gocyclo) Signed-off-by: Peng Tao <bergwolf@gmail.com>

bergwolf mentioned this pull request Mar 27, 2018

enable initrd image tests kata-containers/tests#172

Closed

bergwolf mentioned this pull request Mar 27, 2018

ci: enable initrd testing support kata-containers/tests#173

Merged

bergwolf force-pushed the initrd branch from 98cdfc9 to 423e864 Compare March 28, 2018 09:55

bergwolf merged commit 01f7e46 into kata-containers:master Mar 28, 2018

amshinde removed the review label Mar 28, 2018

This was referenced Mar 28, 2018

Update devguide for initrd kata-containers/documentation#42

Closed

docs: Disable initrd in dev guide kata-containers/documentation#43

Merged

egernst mentioned this pull request Mar 28, 2018

Setting default configuration with respect to initrd #128

Closed

bergwolf deleted the initrd branch April 13, 2018 08:30

jodh-intel mentioned this pull request May 23, 2018

build: Make generate configuration.toml with both initrd and image path #332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support to boot guest with an initrd image #98

support to boot guest with an initrd image #98

bergwolf commented Mar 22, 2018

sboeuf Mar 22, 2018

bergwolf Mar 23, 2018

sboeuf Mar 23, 2018

bergwolf Mar 23, 2018

jodh-intel Mar 22, 2018

bergwolf Mar 23, 2018

devimc commented Mar 22, 2018

sboeuf left a comment

sboeuf Mar 22, 2018

bergwolf Mar 23, 2018

sboeuf Mar 22, 2018

bergwolf Mar 23, 2018

sboeuf Mar 22, 2018

bergwolf Mar 23, 2018

bergwolf commented Mar 23, 2018

sboeuf commented Mar 23, 2018

bergwolf commented Mar 23, 2018

jodh-intel commented Mar 23, 2018 •

edited by amshinde

Loading

jodh-intel commented Mar 23, 2018

bergwolf commented Mar 23, 2018

jodh-intel commented Mar 27, 2018

bergwolf commented Mar 27, 2018

bergwolf commented Mar 27, 2018

bergwolf commented Mar 28, 2018

support to boot guest with an initrd image #98

support to boot guest with an initrd image #98

Conversation

bergwolf commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devimc commented Mar 22, 2018

sboeuf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergwolf commented Mar 23, 2018

sboeuf commented Mar 23, 2018

bergwolf commented Mar 23, 2018

jodh-intel commented Mar 23, 2018 • edited by amshinde Loading

jodh-intel commented Mar 23, 2018

bergwolf commented Mar 23, 2018

jodh-intel commented Mar 27, 2018

bergwolf commented Mar 27, 2018

bergwolf commented Mar 27, 2018

bergwolf commented Mar 28, 2018

jodh-intel commented Mar 23, 2018 •

edited by amshinde

Loading