Firecracker: virtio mmio support #1045

mcastelino · 2018-12-17T23:50:39Z

Add support for virtio-mmio block devices. Firecracker only support virtio-mmio.

Note: Add a firecracker specific workaround to handle the fact that rootfs of the VM itself is also sent in as block device.

mcastelino · 2018-12-17T23:50:52Z

/cc @egernst @sboeuf

mcastelino · 2018-12-17T23:52:32Z

virtcontainers/kata_agent.go

@@ -831,7 +832,10 @@ func (k *kataAgent) appendDevices(deviceList []*grpc.Device, c *Container) []*gr
 			ContainerPath: dev.ContainerPath,
 		}

-		if d.SCSIAddr == "" {
+		if d.VirtPath != "" {


Open for suggestions here on how to figure out the actual type. Today we have no top level type information.

Well I agree the if/else here looks flaky.
I think one approach would be to add a field Type to the BlockDrive structure. This way, we could factorize both VirtPath, SCSIAddr and PCIAddr into one unique field Dest (or similar) to describe how to identify the device from the guest side. The Type field would be used to know how to actually decipher the Dest field.

This has been fixed by deriving the information directly.

egernst · 2018-12-18T00:06:30Z

/test

sboeuf

A few comments but globally LGTM

virtcontainers/device/drivers/block.go

virtcontainers/hyperstart_agent.go

sboeuf · 2018-12-18T01:11:17Z

virtcontainers/kata_agent.go

@@ -831,7 +832,10 @@ func (k *kataAgent) appendDevices(deviceList []*grpc.Device, c *Container) []*gr
 			ContainerPath: dev.ContainerPath,
 		}

-		if d.SCSIAddr == "" {
+		if d.VirtPath != "" {


Well I agree the if/else here looks flaky.
I think one approach would be to add a field Type to the BlockDrive structure. This way, we could factorize both VirtPath, SCSIAddr and PCIAddr into one unique field Dest (or similar) to describe how to identify the device from the guest side. The Type field would be used to know how to actually decipher the Dest field.

virtcontainers/device/drivers/block.go

sboeuf · 2018-12-18T01:24:48Z

/test

egernst · 2018-12-18T16:56:09Z

/retest

virtcontainers/device/drivers/block.go

pkg/katautils/config.go

virtcontainers/qemu_arch_base.go

egernst · 2018-12-19T16:40:49Z

@sboeuf identified issue here. PTAL @mcastelino : mcastelino@30d2279

egernst

see above comment, and fix @ mcastelino@30d2279

mcastelino · 2018-12-19T22:56:19Z

/test

mcastelino · 2018-12-19T23:21:27Z

@egernst @sboeuf @jodh-intel all the review comments should have been addressed.

Can you you review/merge.

sboeuf · 2018-12-19T23:23:34Z

@mcastelino looks fine, but you need to fix the CI checks.

mcastelino · 2018-12-19T23:23:41Z

@egernst @ mcastelino/runtime-1@30d2279 is not needed as we can always use virtPath. Now the flow is uniform across volumes and block devices as it should have been.

virtcontainers/hypervisor.go

This change is no longer needed as we can use virtPath vs mmioaddr

mcastelino · 2018-12-19T23:37:28Z

/test

sboeuf · 2018-12-20T07:58:32Z

/test

bergwolf · 2018-12-20T08:41:39Z

virtcontainers/device/drivers/block.go

+			globalIdx = index + 1
+		}
+
+		driveName, err := utils.GetVirtDriveName(globalIdx)


@amshinde I'm wondering if we still rely on GetVirtDriveName for virtio-blk device identification? I thought we now use pci address for them. Then at least GetVirtDriveName is not needed for virtio-blk. We can still keep it for virtio-mmio. In future we might want to replace it with disk UUID though.

Agree with @bergwolf

Overall it LGTM

jodh-intel · 2018-12-20T08:52:55Z

Thanks @mcastelino !

lgtm

jodh-intel · 2018-12-20T08:54:30Z

/retest

sboeuf · 2018-12-20T09:32:12Z

Just so we don't try to test and retest, here are some consistent failures due to this patch:

Summarizing 3 Failures:

[Fail] run hot plug block devices [It] should be attached 
/tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/tests/integration/docker/run_test.go:135

[Fail] docker volume passing a block device [It] should be mounted 
/tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/tests/integration/docker/volume_test.go:135

[Fail] docker volume remove bind-mount source before container exits [It] should exit cleanly without leaking process 
/tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/tests/integration/docker/volume_test.go:160

Ran 222 of 234 Specs in 1309.588 seconds
FAIL! -- 219 Passed | 3 Failed | 0 Pending | 12 Skipped --- FAIL: TestIntegration (1336.17s)

I'm trying to figure out what's wrong here.

sboeuf · 2018-12-20T11:17:46Z

@mcastelino after more investigations, I found that the failing tests were related to some of our Docker integration tests testing --device option. And the generated error was the same I was seeing when I could not pass an extra block device into my VM!
So please include into this PR the small fix I have submitted here: mcastelino#5, this should fix those issues.

mcastelino · 2018-12-20T17:23:00Z

@mcastelino after more investigations, I found that the failing tests were related to some of our Docker integration tests testing --device option. And the generated error was the same I was seeing when I could not pass an extra block device into my VM!
So please include into this PR the small fix I have submitted here: mcastelino#5, this should fix those issues.

@sboeuf I have pulled in the commit into this PR

sboeuf · 2018-12-20T18:14:58Z

/test

sboeuf · 2018-12-20T19:58:00Z

/test

sboeuf · 2018-12-20T21:59:52Z

/test

Start adding support for virtio-mmio devices starting with block. The devices show within the vm as vda, vdb,... based on order of insertion and such within the VM resemble virtio-blk devices. They need to be explicitly differentiated to ensure that the agent logic within the VM can discover and mount them appropropriately. The agent uses PCI location to discover them for virtio-blk. For virtio-mmio we need to use the predicted device name for now. Note: Kata used a disk for the VM rootfs in the case of Firecracker. (Instead of initrd or virtual-nvdimm). The Kata code today does not handle this case properly. For now as Firecracker is the only Hypervisor in Kata that uses virtio-mmio directly offset the drive index to comprehend this. Longer term we should track if the rootfs is setup as a block device explicitly. Fixes: kata-containers#1046 Signed-off-by: Sebastien Boeuf <[email protected]> Signed-off-by: Manohar Castelino <[email protected]>

sboeuf · 2018-12-20T23:12:21Z

/test

WeiZhang555 · 2018-12-21T01:53:38Z

Except @bergwolf 's comment, other parts
LGTM

mcastelino commented Dec 17, 2018

View reviewed changes

mcastelino mentioned this pull request Dec 17, 2018

Add support for firecracker in addition to QEMU as a supported hypervisor in Kata. #1031

Closed

11 tasks

mcastelino mentioned this pull request Dec 18, 2018

virtio-mmio: Add support for virtio-mmio blk devices kata-containers/agent#437

Merged

mcastelino force-pushed the topic/firecracker-virtio-mmio branch from c68642a to 9684ef2 Compare December 18, 2018 00:55

sboeuf approved these changes Dec 18, 2018

View reviewed changes

jodh-intel reviewed Dec 19, 2018

View reviewed changes

virtcontainers/device/drivers/block.go Outdated Show resolved Hide resolved

virtcontainers/device/drivers/block.go Outdated Show resolved Hide resolved

pkg/katautils/config.go Outdated Show resolved Hide resolved

virtcontainers/qemu_arch_base.go Outdated Show resolved Hide resolved

egernst previously requested changes Dec 19, 2018

View reviewed changes

mcastelino force-pushed the topic/firecracker-virtio-mmio branch 2 times, most recently from a856de3 to e340395 Compare December 19, 2018 22:49

egernst reviewed Dec 19, 2018

View reviewed changes

virtcontainers/hypervisor.go Show resolved Hide resolved

mcastelino force-pushed the topic/firecracker-virtio-mmio branch from e340395 to 027340f Compare December 19, 2018 23:33

egernst approved these changes Dec 20, 2018

View reviewed changes

mcastelino mentioned this pull request Dec 20, 2018

Firecracker VMM support #1044

Merged

bergwolf approved these changes Dec 20, 2018

View reviewed changes

mcastelino force-pushed the topic/firecracker-virtio-mmio branch from 027340f to 676b820 Compare December 20, 2018 17:13

mcastelino force-pushed the topic/firecracker-virtio-mmio branch from 410b69c to e889bb7 Compare December 20, 2018 19:47

mcastelino force-pushed the topic/firecracker-virtio-mmio branch from e889bb7 to 84a3ee9 Compare December 20, 2018 21:55

mcastelino force-pushed the topic/firecracker-virtio-mmio branch from a7b77a8 to 0d84d79 Compare December 20, 2018 23:10

sboeuf merged commit e14071f into kata-containers:master Dec 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firecracker: virtio mmio support #1045

Firecracker: virtio mmio support #1045

mcastelino commented Dec 17, 2018

mcastelino commented Dec 17, 2018

mcastelino Dec 17, 2018

sboeuf Dec 18, 2018

mcastelino Dec 20, 2018

egernst commented Dec 18, 2018

sboeuf left a comment

sboeuf Dec 18, 2018

sboeuf commented Dec 18, 2018

egernst commented Dec 18, 2018

egernst commented Dec 19, 2018

egernst left a comment

mcastelino commented Dec 19, 2018

mcastelino commented Dec 19, 2018

sboeuf commented Dec 19, 2018

mcastelino commented Dec 19, 2018

mcastelino commented Dec 19, 2018

sboeuf commented Dec 20, 2018

bergwolf Dec 20, 2018

WeiZhang555 Dec 21, 2018

jodh-intel commented Dec 20, 2018 •

edited by amshinde

Loading

jodh-intel commented Dec 20, 2018

sboeuf commented Dec 20, 2018

sboeuf commented Dec 20, 2018

mcastelino commented Dec 20, 2018

sboeuf commented Dec 20, 2018

sboeuf commented Dec 20, 2018

sboeuf commented Dec 20, 2018

sboeuf commented Dec 20, 2018

WeiZhang555 commented Dec 21, 2018 •

edited by amshinde

Loading

Firecracker: virtio mmio support #1045

Firecracker: virtio mmio support #1045

Conversation

mcastelino commented Dec 17, 2018

mcastelino commented Dec 17, 2018

mcastelino Dec 17, 2018

Choose a reason for hiding this comment

sboeuf Dec 18, 2018

Choose a reason for hiding this comment

mcastelino Dec 20, 2018

Choose a reason for hiding this comment

egernst commented Dec 18, 2018

sboeuf left a comment

Choose a reason for hiding this comment

sboeuf Dec 18, 2018

Choose a reason for hiding this comment

sboeuf commented Dec 18, 2018

egernst commented Dec 18, 2018

egernst commented Dec 19, 2018

egernst left a comment

Choose a reason for hiding this comment

mcastelino commented Dec 19, 2018

mcastelino commented Dec 19, 2018

sboeuf commented Dec 19, 2018

mcastelino commented Dec 19, 2018

mcastelino commented Dec 19, 2018

sboeuf commented Dec 20, 2018

bergwolf Dec 20, 2018

Choose a reason for hiding this comment

WeiZhang555 Dec 21, 2018

Choose a reason for hiding this comment

jodh-intel commented Dec 20, 2018 • edited by amshinde Loading

jodh-intel commented Dec 20, 2018

sboeuf commented Dec 20, 2018

sboeuf commented Dec 20, 2018

mcastelino commented Dec 20, 2018

sboeuf commented Dec 20, 2018

sboeuf commented Dec 20, 2018

sboeuf commented Dec 20, 2018

sboeuf commented Dec 20, 2018

WeiZhang555 commented Dec 21, 2018 • edited by amshinde Loading

jodh-intel commented Dec 20, 2018 •

edited by amshinde

Loading

WeiZhang555 commented Dec 21, 2018 •

edited by amshinde

Loading