cluster up: use docker engine-api client #14729

csrwng · 2017-06-18T14:13:57Z

Switches cluster up from the fsouza go docker client to the docker engine-api client
Fixes #14546

csrwng · 2017-06-18T14:18:31Z

@openshift/devex ptal

Just a note on the client code ... a good bit of similar code exists in the kube code base and I copied a couple of functions from it. However, I decided against calling the Kube client directly because:

It doesn't include everything I need like the file upload/download functions
It has already moved to a different spot in master than it was in the 1.6 vendored code that we have in the code base
It's uncertain if it belongs in the kube codebase going forward, given that the main interface is CRI and implementations can be external binaries.

csrwng · 2017-06-18T14:21:39Z

[testextended][extended:clusterup]

jim-minter · 2017-06-19T08:10:12Z

pkg/bootstrap/docker/dockerhelper/client.go

+ err error
+}
+
+type apiResult interface{}


jim-minter · 2017-06-19T08:16:59Z

I wish we could get away with not having yet another docker API wrapper, but assuming this wish can't come true, honestly I would prefer it to closely match the naming/semantics of the underlying client - for example:

name the wrapper APIs identically to the underlying APIs wherever possible
remove smarts such as the for ... range in ContainerNames() (see local comment)

My impression is that wherever we add restrictions on the underlying API (e.g. provide 2 underlying API calls in one wrapper function, or hard code arguments to the underlying API), we create additional future work for ourselves when we undo that.

Also I don't see the benefit of adding additional layers of scaffolding (e.g. dockerClient, containerClient, imageClient, execClient...).

Also it feels like we just have a huge amount of "helper" infrastructure in pkg/bootstrap. I think it adds quite a lot of cognitive load, e.g. when diagnosing concurrency issues in the underlying API, which I've had to do twice in the last 6 months.

jim-minter · 2017-06-19T08:17:55Z

pkg/bootstrap/docker/dockerhelper/client.go

+ return c.endpoint
+}
+
+func (c *dockerClient) Version() (*types.Version, error) {


would prefer ServerVersion, etc., etc. (audit all function names)

jim-minter · 2017-06-19T08:18:17Z

pkg/bootstrap/docker/dockerhelper/client.go

+ return &info, err
+}
+
+func (c *dockerClient) ContainerNames() ([]string, error) {


would prefer this to be ContainerList - move the additional smarts to the caller - there's only one.

jim-minter · 2017-06-19T08:19:15Z

pkg/bootstrap/docker/dockerhelper/client.go

+}
+
+type containerClient struct {
+ name string


suggest id (since there's an ID() function)

jim-minter · 2017-06-19T08:19:21Z

pkg/bootstrap/docker/dockerhelper/client.go

+}
+
+type execClient struct {
+ name string


suggest id (since there's an ID() function)

jim-minter · 2017-06-19T08:26:48Z

pkg/bootstrap/docker/dockerhelper/client.go

+ stdinDone := make(chan struct{})
+ go func() {
+ if inputStream != nil {
+ io.Copy(resp.Conn, inputStream)


no error reporting if inputStream copy fails

jim-minter · 2017-06-19T08:37:41Z

pkg/bootstrap/docker/dockerhelper/client.go

+ return c.name
+}
+
+func (c *execClient) StartAndWait(stdIn io.Reader, stdOut, stdErr io.Writer) error {


misleading name

mfojtik · 2017-06-19T14:09:38Z

pkg/bootstrap/docker/dockerhelper/client.go

+
+const (
+ // defaultTimeout is the default timeout of short running docker operations.
+ defaultTimeout = 2 * time.Minute


defaultDockerOpTimeout? :-)

bparees · 2017-06-19T18:41:24Z

pkg/bootstrap/docker/dockerhelper/client.go

+
+const (
+ // defaultTimeout is the default timeout of short running docker operations.
+ defaultTimeout = 2 * time.Minute


i think we've found this to be problematic elsewhere (s2i) and ultimately removed it. at a minimum i'd consider bumping it to 10 mins if we don't think we can live without it entirely. (problematic when the docker daemon is overloaded)

bparees · 2017-06-19T18:56:31Z

pkg/bootstrap/docker/dockerhelper/helper.go

- }
- }
- return names, nil
+ return h.client.ContainerNames()


unless you do what @jim-minter suggested in terms of moving the conversion to array logic here, i don't think there's even a reason for this function to exist anymore.

bparees · 2017-06-19T19:06:56Z

test/extended/clusterup.sh

+ os::cmd::try_until_text "oc get pods -n logging -l logging-infra=deployer" "Completed" $(( 20*minute )) 2
+ os::cmd::try_until_text "oc get endpoints logging-kibana -o jsonpath='{ .subsets[*].ports[?(@.name==\"3000-tcp\")].port }' -n logging" "3000" $(( 10*minute )) 1
+ os::cmd::expect_success "oc login -u developer"
+}


can i haz service-catalog test plz?

So I tried running on my Mac with --service-catalog and I get this:

oc cluster up --service-catalog Starting OpenShift using openshift/origin:v3.6.0-alpha.2 ... -- Checking OpenShift client ... OK -- Checking Docker client ... OK -- Checking Docker version ... OK -- Checking for existing OpenShift container ... OK -- Checking for openshift/origin:v3.6.0-alpha.2 image ... OK -- Checking Docker daemon configuration ... OK -- Checking for available ports ... OK -- Checking type of volume mount ... Using Docker shared volumes for OpenShift volumes -- Creating host directories ... OK -- Finding server IP ... Using 127.0.0.1 as the server IP -- Starting OpenShift container ... Creating initial OpenShift configuration Starting OpenShift using container 'origin' Waiting for API server to start listening OpenShift server started -- Adding default OAuthClient redirect URIs ... OK -- Installing registry ... OK -- Installing router ... OK -- Importing image streams ... OK -- Importing templates ... OK -- Installing service catalog ... FAIL Error: cannot instantiate service catalog template Caused By: Error: cannot create objects from template openshift/service-catalog Caused By: Error: [policybindings.authorization.openshift.io "service-catalog:default" not found, policybindings.authorization.openshift.io "kube-system:default" not found]

The outcome is the same whether using this branch or master. I'll create an issue for it and when it's fixed, it should be very easy to add to the extended test.

it needs to use a new origin image when running. the requirement to have a policybinding before creating a binding was recently lifted.

I would have expected this test to run using the local origin image anyway, though (--version=latest)

csrwng · 2017-06-19T22:27:27Z

Thanks for the reviews, comments addressed

bparees · 2017-06-20T21:10:36Z

lgtm but @jim-minter has stronger opinions so will let him sign off before merging.

jim-minter · 2017-06-21T08:51:16Z

pkg/bootstrap/docker/dockerhelper/client.go

+// holdHijackedConnection holds the HijackedResponse, redirects the inputStream to the connection, and redirects the response
+// stream to stdout and stderr.
+func holdHijackedConnection(inputStream io.Reader, outputStream, errorStream io.Writer, resp types.HijackedResponse) error {
+ receiveStdout := make(chan error)


must be buffered (make(chan error, 1)) otherwise the goroutine will never be able to exit if the channel reader has gone away

jim-minter · 2017-06-21T08:51:25Z

pkg/bootstrap/docker/dockerhelper/client.go

+ }()
+ }
+
+ sendStdin := make(chan error)


also must be buffered

(actually this is the key one)

I'd rather not make the channel buffered if I don't have to. Looking at this function, I don't see how we wouldn't read the channel after starting the goroutine, and I want to wait for a result to be available.

buffered != non-blocking read. As things stand, if the select reads a value from receiveStdout before reading a value from sendStdin, the function will return. However in this case the inputStream goroutine will block forever trying to write a value to sendStdin, because it is unbuffered and no-one will read from it once the function has returned.

ic, got it. Thx for the explanation

jim-minter · 2017-06-21T09:03:18Z

pkg/bootstrap/docker/dockerhelper/client.go

+ return c.client.CopyToContainer(context.Background(), container, dest, src, options)
+}
+
+func (c *dockerClient) CopyFromContainer(container string, src string, dest io.Writer) error {


Suggest you return all the values from c.client.CopyFromContainer straight. Then you get rid of the io.Copy here, and the goroutine and pipe in newContainerDownloader.

jim-minter · 2017-06-21T09:08:14Z

pkg/bootstrap/docker/dockerhelper/client.go

+}
+
+type dockerClient struct {
+ endpoint string


currently appears to be unused - are you intending to use it in the future?

It's used here https://github.com/openshift/origin/pull/14729/files#diff-11bba6ee4c8e50647af674cb95779e73R202

the link didn't work for me - I can see it's used in Endpoint(), but I can't see Endpoint() being used anywhere...

line 202 of dockerhelper/helper.go

Got it, thanks.

jim-minter · 2017-06-21T09:12:34Z

pkg/bootstrap/docker/openshift/helper.go

+
+ // The OSE version will have > 4 parts to the version string
+ // We'll only take the first 3
+ parts := strings.Split(versionStr, ".")


parts := strings.SplitN(versionStr, ".", 3)

That's not what I want. SplitN will give me the last segment including the dot. Here I want to remove 4th segment.

jim-minter · 2017-06-21T09:15:01Z

minor comments, otherwise go for it.

csrwng · 2017-06-21T16:37:35Z

@jim-minter comments addressed

jim-minter · 2017-06-21T16:51:57Z

@csrwng looks good to me, and also, I don't think I was sufficiently careful with the review and probably caused you unnecessary work with it. Apologies, I will take more care next time.

csrwng · 2017-06-21T16:55:02Z

thx for the review, no worries
[merge]

openshift-bot · 2017-06-21T16:55:46Z

Evaluated for origin merge up to 488c111

openshift-bot · 2017-06-21T16:55:47Z

[Test]ing while waiting on the merge queue

openshift-bot · 2017-06-21T17:07:46Z

Evaluated for origin test up to 488c111

bparees · 2017-06-21T17:16:43Z

[merge][severity:blocker] (this fixes a p1)

openshift-bot · 2017-06-21T17:31:32Z

Evaluated for origin testextended up to 488c111

openshift-bot · 2017-06-21T18:23:34Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/678/) (Base Commit: 8c00852) (PR Branch Commit: 488c111) (Extended Tests: clusterup)

openshift-bot · 2017-06-21T18:55:42Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2475/) (Base Commit: 8c00852) (PR Branch Commit: 488c111)

smarterclayton · 2017-06-21T21:00:26Z

pkg/bootstrap/docker/up.go

@@ -675,16 +671,17 @@ func (c *CommonStartConfig) CheckNsenterMounter(out io.Writer) error {
 // CheckDockerVersion checks that the appropriate Docker version is installed based on whether we are using the nsenter mounter
 // or shared volumes for OpenShift
 func (c *CommonStartConfig) CheckDockerVersion(out io.Writer) error {
- ver, _, err := c.DockerHelper().Version()
+ ver, isRHDocker, err := c.DockerHelper().APIVersion()


You have no idea how much this disappoints me

openshift-bot · 2017-06-22T12:07:31Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1085/) (Base Commit: 7657e99) (PR Branch Commit: 488c111) (Extended Tests: blocker) (Image: devenv-rhel7_6392)

csrwng force-pushed the clusterup_docker_engine branch from 1776315 to 72d9a8f Compare June 18, 2017 16:16

jim-minter reviewed Jun 19, 2017

View reviewed changes

pkg/bootstrap/docker/dockerhelper/client.go Outdated

err error

}

type apiResult interface{}

Copy link

Contributor

jim-minter Jun 19, 2017

Choose a reason for hiding this comment

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

unused?

jim-minter reviewed Jun 19, 2017

View reviewed changes

php-coder mentioned this pull request Jun 19, 2017

cluster down: error: json: cannot unmarshal bool into Go struct field APIMount.RW of type string #14737

Closed

mfojtik reviewed Jun 19, 2017

View reviewed changes

bparees reviewed Jun 19, 2017

View reviewed changes

bparees self-assigned this Jun 19, 2017

csrwng force-pushed the clusterup_docker_engine branch from 72d9a8f to 63a9a69 Compare June 19, 2017 22:26

csrwng force-pushed the clusterup_docker_engine branch from 63a9a69 to d1777b0 Compare June 19, 2017 22:29

csrwng mentioned this pull request Jun 20, 2017

Can't oc cluster down on latest centos docker #14763

Closed

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2017

jim-minter reviewed Jun 21, 2017

View reviewed changes

php-coder mentioned this pull request Jun 21, 2017

oc cluster down fails on JSON decode error #14546

Closed

csrwng force-pushed the clusterup_docker_engine branch from d1777b0 to 883c112 Compare June 21, 2017 16:29

Replace fsouza go-docker client with engine-api client in cluster up

488c111

csrwng force-pushed the clusterup_docker_engine branch from 883c112 to 488c111 Compare June 21, 2017 16:36

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2017

smarterclayton reviewed Jun 21, 2017

View reviewed changes

openshift-bot merged commit c015a70 into openshift:master Jun 22, 2017

csrwng mentioned this pull request Jun 23, 2017

cluster up extended tests #12683

Closed

deads2k mentioned this pull request Apr 4, 2018

remove oc cluster up experimental flags broken since 3.6.1 #19209

Merged

cluster up: use docker engine-api client #14729

cluster up: use docker engine-api client #14729

Conversation

csrwng commented Jun 18, 2017

csrwng commented Jun 18, 2017 • edited Loading

csrwng commented Jun 18, 2017

Choose a reason for hiding this comment

jim-minter commented Jun 19, 2017 • edited Loading

jim-minter Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented Jun 19, 2017

bparees commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter commented Jun 21, 2017

csrwng commented Jun 21, 2017

jim-minter commented Jun 21, 2017

csrwng commented Jun 21, 2017

openshift-bot commented Jun 21, 2017

openshift-bot commented Jun 21, 2017

openshift-bot commented Jun 21, 2017

bparees commented Jun 21, 2017

openshift-bot commented Jun 21, 2017

openshift-bot commented Jun 21, 2017

openshift-bot commented Jun 21, 2017

Choose a reason for hiding this comment

openshift-bot commented Jun 22, 2017 • edited Loading

csrwng commented Jun 18, 2017 •

edited

Loading

jim-minter commented Jun 19, 2017 •

edited

Loading

jim-minter Jun 19, 2017 •

edited

Loading

jim-minter Jun 21, 2017 •

edited

Loading

openshift-bot commented Jun 22, 2017 •

edited

Loading