Re: Review Request 60766: Ignored containers that join CNI networks.

2017-10-17 Thread James Peach 

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

2017-09-19 Thread James Peach 


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

2017-09-18 Thread Qian Zhang 

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


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.

2017-09-06 Thread Qian Zhang 

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

2017-09-05 Thread James Peach 

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

2017-09-05 Thread Qian Zhang 

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


We should call `update()` in this case as well.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 442-444 (patched)


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.

2017-08-30 Thread James Peach 

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

2017-08-24 Thread Qian Zhang 


> On Aug. 23, 2017, 5:52 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 394 (original), 436 (patched)
> > 
> >
> > 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- 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-
> 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.

2017-08-23 Thread Qian Zhang 

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


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)


I think it should be `if (infos.contains(rootContainerId)) {`.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 490 (patched)


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.

2017-08-23 Thread James Peach 


> On Aug. 23, 2017, 9:52 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 394 (original), 436 (patched)
> > 
> >
> > 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- 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-
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.

2017-08-23 Thread Qian Zhang 

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


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.

2017-08-22 Thread Qian Zhang 

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


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.

2017-08-22 Thread Qian Zhang 


> On Aug. 21, 2017, 4:32 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Line 244 (original), 245 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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.

2017-08-21 Thread James Peach 


> On Aug. 21, 2017, 8:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Line 244 (original), 245 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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.

2017-08-21 Thread James Peach 

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

2017-08-21 Thread Qian Zhang 

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


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)


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)


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

Re: Review Request 60766: Ignored containers that join CNI networks.

2017-07-28 Thread James Peach 

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