You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@mesos.apache.org by Anton Lindström <an...@alley.se> on 2014/08/05 18:05:30 UTC

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

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

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 c571e915fec461485d2015f58e6e828b00ea089a 
  src/linux/cgroups.cpp ccb86cf172c3c97513b16b90756d7bc12aad278f 
  src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35a1acdb3beb03a91cfd734a59417657b20 
  src/slave/flags.hpp 5ea2f38df5a777f275136b7849d77cb461a32f8d 

Diff: https://reviews.apache.org/r/24316/diff/


Testing
-------

make check


Thanks,

Anton Lindström

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

Posted by Mesos ReviewBot <de...@mesos.apache.org>.

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


Bad patch!

Reviews applied: [24316]

Failed command: git apply --index 24316.patch

Error:
 error: patch failed: src/linux/cgroups.hpp:430
error: src/linux/cgroups.hpp: patch does not apply
error: patch failed: src/linux/cgroups.cpp:1899
error: src/linux/cgroups.cpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/cgroups/mem.cpp:279
error: src/slave/containerizer/isolators/cgroups/mem.cpp: patch does not apply


- Mesos ReviewBot


On Aug. 5, 2014, 4:05 p.m., Anton Lindström wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24316/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2014, 4:05 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 c571e915fec461485d2015f58e6e828b00ea089a 
>   src/linux/cgroups.cpp ccb86cf172c3c97513b16b90756d7bc12aad278f 
>   src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35a1acdb3beb03a91cfd734a59417657b20 
>   src/slave/flags.hpp 5ea2f38df5a777f275136b7849d77cb461a32f8d 
> 
> Diff: https://reviews.apache.org/r/24316/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anton Lindström
> 
>

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

Posted by Jie Yu <yu...@gmail.com>.

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



src/linux/cgroups.cpp
<https://reviews.apache.org/r/24316/#comment87175>

    This should fit in one line



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

    Hum, here is what I think is the right way to do this:
    
    1) Added a field 'limitSwap' in the isolator class.
    
    2) In ::create(), based on the flag.limit_swap and whether "memory.memsw.limit_in_bytes" is available (by calling cgroups::memory::memsw_limit_in_bytes() on cgroups_root), set 'limitSwap' field accordingly (when create the isolator). More specifically:
      - if flag.limit_swap == false, set limitSwap to false
      - if flag.limit_swap == true but memsw_limit_in_bytes is not available, return error on ::create
      - if flag.limit_swap = true and memsw_limit_in_bytes is available, set limitSwap to true
    
    3) If 'limitSwap == true', we always read/write memsw_limit_in_bytes. Otherwise, we always read/write limit_in_bytes.



src/slave/flags.hpp
<https://reviews.apache.org/r/24316/#comment87178>

    Add a period at the end of a sentence.


- Jie Yu


On Aug. 6, 2014, 6:52 p.m., Anton Lindström wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24316/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2014, 6:52 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
> 
>

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

Posted by Jie Yu <yu...@gmail.com>.

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

Ship it!


Please update the diff. I'll get it committed tomorrow! Thanks!


src/linux/cgroups.hpp
<https://reviews.apache.org/r/24316/#comment87486>

    Ditto BenM's comment, we may want to return Try<bool> here (false means the memsw does not exist like when swap is disabled).



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

    s/static bool/const bool/
    
    ALso, move it below 'const string hierarchy'.



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

    Do not use static field. What if in the tests, we want to create two memory isolators , one of them has swap turned on and the other has swap turned off?



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

    return Error("'memory.memsw.limit_in_bytes' is not available");



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

    process::Owned<IsolatorProcess> process(
        new CgroupsMemIsolatorProcess(flags, hierarchy.get(), limitSwap));



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

    return Failure("'memory.memsw.limit_in_bytes' is not available");



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

    To be consistent with below:
    
    return Failure(
        "Failed to set 'memory.memsw.limit_in_bytes': " + write.error());



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

    return Failure("'memory.memsw.limit_in_bytes' is not available");



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

    indent 4 spaces (instead of 2).



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

    What if 'limit.isNone()?'


- Jie Yu


On Aug. 7, 2014, 9:09 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, 9:09 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.hpp 6869ed4a9fa9a1a719e695ac83aeec88ead0efc0 
>   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
> 
>

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

Posted by Vinod Kone <vi...@gmail.com>.

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



src/slave/flags.hpp
<https://reviews.apache.org/r/24316/#comment87496>

    fly by review:
    
    move this flag below cgroups_enable_cfs flag.
    
    also, s/limit_swap/cgroups_limit_swap/


- Vinod Kone


On Aug. 7, 2014, 9:09 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, 9:09 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.hpp 6869ed4a9fa9a1a719e695ac83aeec88ead0efc0 
>   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
> 
>