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/05 14:04:27 UTC
Re: Review Request 71886: WIP: Set container's `cpu.cfs_quota_us` to
its CPU resource limit.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71886/
-----------------------------------------------------------
(Updated Jan. 5, 2020, 10:04 p.m.)
Review request for mesos, Andrei Budnik and Greg Mann.
Changes
-------
Minor changes.
Bugs: MESOS-10047
https://issues.apache.org/jira/browse/MESOS-10047
Repository: mesos
Description
-------
WIP: 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/2/
Changes: https://reviews.apache.org/r/71886/diff/1-2/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 71886: WIP: Set container's `cpu.cfs_quota_us` to
its CPU resource limit.
Posted by Qian Zhang <zh...@gmail.com>.
> On Jan. 8, 2020, 6:11 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 CPU 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/71886/#review219156
-----------------------------------------------------------
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/71886/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2020, 10:04 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
> -------
>
> WIP: 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71886: WIP: Set container's `cpu.cfs_quota_us` to
its CPU resource limit.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71886/#review219156
-----------------------------------------------------------
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/71886/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2020, 2:04 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
> -------
>
> WIP: 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its
CPU resource limit.
Posted by Qian Zhang <zh...@gmail.com>.
> On Feb. 29, 2020, 2:20 a.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp
> > Lines 107 (patched)
> > <https://reviews.apache.org/r/71886/diff/4/?file=2212169#file2212169line108>
> >
> > 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.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/71886/#comment307915>
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.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/71886/#comment308011>
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.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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