mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 63860: Windows: Mapped the Docker network info types.
Date Wed, 10 Jan 2018 12:30:03 GMT

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




src/docker/docker.cpp
Lines 744-749 (patched)
<https://reviews.apache.org/r/63860/#comment274269>

    Does it make sense to also remove the default here? https://github.com/apache/mesos/blob/51a3bd95bd2d740a39b55634251abeadb561e5c8/include/mesos/mesos.proto#L3120



src/docker/docker.cpp
Lines 753 (patched)
<https://reviews.apache.org/r/63860/#comment274270>

    I don't see a list of supported modes on Linux and Windows there.



src/docker/docker.cpp
Lines 756-779 (patched)
<https://reviews.apache.org/r/63860/#comment274272>

    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;
        }
    ```



src/docker/docker.cpp
Lines 758-759 (patched)
<https://reviews.apache.org/r/63860/#comment274271>

    Fits one line.


- Alexander Rukletsov


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