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/02/26 12:24:10 UTC
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/
-----------------------------------------------------------
(Updated Feb. 26, 2020, 8:24 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Accommodated infinite value.
Summary (updated)
-----------------
Set resource limits and OOM score adjustment in Docker executor.
Bugs: MESOS-10053
https://issues.apache.org/jira/browse/MESOS-10053
Repository: mesos
Description (updated)
-------
Set resource limits and OOM score adjustment in Docker executor.
Diffs (updated)
-----
src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
Diff: https://reviews.apache.org/r/72022/diff/2/
Changes: https://reviews.apache.org/r/72022/diff/1-2/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 6, 2020, 4:06 p.m., Greg Mann wrote:
> > src/docker/docker.cpp
> > Lines 685 (patched)
> > <https://reviews.apache.org/r/72022/diff/2/?file=2212187#file2212187line685>
> >
> > Is it not possible for us to handle infinite limits here?
For infinite limits, we actually do not need to handle them, then the corresponding Docker options will not be set when we run `docker run` which means unlimited resource limits. I will add a comment here to explain this.
> On March 6, 2020, 4:06 p.m., Greg Mann wrote:
> > src/docker/docker.cpp
> > Line 663 (original), 714-716 (patched)
> > <https://reviews.apache.org/r/72022/diff/2/?file=2212187#file2212187line714>
> >
> > Do we want to set the memory reservation here as well?
I do not think we want to do that because of backward compatibility.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/#review219816
-----------------------------------------------------------
On March 11, 2020, 5:41 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72022/
> -----------------------------------------------------------
>
> (Updated March 11, 2020, 5:41 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10053
> https://issues.apache.org/jira/browse/MESOS-10053
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set resource limits and OOM score adjustment in Docker executor.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
> src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
> src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
>
>
> Diff: https://reviews.apache.org/r/72022/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 6, 2020, 8:06 a.m., Greg Mann wrote:
> > src/docker/docker.cpp
> > Line 663 (original), 714-716 (patched)
> > <https://reviews.apache.org/r/72022/diff/2/?file=2212187#file2212187line714>
> >
> > Do we want to set the memory reservation here as well?
>
> Qian Zhang wrote:
> I do not think we want to do that because of backward compatibility.
Good point.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/#review219816
-----------------------------------------------------------
On March 16, 2020, 9:16 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72022/
> -----------------------------------------------------------
>
> (Updated March 16, 2020, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10053
> https://issues.apache.org/jira/browse/MESOS-10053
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set resource limits and OOM score adjustment in Docker executor.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
> src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
> src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
>
>
> Diff: https://reviews.apache.org/r/72022/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/#review219816
-----------------------------------------------------------
src/docker/docker.hpp
Line 190 (original), 190-191 (patched)
<https://reviews.apache.org/r/72022/#comment308044>
Do we need the NOLINT here?
```
const Option<mesos::internal::ContainerDNSInfo>&
defaultContainerDNS = None(),
const Option<google::protobuf::Map<std::string, mesos::Value::Scalar>>&
resourceLimits = None());
```
src/docker/docker.cpp
Lines 685 (patched)
<https://reviews.apache.org/r/72022/#comment308047>
Is it not possible for us to handle infinite limits here?
src/docker/docker.cpp
Lines 711 (patched)
<https://reviews.apache.org/r/72022/#comment308045>
Can we factor this out into a helper function so that we don't have this magic formula hard-coded in two places?
src/docker/docker.cpp
Line 663 (original), 714-716 (patched)
<https://reviews.apache.org/r/72022/#comment308046>
Do we want to set the memory reservation here as well?
- Greg Mann
On Feb. 26, 2020, 12:24 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72022/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2020, 12:24 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10053
> https://issues.apache.org/jira/browse/MESOS-10053
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set resource limits and OOM score adjustment in Docker executor.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
> src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
> src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
>
>
> Diff: https://reviews.apache.org/r/72022/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 16, 2020, 7:54 p.m., Greg Mann wrote:
> > src/docker/executor.cpp
> > Lines 201-212 (original), 201-212 (patched)
> > <https://reviews.apache.org/r/72022/diff/4/?file=2214052#file2214052line201>
> >
> > We also have the less common case of a custom executor in a Docker container in `DockerContainerizerProcess::launchExecutorContainer()`, found in the file 'src/slave/containerizer/docker.cpp'. We should probably pass limits through there as well?
Yes, we should. Actually that is the scope of https://issues.apache.org/jira/browse/MESOS-10054 , and I think we can do it in a separate patch rather than in this patch. And as you mentioned it is a less common case, so I think we could do it post MVP, HDYT?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/#review219953
-----------------------------------------------------------
On March 16, 2020, 5:16 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72022/
> -----------------------------------------------------------
>
> (Updated March 16, 2020, 5:16 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10053
> https://issues.apache.org/jira/browse/MESOS-10053
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set resource limits and OOM score adjustment in Docker executor.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
> src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
> src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
>
>
> Diff: https://reviews.apache.org/r/72022/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 16, 2020, 11:54 a.m., Greg Mann wrote:
> > src/docker/executor.cpp
> > Lines 201-212 (original), 201-212 (patched)
> > <https://reviews.apache.org/r/72022/diff/4/?file=2214052#file2214052line201>
> >
> > We also have the less common case of a custom executor in a Docker container in `DockerContainerizerProcess::launchExecutorContainer()`, found in the file 'src/slave/containerizer/docker.cpp'. We should probably pass limits through there as well?
>
> Qian Zhang wrote:
> Yes, we should. Actually that is the scope of https://issues.apache.org/jira/browse/MESOS-10054 , and I think we can do it in a separate patch rather than in this patch. And as you mentioned it is a less common case, so I think we could do it post MVP, HDYT?
sgtm
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/#review219953
-----------------------------------------------------------
On March 16, 2020, 9:16 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72022/
> -----------------------------------------------------------
>
> (Updated March 16, 2020, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10053
> https://issues.apache.org/jira/browse/MESOS-10053
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set resource limits and OOM score adjustment in Docker executor.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
> src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
> src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
>
>
> Diff: https://reviews.apache.org/r/72022/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/#review219953
-----------------------------------------------------------
src/docker/executor.cpp
Lines 201-212 (original), 201-212 (patched)
<https://reviews.apache.org/r/72022/#comment308190>
We also have the less common case of a custom executor in a Docker container in `DockerContainerizerProcess::launchExecutorContainer()`, found in the file 'src/slave/containerizer/docker.cpp'. We should probably pass limits through there as well?
- Greg Mann
On March 16, 2020, 9:16 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72022/
> -----------------------------------------------------------
>
> (Updated March 16, 2020, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10053
> https://issues.apache.org/jira/browse/MESOS-10053
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set resource limits and OOM score adjustment in Docker executor.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
> src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
> src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
>
>
> Diff: https://reviews.apache.org/r/72022/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/#review219978
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On March 16, 2020, 9:16 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72022/
> -----------------------------------------------------------
>
> (Updated March 16, 2020, 9:16 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Greg Mann.
>
>
> Bugs: MESOS-10053
> https://issues.apache.org/jira/browse/MESOS-10053
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set resource limits and OOM score adjustment in Docker executor.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
> src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
> src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
>
>
> Diff: https://reviews.apache.org/r/72022/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/
-----------------------------------------------------------
(Updated March 16, 2020, 5:16 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Rebased.
Bugs: MESOS-10053
https://issues.apache.org/jira/browse/MESOS-10053
Repository: mesos
Description
-------
Set resource limits and OOM score adjustment in Docker executor.
Diffs (updated)
-----
src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
Diff: https://reviews.apache.org/r/72022/diff/4/
Changes: https://reviews.apache.org/r/72022/diff/3-4/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 72022: Set resource limits and OOM score
adjustment in Docker executor.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72022/
-----------------------------------------------------------
(Updated March 11, 2020, 5:41 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Addressed review comments.
Bugs: MESOS-10053
https://issues.apache.org/jira/browse/MESOS-10053
Repository: mesos
Description
-------
Set resource limits and OOM score adjustment in Docker executor.
Diffs (updated)
-----
src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81
src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd
src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107
Diff: https://reviews.apache.org/r/72022/diff/3/
Changes: https://reviews.apache.org/r/72022/diff/2-3/
Testing
-------
Thanks,
Qian Zhang