Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its CPU resource limit.

[Qian Zhang]

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

(Updated March 16, 2020, 5:07 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Rebased.


Bugs: MESOS-10047
https://issues.apache.org/jira/browse/MESOS-10047


Repository: mesos


Description
---

Set container's `cpu.cfs_quota_us` to its CPU resource limit.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
960bd141430387e076a8fab1948d07719613ed90 


Diff: https://reviews.apache.org/r/71886/diff/6/

Changes: https://reviews.apache.org/r/71886/diff/5-6/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its CPU resource limit.

[Qian Zhang]

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

(Updated March 6, 2020, 4:32 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Addressed review comment.


Bugs: MESOS-10047
https://issues.apache.org/jira/browse/MESOS-10047


Repository: mesos


Description
---

Set container's `cpu.cfs_quota_us` to its CPU resource limit.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
960bd141430387e076a8fab1948d07719613ed90 


Diff: https://reviews.apache.org/r/71886/diff/5/

Changes: https://reviews.apache.org/r/71886/diff/4-5/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its CPU resource limit.

[Greg Mann]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp
Lines 124 (patched)


Maybe store this value to reuse it here and below?

```
const double& effectiveLimit =
  cpuLimit.isSome() ? cpuLimit.get() : cpuRequest;
```


- Greg Mann


On Feb. 26, 2020, 12:12 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71886/
> ---
> 
> (Updated Feb. 26, 2020, 12:12 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10047
> https://issues.apache.org/jira/browse/MESOS-10047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set container's `cpu.cfs_quota_us` to its CPU resource limit.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 960bd141430387e076a8fab1948d07719613ed90 
> 
> 
> Diff: https://reviews.apache.org/r/71886/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its CPU resource limit.

[Qian Zhang]

> On Feb. 29, 2020, 2:20 a.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp
> > Lines 107 (patched)
> > 
> >
> > For executor containers, you have logic in `launchExecutor()` which 
> > inspects the value of `flags.cgroups_enable_cfs`, and also sets the limits 
> > equal to the resource requests when appropriate.
> > 
> > It seems strange to me to have some of that logic in the agent code, 
> > and some of that logic in the isolators. I would expect that we either put 
> > it all in the agent, or all in the isolators.
> > 
> > What about adding code to the `LaunchContainer` API handler which does 
> > this for tasks, and then the isolator doesn't have to care about the 
> > semantics of the `cgroups_enable_cfs` flag and the limits being set to the 
> > resource requests in some cases.

The logic in `launchExecutor()` (actually it is in `Slave::__run()` before 
`launchExecutor`) which inspects the value of `--cgroups_enable_cfs` will ONLY 
be hit in the case that an executor launches a task group, some tasks in the 
group have CPU limit set while some others have not. So how do we handle the 
case like command task? For a command task, if it has no CPU limit set, we 
still need to set its CFS quota to its CPU request if `--cgroups_enable_cfs` is 
true which is currently handled in the isolator code. But if we remove such 
code from isolator, the command task case will be missed since it cannot be 
handled in `Slave::__run()` and the `LaunchContainer` API handler.


- Qian


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


On Feb. 26, 2020, 8:12 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71886/
> ---
> 
> (Updated Feb. 26, 2020, 8:12 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10047
> https://issues.apache.org/jira/browse/MESOS-10047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set container's `cpu.cfs_quota_us` to its CPU resource limit.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 960bd141430387e076a8fab1948d07719613ed90 
> 
> 
> Diff: https://reviews.apache.org/r/71886/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its CPU resource limit.

[Greg Mann]

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




src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp
Lines 107 (patched)


For executor containers, you have logic in `launchExecutor()` which 
inspects the value of `flags.cgroups_enable_cfs`, and also sets the limits 
equal to the resource requests when appropriate.

It seems strange to me to have some of that logic in the agent code, and 
some of that logic in the isolators. I would expect that we either put it all 
in the agent, or all in the isolators.

What about adding code to the `LaunchContainer` API handler which does this 
for tasks, and then the isolator doesn't have to care about the semantics of 
the `cgroups_enable_cfs` flag and the limits being set to the resource requests 
in some cases.


- Greg Mann


On Feb. 26, 2020, 12:12 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71886/
> ---
> 
> (Updated Feb. 26, 2020, 12:12 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10047
> https://issues.apache.org/jira/browse/MESOS-10047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set container's `cpu.cfs_quota_us` to its CPU resource limit.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 960bd141430387e076a8fab1948d07719613ed90 
> 
> 
> Diff: https://reviews.apache.org/r/71886/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its CPU resource limit.

[Qian Zhang]

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

(Updated Feb. 26, 2020, 8:12 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Ensured we can set infinite CPU limit correctly.


Bugs: MESOS-10047
https://issues.apache.org/jira/browse/MESOS-10047


Repository: mesos


Description
---

Set container's `cpu.cfs_quota_us` to its CPU resource limit.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
960bd141430387e076a8fab1948d07719613ed90 


Diff: https://reviews.apache.org/r/71886/diff/4/

Changes: https://reviews.apache.org/r/71886/diff/3-4/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its CPU resource limit.

[Qian Zhang]

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

(Updated Feb. 25, 2020, 9:57 a.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Accommodated infinite value.


Summary (updated)
-

Set container's `cpu.cfs_quota_us` to its CPU resource limit.


Bugs: MESOS-10047
https://issues.apache.org/jira/browse/MESOS-10047


Repository: mesos


Description (updated)
---

Set container's `cpu.cfs_quota_us` to its CPU resource limit.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
960bd141430387e076a8fab1948d07719613ed90 


Diff: https://reviews.apache.org/r/71886/diff/3/

Changes: https://reviews.apache.org/r/71886/diff/2-3/


Testing
---


Thanks,

Qian Zhang