search

firecracker-microvm / firectl

firectl is a command-line tool to run Firecracker microVMs
Apache License 2.0
472 stars 73 forks source link

firectl: add metadata after VM has been started #42

Closed nickvanw closed 4 years ago

nickvanw commented 5 years ago

This is a small change that has firectl add the VM Metadata after the VM has been started. Using the latest version from master, I was reliably able to reproduce:

ERRO[0000] Setting metadata: Put http://localhost/mmds: dial unix /home/nick/.firecracker.sock-11716-81: connect: no such file or directory

with a basic

--metadata='{"latest": { "meta-data": { "test": "12345" } } }'

I believe this ordering is necessary, as there is no running/listening firecracker socket to add the metadata to before this step, causing firectl to fail 100% of the time.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

nmeyerhans commented 4 years ago

Thanks for this. I've confirmed that metadata configuration is fixed by this change. We clearly need a proper test for the metadata configuration.

It's trivial, but could I get you to add a DCO sign-off to your commit message? We're trying to ensure that all commits have these in the future, and our buildkite integration is enforcing that (which is why you see a check failure)

nickvanw commented 4 years ago

Hey @nmeyerhans,

Done and done! Let me know if I can contribute anything else here - I'm not deeply familiar with the testing structure in firectl, but I'm happy to take a stab at adding tests if you think it's a prerequisite for this PR. Otherwise, it should be good to go 😄

samuelkarp commented 4 years ago

I don't think we need to block this PR on adding a test, but it would be a good follow-up. Our testing story for firectl is pretty incomplete at this point; the TestFireCTL function validates that we can create a VM, but doesn't validate any of the options right now.

nmeyerhans commented 4 years ago

Awesome, thanks @nickvanw. A test isn't a prereq for getting this merged, so let's get it in.

@samuelkarp pointed out that this change technically introduces a race condition in that there's nothing protecting against a scenario in which the VM finishes booting and tries querying the MMDS services before we've posted the content. I think we'll want to make a change to the Firecracker Go SDK to allow specification of MMDS content when the Machine object is instantiated. But we can get to that later, as well.