mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 49819: Implemented `CgroupsIsolatorProcess::prepare`.
Date Tue, 26 Jul 2016 22:03:41 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message