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

Add (hidden) flags to set containerd namespaces #39500

Merged
merged 1 commit into from
Jul 12, 2019

Conversation

cpuguy83
Copy link
Member

This allows our tests, which all share a containerd instance, to be a
bit more isolated by setting the containerd namespaces to the generated
daemon ID's rather than the default namespaces.

This came about because I found in some cases we had test daemons
failing to start (really very slow to start) because it was (seemingly)
processing events from other tests.

@thaJeztah
Copy link
Member

related previous PR (which did this through the API) #36914

Is the namespace length still a concern? Or don't we reach the maximum length here? #36914 (comment)

@cpuguy83
Copy link
Member Author

Length can still be problematic if you make too long of a name. I ran these locally and it was fine at least.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

found a typo, but SGTM

cmd/dockerd/config_unix.go Outdated Show resolved Hide resolved
@cpuguy83
Copy link
Member Author

Ah I need to deal with Windows issues (validation check doesn't make sense on Windows which doesn't use containerd right now).

But going to let linux tests run through first.

@thaJeztah
Copy link
Member

yes, let's see if it all works 🤞

@thaJeztah
Copy link
Member

this looks weird (converting it to a http url?)

02:11:22     daemon.go:336: [d4259ba841705] error pinging daemon on start: Get http://%2Ftmp%2Fdocker-integration%2Fd4259ba841705.sock/_ping: dial unix /tmp/docker-integration/d4259ba841705.sock: connect: no such file or directory

This allows our tests, which all share a containerd instance, to be a
bit more isolated by setting the containerd namespaces to the generated
daemon ID's rather than the default namespaces.

This came about because I found in some cases we had test daemons
failing to start (really very slow to start) because it was (seemingly)
processing events from other tests.

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83 cpuguy83 force-pushed the custom_containerd_namespace branch from 1fda6e8 to 24ad2f4 Compare July 12, 2019 00:28
@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@2fc3480). Click here to learn what that means.
The diff coverage is 27.27%.

@@            Coverage Diff            @@
##             master   #39500   +/-   ##
=========================================
  Coverage          ?   37.33%           
=========================================
  Files             ?      609           
  Lines             ?    45230           
  Branches          ?        0           
=========================================
  Hits              ?    16886           
  Misses            ?    26055           
  Partials          ?     2289

@cpuguy83
Copy link
Member Author

Failure was because the plugin client was talking to the wrong namespace, fixed.

@cpuguy83
Copy link
Member Author

Clean run on the first shot...

@cpuguy83
Copy link
Member Author

Apparently I can't make gordon rebuild successful tests anymore.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Two questions;

  • do we want to make this visible at some point?
  • do we need separate configuration for plugins and containers, or could plugins take the same ns plus a suffix (e.g. <containerd-namespace> + "p")?

@thaJeztah
Copy link
Member

all green now

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 12, 2019

do we want to make this visible at some point?

🤷‍♂
Seems footgun-ish right now, particularly before containerd is fully integrated.

do we need separate configuration for plugins and containers, or could plugins take the same ns plus a suffix (e.g. + "p")?

I don't see why not, actually makes it a bit simpler to implement I think.
I originally implemented this as a prefix which gets pre-pended to the hardcoded namespaces and then used the test daemon's ID as the prefix... this led to path length issues so I changed to have the prefix just be an auto-incremented counter, but namespaces have validation that prevent an integer from being the first character.
Ultimately is kinda nice to have full control rather than a prefix or suffix.


edit: fixed typo

@thaJeztah
Copy link
Member

Ultimately is kinda nice to have full control rather than a prefix for suffix.

Ok, that's fair; I was thinking if it was too much bells and whistles, but given that it's not a "public" UX yet, we can still change if we change our minds.

Copy link
Contributor

@tiborvass tiborvass left a comment

Choose a reason for hiding this comment

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

Fine for me

@crosbymichael
Copy link
Contributor

LGTM

@crosbymichael crosbymichael merged commit 1d52c0b into moby:master Jul 12, 2019
@cpuguy83 cpuguy83 deleted the custom_containerd_namespace branch July 12, 2019 17:38
@Rid
Copy link
Contributor

Rid commented Jul 24, 2019

Is there any way to set this flag as a user? Even if it's not in the public UX?

@thaJeztah
Copy link
Member

@Rid Yes, this PR adds the --containerd-namespace and --containerd-plugins-namespace flags on docker. Is there a specific use-case you have in mind to change the namespace? It's added here for a very specific use-case (CI is running multiple instances of dockerd on the same machine to test specific situations)

@Rid
Copy link
Contributor

Rid commented Jul 25, 2019

@thaJeztah we're running a public docker cloud, where each user is assigned a docker daemon and we'd like as much separation between users as possible for security reasons.

@cpuguy83
Copy link
Member Author

@Rid It's not really the intention to provide secure isolation with this. We just needed a mechanism to do this for tests and these flags are not guaranteed to be supported going forward.

@Rid
Copy link
Contributor

Rid commented Jul 31, 2019

@cpuguy83 is it something that you'd consider supporting, given that it's useful in the use case where you're running multiple daemons per machine?

@cpuguy83
Copy link
Member Author

@Rid It's definitely something I'd like to do, but I'm not sure these flags are the right place for that, and definitely not until after we have moved to totally using containerd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants