Re: Review Request 61428: Added pid ns sharing based on agent flag and protobuf message field.

[Gilbert Song]

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

[Qian Zhang]

> On Aug. 5, 2017, 8:33 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
> > Lines 130 (patched)
> > 
> >
> > 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.

[Qian Zhang]

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

[Gilbert Song]

> On Aug. 4, 2017, 5:33 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
> > Lines 130 (patched)
> > 
> >
> > 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.

[Qian Zhang]

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

[Qian Zhang]

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

[Qian Zhang]

> On Aug. 5, 2017, 8:33 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
> > Lines 130 (patched)
> > 
> >
> > 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.

[Qian Zhang]

> On Aug. 5, 2017, 8:42 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
> > Lines 85-86 (patched)
> > 
> >
> > 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.

[Gilbert Song]

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


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.

[Gilbert Song]

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


one more space before {



src/slave/containerizer/mesos/isolators/namespaces/pid.cpp
Lines 130 (patched)


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)


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.

[Qian Zhang]

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