Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Jan. 17, 2018, 8:44 p.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park.


Changes
---

fix nolint


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 (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/10/

Changes: https://reviews.apache.org/r/63860/diff/9-10/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Andrew Schwartzmeyer]

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


Fix it, then Ship it!





src/docker/docker.cpp
Lines 752 (patched)


Nit: You want `// NOLINT(whitespace/line_length)` here.


- Andrew Schwartzmeyer


On Jan. 12, 2018, 6:34 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Jan. 12, 2018, 6:34 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/9/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Jan. 12, 2018, 2:34 p.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park.


Changes
---

Address Alex's feedback


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 (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/9/

Changes: https://reviews.apache.org/r/63860/diff/8-9/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

> On Jan. 10, 2018, 12:30 p.m., Alexander Rukletsov wrote:
> > src/docker/docker.cpp
> > Lines 744-749 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Alexander Rukletsov]

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


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)


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



src/docker/docker.cpp
Lines 756-779 (patched)


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)


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

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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


Changes
---

Addressed gaston's feedback


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 (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/8/

Changes: https://reviews.apache.org/r/63860/diff/7-8/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Gaston Kleiman]

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


Fix it, then Ship it!





src/docker/docker.cpp
Lines 742-744 (original), 741-750 (patched)


Nit/subjective personal opinion: I think that avoiding negative logic in 
this case would make the block more readable::

```
  if (dockerInfo.has_network()) {
  network = dockerInfo.network();
  } else {
// If no network was given, then use the OS specific default.
#ifdef __WINDOWS__
network = ContainerInfo::DockerInfo::BRIDGE;
#else
network = ContainerInfo::DockerInfo::HOST;
#endif // __WINDOWS__
  }
```



src/docker/docker.cpp
Lines 758-759 (patched)


Fits in one line.


- Gaston Kleiman


On Jan. 5, 2018, 2:25 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Jan. 5, 2018, 2:25 p.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/7/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Jan. 5, 2018, 10:25 p.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park.


Changes
---

Updated with Jie's feedback


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 (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/7/

Changes: https://reviews.apache.org/r/63860/diff/6-7/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Jie Yu]

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


Fix it, then Ship it!





src/docker/docker.cpp
Line 741 (original), 747-754 (patched)


I'd suggest the following way. `DEFAULT_DOCKER_NETWORK` should be killed.

```
ContainerInfo::DockerInfo::Network network;
if (!dockerInfo.has_network()) {
#ifdef __WINDOWS__
  network = ContainerInfo::DockerInfo::Network::BRIDGE;
#else
  network = ContainerInfo::DockerInfo::Network::HOST;
#endif
} else {
  network = dockerInfo.network();
}

switch (network) {
  ...
}
```



src/docker/docker.cpp
Line 777 (original), 812 (patched)


I don't mention `nat` at all here because from API perspective, `nat` is 
not expose to the users. Users only knows about BRIDGE.

```
Port mappings are only supported for BRIDGE and USER network
```


- Jie Yu


On Jan. 5, 2018, 6:31 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Jan. 5, 2018, 6:31 p.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/6/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Jan. 5, 2018, 6:31 p.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park.


Changes
---

Updated with Jie's feedback.


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


Repository: mesos


Description (updated)
---

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 (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/6/

Changes: https://reviews.apache.org/r/63860/diff/5-6/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Jie Yu]

> On Jan. 4, 2018, 8:42 p.m., Jie Yu wrote:
> > I like the direction in the latest diff (platform specific defaults).
> > 
> > One question is: what's the difference between nat and bridge? are they the 
> > same? If yes, can we not have NAT in the API? In other words, BRIDGE on 
> > Windows will map to "nat".
> 
> Akash Gupta wrote:
> I don't remember if they are 100% the same, but they are very similar. We 
> can map BRIDGE to "nat" on Windows with a note in the doc that says that 
> BRIDGE="nat" on Windows.

Yes, please do that.


- Jie


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


On Jan. 5, 2018, 12:29 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Jan. 5, 2018, 12:29 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 current Network enum in DockerInfo is specific to Linux containers.
> Instead of supporting {host, bridge, none, user} networks, Windows
> docker supports {nat, none, user} networks. Now, the Network enum
> supports {host, bridge, nat, none, user}. If the network isn't
> specified, then the default is host on Linux and nat on Windows.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto c677a8be07d0ef209d42622ae32056d36e55ff78 
>   src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 
> 
> 
> Diff: https://reviews.apache.org/r/63860/diff/5/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Jan. 5, 2018, 12:29 a.m.)


Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
Kleiman, Jie Yu, John Kordich, Joseph Wu, and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

The current Network enum in DockerInfo is specific to Linux containers.
Instead of supporting {host, bridge, none, user} networks, Windows
docker supports {nat, none, user} networks. Now, the Network enum
supports {host, bridge, nat, none, user}. If the network isn't
specified, then the default is host on Linux and nat on Windows.


Diffs (updated)
-

  include/mesos/mesos.proto c677a8be07d0ef209d42622ae32056d36e55ff78 
  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/5/

Changes: https://reviews.apache.org/r/63860/diff/4-5/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

> On Jan. 4, 2018, 8:42 p.m., Jie Yu wrote:
> > I like the direction in the latest diff (platform specific defaults).
> > 
> > One question is: what's the difference between nat and bridge? are they the 
> > same? If yes, can we not have NAT in the API? In other words, BRIDGE on 
> > Windows will map to "nat".

I don't remember if they are 100% the same, but they are very similar. We can 
map BRIDGE to "nat" on Windows with a note in the doc that says that 
BRIDGE="nat" on Windows.


- Akash


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


On Jan. 4, 2018, 9:40 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Jan. 4, 2018, 9:40 p.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 current Network enum in DockerInfo is specific to Linux containers.
> Instead of supporting {host, bridge, none, user} networks, Windows
> docker supports {nat, none, user} networks. Now, the Network enum
> supports {host, bridge, nat, none, user}. If the network isn't
> specified, then the default is host on Linux and nat on Windows.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 839ddb1cb41471d36423a2fc149acf90b973d413 
>   src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 
> 
> 
> Diff: https://reviews.apache.org/r/63860/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Jie Yu]

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



I like the direction in the latest diff (platform specific defaults).

One question is: what's the difference between nat and bridge? are they the 
same? If yes, can we not have NAT in the API? In other words, BRIDGE on Windows 
will map to "nat".

- Jie Yu


On Dec. 7, 2017, 9:34 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Dec. 7, 2017, 9:34 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Jie Yu, John Kordich, Joseph 
> Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current Network enum in DockerInfo is specific to Linux containers.
> Instead of supporting {host, bridge, none, user} networks, Windows
> docker supports {nat, none, user} networks. Now, the Network enum
> supports {host, bridge, nat, none, user}. If the network isn't
> specified, then the default is host on Linux and nat on Windows.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 839ddb1cb41471d36423a2fc149acf90b973d413 
>   src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 
> 
> 
> Diff: https://reviews.apache.org/r/63860/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Andrew Schwartzmeyer]

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




src/docker/docker.cpp
Lines 741-744 (original), 747-782 (patched)


This seems totally reasonable to me, but I need Jie et. al. to review.


- Andrew Schwartzmeyer


On Dec. 7, 2017, 1:34 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Dec. 7, 2017, 1:34 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Jie Yu, John Kordich, Joseph 
> Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current Network enum in DockerInfo is specific to Linux containers.
> Instead of supporting {host, bridge, none, user} networks, Windows
> docker supports {nat, none, user} networks. Now, the Network enum
> supports {host, bridge, nat, none, user}. If the network isn't
> specified, then the default is host on Linux and nat on Windows.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 839ddb1cb41471d36423a2fc149acf90b973d413 
>   src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 
> 
> 
> Diff: https://reviews.apache.org/r/63860/diff/4/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Dec. 7, 2017, 12:04 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

Added `NAT` field to protobuf and now use the protobuf `hasXXX()` field to 
determine the default network value.


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


Repository: mesos


Description (updated)
---

The current Network enum in DockerInfo is specific to Linux containers.
Instead of supporting {host, bridge, none, user} networks, Windows
docker supports {nat, none, user} networks. Now, the Network enum
supports {host, bridge, nat, none, user}. If the network isn't
specified, then the default is host on Linux and nat on Windows.


Diffs (updated)
-

  include/mesos/mesos.proto 839ddb1cb41471d36423a2fc149acf90b973d413 
  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/4/

Changes: https://reviews.apache.org/r/63860/diff/3-4/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

> On Dec. 1, 2017, 1:47 a.m., Jie Yu wrote:
> > src/docker/docker.cpp
> > Lines 742-743 (original), 742-752 (patched)
> > 
> >
> > It's weird that user specifies HOST in the API, but we use "nat" 
> > instead.
> > 
> > Why can't we use transparent? I don't quite get that from the comments.
> 
> Akash Gupta wrote:
> It's not a default network on Windows like it is on Linux. If you do a 
> `docker network ls` on a fresh Windows box, you see a `nat` network and a 
> `none` network. Transparent is the network driver type, so to use it, you 
> need to create an user defined network by doing `docker network create -d 
> transparent ` and then to use it, you do `docker run 
> --network= ...`. I agree that using transparent would make more 
> sense, but we would have to make the agent create the network and pass that 
> in to the executor.

The real issue is that the API is Linux specific, since `HOST` and `BRIDGE` are 
linux only and `NAT` isn't defined, so a Windows user specifying `HOST` doesn't 
make sense. I think the best solution is to add `NAT` to the protobuf and then 
reword the docs to say that if the network mode is not given, then `HOST` and 
`BRIDGE` will be chosen for Linux and Windows respectively. I'm not sure if you 
can have different default settings for the protobuf, but we could have an 
undocumented `HOST -> NAT` conversion, since `HOST` mode isn't valid on Windows.


- Akash


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


On Nov. 27, 2017, 5:37 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Nov. 27, 2017, 5:37 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current Network enum in DockerInfo is specific to Linux containers.
> Instead of supporting {host, bridge, none, user} networks, Windows
> docker supports {nat, none, user} networks. Now, if the host or bridge
> network type is sent to the Windows agent, it will be internally
> converted to nat.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 
> 
> 
> Diff: https://reviews.apache.org/r/63860/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

> On Dec. 1, 2017, 1:47 a.m., Jie Yu wrote:
> > src/docker/docker.cpp
> > Lines 742-743 (original), 742-752 (patched)
> > 
> >
> > It's weird that user specifies HOST in the API, but we use "nat" 
> > instead.
> > 
> > Why can't we use transparent? I don't quite get that from the comments.

It's not a default network on Windows like it is on Linux. If you do a `docker 
network ls` on a fresh Windows box, you see a `nat` network and a `none` 
network. Transparent is the network driver type, so to use it, you need to 
create an user defined network by doing `docker network create -d transparent 
` and then to use it, you do `docker run --network= 
...`. I agree that using transparent would make more sense, but we would have 
to make the agent create the network and pass that in to the executor.


- Akash


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


On Nov. 27, 2017, 5:37 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Nov. 27, 2017, 5:37 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current Network enum in DockerInfo is specific to Linux containers.
> Instead of supporting {host, bridge, none, user} networks, Windows
> docker supports {nat, none, user} networks. Now, if the host or bridge
> network type is sent to the Windows agent, it will be internally
> converted to nat.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 
> 
> 
> Diff: https://reviews.apache.org/r/63860/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Jie Yu]

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




src/docker/docker.cpp
Lines 742-743 (original), 742-752 (patched)


It's weird that user specifies HOST in the API, but we use "nat" instead.

Why can't we use transparent? I don't quite get that from the comments.


- Jie Yu


On Nov. 27, 2017, 5:37 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63860/
> ---
> 
> (Updated Nov. 27, 2017, 5:37 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-7342
> https://issues.apache.org/jira/browse/MESOS-7342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current Network enum in DockerInfo is specific to Linux containers.
> Instead of supporting {host, bridge, none, user} networks, Windows
> docker supports {nat, none, user} networks. Now, if the host or bridge
> network type is sent to the Windows agent, it will be internally
> converted to nat.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 
> 
> 
> Diff: https://reviews.apache.org/r/63860/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/63862/ for test results.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Nov. 27, 2017, 5:37 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Changes
---

Rebased and changed host mode to nat mode on Windows.


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


Repository: mesos


Description (updated)
---

The current Network enum in DockerInfo is specific to Linux containers.
Instead of supporting {host, bridge, none, user} networks, Windows
docker supports {nat, none, user} networks. Now, if the host or bridge
network type is sent to the Windows agent, it will be internally
converted to nat.


Diffs (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/3/

Changes: https://reviews.apache.org/r/63860/diff/2-3/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta

Re: Review Request 63860: Windows: Mapped the Docker network info types.

[Akash Gupta]

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

(Updated Nov. 17, 2017, 10:37 p.m.)


Review request for mesos, Andrew Schwartzmeyer and John Kordich.


Summary (updated)
-

Windows: Mapped the Docker network info types.


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


Repository: mesos


Description
---

The current Network enum in DockerInfo is specific to Linux containers.
Instead of supporting {host, bridge, none, user} networks, Windows
docker supports {nat, none, user} networks. Now, if the host network
type is sent to the Windows agent, it will return an error. If the
bridge network is sent, then it will be internally converted to nat,
since they are equivalent.


Diffs (updated)
-

  src/docker/docker.cpp 722a54ad113fc4e2bb22a8f08e307ab38d5fbfed 


Diff: https://reviews.apache.org/r/63860/diff/2/

Changes: https://reviews.apache.org/r/63860/diff/1-2/


Testing
---

See https://reviews.apache.org/r/63862/ for test results.


Thanks,

Akash Gupta