-
Notifications
You must be signed in to change notification settings - Fork 374
Conversation
42d5faa
to
a4f0086
Compare
Thanks for revendoring @mcastelino ! |
11c961d
to
41b3546
Compare
/test |
In order to let the user choose firecracker hypervisor instead of QEMU (from the configuration.toml), let's add it to the list of supported hypervisors. Fixes kata-containers#1042 Depends-on: github.com/kata-containers#1044 Signed-off-by: Eric Ernst <[email protected]>
In order to let the user choose firecracker hypervisor instead of QEMU (from the configuration.toml), let's add it to the list of supported hypervisors. Fixes kata-containers#1042 Depends-on: github.com/kata-containers#1044 Signed-off-by: Eric Ernst <[email protected]>
In order to let the user choose firecracker hypervisor instead of QEMU (from the configuration.toml), let's add it to the list of supported hypervisors. Fixes kata-containers#1042 Depends-on: github.com/kata-containers#1044 Signed-off-by: Eric Ernst <[email protected]>
In order to let the user choose firecracker hypervisor instead of QEMU (from the configuration.toml), let's add it to the list of supported hypervisors. Fixes kata-containers#1042 Depends-on: github.com/kata-containers#1044 Signed-off-by: Eric Ernst <[email protected]>
Vendor in all firecracker dependencies. This allows virtcontainers to pull call the firecracker REST API. Signed-off-by: Manohar Castelino <[email protected]>
41b3546
to
eedd0e5
Compare
5c9e7a1
to
a290b0c
Compare
@bergwolf @sboeuf @jodh-intel @egernst all the code review comments should have been addressed. When this PR merges will have support for docker+kata+firecracker. The merge order needs to be This also requires the latest agent kata-containers/agent@8e48125 in the kata image which includes It also needs the kernel with virtio-mmio support which was included in The configuration toml needs to have the following set
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. Just have some minor comments.
//The drive placeholder has to exist prior to Update | ||
return nil, fc.fcUpdateBlockDrive(*devInfo.(*config.BlockDrive)) | ||
default: | ||
fc.Logger().WithFields(logrus.Fields{"devInfo": devInfo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means endpoint hotplug is not supported. Do we need to return failure here otherwise kata-runtime network add
and kata-netmon
might be confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Added this. Will test and see if results in any top level failures.
} | ||
|
||
func (fc *firecracker) disconnect() { | ||
fc.state.set(notReady) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is fc state changed here? I thought the state should reflect the real state of the FC guest. And the disconnect()
call only means to close the current runtime <-> hypervisor control channel connection, which is fc.fcClient
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a logical disconnect. We may still need the client connection for cleanup. We can change this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mcastelino - this is looking really good - just a few nits + @bergwolf's questions (which I agree with) to deal with I think.
virtcontainers/fc.go
Outdated
// firecracker guest VM. | ||
// We attach a pool of placeholder drives before the guest has started, and then | ||
// patch the replace placeholder drives with drives with actual contents. | ||
const fcDiskPoolSize = 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could be moved up to the const
's above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a limitation in firecrackers? why 8 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. fixed
|
||
// Use the global block index as an index into the pool of the devices | ||
// created for firecracker. | ||
driveID := "drive-" + strconv.Itoa(drive.Index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: A small amount of duplication with createDiskPool()
. We could create something like the following:
func (fc *firecracker) driveParams(index int) {
driveID := fmt.Sprintf("drive-" + strconv.Itoa(index)
driveParams := ops.NewPutGuestDriveByIDParams()
driveParams.SetDriveID(driveID)
:
}
... but it looks like we'd need to handle the models.Drive{}
vs. models.PartialDrive{}
logic for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind if I close this later. All other review comments have been merged.
0a2aec1
to
91a0518
Compare
In order to let the user choose firecracker hypervisor instead of QEMU (from the configuration.toml), let's add it to the list of supported hypervisors. Fixes kata-containers#1042 Depends-on: github.com/kata-containers#1044 Signed-off-by: Eric Ernst <[email protected]>
@sboeuf @egernst Most review comments addressed. A test branch with all PRs incorporated is here https://github.com/mcastelino/runtime-1/tree/topic/fc-final-hopefully is anyone wants to try it out. With this we have full docker support with proper support for termination. |
91a0518
to
ca4c191
Compare
Initial Support for the firecracker VMM Note: - 9p is unsupported by firecracker - Enable pseudo hotplug block device hotplug capability Initially, this will be a pseudo capability for Firecracker hypervisor, but we will utilize a pool of block devices and block device rescan as a temporary workaround. Fixes: kata-containers#1064 Signed-off-by: Eric Ernst <[email protected]> Signed-off-by: Samuel Ortiz <[email protected]> Signed-off-by: Archana Shinde <[email protected]> Signed-off-by: Sebastien Boeuf <[email protected]> Signed-off-by: Manohar Castelino <[email protected]>
Add firecracker as a supported hypervisor. This connects the newly defined firecracker implementation as a supported hypervisor. Move operation definition to the common hypervisor code. Signed-off-by: Manohar Castelino <[email protected]>
Unlike QEMU firecracker cannot accept a fd as part of the REST API. Close the vsock vhostfd close to the point where we launch the VM. Note: This is still racy. Signed-off-by: Manohar Castelino <[email protected]>
Use the firecracker rescan logic to update the pre-attached drive. This allows us to emulate hotplug. Initially the drive backing stores are set to empty files on the host. Once the actual block based device or file is available swap the backing store. The rescan needs to be issued iff the VM is running. Signed-off-by: Manohar Castelino <[email protected]>
Because firecracker currently does not support a proper stop from the caller, and because we don't want the agent to initiate a reboot to shutdown the VM, the simplest and most efficient solution at the moement is to signal the VM process with SIGTERM first, followed by a SIGKILL if the process is still around. Signed-off-by: Sebastien Boeuf <[email protected]>
ca4c191
to
b4c3a2f
Compare
/test |
1 similar comment
/test |
All green except metrics that is pending... Let's merge it as this introduces only new code related to firecracker and should not impact metrics anyway. |
In order to let the user choose firecracker hypervisor instead of QEMU (from the configuration.toml), let's add it to the list of supported hypervisors. Fixes kata-containers#1042 Depends-on: github.com/kata-containers#1044 Signed-off-by: Eric Ernst <[email protected]>
In order to let the user choose firecracker hypervisor instead of QEMU (from the configuration.toml), let's add it to the list of supported hypervisors. Fixes kata-containers#1042 Depends-on: github.com/kata-containers#1044 Signed-off-by: Eric Ernst <[email protected]>
Initial Firecracker VMM support. This vendors in the firecracker go sdk. It also brings in the initial code to integrate firecracker with Kata.