Skip to content

Include CRI support #255

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

Closed
wants to merge 7 commits into from
Closed

Include CRI support #255

wants to merge 7 commits into from

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Sep 3, 2019

Issue #, if available:

#88

Description of changes:

This change adds CRI support on firecracker-containerd, by including the CRI plugin.

Since Go's plugin system is pretty picky about the versions of the dependencies, it is hard to build either CRI support or Firecracker support as a plugin, separately from containerd itself.

This change adds both of them in firecracker-containerd, to unblock CRI conformance verification work.

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

)

// Workaround for github.com/containerd/containerd issue #3031
replace github.com/docker/distribution v2.7.1+incompatible => github.com/docker/distribution v2.7.1-0.20190205005809-0d3efadf0154+incompatible

replace k8s.io/api => k8s.io/api v0.0.0-20190620084959-7cf5895f2711
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I'm understanding correctly, we need these replace directives because they are indirect dependencies and we thus can't specify specific versions in our go.mod? Kind of unfortunate if so and seems like a major deficiency in go modules, but if there's no better way than it is what it is.

May be worth including a comment in the go.mod file explaining this and that they come from the update-gomod.sh script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reality is a bit sad than that. We cannot even build our binaries without having a bunch of replace directives due to kubernetes/kubernetes#79384.

From the issue above;

k8s.io/kubernetes is not primarily intended to be consumed as a module. Only the published subcomponents are (and go get works properly with those).

Ideally speaking, the CRI plugin should depend on the published subcomponents, but it may not be possible right now. I haven't looked how hard it would be though.

update-gomod.sh Outdated
@@ -0,0 +1,35 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe update-cri-gomod.sh or similar would be a slightly better name so it's apparent this just handles cri-related dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Will rename the script.

kzys added 6 commits September 5, 2019 15:39
By running "bash ./update-cri-gomod.sh 1.15.0". The script is from
https://github.com/kubernetes-sigs/aws-ebs-csi-driver/tree/v0.4.0/hack.

Signed-off-by: Kazuyoshi Kato <[email protected]>
By "go get github.com/opencontainers/runtime-spec@29686dbc5559d93fb1ef402eeda3e35c38d75af4".
The hash is from https://github.com/containerd/containerd/blob/v1.3.0-beta.2/vendor.conf.

Signed-off-by: Kazuyoshi Kato <[email protected]>
By "go get github.com/docker/docker@86f080cff0914e9694068ed78d503701667c4c00".
The hash is from https://github.com/containerd/containerd/blob/v1.3.0-beta.2/vendor.conf.

Signed-off-by: Kazuyoshi Kato <[email protected]>
By "go get github.com/containerd/cri@f1d492b0cdd14e76476ee4dd024696ce3634e501".
The hash is from https://github.com/containerd/containerd/blob/v1.3.0-beta.2/vendor.conf.

Signed-off-by: Kazuyoshi Kato <[email protected]>
Signed-off-by: Kazuyoshi Kato <[email protected]>
@kzys
Copy link
Contributor Author

kzys commented Nov 19, 2019

I'm going to update containerd's dependencies first.

@kzys
Copy link
Contributor Author

kzys commented Jan 21, 2020

Let me close the PR for now, since I wouldn't be working on that right now and containerd's go mod migration (which is in progress) may make the situation better in future.

@kzys kzys closed this Jan 21, 2020
fangn2 pushed a commit to fangn2/firecracker-containerd that referenced this pull request Mar 23, 2023
Jailer configuration API cleanup and improved logging with Debug log level
fangn2 pushed a commit to fangn2/firecracker-containerd that referenced this pull request Mar 23, 2023
* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263)
* Fix Benchmark Goroutine (firecracker-microvm#259)
* Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255)
* Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253)
* Fixed error that was not being test against in `TestWait` (firecracker-microvm#251)
* Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230)
* Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225)
* Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218)

Signed-off-by: xibz <[email protected]>
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.

2 participants