Implement missing network api options for v1.22 by fbuecklers · Pull Request #484 · docker-java/docker-java

fbuecklers · 2016-02-27T12:44:41Z

This pull request is based on your wip pull request #469
I needed the additional networks options, while we are migrating to docker 1.10
I would like to give the outcome of my research back to you, since the complicated part was that docker handle links differently for the default network and user defined networks.
The most interesting part is in the CreateContainerCmdImpl#exec() method.
I do not know if it is the best place to put this specific code there. If i can help you with further tasks, please let me now it.

codecov-io · 2016-02-27T13:03:50Z

Current coverage is 21.37%

Merging #484 into master will decrease coverage by -2.45%

2 files (not in diff) in ...erjava/netty/handler were modified. more
- Misses +30
- Partials -2
- Hits -29
2 files (not in diff) in ...ockerjava/netty/exec were modified. more
- Misses -2
1 files (not in diff) in ...hub/dockerjava/netty were modified. more
- Misses -1
- Hits -1
4 files (not in diff) in ...hub/dockerjava/jaxrs were modified. more
- Misses -4
2 files (not in diff) in ...kerjava/core/command were modified. more
- Misses -4
4 files (not in diff) in ...thub/dockerjava/core were modified. more
- Misses +10
- Partials +1
- Hits -14
3 files (not in diff) in ...dockerjava/api/model were modified. more
- Misses -8
- Hits -52
1 files (not in diff) in ...ithub/dockerjava/api were modified. more
- Misses +12
- Hits -50
2 files (not in diff) in ...ckerjava/api/command were deleted. more
1 files (not in diff) in ...ithub/dockerjava/api were deleted. more

@@             master    #484   diff @@
=======================================
  Files           296     290     -6   
  Lines          6125    6017   -108   
  Methods           0       0          
  Branches        532     535     +3   
=======================================
- Hits           1459    1286   -173   
- Misses         4581    4648    +67   
+ Partials         85      83     -2

Powered by Codecov. Last updated by 9837534...95cc574

fbuecklers · 2016-02-28T19:03:32Z

Uhh, used the wrong object... I have fixed that!

KostyaSha · 2016-02-28T19:23:16Z

I will review and continue 1.22 sync after i resolve and merge my PR.

KostyaSha · 2016-03-04T20:18:55Z

please rebase

fbuecklers · 2016-03-04T21:05:53Z

I have rebased onto the master. Tested against our cluster and still works for me ;-)

marcuslinke · 2016-03-04T21:28:44Z

src/main/java/com/github/dockerjava/core/command/CreateContainerCmdImpl.java

     */
    @Override
    public CreateContainerResponse exec() throws NotFoundException, ConflictException {
+        //code flow taken from https://github.com/docker/docker/blob/master/runconfig/opts/parse.go


I doubt that this logic should be part of the command. Commands in general are just dumb wrappers of the appropriate remote API resource (here: https://docs.docker.com/engine/reference/api/docker_remote_api_v1.22/#create-a-container). They should only take arguments/parameters that are needed to execute the request and nothing else.

Had no time to end review, but i also against of extra logic. In general would be glad to see models and porcelain API separation in future.

So what is your suggestion to handle this properly? The problem is, that links must be set on different objects, whenever the link is made on the old legacy network or on a new user defined network.
I'm also not very happy with this lines of code, but i do not have any better idea up to now

Set them from client side? The idea of exec() and CMD is to do the call just by sending defined objects.

You mean, that the user must know this kind of internal logic docker requires?
It is bad design by dockers remote api, because it is not abstracted by the api.
It tooks me really a lot of time to figure out this issue. And it is not documented anywhere. The docker cli takes the same --link argument for new and old networks and handle it in the client.

So what is your suggestion? Completely remove the CreateContainerCmd#withLinks method and expose the HostConfig and ContainerNetwork directly? And how you would like to expose the ContainerNetwork? CreateContainerCmd#withContainerNetwork() will not work if i follow your guideline, since it must be set on an Map under a key which has the same name as the user defined network. In addition using any other keys on this Map has no effect.

The Remote API is really not developer friendly and it is not understandable right form the API when to use HostConfig#withLinks or ContainerNetwork#withLinks. And since network API is the great new feature of docker it should get a good usable API.

So how to continue? It will be also okay for me, if we expose the two objects directly and remove the CreateContainerCmd#withLink and other HostConfig delegating methods. I can make this changes in this pull requests, if you like?

Maybe place additional methods in CMD, then ExecCmd wouldn't be filled with logic that would never allow set plain model data?

The main problem is, that the CMD is a builder and therefore should be call independent.
Let me explain the main issue i have by implementing the logic directly in the CreateContainerCmd#withLinks method.

//will correctly create links on myNet client.createContainerCmd(...) .withNetworkMode("myNet") .withLinks(...) //will faulty create links on the legacy bridge client.createContainerCmd(...) .withLinks(...) .withNetworkMode("myNet")

The issue is that the decision can't be made in the #withLinks method, because we do not know which NetworkMode is finally used.

Maybe create additional cmd method that will "update references" between objects after they was set?

KostyaSha · 2016-03-08T11:14:45Z

Could you implement tests?

marcuslinke · 2016-03-29T06:15:26Z

@fbuecklers If you could provide some integration test I'm sure we will find a way to integrate your PR.

add missing network options add tests

fbuecklers · 2016-05-01T19:08:49Z

Hi, sorry for my long delay. I have add integration tests which tests the new flags and options
Hopes that it looks good for you now and you can integrate my pull requests :-)

KostyaSha · 2016-06-02T19:56:44Z

src/main/java/com/github/dockerjava/api/model/Network.java


        @JsonProperty("Config")
-        List<Config> config = new ArrayList<>();
+        private List<Config> config = new ArrayList<>();


why it need to be initialized?

it was before, nevermind

Yeah, thanks!

fbuecklers force-pushed the more-network-options branch from be71e31 to 9b37a00 Compare February 27, 2016 12:59

fbuecklers force-pushed the more-network-options branch from 9b37a00 to 14dec7a Compare February 28, 2016 19:00

marcuslinke mentioned this pull request Mar 2, 2016

Support remote API 1.22 subset #488

Merged

fbuecklers force-pushed the more-network-options branch from 14dec7a to e3c4675 Compare March 4, 2016 21:03

fbuecklers force-pushed the more-network-options branch from e3c4675 to 01ad30b Compare March 4, 2016 21:20

marcuslinke reviewed Mar 4, 2016
View reviewed changes

KostyaSha added the API/1.22 label Mar 20, 2016

implement links and aliases for user defined networks

4d60550

add missing network options add tests

fbuecklers force-pushed the more-network-options branch from 01ad30b to 4d60550 Compare May 1, 2016 19:05

cirocosta added a commit to cirocosta/docker-java that referenced this pull request May 13, 2016

rebase fixes to make docker-java#484 work w/ master

a66e663

marcuslinke merged commit 4d60550 into docker-java:master Jun 2, 2016

KostyaSha reviewed Jun 2, 2016
View reviewed changes

rnorth mentioned this pull request Jun 5, 2016

[WIP] Docker compose refactor testcontainers/testcontainers-java#156

Closed

fbuecklers deleted the more-network-options branch October 5, 2017 19:49

Conversation

fbuecklers commented Feb 27, 2016

Uh oh!

codecov-io commented Feb 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 21.37%

Uh oh!

fbuecklers commented Feb 28, 2016

Uh oh!

KostyaSha commented Feb 28, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Uh oh!

fbuecklers commented Mar 4, 2016

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!

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!

KostyaSha commented Mar 8, 2016

Uh oh!

marcuslinke commented Mar 29, 2016

Uh oh!

fbuecklers commented May 1, 2016

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Feb 27, 2016 •

edited

Loading