oc cluster up in user namespace mode: unbreak available ports check #14169

php-coder · 2017-05-12T15:24:37Z

In order to have access to HostConfig.Userns field, I have to update version of go-dockerclient.

PTAL @csrwng @bparees
CC @mfojtik @pweil-

Addressed to #12643

php-coder · 2017-05-12T15:30:04Z

pkg/bootstrap/docker/dockerhelper/helper.go

+		return false, err
+	}
+	for _, val := range info.SecurityOptions {
+		if val == "name=userns" {


This check depends on the format of SecurityOptions and could be broken by updates in Docker but I think that I couldn't do anything better than this. Using strings.Contains() won't make this check better IMHO.

php-coder · 2017-05-12T15:32:32Z

pkg/bootstrap/docker/run/run.go

@@ -112,7 +125,11 @@ func (h *Runner) Env(env ...string) *Runner {

 // Privileged tells Docker to run the container as privileged
 func (h *Runner) Privileged() *Runner {


Privileged option doesn't work in user namespace mode, so I'm emulating it's behavior. Because signature of Privileged hasn't changed, we don't need to update existing code.

privileged + host user namespace combo works. It may be safe here that if this method is called always call HostUserns. Not setting h.hostConfig.Privileged is going to result in different behavior so it should still be set.

I remember that Docker doesn't allow --privileged option so I thought that API also doesn't accept it. Ok, I'll try it on Monday.

Thanks! I've updated the code accordingly.

php-coder · 2017-05-12T15:39:04Z

pkg/bootstrap/docker/up.go

@@ -714,14 +714,19 @@ func (c *ClientStartConfig) EnsureDefaultRedirectURIs(out io.Writer) error {

 // CheckAvailablePorts ensures that ports used by OpenShift are available on the Docker host
 func (c *CommonStartConfig) CheckAvailablePorts(out io.Writer) error {
+	userNamespace, err := c.DockerHelper().UserNamespaceEnabled()


Initial idea was to invoke this function during helpers creation but in this case we couldn't report about possible error. The only way that I found is to invoke it from CheckAvailablePorts(). That's why signature of TestPorts() has been modified.

Why not ? (report about possible error)
If an error is returned by the docker info call, then cluster up should terminate. You can pass a dockerhelper to the NewRunHelper function.

I think it's less robust if you rely on the caller/client to figure out if you have user namespaces turned on.

The comments were addressed. PTAL.

csrwng

some comments

csrwng · 2017-05-12T21:07:14Z

pkg/bootstrap/docker/run/run.go

@@ -46,6 +47,12 @@ func (h *Runner) Name(name string) *Runner {
 	return h
 }

+// UserNamespace sets user namespace mode flag that affects behavior of other options.


I would like to make this part of the constructor for this helper (NewRunHelper). I don't think we should rely on the client to set it to the proper value.

csrwng · 2017-05-12T21:10:02Z

pkg/bootstrap/docker/up.go

@@ -714,14 +714,19 @@ func (c *ClientStartConfig) EnsureDefaultRedirectURIs(out io.Writer) error {

 // CheckAvailablePorts ensures that ports used by OpenShift are available on the Docker host
 func (c *CommonStartConfig) CheckAvailablePorts(out io.Writer) error {
+	userNamespace, err := c.DockerHelper().UserNamespaceEnabled()


Why not ? (report about possible error)
If an error is returned by the docker info call, then cluster up should terminate. You can pass a dockerhelper to the NewRunHelper function.

csrwng · 2017-05-12T21:10:49Z

pkg/bootstrap/docker/up.go

@@ -714,14 +714,19 @@ func (c *ClientStartConfig) EnsureDefaultRedirectURIs(out io.Writer) error {

 // CheckAvailablePorts ensures that ports used by OpenShift are available on the Docker host
 func (c *CommonStartConfig) CheckAvailablePorts(out io.Writer) error {
+	userNamespace, err := c.DockerHelper().UserNamespaceEnabled()


I think it's less robust if you rely on the caller/client to figure out if you have user namespaces turned on.

…a6db5eb48032

csrwng · 2017-05-15T18:38:00Z

pkg/bootstrap/docker/run/run.go

+	if err != nil {
+		return "", "", 0, err
+	} else if userNsMode {
+		h.hostConfig.Privileged = false


Why set this false? If the client has called Privileged on this Runner, then UserNsMode would be set to "host", which would work with Privileged set to true, right?

Actually I would get rid of the HostUserns method introduced above and instead, check here if Privileged is set, then if it is, then set h.hostConfig.UsernsMode to "host". Otherwise, leave it alone.

I would think you don't want to set hostConfig.UsernsMode to host if user namespaces is not abled.

Why set this false?

Because privileged option can't be used in user namespace mode (see the error message in the original issue: Privileged mode is incompatible with user namespaces).

check here if Privileged is set

Yes, thanks. I'll update this condition tomorrow.

Condition has been updated. PTAL.

csrwng · 2017-05-16T12:57:36Z

The cluster up part LGTM

csrwng · 2017-05-16T12:58:38Z

It'd be good for someone who works on rebases like @deads2k to make sure that updating the fsouza client won't cause any issues.

…n user namespace mode.

php-coder · 2017-05-16T14:48:34Z

@csrwng I've moved code for adjusting HostConfig from runWithOutput() to Create() function to make it also work in case when Start() function was being used to run container.

csrwng · 2017-05-16T14:59:58Z

thx, still LGTM

deads2k · 2017-05-16T17:13:56Z

It'd be good for someone who works on rebases like @deads2k to make sure that updating the fsouza client won't cause any issues.

make sure the levels match between kube master and this.

php-coder · 2017-05-16T18:04:19Z

make sure the levels match between kube master and this.

@deads2k which levels? Do you mean versions?

deads2k · 2017-05-16T19:22:26Z

@deads2k which levels? Do you mean versions?

Yeah, kube and origin should agree on the commit SHA to use in order to make the rebase reasonable (or sometimes even possible).

php-coder · 2017-05-18T12:39:48Z

Yeah, kube and origin should agree on the commit SHA to use in order to make the rebase reasonable (or sometimes even possible).

As I can see Kubernetes doesn't have fsouza/go-dockerclient dependency at this time. Does it mean that we can merge this PR?

mfojtik · 2017-05-18T12:51:57Z

@bparees is there a card about getting rid of fsouza client?

bparees · 2017-05-18T12:54:38Z

@mfojtik https://trello.com/c/cWe43zdJ

bparees · 2017-05-18T12:55:48Z

but we now maintain a separate vendored copy of fsouza in pkg/build/vendor so we can run at a different level than the rest of origin.

php-coder · 2017-05-18T12:59:40Z

but we now maintain a separate vendored copy of fsouza in pkg/build/vendor so we can run at a different level than the rest of origin.

Unfortunately oc cluster code doesn't use it and that's why I need this bump. I tried to update fsouza to the same version as we have in pkg/build/vendor but it was really hard because it leads to massive dependencies update (for example, docker api that requires something related to swarm).

php-coder · 2017-05-23T09:39:40Z

What is the outcome? Could we merge it now? If not then when? In the beginning of the next release?

csrwng · 2017-05-25T17:38:05Z

@php-coder the code looks good to me, but would like confirmation that it's ok to update the fsouza docker client. /cc @smarterclayton

php-coder · 2017-06-06T09:48:52Z

the code looks good to me, but would like confirmation that it's ok to update the fsouza docker client

@smarterclayton Ping

smarterclayton · 2017-06-06T13:51:33Z

LGTM [merge]

php-coder · 2017-06-06T17:19:02Z

test flake #14496 and #14497

csrwng · 2017-06-08T14:16:30Z

[merge]

openshift-bot · 2017-06-08T14:21:46Z

Evaluated for origin merge up to d84c262

openshift-bot · 2017-06-08T14:21:47Z

[Test]ing while waiting on the merge queue

openshift-bot · 2017-06-08T14:29:57Z

Evaluated for origin test up to d84c262

openshift-bot · 2017-06-08T16:29:42Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2020/) (Base Commit: ba62cde)

php-coder · 2017-06-08T16:34:22Z

test flake #14385

openshift-bot · 2017-06-08T22:06:01Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/932/) (Base Commit: 4b98e27) (Image: devenv-rhel7_6333)

php-coder commented May 12, 2017

View reviewed changes

bparees assigned csrwng May 12, 2017

csrwng reviewed May 12, 2017

View reviewed changes

bump(github.com/fsouza/go-dockerclient): 3f9370a4738ba8d0ed6eea63582c…

6619a31

…a6db5eb48032

php-coder force-pushed the oc_cluster_up_and_ports_in_userns_env branch 2 times, most recently from eb35c3f to c8553a2 Compare May 15, 2017 18:21

csrwng reviewed May 15, 2017

View reviewed changes

php-coder force-pushed the oc_cluster_up_and_ports_in_userns_env branch 2 times, most recently from 21e9a01 to 378d20e Compare May 16, 2017 12:52

oc cluster up: fix check for available ports when docker is running i…

d84c262

…n user namespace mode.

php-coder force-pushed the oc_cluster_up_and_ports_in_userns_env branch from 378d20e to d84c262 Compare May 16, 2017 14:45

csrwng mentioned this pull request Jun 8, 2017

`oc cluster up' fails with Error: Cannot get TCP port information from Kubernetes host #14512

Closed

openshift-bot merged commit 1257438 into openshift:master Jun 9, 2017

php-coder deleted the oc_cluster_up_and_ports_in_userns_env branch June 9, 2017 10:22

This was referenced Jun 13, 2017

oc cluster up doesn't work when Docker is running with user namespaces support #12643

Closed

oc cluster down fails on JSON decode error #14546

Closed

		@@ -112,7 +125,11 @@ func (h Runner) Env(env ...string) Runner {

		// Privileged tells Docker to run the container as privileged
		func (h Runner) Privileged() Runner {

oc cluster up in user namespace mode: unbreak available ports check #14169

oc cluster up in user namespace mode: unbreak available ports check #14169

Uh oh!

Conversation

php-coder commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csrwng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csrwng commented May 16, 2017

Uh oh!

csrwng commented May 16, 2017

Uh oh!

php-coder commented May 16, 2017

Uh oh!

csrwng commented May 16, 2017

Uh oh!

deads2k commented May 16, 2017

Uh oh!

php-coder commented May 16, 2017

Uh oh!

deads2k commented May 16, 2017

Uh oh!

php-coder commented May 18, 2017

Uh oh!

mfojtik commented May 18, 2017

Uh oh!

bparees commented May 18, 2017

Uh oh!

bparees commented May 18, 2017

Uh oh!

php-coder commented May 18, 2017

Uh oh!

php-coder commented May 23, 2017

Uh oh!

csrwng commented May 25, 2017

Uh oh!

php-coder commented Jun 6, 2017

Uh oh!

smarterclayton commented Jun 6, 2017

Uh oh!

php-coder commented Jun 6, 2017

Uh oh!

csrwng commented Jun 8, 2017

Uh oh!

openshift-bot commented Jun 8, 2017

Uh oh!

openshift-bot commented Jun 8, 2017

php-coder May 12, 2017 •

edited

Loading

openshift-bot commented Jun 8, 2017 •

edited

Loading