You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2020/01/02 01:14:17 UTC
Review Request 71943: WIP: Set container's `memory.limit_in_bytes` to
its memory limit.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/
-----------------------------------------------------------
Review request for mesos, Andrei Budnik and Greg Mann.
Bugs: MESOS-10048
https://issues.apache.org/jira/browse/MESOS-10048
Repository: mesos
Description
-------
WIP: Set container's `memory.limit_in_bytes` to its memory limit.
Diffs
-----
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
Diff: https://reviews.apache.org/r/71943/diff/1/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 71943: WIP: Set container's
`memory.limit_in_bytes` to its memory limit.
Posted by Qian Zhang <zh...@gmail.com>.
> On Jan. 8, 2020, 6:34 a.m., Greg Mann wrote:
> > Is this still WIP? If not, could you update the commit message accordingly?
Yes, it is still WIP since I have not handled infinite memory limit yet. Once https://issues.apache.org/jira/browse/MESOS-10064 is resolved, I will update this patch accordingly.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/#review219157
-----------------------------------------------------------
On Jan. 5, 2020, 10:04 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71943/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2020, 10:04 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10048
> https://issues.apache.org/jira/browse/MESOS-10048
>
>
> Repository: mesos
>
>
> Description
> -------
>
> WIP: Set container's `memory.limit_in_bytes` to its memory limit.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
>
>
> Diff: https://reviews.apache.org/r/71943/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71943: WIP: Set container's
`memory.limit_in_bytes` to its memory limit.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/#review219157
-----------------------------------------------------------
Ship it!
Is this still WIP? If not, could you update the commit message accordingly?
- Greg Mann
On Jan. 5, 2020, 2:04 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71943/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2020, 2:04 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10048
> https://issues.apache.org/jira/browse/MESOS-10048
>
>
> Repository: mesos
>
>
> Description
> -------
>
> WIP: Set container's `memory.limit_in_bytes` to its memory limit.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
>
>
> Diff: https://reviews.apache.org/r/71943/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71943: Set container's `memory.limit_in_bytes` to
its memory limit.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/#review219879
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Line 212 (original), 222-231 (patched)
<https://reviews.apache.org/r/71943/#comment308123>
I find this block a little unintuitive at first glance; perhaps a comment would help?
```
// Rather than trying to represent an infinite limit with
// the `Bytes` type, we represent the infinite case by
// setting `isInfiniteLimit` to `true` and letting
// `hardLimit` be NONE.
```
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 240 (patched)
<https://reviews.apache.org/r/71943/#comment308122>
Wrap at 80 characters.
- Greg Mann
On March 6, 2020, 8:33 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71943/
> -----------------------------------------------------------
>
> (Updated March 6, 2020, 8:33 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10048
> https://issues.apache.org/jira/browse/MESOS-10048
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set container's `memory.limit_in_bytes` to its memory limit.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 27d88e91fb784179effd54781f84000fe85c13eb
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
>
>
> Diff: https://reviews.apache.org/r/71943/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71943: Set container's `memory.limit_in_bytes` to
its memory limit.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/
-----------------------------------------------------------
(Updated March 16, 2020, 5:07 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Addressed review comments.
Bugs: MESOS-10048
https://issues.apache.org/jira/browse/MESOS-10048
Repository: mesos
Description
-------
Set container's `memory.limit_in_bytes` to its memory limit.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 27d88e91fb784179effd54781f84000fe85c13eb
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
Diff: https://reviews.apache.org/r/71943/diff/5/
Changes: https://reviews.apache.org/r/71943/diff/4-5/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 71943: Set container's `memory.limit_in_bytes` to
its memory limit.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/
-----------------------------------------------------------
(Updated March 6, 2020, 4:33 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Addressed review comments.
Bugs: MESOS-10048
https://issues.apache.org/jira/browse/MESOS-10048
Repository: mesos
Description
-------
Set container's `memory.limit_in_bytes` to its memory limit.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 27d88e91fb784179effd54781f84000fe85c13eb
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
Diff: https://reviews.apache.org/r/71943/diff/4/
Changes: https://reviews.apache.org/r/71943/diff/3-4/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 71943: Set container's `memory.limit_in_bytes` to
its memory limit.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 5, 2020, 7:55 p.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
> > Lines 239-240 (patched)
> > <https://reviews.apache.org/r/71943/diff/3/?file=2212171#file2212171line242>
> >
> > What exactly do you mean here by "must be infinite"? It seems possible that it's not infinite? Why do we check this for memory but not for CFS quota?
> >
> > I might suggest that we just set 'mem.limit_in_bytes' to '-1' here the same way we do for CFS quota, and rely on the operator configuring the cgroup root correctly.
Yeah, I agree, let me just set `mem.limit_in_bytes` to -1 here.
> On March 5, 2020, 7:55 p.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
> > Lines 282-290 (original), 310-318 (patched)
> > <https://reviews.apache.org/r/71943/diff/3/?file=2212171#file2212171line321>
> >
> > Isn't it possible that the memory limit may be lowered multiple times? If so, I think `setFunctions` would be empty the second time it is lowered?
Yes, `setFunctions` would be empty the second time it is lowered so we will actually do nothing for hard limit. Here the design is we will only lower hard limit for the first time (i.e., when the executor container is launched), and from then on we will never lower it.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/#review219792
-----------------------------------------------------------
On Feb. 26, 2020, 8:16 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71943/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2020, 8:16 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10048
> https://issues.apache.org/jira/browse/MESOS-10048
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set container's `memory.limit_in_bytes` to its memory limit.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 27d88e91fb784179effd54781f84000fe85c13eb
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
>
>
> Diff: https://reviews.apache.org/r/71943/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71943: Set container's `memory.limit_in_bytes` to
its memory limit.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/#review219792
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 239-240 (patched)
<https://reviews.apache.org/r/71943/#comment308012>
What exactly do you mean here by "must be infinite"? It seems possible that it's not infinite? Why do we check this for memory but not for CFS quota?
I might suggest that we just set 'mem.limit_in_bytes' to '-1' here the same way we do for CFS quota, and rely on the operator configuring the cgroup root correctly.
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 282-290 (original), 310-318 (patched)
<https://reviews.apache.org/r/71943/#comment308013>
Isn't it possible that the memory limit may be lowered multiple times? If so, I think `setFunctions` would be empty the second time it is lowered?
- Greg Mann
On Feb. 26, 2020, 12:16 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71943/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2020, 12:16 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10048
> https://issues.apache.org/jira/browse/MESOS-10048
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set container's `memory.limit_in_bytes` to its memory limit.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 27d88e91fb784179effd54781f84000fe85c13eb
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
>
>
> Diff: https://reviews.apache.org/r/71943/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71943: Set container's `memory.limit_in_bytes` to
its memory limit.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/#review219700
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 242-244 (patched)
<https://reviews.apache.org/r/71943/#comment307916>
See my comment on the patch for the CPU subsystem - I'm wondering if this logic is better kept in the agent code?
- Greg Mann
On Feb. 26, 2020, 12:16 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71943/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2020, 12:16 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10048
> https://issues.apache.org/jira/browse/MESOS-10048
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set container's `memory.limit_in_bytes` to its memory limit.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 27d88e91fb784179effd54781f84000fe85c13eb
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
>
>
> Diff: https://reviews.apache.org/r/71943/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71943: Set container's `memory.limit_in_bytes` to
its memory limit.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/
-----------------------------------------------------------
(Updated Feb. 26, 2020, 8:16 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Accommodated infinite value.
Summary (updated)
-----------------
Set container's `memory.limit_in_bytes` to its memory limit.
Bugs: MESOS-10048
https://issues.apache.org/jira/browse/MESOS-10048
Repository: mesos
Description (updated)
-------
Set container's `memory.limit_in_bytes` to its memory limit.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 27d88e91fb784179effd54781f84000fe85c13eb
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
Diff: https://reviews.apache.org/r/71943/diff/3/
Changes: https://reviews.apache.org/r/71943/diff/2-3/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 71943: WIP: Set container's
`memory.limit_in_bytes` to its memory limit.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71943/
-----------------------------------------------------------
(Updated Jan. 5, 2020, 10:04 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Minor changes.
Bugs: MESOS-10048
https://issues.apache.org/jira/browse/MESOS-10048
Repository: mesos
Description
-------
WIP: Set container's `memory.limit_in_bytes` to its memory limit.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 0896d37761a11f55ba4b866d235c3bd2b79dcfba
Diff: https://reviews.apache.org/r/71943/diff/2/
Changes: https://reviews.apache.org/r/71943/diff/1-2/
Testing
-------
Thanks,
Qian Zhang