mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Travis Hegner <travis.heg...@gmail.com>
Subject Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker
Date Tue, 08 Mar 2016 14:10:34 GMT


> 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.
> 
> Travis Hegner wrote:
>     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.

This issue is now handled by validating the docker version. If the version is greater than
1.9.0, the new API is used, otherwise the old one is used. In this case, returning an error
if network mode is not detected is valid, as the network mode is required to determine the
IP address.


- Travis


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


On Feb. 17, 2016, 10:52 p.m., Travis Hegner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2016, 10:52 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Kapil Arya, and Timothy Chen.
> 
> 
> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message