You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/08/04 16:02:55 UTC
Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/
-----------------------------------------------------------
Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
Bugs: MESOS-7853
https://issues.apache.org/jira/browse/MESOS-7853
Repository: mesos
Description
-------
Added pid ns sharing based on agent flag and protobuf message field.
Diffs
-----
src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
Diff: https://reviews.apache.org/r/61428/diff/1/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/#review182739
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Aug. 8, 2017, 2:40 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61428/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2017, 2:40 a.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added pid ns sharing based on agent flag and protobuf message field.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
> src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
>
>
> Diff: https://reviews.apache.org/r/61428/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/
-----------------------------------------------------------
(Updated Aug. 8, 2017, 5:40 p.m.)
Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-7853
https://issues.apache.org/jira/browse/MESOS-7853
Repository: mesos
Description
-------
Added pid ns sharing based on agent flag and protobuf message field.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
Diff: https://reviews.apache.org/r/61428/diff/5/
Changes: https://reviews.apache.org/r/61428/diff/4-5/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/
-----------------------------------------------------------
(Updated Aug. 7, 2017, 10:55 a.m.)
Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
Changes
-------
Return a `Failure` if both `--disallow_sharing_agent_pid_namespace` and `share_pid_namespace` are `true`.
Bugs: MESOS-7853
https://issues.apache.org/jira/browse/MESOS-7853
Repository: mesos
Description
-------
Added pid ns sharing based on agent flag and protobuf message field.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
Diff: https://reviews.apache.org/r/61428/diff/4/
Changes: https://reviews.apache.org/r/61428/diff/3-4/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/
-----------------------------------------------------------
(Updated Aug. 5, 2017, 11:06 p.m.)
Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-7853
https://issues.apache.org/jira/browse/MESOS-7853
Repository: mesos
Description
-------
Added pid ns sharing based on agent flag and protobuf message field.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
Diff: https://reviews.apache.org/r/61428/diff/3/
Changes: https://reviews.apache.org/r/61428/diff/2-3/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Qian Zhang <zh...@gmail.com>.
> On Aug. 5, 2017, 8:33 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
> > Lines 130 (patched)
> > <https://reviews.apache.org/r/61428/diff/2/?file=1789465#file1789465line132>
> >
> > Could we reverse two logics above? so that we can avoid the size check here. E.g.,
> > ```
> > if (sharePidNamespace) {
> > return launchInfo;
> > }
> > ```
> >
> > similar to the short circuit logic for DEBUG container.
Could you elaborate a bit more? Which two logics are you talking about?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/#review182235
-----------------------------------------------------------
On Aug. 5, 2017, 12:39 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61428/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2017, 12:39 a.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added pid ns sharing based on agent flag and protobuf message field.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
> src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
>
>
> Diff: https://reviews.apache.org/r/61428/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Qian Zhang <zh...@gmail.com>.
> On Aug. 5, 2017, 8:33 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
> > Lines 130 (patched)
> > <https://reviews.apache.org/r/61428/diff/2/?file=1789465#file1789465line132>
> >
> > Could we reverse two logics above? so that we can avoid the size check here. E.g.,
> > ```
> > if (sharePidNamespace) {
> > return launchInfo;
> > }
> > ```
> >
> > similar to the short circuit logic for DEBUG container.
>
> Qian Zhang wrote:
> Could you elaborate a bit more? Which two logics are you talking about?
>
> Gilbert Song wrote:
> Do you think this logic looks clearer (please help verify its correctness first)?
> ```
> ContainerLaunchInfo launchInfo;
>
> bool sharePidNamespace =
> containerConfig.container_info().linux_info().share_pid_namespace();
>
> if (containerId.has_parent()) {
> launchInfo.add_enter_namespaces(CLONE_NEWPID);
>
> if (containerConfig.has_container_class() &&
> containerConfig.container_class() == ContainerClass::DEBUG) {
> return launchInfo;
> }
> } else {
> if (flags.disallow_sharing_agent_pid_namespace && sharePidNamespace) {
> return Failure(
> "Sharing agent pid namespace with "
> "top-level container is not allowed");
> }
> }
>
> if (sharePidNamespace) {
> return launchInfo;
> }
>
> launchInfo.add_clone_namespaces(CLONE_NEWPID);
> launchInfo.add_pre_exec_commands()->set_value(
> "mount -n -t proc proc /proc -o nosuid,noexec,nodev");
>
> return launchInfo;
> ```
Yeah, it's correct and clearer, thanks Gilbert!
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/#review182235
-----------------------------------------------------------
On Aug. 8, 2017, 5:40 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61428/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2017, 5:40 p.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added pid ns sharing based on agent flag and protobuf message field.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
> src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
>
>
> Diff: https://reviews.apache.org/r/61428/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Gilbert Song <so...@gmail.com>.
> On Aug. 4, 2017, 5:33 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
> > Lines 130 (patched)
> > <https://reviews.apache.org/r/61428/diff/2/?file=1789465#file1789465line132>
> >
> > Could we reverse two logics above? so that we can avoid the size check here. E.g.,
> > ```
> > if (sharePidNamespace) {
> > return launchInfo;
> > }
> > ```
> >
> > similar to the short circuit logic for DEBUG container.
>
> Qian Zhang wrote:
> Could you elaborate a bit more? Which two logics are you talking about?
Do you think this logic looks clearer (please help verify its correctness first)?
```
ContainerLaunchInfo launchInfo;
bool sharePidNamespace =
containerConfig.container_info().linux_info().share_pid_namespace();
if (containerId.has_parent()) {
launchInfo.add_enter_namespaces(CLONE_NEWPID);
if (containerConfig.has_container_class() &&
containerConfig.container_class() == ContainerClass::DEBUG) {
return launchInfo;
}
} else {
if (flags.disallow_sharing_agent_pid_namespace && sharePidNamespace) {
return Failure(
"Sharing agent pid namespace with "
"top-level container is not allowed");
}
}
if (sharePidNamespace) {
return launchInfo;
}
launchInfo.add_clone_namespaces(CLONE_NEWPID);
launchInfo.add_pre_exec_commands()->set_value(
"mount -n -t proc proc /proc -o nosuid,noexec,nodev");
return launchInfo;
```
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/#review182235
-----------------------------------------------------------
On Aug. 6, 2017, 7:55 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61428/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2017, 7:55 p.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added pid ns sharing based on agent flag and protobuf message field.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
> src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
>
>
> Diff: https://reviews.apache.org/r/61428/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/#review182235
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
Lines 70 (patched)
<https://reviews.apache.org/r/61428/#comment258123>
one more space before {
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
Lines 130 (patched)
<https://reviews.apache.org/r/61428/#comment258141>
Could we reverse two logics above? so that we can avoid the size check here. E.g.,
```
if (sharePidNamespace) {
return launchInfo;
}
```
similar to the short circuit logic for DEBUG container.
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
Lines 115-117 (original), 135-140 (patched)
<https://reviews.apache.org/r/61428/#comment258140>
We can just return launchInfo, right?
- Gilbert Song
On Aug. 4, 2017, 9:39 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61428/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2017, 9:39 a.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added pid ns sharing based on agent flag and protobuf message field.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
> src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
>
>
> Diff: https://reviews.apache.org/r/61428/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Qian Zhang <zh...@gmail.com>.
> On Aug. 5, 2017, 8:42 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
> > Lines 85-86 (patched)
> > <https://reviews.apache.org/r/61428/diff/2/?file=1789465#file1789465line85>
> >
> > I know in proto2 and proto3 `boolean` defaults to be `false`. But how about `optional bool`? do you know if `has_share_pid_namespace` can be false?
Yes, `has_share_pid_namespace()` can be false if framework does not set the field `share_pid_namespace` at all, and in that case, we will get the default value (i.e., `false`).
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/#review182248
-----------------------------------------------------------
On Aug. 5, 2017, 12:39 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61428/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2017, 12:39 a.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added pid ns sharing based on agent flag and protobuf message field.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
> src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
>
>
> Diff: https://reviews.apache.org/r/61428/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/#review182248
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
Lines 85-86 (patched)
<https://reviews.apache.org/r/61428/#comment258142>
I know in proto2 and proto3 `boolean` defaults to be `false`. But how about `optional bool`? do you know if `has_share_pid_namespace` can be false?
- Gilbert Song
On Aug. 4, 2017, 9:39 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61428/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2017, 9:39 a.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
>
>
> Bugs: MESOS-7853
> https://issues.apache.org/jira/browse/MESOS-7853
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added pid ns sharing based on agent flag and protobuf message field.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
> src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
>
>
> Diff: https://reviews.apache.org/r/61428/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61428: Added pid ns sharing based on agent flag and
protobuf message field.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61428/
-----------------------------------------------------------
(Updated Aug. 5, 2017, 12:39 a.m.)
Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, Kevin Klues, and Vinod Kone.
Changes
-------
Minor changes.
Bugs: MESOS-7853
https://issues.apache.org/jira/browse/MESOS-7853
Repository: mesos
Description
-------
Added pid ns sharing based on agent flag and protobuf message field.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 2b316dbdf4a3735771af5bed80c6251d0d1cbd50
src/slave/containerizer/mesos/isolators/namespaces/pid.cpp f1dfc9f7398ffc029d7180d7f014a515338cb3f4
Diff: https://reviews.apache.org/r/61428/diff/2/
Changes: https://reviews.apache.org/r/61428/diff/1-2/
Testing
-------
Thanks,
Qian Zhang