As my dev machine is a Mac i don't have the appropriate environment to run the integration tests against a local docker via unix socket at the moment. So could someone check this please before i merge?

I configured the Maven run with -Ddocker.io.url=unix://localhost/var/run/docker.sock and it looked to be fine, but now the build is hanging at STARTING testDockerBuilderAddFileInSubfolder. I'll investigate, but if you have some quick hints, I'd be glad :)

in other news: the log output looks a bit funny, with the unix protocol and port 80:
POST unix://localhost:80/v1.15/containers/87d5c24e2b1366532fb1629e9ab6e61b349972f12c22fd3232c9d780a2574db0/start

well, I'm currently debugging through org.apache.http.impl.conn.PoolingHttpClientConnectionManager#leaseConnection - could there be a problem with pooling connections, though the unix socket doesn't support such a behaviour? I'm no expert in that area... what I can say is, that each test run on its own works, only the third test hangs.

hmmm: AbstractConnPool says maxPerRoute == 2

so... how about closing the connections/client after each test method? A simple dockerClient.close(); in afterMethod(...) made subsequent tests fail, so would it be ok to "open" a new dockerClient on each test method? Would you expect similar problems in the production code?

The log output is connected to how the code works. Some other piece of code needs the URI to have a host and a port. So unix://localhost:80 ist used and the UnixConnectionSocketFactory just connects every "unix://" url to the same socket.

The connection pooling is also done in the orginal code, so I don't think connection pooling is causing problems in itself. However, if it happens in the test suite, it may happen in production as well. We should figure this out before merging.

For me the integration test suite still doesn't work. If I run

mvn -Ddocker.io.url=unix://localhost/var/run/docker.sock -Ddocker.io.version=1.14 clean install

It bails out with "Configured username is null." in the PushImageCmdImplTest. However, that's just incomplete configuration again. I am trying to just disable those tests right now.

I had a look at Thread- and HeapDumps of the Deadlock. It is defintely connected to the connection pooling. However, as I said before I believe that the UnixSockets are capabale of being used in a connection pool. I replaced the PoolingConnectionManager with a BasicConnectionManager, I got

javax.ws.rs.ProcessingException: java.lang.IllegalStateException: Connection is still allocated

From almost all tests. To me, this looks like we are actually not closing connections after we used them. Digging a little bit deeper, the RemoveContainerCmdExec does

String response = webResource.request().accept(MediaType.APPLICATION_JSON).delete(String.class);

However, remove container has no return value. So I suspect it is waiting for input (and thereby not releasing the connection) for a long time.

For RemoveImageCmdExec the Response object is obtained, but neither closed nor read as well.

Okay, my newest commit fixes the non-closed connections for me. It looks like the docker-java client had several response objects just never closed.

The problem was that a Response object closes automatically and the library relyed on this. However, they close only if the response has been fully read, which the test suite did not do all the time.

great, I'll try to test the recent changes this evening

LGTM 👍

@alexkamp, @gesellix Thanks for all of your effort. So its OK to merge it now?

I would say yes

hey,

What is the reason that this was not merged yet? Right now, there would be merge conflicts, but that was not the case 19 days ago.

So, does the new method bother you so much that you do not want to merge? It is an API change, so I understand that you are hesitating, but the current method, calling shutdown on the executor, simply does not work.

So, if you do not want to merge this at all, okay, it's your decision.

If you want a different solution than the new method, I can look into it. If you do not want an API change here, a custom implementation of ExecutorService which implements shutdown with the intended semantics would work. However, if we do this the semantics of our shutdown() differ from the semantics that are documented in the Java API, I don't think this is a particularly good idea.

As another remark, the version of docker-java I started with did not close HTTP connections. I wonder why no-one ever noticed that. In a long-running application, the docker server should crash under the load eventually. Some of the problems were mis-behaving unit tests, so maybe no-one noticed because all the production systems did not use the same patterns as the unit tests, but still, it seems to be worth a bug report on it's own.

So, I can work on those merge conflicts, however, I would like to know whether it is worth the effort before I start.

Alex

BTW: Does someone have a large, constantly running application based on docker-java? Can you check how many HTTP connections to the docker server it has open?

@alexkamp Sorry for the delay. I haven't found time to do the merge yet. One problem is with integration tests. I currently don't have the environment to test against a local docker instance so it needs at least a possibility to disable tests running via local unix socket. Probably it would be even better to have a switch that allows to run all integration tests against whether a local docker socket or an remote instance. Maybe this could be realized via maven profiles. Any suggestions?

Another problem is how to handle ssl connections with apache connector...

We could introduce an additional TestNG category for tests that require local socket connections. A maven profile could deactivate this category, e.g.

mvn clean install -Pno-socket-tests

@albers We could assume the http connection as default while switching to local socket communication by passing -Ddocker.io.url=unix://localhost/var/run/docker.sockas @gesellix suggested but via maven profile. This way all tests are executed via socket / remote connection.

The main problem now is how to configure apache connector as needed for unix socket connection to handle the ssl/certificate stuff.

I'm not sure if TLS/SSL is applicable for Unix sockets. wouldn't it be more of a user/group right configuration on the local machine?

@gesellix You are probably right. But we have to use SSL for remote connections with the apache connector. Don't know how to achieve this.

@marcuslinke I would guess that it's possible to disable the TLS depending on the actual socket scheme, but haven't had a look at the actual code, yet. As far as I know, the TLS configuration is only enabled when certificates are configured and found. So when using Unix sockets, I would expect that there won't be any certificates to be found. To summarize: I expect it should work out of the box :)

@gesellix To be clear here: After merging the apache connector stuff into current master branch SSL is not working anymore for remote connections. :(

ok, that's not what I expected. I could investigate this or tomorrow evening, but if anyone else has some time looking at the apache connector (@alexkamp?) - don't hesitate.

Well, I don't think I will be able to get a setup were SSL works, so I don't know how to test that. However, I'll try to give it a look nevertheless. @marcuslinke: Can I access your merge? This way I do not need to deal with the merge conflicts myself.

I just removed one test with hard-coded unix socket connection, it tests behaviour that is also exercised by some other test anyway. Other than that, I would go with a setup which executes all the tests with a unix socket connections via the -D... option. This is the most complete way to test this.

@alexkamp Just pushed the unix-socket branch that contains merge with current master.

any plan to release/merge this issue?

@lordofthejars Can't get SSL working within this branch. As long as that problem exist no merge is possible.

I think that spotify-docker (https://github.com/spotify/docker-client) has implemented this feature so maybe it can be used as guide.

Hey,

as I pointed out in my merge request, I basically took the code from spotify/docker-client and ported it to docker-java.

If you use the version from my fork, you have this feature, if you use marcuslinke's merge, you can even have the most recent docker-java with this feature. However, mind that SSL will NOT be working.

I am still planning to look into the SSL issue, unfortunately I had a deadline in november, another one in midst december and there is another one at end of January. So I will not be looking into this before February.

Alex

Ok, no problem it was to know when we could release Arquillian Cube with unix socket support :)

@marcuslinke do you have a test case for ssl support? I would like to see it actually working on my machine before breaking it with the unix socket - I would at least have a setup in which ssl is enabled so I can finally start checking the current issue.
I actually ran the maven build and only had to fix one test case ( @alexkamp you might consider pull this commit into your branch: gesellix-docker/docker-java@9a6732c ).

@gesellix Whether you run the integration tests with SSL support is a question of configuration. You have to configure a https:// server address as well as the required cert path for this. Something like

docker.io.url=https://192.168.59.103:2376
docker.io.username=dockeruser
docker.io.password=xxxxx
docker.io.email=dockeruser@domain.com
docker.io.version=1.15
docker.io.dockerCertPath=/Users/marcus/.boot2docker/certs/boot2docker-vm

@marcuslinke ah, thanks.

my current progress: I made the SSL config based on master work and got the SSL problems when using @alexkamp's unix socket branch. As @marcuslinke indicated, the Apache Connector needs to be configured with the SSL certificates, which is, what I've started in a quick and dirty way. It should be only a matter of time now, but I must admit, that the concept of different internally used http clients doesn't feel very good...

at least one test works now, the current code is pushed at https://github.com/gesellix-docker/docker-java/tree/unixsocket-rebased - it is rebased on the current master and squashed into one commit, though.

@marcuslinke do you know what the commit a19aa43 by @frankscholten is about?

Test environments:

• Docker daemon on tcp://172.17.42.1:2375 (Linux)
• Docker daemon on https://192.168.59.103:2376 (boot2docker/Windows) and a valid DOCKER_CERT_PATH
• Docker daemon on unix:///var/run/docker.sock (Linux)

All variants (HTTP, HTTPS, Unix socket) are working on my machines with the branch mentioned above.
Would any of you participants like to test on your machines, too?

@alexkamp would you mind pulling my changes or would you prefer @marcuslinke to merge my branch?

@gesellix Great! I will close this PR in favor of #125