You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2017/03/14 00:04:41 UTC
Review Request 57574: Ensured the cgroup memory+swap limit is no less
than the memory limit.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57574/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, haosdent huang, Ian Downes, and Kevin Klues.
Bugs: MESOS-7237
https://issues.apache.org/jira/browse/MESOS-7237
Repository: mesos
Description
-------
This addressed the bug we saw in MESOS-7237. Linux kernel will ensure
cgroup memory+swap limit (i.e., 'memory.memsw.limit_in_bytes') is always
no less than the memory limit ('memory.limit_in_bytes'). Prior to this
patch, the memory subsystem in the cgroups isolator is buggy because
memory+swap limit is always changed after the memory limit. This will
cause EINVAL if we increase the memory limit of the container.
Diffs
-----
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 009e996add25e43e8989600a58b38f2cc88a5d11
Diff: https://reviews.apache.org/r/57574/diff/1/
Testing
-------
sudo make check
Thanks,
Jie Yu
Re: Review Request 57574: Ensured the cgroup memory+swap limit is no
less than the memory limit.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57574/#review168863
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 214-221 (original), 214-221 (patched)
<https://reviews.apache.org/r/57574/#comment241153>
Move these comments down to line 273?
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 266-271 (patched)
<https://reviews.apache.org/r/57574/#comment241155>
Move this to line 282 right above the conditional. It helps understand the order of the functions set better.
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 284 (patched)
<https://reviews.apache.org/r/57574/#comment241156>
Just for clarification maybe add a note:
```
NOTE: The memory limit is already set to the max, so effectively we are reducing the memory limits because of which we need to set the 'memory.limit_in_bytes' before setting 'memory.memsw.limit_in_bytes'
- Avinash sridharan
On March 14, 2017, 12:04 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57574/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 12:04 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, haosdent huang, Ian Downes, and Kevin Klues.
>
>
> Bugs: MESOS-7237
> https://issues.apache.org/jira/browse/MESOS-7237
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addressed the bug we saw in MESOS-7237. Linux kernel will ensure
> cgroup memory+swap limit (i.e., 'memory.memsw.limit_in_bytes') is always
> no less than the memory limit ('memory.limit_in_bytes'). Prior to this
> patch, the memory subsystem in the cgroups isolator is buggy because
> memory+swap limit is always changed after the memory limit. This will
> cause EINVAL if we increase the memory limit of the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 009e996add25e43e8989600a58b38f2cc88a5d11
>
>
> Diff: https://reviews.apache.org/r/57574/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 57574: Ensured the cgroup memory+swap limit is no
less than the memory limit.
Posted by haosdent huang <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57574/#review168856
-----------------------------------------------------------
Ship it!
Ship It!
- haosdent huang
On March 14, 2017, 12:04 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57574/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 12:04 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, haosdent huang, Ian Downes, and Kevin Klues.
>
>
> Bugs: MESOS-7237
> https://issues.apache.org/jira/browse/MESOS-7237
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addressed the bug we saw in MESOS-7237. Linux kernel will ensure
> cgroup memory+swap limit (i.e., 'memory.memsw.limit_in_bytes') is always
> no less than the memory limit ('memory.limit_in_bytes'). Prior to this
> patch, the memory subsystem in the cgroups isolator is buggy because
> memory+swap limit is always changed after the memory limit. This will
> cause EINVAL if we increase the memory limit of the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 009e996add25e43e8989600a58b38f2cc88a5d11
>
>
> Diff: https://reviews.apache.org/r/57574/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 57574: Ensured the cgroup memory+swap limit is no
less than the memory limit.
Posted by Jie Yu <yu...@gmail.com>.
> On March 14, 2017, 4:56 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
> > Lines 266-270 (patched)
> > <https://reviews.apache.org/r/57574/diff/1/?file=1662928#file1662928line277>
> >
> > It's not clear to me how we ensure this invariant with the code below. Maybe just updating the comment is enough to clear things up, but when I look at thsi code, it's not obvious to me how this invariant is mantained.
> >
> > Is the assumption that `memory.limit_in_bytes` and `memory.memsw.limit_in_bytes` are initially equal? Is it also assumed that `currentLimit` will only ever be greater than or equal to the `initialLimit` the first time through this function. If so, why?
> >
> > What keeps us from passing a `limit` to this function that exceeds `initialLimit` such that the case for `(currentLimit.get() >= initialLimit)` gets invoked more than just the first time this function is called?
> >
> > Independent of that, I don't understand how we can ensure that `memory.limit_in_bytes` always remains less than or equal `memory.memsw.limit_in_bytes` in the case of `(limit > currentLimit.get())`. If the new `limit` is greater than the `currentLimit` and we set `memory.memsw.limit_in_bytes` before we set `memory.limit_in_bytes`, doesn't that explicitly violate this invariant?
> >
> > Maybe I'm missing something.
Discussed offline. I adjusted some comments and variable naming to make it more clear.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57574/#review168864
-----------------------------------------------------------
On March 14, 2017, 12:04 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57574/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 12:04 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, haosdent huang, Ian Downes, and Kevin Klues.
>
>
> Bugs: MESOS-7237
> https://issues.apache.org/jira/browse/MESOS-7237
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addressed the bug we saw in MESOS-7237. Linux kernel will ensure
> cgroup memory+swap limit (i.e., 'memory.memsw.limit_in_bytes') is always
> no less than the memory limit ('memory.limit_in_bytes'). Prior to this
> patch, the memory subsystem in the cgroups isolator is buggy because
> memory+swap limit is always changed after the memory limit. This will
> cause EINVAL if we increase the memory limit of the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 009e996add25e43e8989600a58b38f2cc88a5d11
>
>
> Diff: https://reviews.apache.org/r/57574/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 57574: Ensured the cgroup memory+swap limit is no
less than the memory limit.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57574/#review168864
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 266-270 (patched)
<https://reviews.apache.org/r/57574/#comment241154>
It's not clear to me how we ensure this invariant with the code below. Maybe just updating the comment is enough to clear things up, but when I look at thsi code, it's not obvious to me how this invariant is mantained.
Is the assumption that `memory.limit_in_bytes` and `memory.memsw.limit_in_bytes` are initially equal? Is it also assumed that `currentLimit` will only ever be greater than or equal to the `initialLimit` the first time through this function. If so, why?
What keeps us from passing a `limit` to this function that exceeds `initialLimit` such that the case for `(currentLimit.get() >= initialLimit)` gets invoked more than just the first time this function is called?
Independent of that, I don't understand how we can ensure that `memory.limit_in_bytes` always remains less than or equal `memory.memsw.limit_in_bytes` in the case of `(limit > currentLimit.get())`. If the new `limit` is greater than the `currentLimit` and we set `memory.memsw.limit_in_bytes` before we set `memory.limit_in_bytes`, doesn't that explicitly violate this invariant?
Maybe I'm missing something.
- Kevin Klues
On March 14, 2017, 12:04 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57574/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 12:04 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, haosdent huang, Ian Downes, and Kevin Klues.
>
>
> Bugs: MESOS-7237
> https://issues.apache.org/jira/browse/MESOS-7237
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addressed the bug we saw in MESOS-7237. Linux kernel will ensure
> cgroup memory+swap limit (i.e., 'memory.memsw.limit_in_bytes') is always
> no less than the memory limit ('memory.limit_in_bytes'). Prior to this
> patch, the memory subsystem in the cgroups isolator is buggy because
> memory+swap limit is always changed after the memory limit. This will
> cause EINVAL if we increase the memory limit of the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 009e996add25e43e8989600a58b38f2cc88a5d11
>
>
> Diff: https://reviews.apache.org/r/57574/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>