> On Jan. 10, 2018, 12:30 p.m., Alexander Rukletsov wrote: > > src/docker/docker.cpp > > Lines 744-749 (patched) > > <https://reviews.apache.org/r/63860/diff/8/?file=1937999#file1937999line745> > > > > Does it make sense to also remove the default here? > > https://github.com/apache/mesos/blob/51a3bd95bd2d740a39b55634251abeadb561e5c8/include/mesos/mesos.proto#L3120
I'm not sure what the implications of changing the public API is, but yeah, the default value in the protobuf is unnecessary now. > On Jan. 10, 2018, 12:30 p.m., Alexander Rukletsov wrote: > > src/docker/docker.cpp > > Lines 753 (patched) > > <https://reviews.apache.org/r/63860/diff/8/?file=1937999#file1937999line754> > > > > I don't see a list of supported modes on Linux and Windows there. The Microsoft link has the Windows network modes and the docker link has the Linux network modes. > On Jan. 10, 2018, 12:30 p.m., Alexander Rukletsov wrote: > > src/docker/docker.cpp > > Lines 756-779 (patched) > > <https://reviews.apache.org/r/63860/diff/8/?file=1937999#file1937999line757> > > > > I prefer to have preprocessor blocks as small as possible, hence I'd > > suggest the following for your consideration: > > ``` > > case ContainerInfo::DockerInfo::HOST: { > > #ifdef __WINDOWS__ > > return Error("Unsupported Network mode: " + stringify(network)); > > #else > > options.network = "host"; > > #endif // __WINDOWS__ > > break; > > } > > case ContainerInfo::DockerInfo::BRIDGE: { > > #ifdef __WINDOWS__ > > // Windows "nat" network mode is equivalent to Linux "bridge" > > mode. > > options.network = "nat"; > > #else > > options.network = "bridge"; > > #endif // __WINDOWS__ > > break; > > } > > case ContainerInfo::DockerInfo::NONE: { > > options.network = "none"; > > break; > > } > > ``` Will fix. > On Jan. 10, 2018, 12:30 p.m., Alexander Rukletsov wrote: > > src/docker/docker.cpp > > Lines 758-759 (patched) > > <https://reviews.apache.org/r/63860/diff/8/?file=1937999#file1937999line759> > > > > Fits one line. Will fix. - Akash ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63860/#review195130 ----------------------------------------------------------- On Jan. 10, 2018, 1:23 a.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63860/ > ----------------------------------------------------------- > > (Updated Jan. 10, 2018, 1:23 a.m.) > > > Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston > Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park. > > > Bugs: MESOS-7342 > https://issues.apache.org/jira/browse/MESOS-7342 > > > Repository: mesos > > > Description > ------- > > The Network enum in DockerInfo is specific to Linux containers. `HOST` > doesn't exist on Windows and `BRIDGE` is `NAT` on Windows. The current > default docker network setting was always `HOST`, which broke the > Windows docker executor. Now, if a specific network isn't given, the > network mode will default to `HOST` on Linux agents and `NAT` on Windows > agents. Also, `BRIDGE` mode will be translated to `NAT` on Windows. > > > Diffs > ----- > > src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed > > > Diff: https://reviews.apache.org/r/63860/diff/8/ > > > Testing > ------- > > See https://reviews.apache.org/r/63862/ for test results. > > > Thanks, > > Akash Gupta > >
