You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/12/15 20:36:50 UTC
Re: Review Request 33174: Fix for docker not configuring CFS quotas
correctly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33174/#review110547
-----------------------------------------------------------
Initial CFS parameters should be specified to Docker using the appropriate flags, not tacked onto the end of launch() where we don't yet know the cgroup. Subsequent updates done by Mesos in update().
src/slave/containerizer/docker.cpp (line 838)
<https://reviews.apache.org/r/33174/#comment170483>
Docker supports specifying the CFS period and quota to run a container with these flags {{--cpu-period=0}} and {{--cpu-quota=0}}. The correct solution is to set these using Docker itself when it runs the container. Subsequent updates will be done by Mesos in the update() call, as below in this review.
src/slave/containerizer/docker.cpp (line 839)
<https://reviews.apache.org/r/33174/#comment170480>
FYI, it would be {{.then([]() { return true; });}} but this piece of code isn't needed, see previous comment.
src/slave/containerizer/docker.cpp (line 976)
<https://reviews.apache.org/r/33174/#comment170475>
To reiterate my earlier comment:
1) We don't support changing the cfs period so it only needs to be set once, additional writes are unnecessary but aren't a problem. The MesosContainerizer isolator just writes it on every update rather than determining if it has already written it before.
2) The cfs quota changes depending on the CPU resource value so it definitely does need to be re-written at every update. Again, this is a straight copy-and-paste from the MC cpu isolator.
This part of the code looks fine to me. @Tim, do you concur?
- Ian Downes
On April 14, 2015, 1:32 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> -----------------------------------------------------------
>
> (Updated April 14, 2015, 1:32 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix for docker containerizer not configuring CFS quotas correctly.
>
> It would be nice to refactor all this isolation code in a way that can be shared between all containerizers, as this is basically just copied from the CgroupsCpushareIsolator, but that's a much bigger undertaking.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.cpp 5f4b4ce49a9523e4743e5c79da4050e6f9e29ed7
>
> Diff: https://reviews.apache.org/r/33174/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 33174: Fix for docker not configuring CFS quotas
correctly
Posted by Steve Niemitz <sn...@twitter.com>.
> On Dec. 15, 2015, 7:36 p.m., Ian Downes wrote:
> > Initial CFS parameters should be specified to Docker using the appropriate flags, not tacked onto the end of launch() where we don't yet know the cgroup. Subsequent updates done by Mesos in update().
As per our offline conversation, in this code path by the time the code is called we do have the executor PID, and the executor is in the correct cgroup (eg inside the docker container). I really would rather have a single way to set/update the CFS parameters, much like how the cgroup isolator calls update(...) from its prepare(...) method.
However, I agree the other case does need to be addressed, either in this review or a follow up, where tasks launched with the docker executor still don't get their CFS parameters updated.
- Steve
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33174/#review110547
-----------------------------------------------------------
On Dec. 15, 2015, 8:14 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 8:14 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix for docker containerizer not configuring CFS quotas correctly.
>
> It would be nice to refactor all this isolation code in a way that can be shared between all containerizers, as this is basically just copied from the CgroupsCpushareIsolator, but that's a much bigger undertaking.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e
>
> Diff: https://reviews.apache.org/r/33174/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Steve Niemitz
>
>
Re: Review Request 33174: Fix for docker not configuring CFS quotas
correctly
Posted by Ian Downes <ia...@gmail.com>.
> On Dec. 15, 2015, 11:36 a.m., Ian Downes wrote:
> > src/slave/containerizer/docker.cpp, line 838
> > <https://reviews.apache.org/r/33174/diff/2/?file=927472#file927472line838>
> >
> > Docker supports specifying the CFS period and quota to run a container with these flags {{--cpu-period=0}} and {{--cpu-quota=0}}. The correct solution is to set these using Docker itself when it runs the container. Subsequent updates will be done by Mesos in the update() call, as below in this review.
Please add a TODO for this.
> On Dec. 15, 2015, 11:36 a.m., Ian Downes wrote:
> > src/slave/containerizer/docker.cpp, line 976
> > <https://reviews.apache.org/r/33174/diff/2/?file=927472#file927472line976>
> >
> > To reiterate my earlier comment:
> > 1) We don't support changing the cfs period so it only needs to be set once, additional writes are unnecessary but aren't a problem. The MesosContainerizer isolator just writes it on every update rather than determining if it has already written it before.
> > 2) The cfs quota changes depending on the CPU resource value so it definitely does need to be re-written at every update. Again, this is a straight copy-and-paste from the MC cpu isolator.
> >
> > This part of the code looks fine to me. @Tim, do you concur?
@tnachen?
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33174/#review110547
-----------------------------------------------------------
On Dec. 15, 2015, 12:14 p.m., Steve Niemitz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 12:14 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix for docker containerizer not configuring CFS quotas correctly.
>
> It would be nice to refactor all this isolation code in a way that can be shared between all containerizers, as this is basically just copied from the CgroupsCpushareIsolator, but that's a much bigger undertaking.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e
>
> Diff: https://reviews.apache.org/r/33174/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Steve Niemitz
>
>