You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/07/29 00:01:21 UTC
Re: Review Request 60766: Ignored containers that join CNI networks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/
-----------------------------------------------------------
(Updated July 29, 2017, 12:01 a.m.)
Review request for mesos, Qian Zhang and Jiang Yan Xu.
Bugs: MESOS-7675
https://issues.apache.org/jira/browse/MESOS-7675
Repository: mesos
Description
-------
Working on the assumption that containers with CNI networks will
get their own IP addresses and don't need port isolation, ignore
any containers that are joining CNI networks.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 6f100b516f2750732acaed693f7023b1e44b39d9
src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/60766/diff/5/
Changes: https://reviews.apache.org/r/60766/diff/4-5/
Testing
-------
make check (Fedora 26).
Thanks,
James Peach
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by James Peach <jp...@apache.org>.
> On Aug. 23, 2017, 9:52 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 394 (original), 436 (patched)
> > <https://reviews.apache.org/r/60766/diff/11/?file=1801806#file1801806line436>
> >
> > So here an empty `Info` object is put into `infos`? I think we need to populate its `ports` field before that.
The ports are populated in `update()`:
```
0823 08:57:15.686208 21125 linux_launcher.cpp:300] Recovered container d9e180c6-c527-431c-b147-f820c788aa7d
I0823 08:57:15.686942 21133 ports.cpp:437] recovering container d9e180c6-c527-431c-b147-f820c788aa7d
I0823 08:57:15.688449 21123 provisioner.cpp:416] Provisioner recovery complete
I0823 08:57:15.689740 21132 slave.cpp:6110] Sending reconnect request to executor '084372d8-e569-48d9-9bb3-6ce89750207e' of framework ac96cb5d-fbab-402c-8fe8-1042ea1b1986-0000 at executor(1)@17.228.224.108:39141
I0823 08:57:15.691100 21459 exec.cpp:283] Received reconnect request from agent ac96cb5d-fbab-402c-8fe8-1042ea1b1986-S0
I0823 08:57:15.693174 21131 slave.cpp:4071] Received re-registration message from executor '084372d8-e569-48d9-9bb3-6ce89750207e' of framework ac96cb5d-fbab-402c-8fe8-1042ea1b1986-0000
I0823 08:57:15.693802 21132 ports.cpp:372] updating container d9e180c6-c527-431c-b147-f820c788aa7d
I0823 08:57:15.694080 21457 exec.cpp:260] Executor re-registered on agent ac96cb5d-fbab-402c-8fe8-1042ea1b1986-S0
```
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review183586
-----------------------------------------------------------
On Aug. 21, 2017, 10:01 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 10:01 p.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5772421c3078d36225b946a5286b8c1bf2f007e8
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/11/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
> On Aug. 23, 2017, 5:52 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 394 (original), 436 (patched)
> > <https://reviews.apache.org/r/60766/diff/11/?file=1801806#file1801806line436>
> >
> > So here an empty `Info` object is put into `infos`? I think we need to populate its `ports` field before that.
>
> James Peach wrote:
> The ports are populated in `update()`:
>
> ```
> 0823 08:57:15.686208 21125 linux_launcher.cpp:300] Recovered container d9e180c6-c527-431c-b147-f820c788aa7d
> I0823 08:57:15.686942 21133 ports.cpp:437] recovering container d9e180c6-c527-431c-b147-f820c788aa7d
> I0823 08:57:15.688449 21123 provisioner.cpp:416] Provisioner recovery complete
> I0823 08:57:15.689740 21132 slave.cpp:6110] Sending reconnect request to executor '084372d8-e569-48d9-9bb3-6ce89750207e' of framework ac96cb5d-fbab-402c-8fe8-1042ea1b1986-0000 at executor(1)@17.228.224.108:39141
> I0823 08:57:15.691100 21459 exec.cpp:283] Received reconnect request from agent ac96cb5d-fbab-402c-8fe8-1042ea1b1986-S0
> I0823 08:57:15.693174 21131 slave.cpp:4071] Received re-registration message from executor '084372d8-e569-48d9-9bb3-6ce89750207e' of framework ac96cb5d-fbab-402c-8fe8-1042ea1b1986-0000
> I0823 08:57:15.693802 21132 ports.cpp:372] updating container d9e180c6-c527-431c-b147-f820c788aa7d
> I0823 08:57:15.694080 21457 exec.cpp:260] Executor re-registered on agent ac96cb5d-fbab-402c-8fe8-1042ea1b1986-S0
> ```
Yeah, I know that. I do not think we should rely on that for 2 reasons:
1. This `update()` method will be called when executors reregister agent, but what if executor does not reregister agent for some reason, but in this case, we still want `network/ports` to check its listening ports.
2. This `update()` method will not be called for nested container, that means after agent is restarted, all the nested containers will not be checked by `network/ports` isolator anymore. And I would suggest to add a test for it.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review183586
-----------------------------------------------------------
On Aug. 22, 2017, 6:01 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2017, 6:01 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5805dfb4fb6e755e4c23851b0a6b504f2a8b3396
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/13/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review183586
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/ports.cpp
Line 394 (original), 436 (patched)
<https://reviews.apache.org/r/60766/#comment259642>
So here an empty `Info` object is put into `infos`? I think we need to populate its `ports` field before that.
- Qian Zhang
On Aug. 22, 2017, 6:01 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2017, 6:01 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5772421c3078d36225b946a5286b8c1bf2f007e8
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/11/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review183456
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 318-324 (original), 318-341 (patched)
<https://reviews.apache.org/r/60766/#comment259468>
What about merging these two `if` like:
```
if (cniIsolatorEnabled) {
if (!containerId.has_parent()) {
// For top-level containers, ...
if (containerConfig.has_container_info()) {
foreach (const auto& networkInfo,
containerConfig.container_info().network_infos()) {
if (networkInfo.has_name()) {
return None();
}
}
}
} else {
// For nested containers, ...
ContainerID rootContainerId = protobuf::getRootContainerId(containerId);
if (!infos.contains(rootContainerId)) {
return None();
}
}
}
```
- Qian Zhang
On Aug. 22, 2017, 6:01 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2017, 6:01 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5772421c3078d36225b946a5286b8c1bf2f007e8
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/9/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/
-----------------------------------------------------------
(Updated Oct. 17, 2017, 7:13 p.m.)
Review request for mesos, Qian Zhang and Jiang Yan Xu.
Bugs: MESOS-7675
https://issues.apache.org/jira/browse/MESOS-7675
Repository: mesos
Description
-------
Working on the assumption that containers with CNI networks will
get their own IP addresses and don't need port isolation, ignore
any containers that are joining CNI networks.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/60766/diff/19/
Changes: https://reviews.apache.org/r/60766/diff/18-19/
Testing
-------
make check (Fedora 26).
Thanks,
James Peach
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review184636
-----------------------------------------------------------
Ship it!
Ship It!
- Qian Zhang
On Sept. 6, 2017, 1:57 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 1:57 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 4ff014e278100ea99ad93f02c6688ec9ac047059
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/15/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by James Peach <jp...@apache.org>.
> On Sept. 19, 2017, 3:01 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 511 (original), 586-589 (patched)
> > <https://reviews.apache.org/r/60766/diff/18/?file=1828711#file1828711line586>
> >
> > I still think it is better to raise the limitation against the nested container rather than the root container because that will give framework more fine-grained debugging information so that it can know which specific nested container triggered the limitation.
> >
> > So I think we need to enhance the implementation of `waitNestedContainer()` to make it propagate the `reason` and `message` to the default executor, and then the default executor can send the limitation for the nested container.
We discussed this a bit and I thought about it for a while. Right now, the resources model for nested containers accrues resources to the root of the container tree. In all the other isolators, when anything in the tree violates a resource constraint the whole tree is torn down. If we send the limitation to the nested container, then an executor could reasonably choose to leave the container tree in place and we would have some inexplicable inconsistent behaviour.
I do generally agree that allowing leaf containers to fail would be kinder, but that seems like part of a larger discussion about how nested containers should work. In [MESOS-7963](https://issues.apache.org/jira/browse/MESOS-7963) I would propose retaining the current semantics of the `network/ports` isolator but ensuring the limitation is correctly propagated.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review185637
-----------------------------------------------------------
On Sept. 5, 2017, 5:57 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 5:57 p.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 4d5dc13f363f5d8886983d7dd06a5cecc177c345
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/18/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review185637
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/ports.cpp
Line 511 (original), 586-589 (patched)
<https://reviews.apache.org/r/60766/#comment261950>
I still think it is better to raise the limitation against the nested container rather than the root container because that will give framework more fine-grained debugging information so that it can know which specific nested container triggered the limitation.
So I think we need to enhance the implementation of `waitNestedContainer()` to make it propagate the `reason` and `message` to the default executor, and then the default executor can send the limitation for the nested container.
- Qian Zhang
On Sept. 6, 2017, 1:57 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 1:57 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 4d5dc13f363f5d8886983d7dd06a5cecc177c345
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/18/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/
-----------------------------------------------------------
(Updated Sept. 5, 2017, 5:57 p.m.)
Review request for mesos, Qian Zhang and Jiang Yan Xu.
Bugs: MESOS-7675
https://issues.apache.org/jira/browse/MESOS-7675
Repository: mesos
Description
-------
Working on the assumption that containers with CNI networks will
get their own IP addresses and don't need port isolation, ignore
any containers that are joining CNI networks.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 4ff014e278100ea99ad93f02c6688ec9ac047059
src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/60766/diff/15/
Changes: https://reviews.apache.org/r/60766/diff/14-15/
Testing
-------
make check (Fedora 26).
Thanks,
James Peach
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review184517
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 405-408 (patched)
<https://reviews.apache.org/r/60766/#comment260685>
We should call `update()` in this case as well.
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 442-444 (patched)
<https://reviews.apache.org/r/60766/#comment260686>
Better to merge these 3 lines into 2 lines.
- Qian Zhang
On Aug. 31, 2017, 7:26 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 7:26 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5f9373a88819ae1e0ceae708dccbed5383684918
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/14/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/
-----------------------------------------------------------
(Updated Aug. 30, 2017, 11:26 p.m.)
Review request for mesos, Qian Zhang and Jiang Yan Xu.
Bugs: MESOS-7675
https://issues.apache.org/jira/browse/MESOS-7675
Repository: mesos
Description
-------
Working on the assumption that containers with CNI networks will
get their own IP addresses and don't need port isolation, ignore
any containers that are joining CNI networks.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 5f9373a88819ae1e0ceae708dccbed5383684918
src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/60766/diff/14/
Changes: https://reviews.apache.org/r/60766/diff/13-14/
Testing
-------
make check (Fedora 26).
Thanks,
James Peach
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review183693
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/ports.cpp
Line 406 (original), 451 (patched)
<https://reviews.apache.org/r/60766/#comment259758>
Can we put all nested containers in a list in the above `foreach` loop so that we do not have to go through all containers here?
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 462 (patched)
<https://reviews.apache.org/r/60766/#comment259759>
I think it should be `if (infos.contains(rootContainerId)) {`.
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 490 (patched)
<https://reviews.apache.org/r/60766/#comment259761>
Kill this empty line.
- Qian Zhang
On Aug. 22, 2017, 6:01 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2017, 6:01 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5805dfb4fb6e755e4c23851b0a6b504f2a8b3396
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/13/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/
-----------------------------------------------------------
(Updated Aug. 21, 2017, 10:01 p.m.)
Review request for mesos, Qian Zhang and Jiang Yan Xu.
Changes
-------
Addressed review feedback and updated to handle nested CNI containers.
Bugs: MESOS-7675
https://issues.apache.org/jira/browse/MESOS-7675
Repository: mesos
Description
-------
Working on the assumption that containers with CNI networks will
get their own IP addresses and don't need port isolation, ignore
any containers that are joining CNI networks.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 5772421c3078d36225b946a5286b8c1bf2f007e8
src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/60766/diff/9/
Changes: https://reviews.apache.org/r/60766/diff/8-9/
Testing
-------
make check (Fedora 26).
Thanks,
James Peach
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
> On Aug. 21, 2017, 4:32 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Line 244 (original), 245 (patched)
> > <https://reviews.apache.org/r/60766/diff/8/?file=1800294#file1800294line245>
> >
> > So here we only count `network/cni` isolator and `network/port_mapping` isolator, either of them (but not both of them) can work with `network/ports` isolator. Can you please also update the comments accordingly?
>
> James Peach wrote:
> This is already commented just above.
The comments is:
> One and only one `network` isolator is required
However, I think we may need to be more explicitly, like: Only one of `network/cni` and `network/port_mapping` isolators is required.
> On Aug. 21, 2017, 4:32 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 392-402 (patched)
> > <https://reviews.apache.org/r/60766/diff/8/?file=1800296#file1800296line392>
> >
> > Can we check `state->executor_info().container().network_infos()` rather than checking CNI container dir?
>
> James Peach wrote:
> Are we guaranteed to have a named network in `state->executor_info` in the case of nested containers joining the parent network? If not, then I think we still have to check whether the root container has a CNI configuration. I updated the patch to do this.
Why do we need to check `state->executor_info` for nested containers? In this `recover()` method, for a nested container, I think we still need to check `state->executor_info` for its root container. Or maybe you can just check `infos.contains(rootContainerId)` for nested container like what you did in `prepare()`, but I am not sure if nested containers always come after their root container in the `state` list, it looks like it can be guaranteed.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review183307
-----------------------------------------------------------
On Aug. 22, 2017, 6:01 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2017, 6:01 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5772421c3078d36225b946a5286b8c1bf2f007e8
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/9/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by James Peach <jp...@apache.org>.
> On Aug. 21, 2017, 8:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Line 244 (original), 245 (patched)
> > <https://reviews.apache.org/r/60766/diff/8/?file=1800294#file1800294line245>
> >
> > So here we only count `network/cni` isolator and `network/port_mapping` isolator, either of them (but not both of them) can work with `network/ports` isolator. Can you please also update the comments accordingly?
This is already commented just above.
> On Aug. 21, 2017, 8:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 308-311 (original), 313-323 (patched)
> > <https://reviews.apache.org/r/60766/diff/8/?file=1800296#file1800296line313>
> >
> > I think we only need to do this check for top-level container, but not for nested container since nested container always share network namespace with its parent. So we may need to add `!containerId.has_parent()` into the condition of the first `if`.
The check for nested containers needs to be separate since the child might be nested within a CNI network or a host network. When `prepare` a nested container, we only isolate it if we already isolated the corresponding root of the container tree.
> On Aug. 21, 2017, 8:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 392-402 (patched)
> > <https://reviews.apache.org/r/60766/diff/8/?file=1800296#file1800296line392>
> >
> > Can we check `state->executor_info().container().network_infos()` rather than checking CNI container dir?
Are we guaranteed to have a named network in `state->executor_info` in the case of nested containers joining the parent network? If not, then I think we still have to check whether the root container has a CNI configuration. I updated the patch to do this.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review183307
-----------------------------------------------------------
On Aug. 21, 2017, 10:01 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 10:01 p.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5772421c3078d36225b946a5286b8c1bf2f007e8
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/9/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60766: Ignored containers that join CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60766/#review183307
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp
Line 244 (original), 245 (patched)
<https://reviews.apache.org/r/60766/#comment259349>
So here we only count `network/cni` isolator and `network/port_mapping` isolator, either of them (but not both of them) can work with `network/ports` isolator. Can you please also update the comments accordingly?
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 308-311 (original), 313-323 (patched)
<https://reviews.apache.org/r/60766/#comment259352>
I think we only need to do this check for top-level container, but not for nested container since nested container always share network namespace with its parent. So we may need to add `!containerId.has_parent()` into the condition of the first `if`.
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 392-402 (patched)
<https://reviews.apache.org/r/60766/#comment259353>
Can we check `state->executor_info().container().network_infos()` rather than checking CNI container dir?
- Qian Zhang
On July 29, 2017, 8:01 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> -----------------------------------------------------------
>
> (Updated July 29, 2017, 8:01 a.m.)
>
>
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
>
>
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 5772421c3078d36225b946a5286b8c1bf2f007e8
> src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/60766/diff/8/
>
>
> Testing
> -------
>
> make check (Fedora 26).
>
>
> Thanks,
>
> James Peach
>
>