Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Sept. 22, 2014, 8:35 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 22, 2014, 8:35 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/containerizer/linux_launcher.cpp 
> d5ef1d6aa762cf81a3e8384552d97fe95b9cbd95 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Jie Yu]

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

(Updated Sept. 22, 2014, 8:35 p.m.)


Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and Vinod 
Kone.


Changes
---

Review comments.


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


Repository: mesos-git


Description
---

A dynamic version after discussed with Tim.
https://reviews.apache.org/r/25695

Did a few consistency fixes as well.


Diffs (updated)
-

  src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
  src/slave/containerizer/isolators/cgroups/cpushare.hpp 
d4df5f37e8d2e356d35ca40d799197a47393fa9a 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp 
b1cad472a561e81422f980182fd24eb95701140a 
  src/slave/containerizer/isolators/cgroups/mem.cpp 
fb3db88af7b2ffa79272743f571c4c021c619c48 
  src/slave/containerizer/isolators/cgroups/perf_event.cpp 
ff047d37c1b2e659b18b5d4a1e97301192d05e55 
  src/slave/containerizer/linux_launcher.cpp 
d5ef1d6aa762cf81a3e8384552d97fe95b9cbd95 
  src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 

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


Testing
---

make check
sudo make check


Thanks,

Jie Yu

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Timothy Chen]

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



src/linux/cgroups.cpp


Failed to check existence of the nested test cgroup


- Timothy Chen


On Sept. 20, 2014, 12:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 20, 2014, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Timothy St. Clair]

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

Ship it!


Ship It!

- Timothy St. Clair


On Sept. 20, 2014, 12:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 20, 2014, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Jie Yu]

> On Sept. 22, 2014, 5:33 p.m., Ian Downes wrote:
> > src/linux/cgroups.cpp, line 517
> > 
> >
> > Not your code, but it's a little silly to do this every time...

Added a TODO.


- Jie


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


On Sept. 20, 2014, 12:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 20, 2014, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Ian Downes]

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

Ship it!



src/linux/cgroups.cpp


Not your code, but it's a little silly to do this every time...



src/slave/containerizer/isolators/cgroups/cpushare.hpp


I don't think it's exclusively systems using systemd. What about, "(e.g., 
systems using systemd)" ?



src/slave/containerizer/isolators/cgroups/cpushare.cpp


So cgroups::destroy gets called with col-located hierarchies? Do we not 
need to leave this to systemd to clean up?



src/slave/slave.cpp


keep the NOTE on a new line?


- Ian Downes


On Sept. 19, 2014, 5:21 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 19, 2014, 5:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Jie Yu]

> On Sept. 22, 2014, 4:31 p.m., Timothy St. Clair wrote:
> > Thanks Jie! 1st pass testing worked, but I need to beat on it some more. 
> > 
> > Normally I would say we should probably separate out the cleanup work from 
> > the feature mod, but it's not that important to me.

Sorry about that. I should have separated this patch:)


- Jie


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


On Sept. 20, 2014, 12:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 20, 2014, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Jie Yu]

> On Sept. 22, 2014, 4:31 p.m., Timothy St. Clair wrote:
> > src/linux/cgroups.cpp, line 1795
> > 
> >
> > I think I've missed something subtle, how did you bypass the cleanup 
> > semantics?

The isolators won't try to unmount and rm the hierarchies (i.e., it won't call 
cgroups::cleanup).

The only place we call cgroups::cleanup is in tests, I will do a sweep today to 
make sure we don't accidentally umount systemd managed hierarchies.


> On Sept. 22, 2014, 4:31 p.m., Timothy St. Clair wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, line 112
> > 
> >
> > So does this rely on realpath squashing the symbolic link such that 
> > they are = ?

In fact, we obtain the hierarchy from /proc/mounts. Yes, we also call realpath. 
So I think this part is safe.


- Jie


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


On Sept. 20, 2014, 12:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 20, 2014, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Timothy St. Clair]

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


Thanks Jie! 1st pass testing worked, but I need to beat on it some more. 

Normally I would say we should probably separate out the cleanup work from the 
feature mod, but it's not that important to me.


src/linux/cgroups.cpp


I think I've missed something subtle, how did you bypass the cleanup 
semantics?



src/slave/containerizer/isolators/cgroups/cpushare.cpp






src/slave/containerizer/isolators/cgroups/cpushare.cpp


So does this rely on realpath squashing the symbolic link such that they 
are = ?


- Timothy St. Clair


On Sept. 20, 2014, 12:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 20, 2014, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [25858]

All tests passed.

- Mesos ReviewBot


On Sept. 20, 2014, 12:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25858/
> ---
> 
> (Updated Sept. 20, 2014, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> A dynamic version after discussed with Tim.
> https://reviews.apache.org/r/25695
> 
> Did a few consistency fixes as well.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
>   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
> d4df5f37e8d2e356d35ca40d799197a47393fa9a 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
> b1cad472a561e81422f980182fd24eb95701140a 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> fb3db88af7b2ffa79272743f571c4c021c619c48 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> ff047d37c1b2e659b18b5d4a1e97301192d05e55 
>   src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 
> 
> Diff: https://reviews.apache.org/r/25858/diff/
> 
> 
> Testing
> ---
> 
> make check
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 25858: Allowed co-mounted cgroup subsystems to enable Mesos on machines with systemd.

[Jie Yu]

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

Review request for mesos, Ben Mahler, Ian Downes, Timothy St. Clair, and Vinod 
Kone.


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


Repository: mesos-git


Description
---

A dynamic version after discussed with Tim.
https://reviews.apache.org/r/25695

Did a few consistency fixes as well.


Diffs
-

  src/linux/cgroups.cpp 5093b4ca1ac17238234d96613b7f4ceab4373c48 
  src/slave/containerizer/isolators/cgroups/cpushare.hpp 
d4df5f37e8d2e356d35ca40d799197a47393fa9a 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp 
b1cad472a561e81422f980182fd24eb95701140a 
  src/slave/containerizer/isolators/cgroups/mem.cpp 
fb3db88af7b2ffa79272743f571c4c021c619c48 
  src/slave/containerizer/isolators/cgroups/perf_event.cpp 
ff047d37c1b2e659b18b5d4a1e97301192d05e55 
  src/slave/slave.cpp 28eb02852ddcc10efe589a8069dba9c895bc160e 

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


Testing
---

make check
sudo make check


Thanks,

Jie Yu