Re: Review Request 71858: WIP: Set resource limits when launching executor container.

[Qian Zhang]

> On Jan. 8, 2020, 5:43 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3623 (patched)
> > 
> >
> > Should this be an `Option`? So that we can only set 
> > `containerConfig.limits` when limits have actually been set?
> 
> Qian Zhang wrote:
> I added a check `if (!executorLimits.empty()) {` before setting 
> `containerConfig.limit`, HDYT?
> 
> Greg Mann wrote:
> If we use an option, then the type in the function signature more 
> precisely expresses the semantics of the function, which improves readability 
> IMO.
> 
> Qian Zhang wrote:
> I actually thought about it before, but it may make the code a bit more 
> complicated. Here is the code where we call `launchExecutor()`:
> ```
> defer(
>   self(),
>   ::launchExecutor,
>   lambda::_1,
>   frameworkId,
>   executorInfo_,
>   executorLimits,
>   taskGroup.isNone() ? task.get() : Option::none()));
> ```
> 
> The type of the variable `executorLimits` is `google::protobuf::Map` 
> rather than `Option`. So if we change the type of the 
> parameter `executorLimits` of the `launchExecutor` method to 
> `Option`, its `isSome()` will actually always be true 
> since a map `executorLimits` will always be passed to it, that means checking 
> `executorLimits.isSome()` in `launchExecutor` is actually redundant. To make 
> it not redundant, I may need to change the type of the variable 
> `executorLimits` from `google::protobuf::Map` to 
> `Option`in the caller's code and define another local 
> variable of type `google::protobuf::Map` and set `executorLimits` to that 
> variable when we need to set executor's resource limits, I think it is bit 
> more complicated than the current code in this patch.
> 
> Another option would be set the default value of the `executorLimits` 
> parameter to an empty map (i.e. `{}`), like: 
> ```
> const google::protobuf::Map& executorLimits = {},
> ```
> Does it help?

The reason that I did not set the default value of the `executorLimits` 
parameter to `{}` is, the variable `executorLimits` in the caller side is `{}` 
by default :-)


- Qian


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


On Jan. 8, 2020, 10:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Jan. 8, 2020, 10:40 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10046
> https://issues.apache.org/jira/browse/MESOS-10046
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb 
>   src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71858: WIP: Set resource limits when launching executor container.

[Qian Zhang]

> On Jan. 8, 2020, 5:43 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3623 (patched)
> > 
> >
> > Should this be an `Option`? So that we can only set 
> > `containerConfig.limits` when limits have actually been set?
> 
> Qian Zhang wrote:
> I added a check `if (!executorLimits.empty()) {` before setting 
> `containerConfig.limit`, HDYT?
> 
> Greg Mann wrote:
> If we use an option, then the type in the function signature more 
> precisely expresses the semantics of the function, which improves readability 
> IMO.

I actually thought about it before, but it may make the code a bit more 
complicated. Here is the code where we call `launchExecutor()`:
```
defer(
  self(),
  ::launchExecutor,
  lambda::_1,
  frameworkId,
  executorInfo_,
  executorLimits,
  taskGroup.isNone() ? task.get() : Option::none()));
```

The type of the variable `executorLimits` is `google::protobuf::Map` rather 
than `Option`. So if we change the type of the parameter 
`executorLimits` of the `launchExecutor` method to 
`Option`, its `isSome()` will actually always be true 
since a map `executorLimits` will always be passed to it, that means checking 
`executorLimits.isSome()` in `launchExecutor` is actually redundant. To make it 
not redundant, I may need to change the type of the variable `executorLimits` 
from `google::protobuf::Map` to `Option`in the caller's 
code and define another local variable of type `google::protobuf::Map` and set 
`executorLimits` to that variable when we need to set executor's resource 
limits, I think it is bit more complicated than the current code in this patch.

Another option would be set the default value of the `executorLimits` parameter 
to an empty map (i.e. `{}`), like: 
```
const google::protobuf::Map& executorLimits = {},
```
Does it help?


- Qian


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


On Jan. 8, 2020, 10:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Jan. 8, 2020, 10:40 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10046
> https://issues.apache.org/jira/browse/MESOS-10046
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb 
>   src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71858: WIP: Set resource limits when launching executor container.

[Qian Zhang]

> On Jan. 8, 2020, 5:43 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3140-3201 (patched)
> > 
> >
> > I think maybe this logic could be easier to read if we do something 
> > like:
> > 
> > ```
> > auto limitsAreSet = [](const vector& tasks) {
> >   foreach (const TaskInfo& task, tasks) {
> > if (!task.limits().empty()) {
> >   return true;
> > }
> >   }
> >   
> >   return false;
> > };
> > 
> > Option> executorLimits;
> > if (limitsAreSet(tasks)) {
> >   executorLimits = Map();
> >   foreach (const TaskInfo& task, tasks) {
> > // Add up task limits/requests here.
> >   }
> > }
> > ```
> > 
> > What do you think?
> 
> Qian Zhang wrote:
> I am a bit confused how it will simplify the logic here, like: how will 
> you do the `Add up task limits/requests here`? I guess you still need the 
> code from L3140 to L3201, right?
> 
> Greg Mann wrote:
> Ah sorry, before I answer your question I have another one: currently, 
> your code will only set the executor's cpu limit if one or more tasks have a 
> cpu limit set, and will only set the executor's mem limit if one or more 
> tasks have the memory limit set. However, I think we also want to set the cpu 
> limit if one or more tasks has a _memory_ limit set, and we want to set the 
> memory limit if one or more tasks has a _cpu_ limit set, right? This way, if 
> a single task under an executor sets either a cpu or memory limit, then all 
> tasks will have both the cpu and memory limit set (and if it wasn't specified 
> for a particular task, it will be set to the default, which is the value of 
> the request).

So if a framework launches two tasks (t1 and t2) with a same executor, t1 has 
CPU limit specified but no memory limit specified, t2 has both of the CPU and 
memory limits not specified, then we should not only set CPU hard limit but 
also the memory hard limit (i.e., set it to t1's memory request + t2's memory 
request) in the executor container's cgroups, right? I think we have already 
done it because executor's resource requests always includes all task's 
resource requests (see L3209 in this patch 
https://reviews.apache.org/r/71858/diff/6 ), and in the memory cgroups 
(`memory.cpp`) we will set the executor container's memory hard limit 
(`memory.limit_in_bytes`) to its memory request if its memory limit is not 
specified (see https://reviews.apache.org/r/71943/ for details).

And similarly if t1 has memory limit specified but no CPU limit specified, in 
the CPU cgroups we will set the executor container's CPU hard limit (CFS quota) 
to the executor's CPU request if `--cgroups_enable_cfs` is true.


- Qian


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


On Jan. 8, 2020, 10:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Jan. 8, 2020, 10:40 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10046
> https://issues.apache.org/jira/browse/MESOS-10046
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb 
>   src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 71991: Added the 'TASK_RESOURCE_LIMITS' agent capability.

[Greg Mann]

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

Review request for mesos, Andrei Budnik and Qian Zhang.


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


Repository: mesos


Description
---

This capability will be used by the master to detect whether
or not an agent can handle task resource limits.


Diffs
-

  include/mesos/mesos.proto b0f590545df720df644d049e9d8f1e81bdbe814c 
  include/mesos/v1/mesos.proto 53a7b9bc6190d826868a1633c11c9a0ecf9acf0a 
  src/common/protobuf_utils.hpp 3852f59986caf244a2512d10400298246e8ba8f1 
  src/common/protobuf_utils.cpp 7fe4a44b1e7ded998dffb0490c1d61ced697ebd5 
  src/slave/constants.cpp 1963890fd6c0eadaac174755609287ecf4211661 
  src/slave/flags.cpp 7653c397cbf27490f7b071b5e151bcdf77e6478c 
  src/tests/master_tests.cpp 9688f5f0266f7c7142b54d488f2c13b427e542c0 
  src/tests/slave_tests.cpp fd4fd6bd1baca0e4521999d2c08e6eab78e57a4f 


Diff: https://reviews.apache.org/r/71991/diff/1/


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 71944: Set container process's OOM score adjust.

[Greg Mann]

> On Jan. 7, 2020, 11:07 p.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
> > Lines 199 (patched)
> > 
> >
> > Do we really want to do this? My concern is that this will make any 
> > non-Mesos-task processes on the node (networking and security components, 
> > for example) more likely to be OOM-killed than Mesos tasks. Perhaps we 
> > should only set the OOM score adjustment for burstable tasks. What do you 
> > think?
> 
> Qian Zhang wrote:
> I think it depends on which one is in higher priority and more important, 
> guaranteed task or non-Mesos-task processes? In Kubernetes implementation 
> (https://github.com/kubernetes/kubernetes/blob/v1.16.2/pkg/kubelet/qos/policy.go#L51:L53),
>  the OOM score adjust of guaranteed container is set to -998, and kubelet's 
> OOM score adjust is set to -998 too, I think we should do the same to protect 
> guaranteed containers and Mesos agent, what do you think?
> 
> Greg Mann wrote:
> One significant difference in the Kubernetes case is that they have 
> user-space code which kills pod processes to reclaim memory when necessary. 
> Consequently, there will be less impact if the OOM killer shows a strong 
> preference against killing guaranteed tasks.
> 
> My intuition is that we should not set the OOM score adjustment for 
> non-bursting processes. Even if we leave it at zero, guaranteed tasks will 
> still be treated preferentially with respect to bursting tasks, since all 
> bursting tasks will have an adjustment greater than zero.
> 
> Qian Zhang wrote:
> I agree that guaranteed tasks will be treated preferentially with respect 
> to bursting tasks, but I am thinking about the guaranteed tasks v.s. the 
> non-Mesos-tasks, let's say two guaranteed tasks running on a node, and each 
> of them's memory request/limit is half of the node's memory, and both of them 
> has almost used all of its memory request/limit, so their OOM score will be 
> very high (like 490+). Now if a non-mesos-task (e.g., a system component or 
> even Mesos agent itself) tries to use a lot of memory suddenly, the node will 
> be short of memory, and then OOM killer will definitely kill one of the two 
> guaranteed tasks since their OOM score are the top 2 in the node. But I do 
> not think K8s will have this issue since the guaranteed containers OOM score 
> adjust is -998.
> 
> Qian Zhang wrote:
> And even we think about the case guaranteed tasks v.s. burstable tasks, I 
> think it is also a bit risky if we leave guaranteed task's OOM score adjust 
> to 0. For example, one guaranteed task (T1) and one burstable task (T2) 
> running on a node, each of them's memory request is half of the node's 
> memory. T1 has almost used all of its memory request/limit, so its OOM score 
> will be something like 490+. T2 uses very little memory, so its OOM score 
> will be a bit beyond 500 (like 510). I think in this case the OOM scores of 
> T1 and T2 are too close, the actual OOM score is calculated in a more complex 
> way, so I am afraid there will be a moment that the OOM score of T1 is even 
> higher than T2, that's why I think it is a bit risky.
> 
> Qian Zhang wrote:
> Just want to add one point, it seems a small amount (30) will be 
> subtracted from the OOM score of root-owned processes, so in the above 
> example, if T2 is owned by root but T1 is owned by a normal user, it might be 
> possible T2 get a smaller OOM score that T1.

In the case above of tasks T1 and T2, I don't think we need to provide any 
guarantee of which process is killed first in this case. If neither task is 
above its memory request, then I think it's OK for the OOM killer to decide 
which one is killed first. The resource limits feature doesn't add a notion of 
priority, like "guaranteed" vs. "burstable", I think we just want to make sure 
that tasks which have exceeded their memory request are killed preferentially. 
So I think it's OK to leave the OOM score adjustment of non-burstable tasks at 
zero.


- Greg


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


On Jan. 8, 2020, 3:28 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71944/
> ---
> 
> (Updated Jan. 8, 2020, 3:28 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 process's OOM score adjust.
> 
> 
> Diffs
> -
> 
>   

Re: Review Request 71858: WIP: Set resource limits when launching executor container.

[Greg Mann]

> On Jan. 7, 2020, 9:43 p.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3140-3201 (patched)
> > 
> >
> > I think maybe this logic could be easier to read if we do something 
> > like:
> > 
> > ```
> > auto limitsAreSet = [](const vector& tasks) {
> >   foreach (const TaskInfo& task, tasks) {
> > if (!task.limits().empty()) {
> >   return true;
> > }
> >   }
> >   
> >   return false;
> > };
> > 
> > Option> executorLimits;
> > if (limitsAreSet(tasks)) {
> >   executorLimits = Map();
> >   foreach (const TaskInfo& task, tasks) {
> > // Add up task limits/requests here.
> >   }
> > }
> > ```
> > 
> > What do you think?
> 
> Qian Zhang wrote:
> I am a bit confused how it will simplify the logic here, like: how will 
> you do the `Add up task limits/requests here`? I guess you still need the 
> code from L3140 to L3201, right?

Ah sorry, before I answer your question I have another one: currently, your 
code will only set the executor's cpu limit if one or more tasks have a cpu 
limit set, and will only set the executor's mem limit if one or more tasks have 
the memory limit set. However, I think we also want to set the cpu limit if one 
or more tasks has a _memory_ limit set, and we want to set the memory limit if 
one or more tasks has a _cpu_ limit set, right? This way, if a single task 
under an executor sets either a cpu or memory limit, then all tasks will have 
both the cpu and memory limit set (and if it wasn't specified for a particular 
task, it will be set to the default, which is the value of the request).


- Greg


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


On Jan. 8, 2020, 2:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Jan. 8, 2020, 2:40 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10046
> https://issues.apache.org/jira/browse/MESOS-10046
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb 
>   src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71965: Cgroups isolator: added support for nested cgroups during launch.

[Greg Mann]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Line 414 (original), 429 (patched)


s/share_cgroups/shareCgroups/


- Greg Mann


On Jan. 9, 2020, 9:03 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> ---
> 
> (Updated Jan. 9, 2020, 9:03 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Qian Zhang.
> 
> 
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 4bd3d6dad37dee031660c15e957cc36f63e21fcb 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> b12b73d8e0161d448075378765e77867521de04e 
> 
> 
> Diff: https://reviews.apache.org/r/71965/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 71983: Added a test `CgroupsIsolatorTest.ROOT_CGROUPS_CFS_TaskGroupLimits`.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71855, 71931, 71856, 71858, 71884, 71885, 71886, 71943, 
71944, 71950, 71951, 71952, 71953, 71955, 71956, 71983]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_71983"]

Error:
..
 for 5secs
I0113 14:42:50.135164 18746 slave.cpp:8649] Handling resource provider message 
'UPDATE_OPERATION_STATUS: (uuid: fd3922bf-da98-42dc-9580-c844ec08d423) for 
framework  (latest state: OPERATION_FINISHED, status update state: 
OPERATION_FINISHED)'
I0113 14:42:50.141620 18746 slave.cpp:9102] Updating the state of operation 
with no ID (uuid: fd3922bf-da98-42dc-9580-c844ec08d423) for an operation API 
call (latest state: OPERATION_FINISHED, status update state: OPERATION_FINISHED)
I0113 14:42:50.141710 18746 slave.cpp:8856] Forwarding status update of 
operation with no ID (operation_uuid: fd3922bf-da98-42dc-9580-c844ec08d423) for 
an operator API call
I0113 14:42:50.142206 18740 master.cpp:11789] Updating the state of operation 
'' (uuid: fd3922bf-da98-42dc-9580-c844ec08d423) for an operator API call 
(latest state: OPERATION_PENDING, status update state: OPERATION_FINISHED)
I0113 14:42:50.143069 18746 slave.cpp:4504] Ignoring new checkpointed resources 
and operations identical to the current version
I0113 14:42:50.145885 18742 master.cpp:12137] Sending operation '' (uuid: 
610118ba-c9c5-4038-af95-618974e68148) to agent 
b00a682f-f28c-4094-b628-fc274d7f3dab-S0 at slave(1252)@172.17.0.2:38425 
(c27cf6ba285d)
I0113 14:42:50.146699 18750 slave.cpp:4504] Ignoring new checkpointed resources 
and operations identical to the current version
I0113 14:42:50.149614 18735 provider.cpp:498] Received 
ACKNOWLEDGE_OPERATION_STATUS event
I0113 14:42:50.150440 18735 status_update_manager_process.hpp:252] Received 
operation status update acknowledgement (UUID: 
84eb7230-1cb5-4427-be3e-4d06541a4c49) for stream 
fd3922bf-da98-42dc-9580-c844ec08d423
I0113 14:42:50.150903 18735 status_update_manager_process.hpp:929] 
Checkpointing ACK for operation status update OPERATION_FINISHED (Status UUID: 
84eb7230-1cb5-4427-be3e-4d06541a4c49) for operation UUID 
fd3922bf-da98-42dc-9580-c844ec08d423 on agent 
b00a682f-f28c-4094-b628-fc274d7f3dab-S0
I0113 14:42:50.151826 18751 provider.cpp:498] Received APPLY_OPERATION event
I0113 14:42:50.151877 18751 provider.cpp:1351] Received CREATE operation '' 
(uuid: 610118ba-c9c5-4038-af95-618974e68148)
I0113 14:42:50.152490 18734 master.cpp:5959] Processing REVIVE call for 
framework b00a682f-f28c-4094-b628-fc274d7f3dab- (default) at 
scheduler-f1e918fb-f3fd-4568-b807-112ca8f47205@172.17.0.2:38425
I0113 14:42:50.153231 18734 hierarchical.cpp:1721] Unsuppressed offers and 
cleared filters for roles { storage/default-role } of framework 
b00a682f-f28c-4094-b628-fc274d7f3dab-
I0113 14:42:50.155328 18734 hierarchical.cpp:1853] Performed allocation for 1 
agents in 1.775707ms
I0113 14:42:50.156787 18734 master.cpp:9975] Sending offers [ 
b00a682f-f28c-4094-b628-fc274d7f3dab-O4 ] to framework 
b00a682f-f28c-4094-b628-fc274d7f3dab- (default) at 
scheduler-f1e918fb-f3fd-4568-b807-112ca8f47205@172.17.0.2:38425
I0113 14:42:50.158025 18734 sched.cpp:934] Scheduler::resourceOffers took 
123096ns
I0113 14:42:50.171032 18743 hierarchical.cpp:1853] Performed allocation for 1 
agents in 389864ns
I0113 14:42:50.235703 18735 status_update_manager_process.hpp:490] Cleaning up 
operation status update stream fd3922bf-da98-42dc-9580-c844ec08d423
I0113 14:42:50.295411 18739 status_update_manager_process.hpp:152] Received 
operation status update OPERATION_FINISHED (Status UUID: 
4b993a71-5380-4d75-b388-1002ffdbfee5) for operation UUID 
610118ba-c9c5-4038-af95-618974e68148 on agent 
b00a682f-f28c-4094-b628-fc274d7f3dab-S0
I0113 14:42:50.296057 18739 status_update_manager_process.hpp:414] Creating 
operation status update stream 610118ba-c9c5-4038-af95-618974e68148 
checkpoint=true
I0113 14:42:50.296684 18739 status_update_manager_process.hpp:929] 
Checkpointing UPDATE for operation status update OPERATION_FINISHED (Status 
UUID: 4b993a71-5380-4d75-b388-1002ffdbfee5) for operation UUID 
610118ba-c9c5-4038-af95-618974e68148 on agent 
b00a682f-f28c-4094-b628-fc274d7f3dab-S0
I0113 14:42:50.354960 18739 status_update_manager_process.hpp:528] Forwarding 
operation status update OPERATION_FINISHED (Status UUID: 
4b993a71-5380-4d75-b388-1002ffdbfee5) for operation UUID 
610118ba-c9c5-4038-af95-618974e68148 on agent 
b00a682f-f28c-4094-b628-fc274d7f3dab-S0
I0113 14:42:50.356869 18731 http_connection.hpp:131] Sending 

Re: Review Request 71966: Cgroups isolator: added support for nested cgroups during recovery.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71855, 71931, 71856, 71858, 71884, 71885, 71886, 71943, 
71944, 71950, 71951, 71952, 71953, 71955, 71956, 71965, 71966]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Jan. 7, 2020, 5:06 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71966/
> ---
> 
> (Updated Jan. 7, 2020, 5:06 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Qian Zhang.
> 
> 
> Bugs: MESOS-10079
> https://issues.apache.org/jira/browse/MESOS-10079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables recovery for nested cgroups and implements
> the detection of orphaned nested cgroups.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 4bd3d6dad37dee031660c15e957cc36f63e21fcb 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> b12b73d8e0161d448075378765e77867521de04e 
> 
> 
> Diff: https://reviews.apache.org/r/71966/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

[GitHub] [mesos] bbannier merged pull request #349: site: update the year to 2020 in the footer

[GitBox]

bbannier merged pull request #349: site: update the year to 2020 in the footer
URL: https://github.com/apache/mesos/pull/349
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Review Request 71987: Disabled `DefaultExecutorTest.DomainSockets` on non-Linux platforms.

[Benno Evers]

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


Ship it!




Ship It!

- Benno Evers


On Jan. 13, 2020, 10:22 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71987/
> ---
> 
> (Updated Jan. 13, 2020, 10:22 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While the interfaces are available on all platforms, currently domain
> sockets for executor communication are only available on Linux. This
> patch ifdef's the code away on non-Linux platforms; this is consistent
> with how we disable tests on non-Linux platforms elsewhere where we
> never use a test filter.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 6c71b3c2231d3da77d2fb793cb2bbc38e02cef24 
> 
> 
> Diff: https://reviews.apache.org/r/71987/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on macos
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 71987: Disabled `DefaultExecutorTest.DomainSockets` on non-Linux platforms.

[Benjamin Bannier]

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

Review request for mesos and Benno Evers.


Repository: mesos


Description
---

While the interfaces are available on all platforms, currently domain
sockets for executor communication are only available on Linux. This
patch ifdef's the code away on non-Linux platforms; this is consistent
with how we disable tests on non-Linux platforms elsewhere where we
never use a test filter.


Diffs
-

  src/tests/default_executor_tests.cpp 6c71b3c2231d3da77d2fb793cb2bbc38e02cef24 


Diff: https://reviews.apache.org/r/71987/diff/1/


Testing
---

`make check` on macos


Thanks,

Benjamin Bannier

Re: Review Request 71986: Removed redundant calls to `c_str` flagged by mesos-tidy.

[Till Toenshoff via Review Board]

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


Ship it!




Ship It!

- Till Toenshoff


On Jan. 13, 2020, 9:49 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71986/
> ---
> 
> (Updated Jan. 13, 2020, 9:49 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These calls were flagged by the upstream
> `readability-redundant-string-cstr` check.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp d9cb5662cbf8e4d3531518af8bb633d9bb12d038 
> 
> 
> Diff: https://reviews.apache.org/r/71986/diff/1/
> 
> 
> Testing
> ---
> 
> confirmed that `clang-tidy` does not report anything from these lines anymore
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71985: Properly initialized dummy variable.

[Till Toenshoff via Review Board]

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


Ship it!




Ship It!

- Till Toenshoff


On Jan. 13, 2020, 9:48 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71985/
> ---
> 
> (Updated Jan. 13, 2020, 9:48 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since `Try` does not have a default constructor we need to provide a
> dummy value.
> 
> 
> Diffs
> -
> 
>   src/slave/main.cpp 9e40743a9d379d5b1ff9f97372826d4296167ce3 
> 
> 
> Diff: https://reviews.apache.org/r/71985/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on macos
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 71986: Removed redundant calls to `c_str` flagged by mesos-tidy.

[Benjamin Bannier]

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

Review request for mesos and Benno Evers.


Repository: mesos


Description
---

These calls were flagged by the upstream
`readability-redundant-string-cstr` check.


Diffs
-

  src/linux/systemd.cpp d9cb5662cbf8e4d3531518af8bb633d9bb12d038 


Diff: https://reviews.apache.org/r/71986/diff/1/


Testing
---

confirmed that `clang-tidy` does not report anything from these lines anymore


Thanks,

Benjamin Bannier

Review Request 71985: Properly initialized dummy variable.

[Benjamin Bannier]

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

Review request for mesos and Benno Evers.


Repository: mesos


Description
---

Since `Try` does not have a default constructor we need to provide a
dummy value.


Diffs
-

  src/slave/main.cpp 9e40743a9d379d5b1ff9f97372826d4296167ce3 


Diff: https://reviews.apache.org/r/71985/diff/1/


Testing
---

`make check` on macos


Thanks,

Benjamin Bannier

Re: Review Request 71983: Added a test `CgroupsIsolatorTest.ROOT_CGROUPS_CFS_TaskGroupLimits`.

[Qian Zhang]

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

(Updated Jan. 13, 2020, 4:06 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a test `CgroupsIsolatorTest.ROOT_CGROUPS_CFS_TaskGroupLimits`.


Diffs (updated)
-

  src/tests/containerizer/cgroups_isolator_tests.cpp 
f72e6cdab417368e63349915114aeed586e0ef0e 


Diff: https://reviews.apache.org/r/71983/diff/2/

Changes: https://reviews.apache.org/r/71983/diff/1-2/


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 71956: Added a test `ROOT_CGROUPS_CFS_CommandTaskLimits`.

[Qian Zhang]

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

(Updated Jan. 13, 2020, 4:06 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Checked the CPU and memory soft limits.


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


Repository: mesos


Description
---

Added a test `ROOT_CGROUPS_CFS_CommandTaskLimits`.


Diffs (updated)
-

  src/tests/containerizer/cgroups_isolator_tests.cpp 
f72e6cdab417368e63349915114aeed586e0ef0e 


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

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


Testing
---

sudo make check


Thanks,

Qian Zhang