Skip to content

Create CNI-enabled firecracker-runtime.json on the fly #292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 17, 2019

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Oct 16, 2019

Issue #, if available:

#291 but I'd like to keep the issue open until we remove tools/docker/firecracker-runtime.json.

Description of changes:

As #291 suggested, this change adds a helper that modifies firecracker-runtime.json.

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

@kzys kzys requested a review from sipsma October 16, 2019 20:27
@kzys kzys force-pushed the config-on-the-fly branch from 5fa964a to 0248ad3 Compare October 16, 2019 21:19
Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

Thanks as I was planning on picking this up myself after my current task. This looks good but I think we should just update all of the tests to use the generator and get rid of tools/docker/firecracker-runtime.json too; I don't see any need to split the effort up.

@kzys
Copy link
Contributor Author

kzys commented Oct 16, 2019

The biggest reason is the jailer PR which modifies tools/docker/firecracker-runtime.json and runtime/service_integ_test.go.

Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'm fine shipping it now, but we'll need to follow up after the jailing work is done to clean up the rest of the tests.

@sipsma
Copy link
Contributor

sipsma commented Oct 16, 2019

The biggest reason is the jailer PR which modifies tools/docker/firecracker-runtime.json and runtime/service_integ_test.go.

It's worth check with @xibz as it seems like rebasing the jailer PR would require just adding a few lines to defaultRuntimeConfig, which is pretty trivial and then just gets it all done at once. I don't care strongly either way though.

@xibz
Copy link
Contributor

xibz commented Oct 16, 2019

@sipsma - Yea, it is minor to rebase on top of this.

@kzys
Copy link
Contributor Author

kzys commented Oct 16, 2019

@sipsma/@xibz Thanks. Let me have a different additional commit to completely remove the file. If it looks fine. I'm going squash the commit with the existing one.

@@ -5,7 +5,5 @@
"root_drive": "/var/lib/firecracker-containerd/runtime/default-rootfs.img",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since most customers wouldn't generate the configuration file on-the-fly, I'd like to keep this JSON file inside examples/.

As firecracker-microvm#291 suggested, this change adds a helper that modifies
firecracker-runtime.json.

Signed-off-by: Kazuyoshi Kato <[email protected]>
@kzys kzys force-pushed the config-on-the-fly branch from 54876b2 to 193b303 Compare October 17, 2019 20:19
@kzys kzys merged commit 0165b0b into firecracker-microvm:master Oct 17, 2019
@kzys kzys deleted the config-on-the-fly branch October 17, 2019 20:47
fangn2 pushed a commit to fangn2/firecracker-containerd that referenced this pull request Mar 23, 2023
…ependabot/go_modules/github.com/go-openapi/errors-0.19.9

Bump github.com/go-openapi/errors from 0.19.8 to 0.19.9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants