----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49819/#review143618 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 255) <https://reviews.apache.org/r/49819/#comment209438> no need for the quote here because containerId is generated by the agent. src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 258 - 261) <https://reviews.apache.org/r/49819/#comment209450> I'd move this down right above when we actually need 'user'. I.e., ``` if (containerConfig.has_user()) { Try<Nothing> chown = os::chown( containerConfig.user(), path::join(hierarchy, cgroup), false); ... } ``` src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 263 - 264) <https://reviews.apache.org/r/49819/#comment209443> I'd rephrase here: ``` We save 'Info' into 'infos' first so that even if 'prepare' fails, we can properly cleanup the *side effects* created below. ``` src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 271 - 272) <https://reviews.apache.org/r/49819/#comment209444> Ditto on no quote for containerid. src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 275) <https://reviews.apache.org/r/49819/#comment209442> I'd just inline `createCgroup` here as it's only used here. src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 287 - 288) <https://reviews.apache.org/r/49819/#comment209451> Do you need the temp variable? ``` prepares.push_back(subsystem->prepare(containerId)); ``` src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 296) <https://reviews.apache.org/r/49819/#comment209456> I would add a TODO here sayting that here we assume command executor's resources include the task's resources. Revisit here if this semantics changes. src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 297) <https://reviews.apache.org/r/49819/#comment209452> Move this to the end of the above line. src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 311) <https://reviews.apache.org/r/49819/#comment209445> ``` return Error( "Failed to check the existence of cgroup at 'xxx': <error>");``` src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 313 - 314) <https://reviews.apache.org/r/49819/#comment209446> Let's be consistent on the indentation for multi-line arguments. I'd prefer consistently putting it into the next line: ``` return Error( "Cgroup at 'xxx' already exists"); ``` src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 319) <https://reviews.apache.org/r/49819/#comment209447> ``` "Failed to create the cgroup at 'xxx': <error>" ``` src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 322 - 324) <https://reviews.apache.org/r/49819/#comment209448> I'd add a TODO here. To me, this is not the best way to achieve the goal. For instance, two tasks under the same user can change cgroups setting for each other. The right solution should be using cgroups namespaces + user namespaces. src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 332) <https://reviews.apache.org/r/49819/#comment209449> ``` "Failed to chown the cgroup at `xxx`: <error>" ``` - Jie Yu On July 26, 2016, 5:10 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49819/ > ----------------------------------------------------------- > > (Updated July 26, 2016, 5:10 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha. > > > Bugs: MESOS-5041 > https://issues.apache.org/jira/browse/MESOS-5041 > > > Repository: mesos > > > Description > ------- > > Implemented `CgroupsIsolatorProcess::prepare`. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp > c57baec88437f68886702a40ec8a6a6458546119 > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp > 348f105f9c3109a02f1dde0649f1b829cb9ddd04 > > Diff: https://reviews.apache.org/r/49819/diff/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
