mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Akash Gupta <akash-gu...@hotmail.com>
Subject Re: Review Request 63860: Windows: Mapped the Docker network info types.
Date Wed, 10 Jan 2018 22:28:15 GMT


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


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