Re: Review Request 72022: Set resource limits and OOM score adjustment in Docker executor.

[Greg Mann]

> On March 16, 2020, 11:54 a.m., Greg Mann wrote:
> > src/docker/executor.cpp
> > Lines 201-212 (original), 201-212 (patched)
> > 
> >
> > 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.

[Greg Mann]

---
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.

[Qian Zhang]

> On March 16, 2020, 7:54 p.m., Greg Mann wrote:
> > src/docker/executor.cpp
> > Lines 201-212 (original), 201-212 (patched)
> > 
> >
> > 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.

[Greg Mann]

---
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)


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.

[Greg Mann]

> On March 6, 2020, 8:06 a.m., Greg Mann wrote:
> > src/docker/docker.cpp
> > Line 663 (original), 714-716 (patched)
> > 
> >
> > 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.

[Qian Zhang]

---
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.

[Qian Zhang]

> On March 6, 2020, 4:06 p.m., Greg Mann wrote:
> > src/docker/docker.cpp
> > Lines 685 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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.

[Qian Zhang]

---
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

Re: Review Request 72022: Set resource limits and OOM score adjustment in Docker executor.

[Greg Mann]

---
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)


Do we need the NOLINT here?
```
const Option&
  defaultContainerDNS = None(),
const Option>&
  resourceLimits = None());
```



src/docker/docker.cpp
Lines 685 (patched)


Is it not possible for us to handle infinite limits here?



src/docker/docker.cpp
Lines 711 (patched)


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)


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.

[Qian Zhang]

---
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