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. > > Guangya Liu wrote: > 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. > > Travis Hegner wrote: > I'm glad you sent that link. The progress for supporting user defined > networks will change this patch completely. We are using user defined > networks currently, and that is the point of this patch that I'm working on. > Your link just helped me to realize that mesos still believes that our > containers are running in bridged mode, while docker believes we are using > user defined networks. > > Basically, we are using the "parameters" field in marathon to set up our > custom "--net" parameter, even though marathon also passes "--net=bridge". > Mesos seems to be honoring the first "--net" parameter, while docker honors > the second. > > I should really wait for that patch, or work with that author to ensure > it will work for our environment as well.
Please note that https://reviews.apache.org/r/43032/ may introduce docker version and save it in the Docker class, if that finished, then this patch can just leverage the docker version directly. - 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 <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 > >
