Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

> On March 9, 2016, 9:03 p.m., Ezra Silvera wrote:
> > src/docker/docker.cpp, lines 304-305
> > 
> >
> > Please note that a container can be attached to more then one network! 
> > In that case HostConfig.NetworkMode will contain the first NW the container 
> > was attached two. The desired IP, however, might be the one on a different 
> > NW in the NetworkSettings.Networks array.

FYI, the work for the issue that this patch originally addressed are now being 
done in https://reviews.apache.org/r/44531/.

In what way (other than the docker overlay network, as mesos doesn't support 
user defined networks yet) are you able to get a container to connect to more 
than one network? In my experience, docker run has always clobbered the former 
"net" parameter names with the latter ones. I don't have a way to test the 
overlay network at the moment, but would be happy to analyze the inspect of a 
container you are saying will fail with the linked patch.


- Travis


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


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Ezra Silvera]

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




src/docker/docker.cpp (lines 304 - 305)


Please note that a container can be attached to more then one network! In 
that case HostConfig.NetworkMode will contain the first NW the container was 
attached two. The desired IP, however, might be the one on a different NW in 
the NetworkSettings.Networks array.


- Ezra Silvera


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Ezra Silvera]

> On March 9, 2016, 8:48 p.m., Ezra Silvera wrote:
> > Please note that a container can be attached to more then one network! In 
> > that case HostConfig.NetworkMode will contain the first NW the container 
> > was attached two. The desired IP, however, might be the one on a different 
> > NW in the NetworkSettings.Networks array.

Please ignore this. I opened a specific issue instead.


- Ezra


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


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Ezra Silvera]

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



Please note that a container can be attached to more then one network! In that 
case HostConfig.NetworkMode will contain the first NW the container was 
attached two. The desired IP, however, might be the one on a different NW in 
the NetworkSettings.Networks array.

- Ezra Silvera


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

> On March 7, 2016, 5:36 p.m., Timothy Chen wrote:
> > Are you still be able to work on this? We like to get this merged, so if 
> > you can't or don't reply we will create a new patch based on this.
> 
> Travis Hegner wrote:
> Hi Timothy,
> 
> I've been stalling this for https://reviews.apache.org/r/42516, as that 
> patch will change the way this patch should be written. It only works now out 
> of pure luck in the way docker intereprets multiple "--net" parameters, since 
> mesos doesn't yet officially support user defined networks.
> 
> The linked patch will make this patch support it properly, as well as 
> make this patch easier to write, as the network name will not have to be 
> queried at all from json; it will be available as a variable.
> 
> Timothy Chen wrote:
> I don't quite understand the pure luck part, IIUC your patch is updating 
> how we parse the Docker inspect output for Network IPAddress, which shouldn't 
> matter how we actually launched the container right? How is the linked patch 
> related?
> 
> Timothy Chen wrote:
> At this moment this is actually like becomes a blocker for 0.28 as we're 
> not returning the right IP address with the latest Docker daemon, so I'd like 
> to get this in ASAP. For now I'll create a new patch based on your changes, 
> and once the dependend patch goes in will love if you can continue the work.
> 
> Travis Hegner wrote:
> I think I understand the miscommunication better now. As you know, 
> beginning with docker 1.9 the docker inspect output changed the location of 
> the IPAddress. The original location was still populated for backwards 
> compatability, but only for the common "bridge" and "host" network types. 
> Mesos is written to fail with any other network type. With the new user 
> defined networks feature, the old location was not populated. My patch was 
> originally intended to address the fact that with user defined networks, the 
> original ip location was null.
> 
> In order to utilize user defined networks in my environment, we are 
> passing arbitraty docker parameters to mesos with the docker containerizer 
> from marathon. This results in multiple "--net" parameters passed to docker. 
> The luck comes into play because mesos interperets the first --net parameter 
> of "bridge" and succeeds, and docker interperets the second --net parameter 
> of my UDN, and connects to the right network. I would consider this behavior 
> unstable at best.
> 
> Based on the sudden up-tick in interest in this patch, I am speculating 
> that docker 1.10 is no longer populating the original ip address field (I 
> would be un-aware, because I've been running my cluster with this patch), 
> which this patch will successfully fix, and even be stable for the typical 
> "host" and "bridge" networks.
> 
> All that said, I can see why this patch is now more important, even 
> though it should be re-structured after review 42516 is implemented. I'll see 
> if I can spend some time today and address the remaining issues with this 
> patch.
> 
> Timothy Chen wrote:
> Yes IP Address is now being populated in a different field now, so 
> basically the patch has to handle two changes in the API, one is the addition 
> of network mode and another is the change of IPAddress.
> 
> I'm not sure your availability is so I'm having a patch similiar to 
> yours. Sorry for this but we like to see this get into 0.28.0.

I've been working this, and I believe the appropriate way to fix it is by 
checking the docker version, and behaving accordingly. The problem is that the 
validateVersion() function is not available within the 
Docker::Container::create() scope... (have a look at 
https://github.com/travishegner/mesos/blob/networkInfo/src/docker/docker.cpp#L305)

Any ideas on the best way to check the docker version within this scope? I 
didn't want to pass a reference in to the Docker object, as that requires 
changes to some of the tests as well.

Of course, if we don't want to maintain backwards compatability with older 
docker versions, then we can eliminate the need for the version check.

@Timothy, are you on IRC? We may be able to work together on this...


- Travis


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


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
> 
> 
> 

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Timothy Chen]

> On March 7, 2016, 5:36 p.m., Timothy Chen wrote:
> > Are you still be able to work on this? We like to get this merged, so if 
> > you can't or don't reply we will create a new patch based on this.
> 
> Travis Hegner wrote:
> Hi Timothy,
> 
> I've been stalling this for https://reviews.apache.org/r/42516, as that 
> patch will change the way this patch should be written. It only works now out 
> of pure luck in the way docker intereprets multiple "--net" parameters, since 
> mesos doesn't yet officially support user defined networks.
> 
> The linked patch will make this patch support it properly, as well as 
> make this patch easier to write, as the network name will not have to be 
> queried at all from json; it will be available as a variable.
> 
> Timothy Chen wrote:
> I don't quite understand the pure luck part, IIUC your patch is updating 
> how we parse the Docker inspect output for Network IPAddress, which shouldn't 
> matter how we actually launched the container right? How is the linked patch 
> related?
> 
> Timothy Chen wrote:
> At this moment this is actually like becomes a blocker for 0.28 as we're 
> not returning the right IP address with the latest Docker daemon, so I'd like 
> to get this in ASAP. For now I'll create a new patch based on your changes, 
> and once the dependend patch goes in will love if you can continue the work.
> 
> Travis Hegner wrote:
> I think I understand the miscommunication better now. As you know, 
> beginning with docker 1.9 the docker inspect output changed the location of 
> the IPAddress. The original location was still populated for backwards 
> compatability, but only for the common "bridge" and "host" network types. 
> Mesos is written to fail with any other network type. With the new user 
> defined networks feature, the old location was not populated. My patch was 
> originally intended to address the fact that with user defined networks, the 
> original ip location was null.
> 
> In order to utilize user defined networks in my environment, we are 
> passing arbitraty docker parameters to mesos with the docker containerizer 
> from marathon. This results in multiple "--net" parameters passed to docker. 
> The luck comes into play because mesos interperets the first --net parameter 
> of "bridge" and succeeds, and docker interperets the second --net parameter 
> of my UDN, and connects to the right network. I would consider this behavior 
> unstable at best.
> 
> Based on the sudden up-tick in interest in this patch, I am speculating 
> that docker 1.10 is no longer populating the original ip address field (I 
> would be un-aware, because I've been running my cluster with this patch), 
> which this patch will successfully fix, and even be stable for the typical 
> "host" and "bridge" networks.
> 
> All that said, I can see why this patch is now more important, even 
> though it should be re-structured after review 42516 is implemented. I'll see 
> if I can spend some time today and address the remaining issues with this 
> patch.

Yes IP Address is now being populated in a different field now, so basically 
the patch has to handle two changes in the API, one is the addition of network 
mode and another is the change of IPAddress.

I'm not sure your availability is so I'm having a patch similiar to yours. 
Sorry for this but we like to see this get into 0.28.0.


- Timothy


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


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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 

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

> On Feb. 9, 2016, 7:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 307
> > 
> >
> > 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

> On March 7, 2016, 5:36 p.m., Timothy Chen wrote:
> > Are you still be able to work on this? We like to get this merged, so if 
> > you can't or don't reply we will create a new patch based on this.
> 
> Travis Hegner wrote:
> Hi Timothy,
> 
> I've been stalling this for https://reviews.apache.org/r/42516, as that 
> patch will change the way this patch should be written. It only works now out 
> of pure luck in the way docker intereprets multiple "--net" parameters, since 
> mesos doesn't yet officially support user defined networks.
> 
> The linked patch will make this patch support it properly, as well as 
> make this patch easier to write, as the network name will not have to be 
> queried at all from json; it will be available as a variable.
> 
> Timothy Chen wrote:
> I don't quite understand the pure luck part, IIUC your patch is updating 
> how we parse the Docker inspect output for Network IPAddress, which shouldn't 
> matter how we actually launched the container right? How is the linked patch 
> related?
> 
> Timothy Chen wrote:
> At this moment this is actually like becomes a blocker for 0.28 as we're 
> not returning the right IP address with the latest Docker daemon, so I'd like 
> to get this in ASAP. For now I'll create a new patch based on your changes, 
> and once the dependend patch goes in will love if you can continue the work.

I think I understand the miscommunication better now. As you know, beginning 
with docker 1.9 the docker inspect output changed the location of the 
IPAddress. The original location was still populated for backwards 
compatability, but only for the common "bridge" and "host" network types. Mesos 
is written to fail with any other network type. With the new user defined 
networks feature, the old location was not populated. My patch was originally 
intended to address the fact that with user defined networks, the original ip 
location was null.

In order to utilize user defined networks in my environment, we are passing 
arbitraty docker parameters to mesos with the docker containerizer from 
marathon. This results in multiple "--net" parameters passed to docker. The 
luck comes into play because mesos interperets the first --net parameter of 
"bridge" and succeeds, and docker interperets the second --net parameter of my 
UDN, and connects to the right network. I would consider this behavior unstable 
at best.

Based on the sudden up-tick in interest in this patch, I am speculating that 
docker 1.10 is no longer populating the original ip address field (I would be 
un-aware, because I've been running my cluster with this patch), which this 
patch will successfully fix, and even be stable for the typical "host" and 
"bridge" networks.

All that said, I can see why this patch is now more important, even though it 
should be re-structured after review 42516 is implemented. I'll see if I can 
spend some time today and address the remaining issues with this patch.


- Travis


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


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Timothy Chen]

> On March 7, 2016, 5:36 p.m., Timothy Chen wrote:
> > Are you still be able to work on this? We like to get this merged, so if 
> > you can't or don't reply we will create a new patch based on this.
> 
> Travis Hegner wrote:
> Hi Timothy,
> 
> I've been stalling this for https://reviews.apache.org/r/42516, as that 
> patch will change the way this patch should be written. It only works now out 
> of pure luck in the way docker intereprets multiple "--net" parameters, since 
> mesos doesn't yet officially support user defined networks.
> 
> The linked patch will make this patch support it properly, as well as 
> make this patch easier to write, as the network name will not have to be 
> queried at all from json; it will be available as a variable.
> 
> Timothy Chen wrote:
> I don't quite understand the pure luck part, IIUC your patch is updating 
> how we parse the Docker inspect output for Network IPAddress, which shouldn't 
> matter how we actually launched the container right? How is the linked patch 
> related?

At this moment this is actually like becomes a blocker for 0.28 as we're not 
returning the right IP address with the latest Docker daemon, so I'd like to 
get this in ASAP. For now I'll create a new patch based on your changes, and 
once the dependend patch goes in will love if you can continue the work.


- Timothy


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


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Timothy Chen]

> On March 7, 2016, 5:36 p.m., Timothy Chen wrote:
> > Are you still be able to work on this? We like to get this merged, so if 
> > you can't or don't reply we will create a new patch based on this.
> 
> Travis Hegner wrote:
> Hi Timothy,
> 
> I've been stalling this for https://reviews.apache.org/r/42516, as that 
> patch will change the way this patch should be written. It only works now out 
> of pure luck in the way docker intereprets multiple "--net" parameters, since 
> mesos doesn't yet officially support user defined networks.
> 
> The linked patch will make this patch support it properly, as well as 
> make this patch easier to write, as the network name will not have to be 
> queried at all from json; it will be available as a variable.

I don't quite understand the pure luck part, IIUC your patch is updating how we 
parse the Docker inspect output for Network IPAddress, which shouldn't matter 
how we actually launched the container right? How is the linked patch related?


- Timothy


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


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

> On March 7, 2016, 5:36 p.m., Timothy Chen wrote:
> > Are you still be able to work on this? We like to get this merged, so if 
> > you can't or don't reply we will create a new patch based on this.

Hi Timothy,

I've been stalling this for https://reviews.apache.org/r/42516, as that patch 
will change the way this patch should be written. It only works now out of pure 
luck in the way docker intereprets multiple "--net" parameters, since mesos 
doesn't yet officially support user defined networks.

The linked patch will make this patch support it properly, as well as make this 
patch easier to write, as the network name will not have to be queried at all 
from json; it will be available as a variable.


- Travis


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


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Timothy Chen]

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



Are you still be able to work on this? We like to get this merged, so if you 
can't or don't reply we will create a new patch based on this.

- Timothy Chen


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Timothy Chen]

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




src/docker/docker.cpp (line 310)


This can fit in 80 char width line right?



src/docker/docker.cpp (line 314)


Also fix the formatting for the strings concat. Move 
networkModeValue.get().value aligned with the "NetworkSettings"


- Timothy Chen


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Guangya Liu]

On 二月 9, 2016, 7:13 p.m., Travis Hegner wrote:
> > hanks
> 
> Joerg Schad wrote:
> Oh btw regarding the commit message:
> We usually have commit messages stating what changed, so in your case it 
> could be something along the lines of 'Added support for new docker network 
> setting.'
> 
> Travis Hegner wrote:
> Understood. I will take care of both of these.
> 
> Travis Hegner wrote:
> A couple of quick questions if I may:
> 
> 1. Would it be possible or advisable to refactor this code based off of a 
> detected docker version? I saw some tests with regard to the docker version, 
> but I'm not familiar enough with the project to know if that is a route to 
> go. Essentially, if docker version > 1.9.1 use new API otherwise use the old 
> one? Perhaps there is a detection for the actual API version as well?
> 2. How would one test the various execution paths? Would I have to supply 
> a sample inspect output for the various versions as part of the test, and 
> then validate how the code handles each of the different version samples? I 
> don't know how else one would test each possible condition. I did see that 
> some tests supplied a sample json blob to test against... would that be 
> advisable in this case?
> 3. Should I be basing this patch off of the master branch, or the latest 
> release? If it makes a difference, I am hopeful to get this patch into 
> 0.27.1, if it's not already too late for that.

For 1), I think that you can take a look at 
https://reviews.apache.org/r/42516/diff/11#2 for how to handle different 
versions. Yes, checking version may be better, if version >= 1.9.1, use new 
way; otherwise, use old way.

For 2) You may want to supply a sample inspect output for the various versions 
as part of the test and then validate the code.


- Guangya


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


On 二月 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 二月 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

On Feb. 9, 2016, 7:13 p.m., Travis Hegner wrote:
> > hanks
> 
> Joerg Schad wrote:
> Oh btw regarding the commit message:
> We usually have commit messages stating what changed, so in your case it 
> could be something along the lines of 'Added support for new docker network 
> setting.'

Understood. I will take care of both of these.


- 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

> On Feb. 9, 2016, 7:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 307
> > 
> >
> > 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
> > 
> >
> > 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

On Feb. 9, 2016, 7:13 p.m., Travis Hegner wrote:
> > hanks
> 
> Joerg Schad wrote:
> Oh btw regarding the commit message:
> We usually have commit messages stating what changed, so in your case it 
> could be something along the lines of 'Added support for new docker network 
> setting.'
> 
> Travis Hegner wrote:
> Understood. I will take care of both of these.

A couple of quick questions if I may:

1. Would it be possible or advisable to refactor this code based off of a 
detected docker version? I saw some tests with regard to the docker version, 
but I'm not familiar enough with the project to know if that is a route to go. 
Essentially, if docker version > 1.9.1 use new API otherwise use the old one? 
Perhaps there is a detection for the actual API version as well?
2. How would one test the various execution paths? Would I have to supply a 
sample inspect output for the various versions as part of the test, and then 
validate how the code handles each of the different version samples? I don't 
know how else one would test each possible condition. I did see that some tests 
supplied a sample json blob to test against... would that be advisable in this 
case?
3. Should I be basing this patch off of the master branch, or the latest 
release? If it makes a difference, I am hopeful to get this patch into 0.27.1, 
if it's not already too late for that.


- 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Joerg Schad]

> On Feb. 9, 2016, 7:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 307
> > 
> >
> > 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.

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?


- Joerg


---
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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Kapil Arya]

> On Feb. 9, 2016, 2:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 307
> > 
> >
> > 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.

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.


- Kapil


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


On Feb. 4, 2016, 4: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, 4: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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[haosdent huang]

> On Feb. 9, 2016, 7:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 307
> > 
> >
> > 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?

>Is that a scenario which might break currently working setups

Yes, `HostConfig.NetworkMode` need make sure docker version >=1.3.x.


- haosdent


---
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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Joerg Schad]

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



Thanks for taking this on! Some small remarks below.
Overall would it make sense -given that the API is ...unstable- to add some 
tests here?


src/docker/docker.cpp (line 307)


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!



src/docker/docker.cpp (line 319)


Any reason you have a blank line here?



src/docker/docker.cpp (line 322)


We don't end log messages with a period.


hanks

- Joerg Schad


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Joerg Schad]

On Feb. 9, 2016, 7:13 p.m., Travis Hegner wrote:
> > hanks

Oh btw regarding the commit message:
We usually have commit messages stating what changed, so in your case it could 
be something along the lines of 'Added support for new docker network setting.'


- Joerg


---
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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[haosdent huang]

> On Feb. 9, 2016, 7:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 307
> > 
> >
> > 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!

`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.


- haosdent


---
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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Guangya Liu]

> On 二月 9, 2016, 7:13 p.m., Joerg Schad wrote:
> > src/docker/docker.cpp, line 307
> > 
> >
> > 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.

+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.


- Guangya


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


On 二月 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 二月 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

---
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.


Changes
---

Couple of build errors fixed.


Bugs: MESOS-4370
https://issues.apache.org/jira/browse/MESOS-4370


Repository: mesos


Description
---

Fixes [MESOS-4370]


Diffs (updated)
-

  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 
 to the docker run command). This name is then used as a key in 
NetworkSettings.Networks..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

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [43093]

Failed command: ./support/apply-review.sh -n -r 43093

Error:
2016-02-05 01:44:23 URL:https://reviews.apache.org/r/43093/diff/raw/ 
[2180/2180] -> "43093.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.

Full log: https://builds.apache.org/job/mesos-reviewbot/11254/console

- Mesos ReviewBot


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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[haosdent huang]

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




src/docker/docker.cpp (line 326)


If we could add some assert code about ip value in `TEST_F(DockerTest, 
ROOT_DOCKER_interface)` would make this patch looks better.

```
TEST_F(DockerTest, ROOT_DOCKER_interface)
...
  // Test some fields of the container.
  EXPECT_NE("", inspect.get().id);
  EXPECT_EQ("/" + containerName, inspect.get().name);
  EXPECT_SOME(inspect.get().pid);
...
```


- haosdent huang


On Feb. 2, 2016, 10:06 p.m., Travis Hegner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> ---
> 
> (Updated Feb. 2, 2016, 10:06 p.m.)
> 
> 
> Review request for mesos 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 a831726 
> 
> 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[haosdent huang]

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




src/docker/docker.cpp (line 316)


Seems use a variable to store the string `"NetworkSettings.Networks." + 
networkMode + ".IPAddress"` would be better. And that we could continue use 
that in the following code.



src/docker/docker.cpp (line 321)


Should we try got `"NetworkSettings.IPAddress"` to compatiable old api if 
could not the by new api spec.


- haosdent huang


On Feb. 2, 2016, 10:06 p.m., Travis Hegner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> ---
> 
> (Updated Feb. 2, 2016, 10:06 p.m.)
> 
> 
> Review request for mesos 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 a831726 
> 
> 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [43093]

Failed command: ./support/apply-review.sh -n -r 43093

Error:
2016-02-03 19:35:52 URL:https://reviews.apache.org/r/43093/diff/raw/ 
[2180/2180] -> "43093.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.

Full log: https://builds.apache.org/job/mesos-reviewbot/11220/console

- Mesos ReviewBot


On Feb. 3, 2016, 7:04 p.m., Travis Hegner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> ---
> 
> (Updated Feb. 3, 2016, 7:04 p.m.)
> 
> 
> Review request for mesos 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 a831726 
> 
> 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

> On Feb. 3, 2016, 8:47 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 304
> > 
> >
> > According 
> > https://docs.docker.com/engine/reference/api/docker_remote_api/ Seems this 
> > would have problem in old docker version which don't support 1.21 remote 
> > api.

I had a similar thought after writing the patch, and then never implemented it. 
Good Catch.


- Travis


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


On Feb. 3, 2016, 7:04 p.m., Travis Hegner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> ---
> 
> (Updated Feb. 3, 2016, 7:04 p.m.)
> 
> 
> Review request for mesos 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 a831726 
> 
> 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

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

(Updated Feb. 3, 2016, 7:04 p.m.)


Review request for mesos and Kapil Arya.


Changes
---

Uploaded a new diff to address a couple of the issues.


Bugs: MESOS-4370
https://issues.apache.org/jira/browse/MESOS-4370


Repository: mesos


Description
---

Fixes [MESOS-4370]


Diffs (updated)
-

  src/docker/docker.cpp a831726 

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 
 to the docker run command). This name is then used as a key in 
NetworkSettings.Networks..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

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

> On Feb. 3, 2016, 8:58 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 326
> > 
> >
> > If we could add some assert code about ip value in `TEST_F(DockerTest, 
> > ROOT_DOCKER_interface)` would make this patch looks better.
> > 
> > ```
> > TEST_F(DockerTest, ROOT_DOCKER_interface)
> > ...
> >   // Test some fields of the container.
> >   EXPECT_NE("", inspect.get().id);
> >   EXPECT_EQ("/" + containerName, inspect.get().name);
> >   EXPECT_SOME(inspect.get().pid);
> > ...
> > ```

I am unsure of what you are looking for in this case. I can't really assert any 
of the fields related to this patch, as they will have different values or not 
exist depending on the docker API version being queried.

With the new API, "NetworkSettings.IPAddress" is expected to be empty (perhaps 
not exist in the future), but not with the old API. Similarly, with the old API 
"NetworkSettings.Networks..IPAddress" will never exist, but 
exists in the new API.

Any testing of these fields would be mutually exclusive.


- Travis


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


On Feb. 3, 2016, 7:04 p.m., Travis Hegner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> ---
> 
> (Updated Feb. 3, 2016, 7:04 p.m.)
> 
> 
> Review request for mesos 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 a831726 
> 
> 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

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

(Updated Feb. 2, 2016, 6:52 p.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Fixes [MESOS-4370]


Diffs
-

  src/docker/docker.cpp a831726 

Diff: https://reviews.apache.org/r/43093/diff/


Testing (updated)
---

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 
 to the docker run command). This name is then used as a key in 
NetworkSettings.Networks..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

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [43093]

Failed command: ./support/apply-review.sh -n -r 43093

Error:
2016-02-02 20:06:11 URL:https://reviews.apache.org/r/43093/diff/raw/ 
[1765/1765] -> "43093.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.

Full log: https://builds.apache.org/job/mesos-reviewbot/11200/console

- Mesos ReviewBot


On Feb. 2, 2016, 6:52 p.m., Travis Hegner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> ---
> 
> (Updated Feb. 2, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixes [MESOS-4370]
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp a831726 
> 
> 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  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..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
> 
>

Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

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

Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Fixes [MESOS-4370]


Diffs
-

  src/docker/docker.cpp a831726 

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 
 to the docker run command). This name is then used as a key in 
NetworkSettings.Networks..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

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

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

(Updated Feb. 2, 2016, 5:06 p.m.)


Review request for mesos 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 a831726 

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 
 to the docker run command). This name is then used as a key in 
NetworkSettings.Networks..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