> On April 18, 2015, 10:43 a.m., Timothy Chen wrote: > > src/slave/containerizer/docker.cpp, line 838 > > <https://reviews.apache.org/r/33174/diff/2/?file=927472#file927472line838> > > > > The pid here is the executor pid, which is not the docker container > > pid, so setting cgroup on the command executor doesn't buy you much. > > Steve Niemitz wrote: > So what is your suggestion?
Look at how update() does it, we need to ask Docker what the actual pid of the container and use that pid to update it's cgroups. > On April 18, 2015, 10:43 a.m., Timothy Chen wrote: > > src/slave/containerizer/docker.cpp, line 976 > > <https://reviews.apache.org/r/33174/diff/2/?file=927472#file927472line976> > > > > We shouldn't need to call update right after for cfs, and also this > > seems like it's going to write to cfs cgroup every update, which seems > > wrong too. > > > > If we want to set up this we should have a patch in launch that is only > > responsible for setting cfs if it's configured. > > Steve Niemitz wrote: > Isn't that exactly what I'm doing? This update() is now called from > launch to set the CFS quota (and all quotas). > The normaly update() path only checks if the resources are different, and > if they aren't doesn't write anything to the cgroups. So I don't know what > you mean by "and also this seems like it's going to write to cfs cgroup every > update." > Also, why would it be different than the non-CFS code above it? Wouldn't > that have the same issue of writing "every update"? So I think what's I'm trying to convey is that update() has a specific meaning to the slave and it's being called during different times (task launched, destroy, etc) for the same executor. The CFS values only need to be setup once per container, so we should create a new method and let it being used just for launch instead of modifying update, since update will be called several times. We change update unless we want the CFS quota to change over the lifetime of the container, which I don't believe so. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33174/#review80595 ----------------------------------------------------------- On April 14, 2015, 8: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, 8: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 > >
