Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-29 Thread Jie Yu 

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 468)


You don't need this PIPE if you don't care about stderr. I'll fix it for 
you.


- Jie Yu


On March 29, 2016, 10:58 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 29, 2016, 10:58 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-29 Thread Qian Zhang 

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

(Updated March 29, 2016, 6:58 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented isolate() method of "network/cni" isolator.


Diffs (updated)
-

  src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7cda5715814a0cfc4b394eb04437831e6dc44e3f 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44706/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-29 Thread Qian Zhang 


> On March 29, 2016, 1:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 354
> > 
> >
> > Should we return a Failure here  instead?

I think we should not return a Failure because there can be containers which 
does not opt in CNI networks in which case "network/cni" isolator should act as 
a no-op.


> On March 29, 2016, 1:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 396
> > 
> >
> > There's a problem with the current code. 'collect' (in isolate()) on 
> > 'attach' will fail if any of the call to the plugin (ADD) fails. And we'll 
> > call 'cleanup' right after that, it's likely that another call to the 
> > plugin (ADD) is still pending. I don't think calling DEL while an ADD is 
> > still pending is gonna work.
> > 
> > Therefore, I think we should use 'await' in isolate instead of 
> > 'collect' so that we can make sure all of them are in termimal state before 
> > we return the result.

Agree, will fix it soon.


> On March 29, 2016, 1:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 451
> > 
> >
> > You want to read 'stderr' as well.

But as per CNI spec (https://github.com/appc/cni/blob/master/SPEC.md#result), 
CNI plugins will never write to "stderr", they will always write to "stdout" in 
both success and failure cases.


- Qian


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


On March 29, 2016, 3:04 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 29, 2016, 3:04 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-29 Thread Qian Zhang 

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

(Updated March 29, 2016, 3:04 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented isolate() method of "network/cni" isolator.


Diffs (updated)
-

  src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7cda5715814a0cfc4b394eb04437831e6dc44e3f 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44706/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-28 Thread Jie Yu 

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 353)


Should we return a Failure here  instead?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 395)


There's a problem with the current code. 'collect' (in isolate()) on 
'attach' will fail if any of the call to the plugin (ADD) fails. And we'll call 
'cleanup' right after that, it's likely that another call to the plugin (ADD) 
is still pending. I don't think calling DEL while an ADD is still pending is 
gonna work.

Therefore, I think we should use 'await' in isolate instead of 'collect' so 
that we can make sure all of them are in termimal state before we return the 
result.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 450)


You want to read 'stderr' as well.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 525)


Could you please add a comment about the fact that destroy cannot happen in 
the middle of attach and `_attach` because the containerizer will wait for 
isolate to finish before destroying the container.

Also, add a CHECK(infos.container(containerId));


- Jie Yu


On March 26, 2016, 2:53 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 26, 2016, 2:53 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-28 Thread Jie Yu 

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 423)


maybe use getenv("PATH")?


- Jie Yu


On March 26, 2016, 2:53 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 26, 2016, 2:53 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-27 Thread Jie Yu 


> On March 25, 2016, 12:20 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 438
> > 
> >
> > Why do you need to get os::environment()?
> 
> Qian Zhang wrote:
> The reason is, CNI plugin needs to call `iptables` to set up IPMasq, if 
> we do not do this, the plugin will fail with an error:
>   {
>   "code": 100,
>   "msg": "failed to locate iptables: exec: \"iptables\": executable 
> file not found in $PATH"
>   }
>   
> Maybe we could explicitly set `$PATH` as 
> `/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin` rather than 
> call `os::environment()`?

Or you can only keep PATH in os::environment() and strip the rest. Please add a 
NOTE about that.


- Jie


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


On March 26, 2016, 2:53 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 26, 2016, 2:53 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-26 Thread Qian Zhang 

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

(Updated March 26, 2016, 10:53 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented isolate() method of "network/cni" isolator.


Diffs (updated)
-

  src/CMakeLists.txt 7bda2ac684f38da94e334f0cef843614687ae4aa 
  src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
7cda5715814a0cfc4b394eb04437831e6dc44e3f 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44706/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-26 Thread Qian Zhang 


> On March 25, 2016, 8:20 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 438
> > 
> >
> > Why do you need to get os::environment()?

The reason is, CNI plugin needs to call `iptables` to set up IPMasq, if we do 
not do this, the plugin will fail with an error:
  {
  "code": 100,
  "msg": "failed to locate iptables: exec: \"iptables\": executable file 
not found in $PATH"
  }
  
Maybe we could explicitly set `$PATH` as 
`/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin` rather than call 
`os::environment()`?


> On March 25, 2016, 8:20 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 464-472
> > 
> >
> > We cannot read stdout and stderr after the process has finished. THis 
> > is becasuse the PIPE size is bounded. If the subprocess writes a lot of 
> > data, the write will block if there's no reader.
> > 
> > Please follow the same pattern in:
> > 
> > https://github.com/apache/mesos/blob/master/src/uri/fetchers/curl.cpp#L98

Agree. However I see there are other code doing it as well, e.g., 
https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L3173:L3199
https://github.com/apache/mesos/blob/0.28.0/src/docker/docker.cpp#L153:L173

Maybe we need to fix them as well?


- Qian


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


On March 23, 2016, 10:22 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 23, 2016, 10:22 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0bd7a978306488b687a7e2eeeb8a5c9766d43548 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-26 Thread Qian Zhang 


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 445
> > 
> >
> > Why the `CHECK_READY` here ? If the future is not READY it could be in 
> > error, which is fine we should just return an ERROR ?
> 
> Qian Zhang wrote:
> Because when `NetworkCniIsolatorProcess::__connect` is called, we expect 
> `output` must be ready otherwise `NetworkCniIsolatorProcess::__connect` 
> should not be called. I think it is a common way to use `then`, please take a 
> look at: 
> https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L3173:L3186
>  as a reference.
> 
> Avinash sridharan wrote:
> The fact that a plugin might misbehave should not cause the agent to 
> crash. I think this defensive check doesn't serve much purpose apart from 
> causing a potential panic in the Agent.

I have revised these code based on Jie's comments, so we do not need to call 
`CHECK_READY()` anymore.


- Qian


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


On March 23, 2016, 10:22 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 23, 2016, 10:22 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0bd7a978306488b687a7e2eeeb8a5c9766d43548 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-24 Thread Jie Yu 

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 356)


s/netInfoDir/networkInfoDir/



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 362)


Can you include the 'networkInfoDir' in the error message?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 391)


No need for the dispatch here. You can just do:
```
foreachkey (const string& name, infos[.]->..) {
  futures.push_back(attach(containerId, name, target));
}
```

I would suggest we obtain NetworkInfo in 'attach' using containerId and 
name. We rarely use raw pointers.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 399 - 401)


YOu can combine these two statements:

```
return collect(futures)
  .then([]() { return Nothing(); });
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 410 - 420)


No need for this step. mkdir is recursive.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 437)


Why do you need to get os::environment()?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 445)


s/netConfig/networkConfig/



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 463 - 471)


We cannot read stdout and stderr after the process has finished. THis is 
becasuse the PIPE size is bounded. If the subprocess writes a lot of data, the 
write will block if there's no reader.

Please follow the same pattern in:
https://github.com/apache/mesos/blob/master/src/uri/fetchers/curl.cpp#L98


- Jie Yu


On March 23, 2016, 2:22 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 23, 2016, 2:22 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0bd7a978306488b687a7e2eeeb8a5c9766d43548 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-24 Thread Qian Zhang 


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 328-336
> > 
> >
> > why do we need this dispatch ? The dispatch is to itself, so seems a 
> > bit odd. Can we invoke the `subprocess` for the plugin in _connect directly 
> > ?
> > 
> > Basically not sure why we need connect & _connect.
> 
> Qian Zhang wrote:
> The idea is similar with how `MesosContainerizer` call each isolator: 
> https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/containerizer.cpp#L1171:L1181
> 
> I'd like to handle each call to a CNI plugin in a separate libprocess 
> dispatch event, so that's why I call `connect` via `dispatch`.
> 
> Avinash sridharan wrote:
> In the example you gave the `isolator` does a dispatch on an isolator 
> process. So the `MesosContainerizer` effectively does a dispatch on a 
> separate libprocess thread, which is the intended behavior. Here it seems a 
> bit odd that dispatch is scheduling an event on itself. Functionally nothing 
> wrong, but this is not idiomatic and we should avoid this.

Actually, when I wrote these code, instead of using `dispatch()`, I was 
thinking to directly call `subprocess()` to invoke CNI plugin in the 
`foreachvalue(NetworkInfo& networkInfo, infos[containerId]->networkInfos)` 
loop, and then use `defer()` to checkpoint the output return by the CNI plugin. 
However, the problem with this approach is, say a container wants to join two 
CNI networks (e.g., net1 and net2), if `subprocess()` for net1 succeeds, but 
`subprocess()` for net2 fails for a reason (it will return an `Error`), in this 
case we need to directly return a `Failure` in `isolate()`, but the issue is no 
one will be responsible for checkpointing the output for net1 which will cause 
IP leak since when we cleanup this container, we will not call CNI plugin to 
disconnect the container from net1 due to no checkpointed data. That's why I'd 
like to handle each call to a CNI plugin in a separate libprocess dispatch 
event, then the failure of one call will not affect any other calls.


- Qian


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


On March 23, 2016, 10:22 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 23, 2016, 10:22 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0bd7a978306488b687a7e2eeeb8a5c9766d43548 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-23 Thread Qian Zhang 


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 471
> > 
> >
> > Again why the CHECK ? The result might not have an IPv4 or an IPv6 
> > allocation due to an IPAM error in which case we should return a failure.
> 
> Qian Zhang wrote:
> I think the output of IPAM plugin has no IPv4 address and no IPv6 address 
> is an unexpected result, so that's why I use `CHECK` here, but I agree with 
> you returning a `Failure` is more appropriate, will update the patch 
> accordingly later.
> 
> Avinash sridharan wrote:
> Why is it an unexpected result? What happens if the IPAM is 
> oversubscribed and runs out of addresses? That should definitely not cause 
> the Agent to crash. We should not be able to launch containers, but the Agent 
> should not be crashing. IPAM running out of addresses is not a terminal 
> behavior, it will recover when containers are destroyed and IP addresses are 
> freed.

Yes, I agree, let me remove that `CHECK`.


- Qian


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


On March 21, 2016, 12:27 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 21, 2016, 12:27 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-22 Thread Avinash sridharan 


> On March 21, 2016, 3:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 339
> > 
> >
> > This is just a thought. Maybe its better to use `await` over here, and 
> > use a lambda or `.onAny` on the await to parse the list of futures returned 
> > on await and clean up any checkpointed data for networks that we were not 
> > able to join due to plugin errors ?
> 
> Qian Zhang wrote:
> I think any clean up works should be handled in the `cleanup` method, 
> that's should be the design idea of isolator. Here we are doing all or 
> nothing, so if there is a call to a CNI plugin fails for any reasons, we will 
> return a `Failure` which should be eventually handled in 
> `Slave::executorLaunched`, and it will call `MesosContainerizer::destroy` 
> which will eventually call each isolator's `cleanup` method to do the cleanup.

Sounds reasonable.


> On March 21, 2016, 3:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 391
> > 
> >
> > Shouldn't we be cleaning up the check pointed data for this ifname if 
> > there is an error while launching the plugin for this ifname ?
> 
> Qian Zhang wrote:
> I think we should do the cleanup in the `cleanup` method.

Agreed.


> On March 21, 2016, 3:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 445
> > 
> >
> > Why the `CHECK_READY` here ? If the future is not READY it could be in 
> > error, which is fine we should just return an ERROR ?
> 
> Qian Zhang wrote:
> Because when `NetworkCniIsolatorProcess::__connect` is called, we expect 
> `output` must be ready otherwise `NetworkCniIsolatorProcess::__connect` 
> should not be called. I think it is a common way to use `then`, please take a 
> look at: 
> https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L3173:L3186
>  as a reference.

The fact that a plugin might misbehave should not cause the agent to crash. I 
think this defensive check doesn't serve much purpose apart from causing a 
potential panic in the Agent.


> On March 21, 2016, 3:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 471
> > 
> >
> > Again why the CHECK ? The result might not have an IPv4 or an IPv6 
> > allocation due to an IPAM error in which case we should return a failure.
> 
> Qian Zhang wrote:
> I think the output of IPAM plugin has no IPv4 address and no IPv6 address 
> is an unexpected result, so that's why I use `CHECK` here, but I agree with 
> you returning a `Failure` is more appropriate, will update the patch 
> accordingly later.

Why is it an unexpected result? What happens if the IPAM is oversubscribed and 
runs out of addresses? That should definitely not cause the Agent to crash. We 
should not be able to launch containers, but the Agent should not be crashing. 
IPAM running out of addresses is not a terminal behavior, it will recover when 
containers are destroyed and IP addresses are freed.


> On March 21, 2016, 3:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 328-336
> > 
> >
> > why do we need this dispatch ? The dispatch is to itself, so seems a 
> > bit odd. Can we invoke the `subprocess` for the plugin in _connect directly 
> > ?
> > 
> > Basically not sure why we need connect & _connect.
> 
> Qian Zhang wrote:
> The idea is similar with how `MesosContainerizer` call each isolator: 
> https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/containerizer.cpp#L1171:L1181
> 
> I'd like to handle each call to a CNI plugin in a separate libprocess 
> dispatch event, so that's why I call `connect` via `dispatch`.

In the example you gave the `isolator` does a dispatch on an isolator process. 
So the `MesosContainerizer` effectively does a dispatch on a separate 
libprocess thread, which is the intended behavior. Here it seems a bit odd that 
dispatch is scheduling an event on itself. Functionally nothing wrong, but this 
is not idiomatic and we should avoid this.


- Avinash


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


On March 20, 2016, 4:27 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail.

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-22 Thread Qian Zhang 


> On March 21, 2016, 3:08 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, lines 32-56
> > 
> >
> > Can we introduce paths.hpp|cpp under cni/ directory  for the canonical 
> > locations.
> > 
> > ```
> > constexpr char ROOT_DIR[] = "...";
> > 
> > string cni::paths::getNamespaceHandle(
> > const string& rootDir,
> > const ContainerID& containerId);
> > 
> > string cni::paths::getNetworkPath(
> > const string& rootDir,
> > const ContainerID& containerId,
> > const string& name);
> > 
> > string cni::paths::getIPv4Path(
> > const string& rootDir,
> > const ContainerID& containerId,
> > const string& name,
> > const string& ifname);
> > 
> > string cni::paths::getIPv6Path(
> > const string& rootDir,
> > const ContainerID& containerId,
> > const string& name,
> > const string& ifname);
> > ```

Thanks for the comments, I have two questions:
1. Do you mean we define rootDir as a const string like this: `constexpr char 
ROOT_DIR[] = "/var/run/mesos/isolators/network/cni/"`? If so, I'd like to know 
why we still need `rootDir` as the first parameter of those methods (e.g., 
`getNamespaceHandle`), I think we should be able to directly use that const 
string inside of those methods rather than passing it as a parameter, right?
2. Take `getNamespaceHandle` as an example, so it will be like:
string getNamespaceHandle(const ContainerID& containerId)
{
  return path::join(ROOT_DIR, containerId);
}
I think each of those `getXXX` method will be just a call to `path::join()`, so 
what is its advantage against directly calling `path::join()` in cni.cpp?


> On March 21, 2016, 3:08 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 206-212
> > 
> >
> > I suggest we save a rootDir in the isolator process. We can easily 
> > switch to use a flag later. Also, we need to call 'realpath' here to make 
> > sure it's a realpath.
> > 
> > We also need to make sure ROOT_DIR is a self bind mounted directory 
> > (slave+shared) so that namespace bind mount does not leak into containers.

Do you mean we call `realpath()` to get the real path of the const string 
`ROOT_DIR` first and then call `mkdir` with the real path as its parameter to 
create the directory?

And can you please elaborate why the namespace bind mount can be leaked into 
containers if we do not make `ROOT_DIR` as a self bind mounted directory? I 
just want to know the rationale behind it :-)


- Qian


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


On March 21, 2016, 12:27 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 21, 2016, 12:27 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-21 Thread Avinash sridharan 

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




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 33)


s/like the following/as follows



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 248)


s/i/ifindex/

more descriptive.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 328 - 336)


why do we need this dispatch ? The dispatch is to itself, so seems a bit 
odd. Can we invoke the `subprocess` for the plugin in _connect directly ?

Basically not sure why we need connect & _connect.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 339)


This is just a thought. Maybe its better to use `await` over here, and use 
a lambda or `.onAny` on the await to parse the list of futures returned on 
await and clean up any checkpointed data for networks that we were not able to 
join due to plugin errors ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 345)


The name of the method `connect` seems a bit odd. I think you mean 
"connecting container to the network". Maybe a better name would be `attach` ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 391)


Shouldn't we be cleaning up the check pointed data for this ifname if there 
is an error while launching the plugin for this ifname ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 445)


Why the `CHECK_READY` here ? If the future is not READY it could be in 
error, which is fine we should just return an ERROR ?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 471)


Again why the CHECK ? The result might not have an IPv4 or an IPv6 
allocation due to an IPAM error in which case we should return a failure.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 481)


s/write/checkpoint/



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 494)


s/write/checkpoint/


- Avinash sridharan


On March 20, 2016, 4:27 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 20, 2016, 4:27 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-20 Thread Jie Yu 

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



Some initial comments. Will do a pass to finish the rest later.


src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (lines 32 - 56)


Can we introduce paths.hpp|cpp under cni/ directory  for the canonical 
locations.

```
constexpr char ROOT_DIR[] = "...";

string cni::paths::getNamespaceHandle(
const string& rootDir,
const ContainerID& containerId);

string cni::paths::getNetworkPath(
const string& rootDir,
const ContainerID& containerId,
const string& name);

string cni::paths::getIPv4Path(
const string& rootDir,
const ContainerID& containerId,
const string& name,
const string& ifname);

string cni::paths::getIPv6Path(
const string& rootDir,
const ContainerID& containerId,
const string& name,
const string& ifname);
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 206 - 212)


I suggest we save a rootDir in the isolator process. We can easily switch 
to use a flag later. Also, we need to call 'realpath' here to make sure it's a 
realpath.

We also need to make sure ROOT_DIR is a self bind mounted directory 
(slave+shared) so that namespace bind mount does not leak into containers.


- Jie Yu


On March 20, 2016, 4:27 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 20, 2016, 4:27 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-20 Thread Qian Zhang 

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

(Updated March 21, 2016, 12:27 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented isolate() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44706/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-20 Thread Qian Zhang 


> On March 12, 2016, 2:45 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 277
> > 
> >
> > either s/realpath/`realpath`
> > 
> > or s/realpath/location

I see cgroups.cpp uses the word `canonical path` which I think is good, so 
let's just use it :-)


> On March 12, 2016, 2:45 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 358
> > 
> >
> > If we block here, it will lock the isolator, and prevent the 
> > `MesosContainerizer` from launching any more containers?
> 
> Qian Zhang wrote:
> I do not think it will prevent `MesosContainerizer` from launching any 
> more containers. Please take a look at: 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/containerizer.cpp#L945:L1034,
>  the whole code to call launcher to fork executor process and call isolator's 
> `isolate()` are already in a `defer()` as a lambda, and it is also a very big 
> lambda.
> 
> Avinash sridharan wrote:
> Hi Qian,
>  Didn't understand your comment. Are you implying calling `await` on the 
> `Future` won't block?

I think it will not block `MesosContainerizerProcess`, but it will indeed block 
`NetworkCniIsolatorProcess`. So yes, I agree we should use `defer`. And I have 
another question, for the container which want to join multiple networks, do we 
want to call plugin to join multiple networks in parallel or sequentially? I 
think joining one network should be independent on joining another, but if we 
do them in parallel, I am afraid that user may see "`eth2, eth0, eth1`" rather 
than "`eth0, eth1, eth2`" in the container. Please let me know your thought :-)


- Qian


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


On March 16, 2016, 3:50 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 16, 2016, 3:50 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-19 Thread Avinash sridharan 


> On March 11, 2016, 6:45 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 358
> > 
> >
> > If we block here, it will lock the isolator, and prevent the 
> > `MesosContainerizer` from launching any more containers?
> 
> Qian Zhang wrote:
> I do not think it will prevent `MesosContainerizer` from launching any 
> more containers. Please take a look at: 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/containerizer.cpp#L945:L1034,
>  the whole code to call launcher to fork executor process and call isolator's 
> `isolate()` are already in a `defer()` as a lambda, and it is also a very big 
> lambda.
> 
> Avinash sridharan wrote:
> Hi Qian,
>  Didn't understand your comment. Are you implying calling `await` on the 
> `Future` won't block?
> 
> Qian Zhang wrote:
> I think it will not block `MesosContainerizerProcess`, but it will indeed 
> block `NetworkCniIsolatorProcess`. So yes, I agree we should use `defer`. And 
> I have another question, for the container which want to join multiple 
> networks, do we want to call plugin to join multiple networks in parallel or 
> sequentially? I think joining one network should be independent on joining 
> another, but if we do them in parallel, I am afraid that user may see "`eth2, 
> eth0, eth1`" rather than "`eth0, eth1, eth2`" in the container. Please let me 
> know your thought :-)
> 
> Qian Zhang wrote:
> Second thought: since one libprocess can only handle one event at a time, 
> I think container has to join multiple networks sequentially, so we do not 
> need to worry about the interface names may be disordered.

You don't need to force the container to join multiple networks sequentially. 
You can invoke multiple calls to the plugin (each being a subprocess) and then 
use `collect` or `await` to aggregate the futures. 

Since we are supporting an all or none behavior, in terms of containers joining 
CNI networks, we could possibly use `collect`. 

https://github.com/apache/mesos/blob/fcc6f95499559e55ebe6b0a5c5ca0665ad5c7742/3rdparty/libprocess/include/process/collect.hpp#L38


- Avinash


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


On March 16, 2016, 7:50 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 16, 2016, 7:50 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-18 Thread Qian Zhang 


> On March 12, 2016, 2:45 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 358
> > 
> >
> > If we block here, it will lock the isolator, and prevent the 
> > `MesosContainerizer` from launching any more containers?
> 
> Qian Zhang wrote:
> I do not think it will prevent `MesosContainerizer` from launching any 
> more containers. Please take a look at: 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/containerizer.cpp#L945:L1034,
>  the whole code to call launcher to fork executor process and call isolator's 
> `isolate()` are already in a `defer()` as a lambda, and it is also a very big 
> lambda.
> 
> Avinash sridharan wrote:
> Hi Qian,
>  Didn't understand your comment. Are you implying calling `await` on the 
> `Future` won't block?
> 
> Qian Zhang wrote:
> I think it will not block `MesosContainerizerProcess`, but it will indeed 
> block `NetworkCniIsolatorProcess`. So yes, I agree we should use `defer`. And 
> I have another question, for the container which want to join multiple 
> networks, do we want to call plugin to join multiple networks in parallel or 
> sequentially? I think joining one network should be independent on joining 
> another, but if we do them in parallel, I am afraid that user may see "`eth2, 
> eth0, eth1`" rather than "`eth0, eth1, eth2`" in the container. Please let me 
> know your thought :-)

Second thought: since one libprocess can only handle one event at a time, I 
think container has to join multiple networks sequentially, so we do not need 
to worry about the interface names may be disordered.


- Qian


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


On March 16, 2016, 3:50 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 16, 2016, 3:50 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-16 Thread Qian Zhang 

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

(Updated March 16, 2016, 3:50 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented isolate() method of "network/cni" isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44706/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-14 Thread Avinash sridharan 


> On March 11, 2016, 6:45 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 358
> > 
> >
> > If we block here, it will lock the isolator, and prevent the 
> > `MesosContainerizer` from launching any more containers?
> 
> Qian Zhang wrote:
> I do not think it will prevent `MesosContainerizer` from launching any 
> more containers. Please take a look at: 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/containerizer.cpp#L945:L1034,
>  the whole code to call launcher to fork executor process and call isolator's 
> `isolate()` are already in a `defer()` as a lambda, and it is also a very big 
> lambda.

Hi Qian,
 Didn't understand your comment. Are you implying calling `await` on the 
`Future` won't block?


- Avinash


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


On March 11, 2016, 2:10 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 11, 2016, 2:10 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-14 Thread Qian Zhang 


> On March 12, 2016, 2:45 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 358
> > 
> >
> > If we block here, it will lock the isolator, and prevent the 
> > `MesosContainerizer` from launching any more containers?

I do not think it will prevent `MesosContainerizer` from launching any more 
containers. Please take a look at: 
https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/containerizer.cpp#L945:L1034,
 the whole code to call launcher to fork executor process and call isolator's 
`isolate()` are already in a `defer()` as a lambda, and it is also a very big 
lambda.


- Qian


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


On March 11, 2016, 10:10 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 11, 2016, 10:10 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-11 Thread Avinash sridharan 

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




src/slave/containerizer/mesos/isolators/network/cni.hpp (line 85)


Please see my comment of using a vector instead of map in the previous 
patches?



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 277)


either s/realpath/`realpath`

or s/realpath/location



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 329 - 389)


Firstly, lambdas should be short. Secondly, as per my other comment with 
code we will block the isolator. A better strategy would be to move this entire 
code into a private method, and invoke that function with a `defer`. This would 
allow you to await on the futures of the subprocess return without blocking the 
isolator. You can use this code as a reference 
(https://github.com/apache/mesos/blob/a1a9cd5939d25f82214a5c533bde96a3493f81f3/src/slave/containerizer/mesos/containerizer.cpp#L1318)
 This uses a static function, but I think you can use a private method and 
`defer` to the isolator libprocess thread to invoke it.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 341)


s/, so here we will only get its stdout//



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 358)


If we block here, it will lock the isolator, and prevent the 
`MesosContainerizer` from launching any more containers?


- Avinash sridharan


On March 11, 2016, 2:10 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 11, 2016, 2:10 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-11 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [44004, 44200, 44549, 44555, 44514, 44622, 44706]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 11, 2016, 2:10 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 11, 2016, 2:10 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 44706: Implemented isolate() method of "network/cni" isolator.

2016-03-11 Thread Qian Zhang 

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

Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented isolate() method of "network/cni" isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44706/diff/


Testing
---

make check


Thanks,

Qian Zhang