Skip to content
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

[Investigation] Remove Containerd dependency from root go.mod #1148

Open
dcantah opened this issue Sep 3, 2021 · 3 comments
Open

[Investigation] Remove Containerd dependency from root go.mod #1148

dcantah opened this issue Sep 3, 2021 · 3 comments

Comments

@dcantah
Copy link
Contributor

dcantah commented Sep 3, 2021

The containerd/containerd repo brings in quite a lot of transitive dependencies for what we actually use the import for. We currently use it for the task definitions, error definitions and a couple of other things for our shim that's built out of this repository. If the shim had it's own go.mod this may be alleviated, although with adding a bit of annoyance to maintenance.

A real pain point of this however that we'd encountered in the wild is when trying to update kubernetes/kubernetes to a new tag of hcsshim, which was that Containerd 1.5+ (before 1.5 containerd was not on go modules) currently errors out when trying to get vendored into k8s. This is due to Containerd having dependencies on a lot of the k8s subprojects that live in the staging directory in kubernetes/kubernetes and the projects in the staging directory having dependencies on each other through replace directives. I'm not sure of the root problem here or what's the best solution long term, but for our case removing this dep from the root go.mod will solve quite a lot.

Containerd has already done some work to try and alleviate some scenarios where the only thing you might need from Containerd is just api definitions, see containerd/containerd#5716 (thanks @dims!!!) but we use the dep for a bit more than what this change offers.

Perhaps there can be some work done in kubernetes itself to alleviate some of these issues, as it doesn't seem uncommon to have a project that depends on some of the subprojects that needs to be vendored into k8s itself. I've made a dead simple repo that just depends on a couple of the sub projects at the same tags that ctrd 1.5 was on, and the same issue arises on trying to add this as a dependency to K8s https://github.com/dcantah/deps.

@dims
Copy link
Contributor

dims commented Sep 6, 2021

cc @mikebrow @liggitt

@dcantah
Copy link
Contributor Author

dcantah commented Sep 8, 2021

Looks like the option is to trim from containerd and hcsshim as expected (thanks @liggitt) kubernetes/kubernetes#104827 (comment)

@TBBle
Copy link
Contributor

TBBle commented Sep 26, 2021

Poking around, it seems almost all the needed imports are to get access to either github.com/containerd/containerd/runtime or github.com/containerd/containerd/errdefs. So it might make sense for those to be exposed as modules in containerd, or the relevant things to be made available via github.com/containerd/containerd/api.

Specifically, there's some constants in github.com/containerd/containerd/runtime (events.go), and github.com/containerd/containerd/runtime/v2/task is actually a Protobuf API definition, and would make sense to be published as a module like github.com/containerd/containerd/api.

github.com/containerd/containerd/errdefs is also being used for generating and serialising containerd error codes, and is also used by callers so probably needs to be API-stabilised in containerd anyway.

github.com/containerd/containerd/namespaces is in a similar boat, it's used to add information to the events published back to containerd (per #689) and I suspect the other uses of it are unnecessary, as it's used to extract a namespace from a context, but we already know our namespace, as used in #689.

After that, there's only a couple of constants (log.RFC3339NanoFixed, mount.ParentLayerPathsFlag) to deal with, both of which are also dealing with containerd-specific protocol details: the npipe log format and the details of smuggling windows layer parent paths through the API-exposed Mount.Options field respectively. Both of those constants could probably be just copied into the hcsshim codebase, as a change in their value would be a containerd shim API break. I don't know if it's possible (or better) to add (m* Mount) GetParentPaths to containerd/api/types or not.

Anyway, this is almost all containerd-side changes I'm proposing, which would mean containerd 1.6 at best. There probably needs to be a tracking bug on the containerd side for "stabilising Go API for shim implementers"?

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

No branches or pull requests

3 participants