Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

[WIP] feat: expose mmds #304

Closed
wants to merge 1 commit into from
Closed

Conversation

alexeldeib
Copy link
Contributor

@alexeldeib alexeldeib commented Aug 8, 2019

ref #77

I took an initial stab at exposing MMDS. I'm not super familiar with vsock -- does this approach make sense? is a constant okay for CID/path? I can pull those into the constants file.

One issue with MMDS is it requires a firecracker binary compiled with vsock support. This is fairly easy on a local machine but I am still struggling to build it in the docker image. So definitely don't merge this 😉

Additionally, since the metadata must be injected to firecracker VM from inside the container, I've opted to pass the metadata as Environment variable at Boot time (should hook in properly to start as well as run).


if enableMmds {
//cfg.KernelArgs = fmt.Sprintf("%s -device vhost-vsock-pci,id=vhost-vsock-pci0,guest-cid=3")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup

@@ -22,12 +22,15 @@ import (
// ExecuteFirecracker executes the firecracker process using the Go SDK
func ExecuteFirecracker(vm *api.VM, dhcpIfaces []DHCPInterface) error {
drivePath := vm.SnapshotDev()
mmdsData := os.Getenv("FC_META")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup; take from vm spec

@alexeldeib
Copy link
Contributor Author

alexeldeib commented Aug 8, 2019

Hm! I’ve already learned a few things, notably that CID should be unique per VM. Not sure how to track that though, maybe some sort of global cache? Or check existence first somehow?

@luxas
Copy link
Contributor

luxas commented Aug 9, 2019

Thanks for this contribution @alexeldeib ✨💯🎉!
I will take a look at this PR on Monday and give feedback :). I don't think this will make v0.5.0 that we're in the process pf pushing out at the moment, but we hope to release v0.6.0 shortly afterwards :)

@alexeldeib
Copy link
Contributor Author

alexeldeib commented Aug 9, 2019

(notes to self as I work through this)

I think I should also use kernel args -device vhost-vsock-pci,id=vhost-vsock-pci0,guest-cid=$CID instead of mounting /dev/vsock like i've done? e: maybe I should actually leave the bind devices, as they need to be passed through to docker while the kernel args need to get to FC. But seems like it works..

TODO

@luxas luxas added this to the v0.6.0 milestone Aug 12, 2019
@luxas luxas added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 12, 2019
@alexeldeib
Copy link
Contributor Author

(had some issues with my dev environment, will work on finishing this up soon)

Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
Left some comments here, but overall your previous comments sums up the remaining items pretty nicely.
What we need to decide how to do is how to deliver the firecracker binary (source compiled with mmds built in) to the user.

@@ -70,6 +70,7 @@ func addCreateFlags(fs *pflag.FlagSet, cf *run.CreateFlags) {
// Register flags for simple types (int, string, etc.)
fs.Uint64Var(&cf.VM.Spec.CPUs, "cpus", cf.VM.Spec.CPUs, "VM vCPU count, 1 or even numbers between 1 and 32")
fs.StringVar(&cf.VM.Spec.Kernel.CmdLine, "kernel-args", cf.VM.Spec.Kernel.CmdLine, "Set the command line for the kernel")
fs.StringVar(&cf.VM.Spec.Metadata, "metadata", cf.VM.Spec.Metadata, "Enable MMDS and use this JSON value for creation metadata")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd also want to point this to a file; and use that content instead of requiring the JSON to be inline.

@@ -185,7 +185,8 @@ type VMSpec struct {
// Specifying a path in SSH.Generate means "use this public key"
// If SSH.PublicKey is set, this struct will marshal as a string using that path
// If SSH.Generate is set, this struct will marshal as a bool => true
SSH *SSH `json:"ssh,omitempty"`
SSH *SSH `json:"ssh,omitempty"`
Metadata string `json:"metadata,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would []byte be a better format?
also add this field to v1alpha2 please

c, err := dc.client.ContainerCreate(context.Background(), &container.Config{
Hostname: config.Hostname,
Tty: true, // --tty
OpenStdin: true, // --interactive
Cmd: config.Cmd,
Env: config.Env,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup later?

@@ -74,6 +75,8 @@ func StartVM(vm *api.VM, debug bool) error {
runtime.BindBoth("/dev/net/tun"), // Needed for creating TAP adapters
runtime.BindBoth("/dev/kvm"), // Pass through virtualization support
runtime.BindBoth(vm.SnapshotDev()), // The block device to boot from
runtime.BindBoth("/dev/vhost-vsock"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only if metadata is present?

@luxas luxas modified the milestones: v0.6.0, v0.7.0 Aug 30, 2019
@luxas
Copy link
Contributor

luxas commented Sep 9, 2019

Firecracker v0.18.0 released with "real" vsock support :) https://github.com/firecracker-microvm/firecracker/blob/master/docs/vsock.md

@alexeldeib
Copy link
Contributor Author

🙁 sorry for leaving this dangling.

I haven't had the time to properly invest (or i've been diverting it elsehwere, unfortunately). I'm going to close this. If I am able to brush up on vsock and implement the real thing, I'll open it in a new PR.

Still appreciate the project and enjoy playing with it 🙂

@alexeldeib alexeldeib closed this Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants