You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by "Timothy St. Clair" <ts...@redhat.com> on 2014/06/03 03:39:52 UTC
Re: Review Request 21799: Fixes to enable cgroup + systemd.slice behavior
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21799/
-----------------------------------------------------------
(Updated June 3, 2014, 1:39 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Changes
-------
minor update per review.
Bugs: MESOS-1195
https://issues.apache.org/jira/browse/MESOS-1195
Repository: mesos-git
Description
-------
Fixes to enable cgroups to function when initialized via systemd defaults:
+ Fix for co-mounting cpu,cpuacct
+ Fix for nested cgroups_root='system.slice/mesos-slave.service'
+ Diagnostics to propagate error details.
Diffs (updated)
-----
src/linux/cgroups.hpp 21d87a0
src/linux/cgroups.cpp 142ac43
src/slave/containerizer/isolators/cgroups/mem.cpp f2509ad
Diff: https://reviews.apache.org/r/21799/diff/
Testing
-------
make check
Thanks,
Timothy St. Clair
Re: Review Request 21799: Fixes to enable cgroup + systemd.slice behavior
Posted by "Timothy St. Clair" <ts...@redhat.com>.
> On June 19, 2014, 1:46 a.m., Mesos ReviewBot wrote:
> > Patch looks great!
> >
> > Reviews applied: [21799]
> >
> > All tests passed.
Any chance of getting to this one? It's pretty critical for folks downstream.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21799/#review46168
-----------------------------------------------------------
On June 18, 2014, 8:24 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21799/
> -----------------------------------------------------------
>
> (Updated June 18, 2014, 8:24 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes to enable cgroups to function when initialized via systemd defaults:
> + Fix for co-mounting cpu,cpuacct
> + Fix for nested cgroups_root='system.slice/mesos-slave.service'
> + Diagnostics to propagate error details.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 75c5602
> src/linux/cgroups.cpp a4274a8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.cpp 3b731f9
>
> Diff: https://reviews.apache.org/r/21799/diff/
>
>
> Testing
> -------
>
> make check +
>
> ran jobs via Marathon and verified isolation.
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 21799: Fixes to enable cgroup + systemd.slice behavior
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21799/#review46168
-----------------------------------------------------------
Patch looks great!
Reviews applied: [21799]
All tests passed.
- Mesos ReviewBot
On June 18, 2014, 8:24 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21799/
> -----------------------------------------------------------
>
> (Updated June 18, 2014, 8:24 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes to enable cgroups to function when initialized via systemd defaults:
> + Fix for co-mounting cpu,cpuacct
> + Fix for nested cgroups_root='system.slice/mesos-slave.service'
> + Diagnostics to propagate error details.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 75c5602
> src/linux/cgroups.cpp a4274a8
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.cpp 3b731f9
>
> Diff: https://reviews.apache.org/r/21799/diff/
>
>
> Testing
> -------
>
> make check +
>
> ran jobs via Marathon and verified isolation.
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 21799: Fixes to enable cgroup + systemd.slice behavior
Posted by "Timothy St. Clair" <ts...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21799/
-----------------------------------------------------------
(Updated June 18, 2014, 8:24 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Changes
-------
Update per-reviews plus additional testing.
Bugs: MESOS-1195
https://issues.apache.org/jira/browse/MESOS-1195
Repository: mesos-git
Description
-------
Fixes to enable cgroups to function when initialized via systemd defaults:
+ Fix for co-mounting cpu,cpuacct
+ Fix for nested cgroups_root='system.slice/mesos-slave.service'
+ Diagnostics to propagate error details.
Diffs (updated)
-----
src/linux/cgroups.hpp 75c5602
src/linux/cgroups.cpp a4274a8
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.cpp 3b731f9
Diff: https://reviews.apache.org/r/21799/diff/
Testing (updated)
-------
make check +
ran jobs via Marathon and verified isolation.
Thanks,
Timothy St. Clair
Re: Review Request 21799: Fixes to enable cgroup + systemd.slice behavior
Posted by "Timothy St. Clair" <ts...@redhat.com>.
> On June 4, 2014, 6:39 p.m., Vinod Kone wrote:
> > src/linux/cgroups.cpp, lines 455-464
> > <https://reviews.apache.org/r/21799/diff/2/?file=602349#file602349line455>
> >
> > Lets split this into its own review, since this is a change that has bigger impact.
re: splitting, this is the primary issue here.
There are a couple of minor bug fixes that get reported back, which could be split out, but I see no need.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21799/#review44734
-----------------------------------------------------------
On June 3, 2014, 1:39 a.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21799/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 1:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes to enable cgroups to function when initialized via systemd defaults:
> + Fix for co-mounting cpu,cpuacct
> + Fix for nested cgroups_root='system.slice/mesos-slave.service'
> + Diagnostics to propagate error details.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 21d87a0
> src/linux/cgroups.cpp 142ac43
> src/slave/containerizer/isolators/cgroups/mem.cpp f2509ad
>
> Diff: https://reviews.apache.org/r/21799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 21799: Fixes to enable cgroup + systemd.slice behavior
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21799/#review44734
-----------------------------------------------------------
src/linux/cgroups.hpp
<https://reviews.apache.org/r/21799/#comment79229>
Is this statement about not creating cgroups recursively still true?
src/linux/cgroups.hpp
<https://reviews.apache.org/r/21799/#comment79225>
Please use the same multi-argument format as above.
Try<Nothing> create(
const std::string& hierarchy,
const std::string& cgroup,
bool recursive = false);
src/linux/cgroups.cpp
<https://reviews.apache.org/r/21799/#comment79233>
ditto. is the last statement still valid?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/21799/#comment79231>
formatting. see above.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/21799/#comment79241>
Lets split this into its own review, since this is a change that has bigger impact.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/21799/#comment79237>
formatting.
src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/21799/#comment79239>
if exists.get() is false then this will throw an exception!
instead do,
(exists.isError() ? exists.error() : "does not exist");
- Vinod Kone
On June 3, 2014, 1:39 a.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21799/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 1:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes to enable cgroups to function when initialized via systemd defaults:
> + Fix for co-mounting cpu,cpuacct
> + Fix for nested cgroups_root='system.slice/mesos-slave.service'
> + Diagnostics to propagate error details.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 21d87a0
> src/linux/cgroups.cpp 142ac43
> src/slave/containerizer/isolators/cgroups/mem.cpp f2509ad
>
> Diff: https://reviews.apache.org/r/21799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 21799: Fixes to enable cgroup + systemd.slice behavior
Posted by "Timothy St. Clair" <ts...@redhat.com>.
> On June 9, 2014, 5:17 a.m., Ian Downes wrote:
> > src/linux/cgroups.cpp, line 855
> > <https://reviews.apache.org/r/21799/diff/2/?file=602349#file602349line855>
> >
> > Why is recursively creating the cgroup needed now when it wasn't before?
systemd creates a nested structure.
> On June 9, 2014, 5:17 a.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/cgroups/mem.cpp, line 98
> > <https://reviews.apache.org/r/21799/diff/2/?file=602350#file602350line98>
> >
> > Can you pull the mem isolator clean ups into a separate review?
I need to fix Jie's issues 1st, then it makes sense to assess overall impact. We "could" but those additions helped in diagnosing what the problems were.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21799/#review45052
-----------------------------------------------------------
On June 3, 2014, 1:39 a.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21799/
> -----------------------------------------------------------
>
> (Updated June 3, 2014, 1:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes to enable cgroups to function when initialized via systemd defaults:
> + Fix for co-mounting cpu,cpuacct
> + Fix for nested cgroups_root='system.slice/mesos-slave.service'
> + Diagnostics to propagate error details.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 21d87a0
> src/linux/cgroups.cpp 142ac43
> src/slave/containerizer/isolators/cgroups/mem.cpp f2509ad
>
> Diff: https://reviews.apache.org/r/21799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>
Re: Review Request 21799: Fixes to enable cgroup + systemd.slice behavior
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21799/#review45052
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/21799/#comment79743>
What about writing this so it looks more like a set equality test, i.e.,
attached.contains("cpu") && attached.contains("cpuacct") && attached.size() == 2
src/linux/cgroups.cpp
<https://reviews.apache.org/r/21799/#comment79745>
Why is recursively creating the cgroup needed now when it wasn't before?
src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/21799/#comment79744>
Can you pull the mem isolator clean ups into a separate review?
- Ian Downes
On June 2, 2014, 6:39 p.m., Timothy St. Clair wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21799/
> -----------------------------------------------------------
>
> (Updated June 2, 2014, 6:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Bugs: MESOS-1195
> https://issues.apache.org/jira/browse/MESOS-1195
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes to enable cgroups to function when initialized via systemd defaults:
> + Fix for co-mounting cpu,cpuacct
> + Fix for nested cgroups_root='system.slice/mesos-slave.service'
> + Diagnostics to propagate error details.
>
>
> Diffs
> -----
>
> src/linux/cgroups.hpp 21d87a0
> src/linux/cgroups.cpp 142ac43
> src/slave/containerizer/isolators/cgroups/mem.cpp f2509ad
>
> Diff: https://reviews.apache.org/r/21799/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy St. Clair
>
>