> On Feb. 9, 2016, 7:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 307
> > <https://reviews.apache.org/r/43093/diff/3/?file=1233160#file1233160line307>
> >
> >     Are these additional checks which should apply in both cases (i.e. 
> > deprecated and new `addressLocation`? I.e. prior to your change it did not 
> > return an error if `HostConfig.NetworkMode` was not set, or am I mistaken?
> >     Feel free to drop if this is the intended behavior!
> 
> haosdent huang wrote:
>     `HostConfig.NetworkMode` exists since api 
> [1.15](https://docs.docker.com/engine/reference/api/docker_remote_api_v1.15/) 
> I think it's better we use the old way to get ip first. If we get the ip by 
> old way failed, try use `HostConfig.NetworkMode` and the new way to get ip 
> again.
> 
> Guangya Liu wrote:
>     +1 to haosdent, we can first use old way to get IP address then if old 
> way failed, use the new way to get IP address.
> 
> Kapil Arya wrote:
>     I think it's better to first attempt the new API, since that's forward 
> looking. If that fails, then we can go for the deprecated route.
> 
> Joerg Schad wrote:
>     I wasn't so much concerned about the order of old vs new. The additional 
> check for `HostConfig.NetworkMode` just made me curios. Imagine a scenario 
> using the old way via `NetworkSettings.IPAddress` but not setting 
> `HostConfig.NetworkMode`: After this patch this will fail with `Unable to 
> detect network mode.`. Is that a scenario which might break currently working 
> setups?
> 
> haosdent huang wrote:
>     >Is that a scenario which might break currently working setups
>     
>     Yes, `HostConfig.NetworkMode` need make sure docker version >=1.3.x.

I don't believe that using the old way first is the correct route to go because 
that field still exists in the new docker API, it is just left blank when using 
a custom network driver. I can't base it off of it's existence, and I can't 
base it off of whether or not the field is blank, because blank might be an 
expected scenario if the container was started without networking.

I agree with Kapil that we should check the new API first, but Haosdent has a 
valid point that the check of HostConfig.NetworkMode could cause a failure 
scenario with older docker versions. I will work on handling that error 
condition in a more graceful way.


> On Feb. 9, 2016, 7:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 319
> > <https://reviews.apache.org/r/43093/diff/3/?file=1233160#file1233160line319>
> >
> >     Any reason you have a blank line here?

Likely missed it, or I'm not accustomed to the mesos style yet. I will take 
care of this.


- Travis


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43093/#review118436
-----------------------------------------------------------


On Feb. 4, 2016, 9:27 p.m., Travis Hegner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2016, 9:27 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Kapil Arya.
> 
> 
> Bugs: MESOS-4370
>     https://issues.apache.org/jira/browse/MESOS-4370
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fixes [MESOS-4370]
> 
> 
> Diffs
> -----
> 
>   src/docker/docker.cpp b4b8d3e 
> 
> Diff: https://reviews.apache.org/r/43093/diff/
> 
> 
> Testing
> -------
> 
> This patch will first query the docker API for the HostConfig.NetworkMode, 
> which is populated with the network name. (Essentially what was passed in 
> --net <name> to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks.<name>.IPAddress to get the IP address that is 
> currently in use by the container.
> 
> It appears that even though the docker API has been set up to allow for 
> multiple networks, our testing has indicated that it's still only applying 
> one network to the container (the last one via the --net argument on the run 
> line). I can only speculate that the docker API will change again in the near 
> future, but I can't speculate how, so at least this fixes the problem as it 
> stands right now.
> 
> Tested and working with Docker 1.9.1, Mesos 0.27.0, on Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Travis Hegner
> 
>

Reply via email to