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

daemon, plugin: follow containerd namespace conventions #35812

Merged
merged 1 commit into from Dec 19, 2017

Conversation

@stevvooe
Copy link
Contributor

commented Dec 15, 2017

Follow the conventions for namespace naming set out by other projects,
such as linuxkit and cri-containerd. Typically, they are some sort of
host name, with a subdomain describing functionality of the namespace.
In the case of linuxkit, services are launched in services.linuxkit.
In cri-containerd, pods are launched in k8s.io, making it clear that
these are from kubernetes.

Signed-off-by: Stephen J Day stephen.day@docker.com

This needs to be backported to the rc.

@stevvooe stevvooe force-pushed the stevvooe:follow-conventions branch from c785c8a to eae7e06 Dec 15, 2017

@stevvooe

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

Failures are from &errors.errorString{s:"Error response from daemon: \"/containerd-shim/containers.moby/40fae444a4b4ca63cdafe4f5e973c66c54e02490672fb11be2e02beb7928c97e/shim.sock\": unix socket path too long (limit 106): unknown"}. I can fix that once we get a little feedback.

// MainNamespace is the name of the namespace used for users containers
const MainNamespace = "moby"
// ContainersNamespace is the name of the namespace used for users containers
const ContainersNamespace = "containers.moby"

This comment has been minimized.

Copy link
@stevvooe

stevvooe Dec 15, 2017

Author Contributor

We can shorten this to just moby, and that will limit the impact.

This comment has been minimized.

Copy link
@vdemeester

vdemeester Dec 19, 2017

Member

Arf, I like containers.moby here though 😓.
If we need to do a migration anyway, let's do this one right (and thus I would like to have containers.moby instead).

daemon, plugin: follow containerd namespace conventions
Follow the conventions for namespace naming set out by other projects,
such as linuxkit and cri-containerd. Typically, they are some sort of
host name, with a subdomain describing functionality of the namespace.
In the case of linuxkit, services are launched in `services.linuxkit`.
In cri-containerd, pods are launched in `k8s.io`, making it clear that
these are from kubernetes.

Signed-off-by: Stephen J Day <stephen.day@docker.com>

@stevvooe stevvooe force-pushed the stevvooe:follow-conventions branch from eae7e06 to 521e7eb Dec 16, 2017

@@ -16,7 +16,7 @@ import (
)

// PluginNamespace is the name used for the plugins namespace
var PluginNamespace = "moby-plugins"
var PluginNamespace = "plugins.moby"

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Dec 18, 2017

Contributor

We'll need a way to migrate users from the old namespace. This is particularly a problem when live-restore is enabled... without live-restore we could just delete the old container and create a new one in the new namespace.

Even with live-restore we can probably get away with shutting down the containers and re-creating them in the new namespace.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Dec 18, 2017

Member

Yes, we should have a look at migration, but impact is limited currently to people running 17.11 (which is an edge release); it's important we make sure that this change will be in 17.12.0 (worst case scenario: without the migration step, but instructions in the release notes to remove plugins and re-add them after updating)

This comment has been minimized.

Copy link
@vdemeester

vdemeester Dec 19, 2017

Member

@stevvooe @crosbymichael is it possible to "change" an object namespace in containerd ?

This comment has been minimized.

Copy link
@stevvooe

stevvooe Dec 19, 2017

Author Contributor

The namespace cannot be renamed.

We should have considered this before merging containerd 1.0 support. We can migrate but the only impact here is that plugins will be impacted on upgrade.

This comment has been minimized.

Copy link
@seemethere

seemethere Dec 19, 2017

Contributor

As far as the impact goes we can include a note in the changelog with a link to a script that can do the migration.

I think that should suffice since the number of users using 17.11 along with plugins, in production, is probably low.

This comment has been minimized.

Copy link
@stevvooe

stevvooe Dec 19, 2017

Author Contributor

I may have already answered this elsewhere, but no.

@seemethere
Copy link
Contributor

left a comment

LGTM

@cpuguy83
Copy link
Contributor

left a comment

LGTM

@vieux
vieux approved these changes Dec 19, 2017
Copy link
Collaborator

left a comment

LGTM

@vieux vieux merged commit 745278d into moby:master Dec 19, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38435 has succeeded
Details
janky Jenkins build Docker-PRs 47169 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7609 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18687 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7450 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.