Re: Review Request 24316: Use memory.memsw.limit_in_bytes in cgroup mem isolator with --limit-swap

[Jie Yu]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24316/#review49925
-----------------------------------------------------------


Getting close! Thanks Anton!


src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87368>

    Make that a field member of CgroupsMemIsolatorProcess



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87369>

    You don't need to check cgroups::memory::memsw_limit_in_bytes if 
flags.limit_swap is false, right? So here is what I would do:
    
    bool limitSwap = false;
    
    if (flags.limit_swap) {
      Result<Bytes> check = cgroups::memory::memsw_limit_in_bytes(...);
      if (check.isError()) {
        return Error(...);
      } else if (check.isNone()) {
        return Error(...);
      }
      
      limitSwap = true;
    }



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87385>

    At most one empty line here.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87376>

    Move this down right above 'if (limitSwap)'.
    
    Also, consider naming it as 'currentLimit' and use _ prefix for that 
returned from cgroups::memory::...



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87378>

    Result<Bytes> _currentLimit =
      cgroups::memory::memsw_limit_in_bytes(hierarchy, info->cgroup);



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87377>

    what about currentLimit.isNone()?
    
    if (_currentLimit.isError()) {
    } else if (_currentLimit.isNone()) {
    }
    
    currentLimit = _currentLimit.get();



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87379>

    Ditto.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87380>

    s/writeMemSw/write/



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87381>

    Again, what if write.isNone()?



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87382>

    Align it properly:
    
    return Failure(
        "..." + write.error());



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87383>

    Try<Nothing> write = ...



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87384>

    Kill blank line here.



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87372>

    Align '<<':
    
    LOG(ERROR) << "..."
               << limit.error();



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87374>

    Be consistent:
    
    Try<Bytes> limit =
      cgroups::memory::limit_in_bytes(hierarchy, info->cgroup);



src/slave/containerizer/isolators/cgroups/mem.cpp
<https://reviews.apache.org/r/24316/#comment87371>

    Align '<<':
    
    LOG(ERROR) << "..."
               << limit.error();


- Jie Yu


On Aug. 7, 2014, 2:56 p.m., Anton Lindström wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24316/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2014, 2:56 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-1662
>     https://issues.apache.org/jira/browse/MESOS-1662
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Changed memory.limit_in_bytes to memory.memsw.limit_in_bytes where memory is 
> limited. This makes Mesos limit both memory and swap as described in the Jira 
> ticket.
> 
> Using --limit-swap, it's possible to use memory.memsw.limit_in_bytes. If it 
> doesn't exist it will fallback on memory.limit_in_bytes.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.hpp 9dfba6eb9059728386d053f8c2e92edab0caa295 
>   src/linux/cgroups.cpp 39a4874ee0030110d7a61f1b73653af116d66549 
>   src/slave/containerizer/isolators/cgroups/mem.cpp 
> e8160efc4f5baa06470a8a3c774bf97d5b9e83cc 
>   src/slave/flags.hpp 841de2364085c66e4272734d5b11dbf8efa845a5 
> 
> Diff: https://reviews.apache.org/r/24316/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anton Lindström
> 
>