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

[Qian Zhang]

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

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


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Set resource limits when launching executor container.


Diffs (updated)
-

  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp a914de4bae5630126ae26b9e7dee8c8784ce1ee0 


Diff: https://reviews.apache.org/r/71858/diff/12/

Changes: https://reviews.apache.org/r/71858/diff/11-12/


Testing
---


Thanks,

Qian Zhang

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

[Greg Mann]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 191 (patched)


Could you add a comment here explaining this helper?


- Greg Mann


On March 9, 2020, 2:04 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated March 9, 2020, 2:04 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10046
> https://issues.apache.org/jira/browse/MESOS-10046
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp a914de4bae5630126ae26b9e7dee8c8784ce1ee0 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Qian Zhang]

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

(Updated March 9, 2020, 10:04 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Refactored the code to compute executor resource limits into 
`Slave::computeExecutorLimits()`.


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


Repository: mesos


Description
---

Set resource limits when launching executor container.


Diffs (updated)
-

  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp a914de4bae5630126ae26b9e7dee8c8784ce1ee0 


Diff: https://reviews.apache.org/r/71858/diff/11/

Changes: https://reviews.apache.org/r/71858/diff/10-11/


Testing
---


Thanks,

Qian Zhang

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

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Feb. 25, 2020, 1:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Feb. 25, 2020, 1:46 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
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71858: 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 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?
> 
> Qian Zhang wrote:
> 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 :-)
> 
> Greg Mann wrote:
> > 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
> 
> Personally, that approach sounds better to me. I might be missing 
> something, but this seems like a classic use case for the `Option` type.
> 
> Qian Zhang wrote:
> I think the classic use case for the `Option` type is like:
> ```
> Option var;
> if (...) {
>   var = 
> }
> ```
> 
> But here with the `Option` type, the code will be something like:
> ```
> Option executorLimits;
> Map executorLimits_;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits_.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits_.insert({"mem", executorMemLimit.get()});
> }
> 
> if (!executorLimits_.empty()) {
>  executorLimits = executorLimits_;
> }
> 
> ```
> 
> The above code is a bit redundant to me, I think the code like the below 
> is better:
> ```
> Map executorLimits;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits.insert({"mem", executorMemLimit.get()});
> }
> ```
> 
> Greg Mann wrote:
> Ah, this is related to my other comment about refactoring L3140 to L3201 
> :)
> 
> If we used an `Option` and refactored as I proposed, and if we will 
> always set the CPU and mem hard limits together (i.e. never set just CPU hard 
> limits but not mem hard limits), then we could do something like:
> 
> ```
> foreach (const TaskInfo& task, tasks) {
> if (!task.limits().empty()) {
>   return true;
> }
>   }
> 
>   return false;
> };
> 
> Option> executorLimits;
> if (limitsAreSet(tasks)) {
>   executorLimitsResult = Map(
>   {"cpus": Value::Scalar(),
>"mem":  Value::Scalar()});
>   foreach (const TaskInfo& _task, tasks) {
> if (_task.limits().count("cpus")) {
>   executorLimitsResult.at("cpus") += _task.limits().at("cpus");
> } else {
>   Option taskCpus =
> Resources(_task.resources()).get("cpus");
>   if (taskCpus.isSome()) {
> executorLimitsResult.at("cpus") += taskCpus.get();
>   }
> }
> 
> if 

Re: Review Request 71858: 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?
> 
> Qian Zhang wrote:
> 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 :-)
> 
> Greg Mann wrote:
> > 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
> 
> Personally, that approach sounds better to me. I might be missing 
> something, but this seems like a classic use case for the `Option` type.
> 
> Qian Zhang wrote:
> I think the classic use case for the `Option` type is like:
> ```
> Option var;
> if (...) {
>   var = 
> }
> ```
> 
> But here with the `Option` type, the code will be something like:
> ```
> Option executorLimits;
> Map executorLimits_;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits_.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits_.insert({"mem", executorMemLimit.get()});
> }
> 
> if (!executorLimits_.empty()) {
>  executorLimits = executorLimits_;
> }
> 
> ```
> 
> The above code is a bit redundant to me, I think the code like the below 
> is better:
> ```
> Map executorLimits;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits.insert({"mem", executorMemLimit.get()});
> }
> ```
> 
> Greg Mann wrote:
> Ah, this is related to my other comment about refactoring L3140 to L3201 
> :)
> 
> If we used an `Option` and refactored as I proposed, and if we will 
> always set the CPU and mem hard limits together (i.e. never set just CPU hard 
> limits but not mem hard limits), then we could do something like:
> 
> ```
> foreach (const TaskInfo& task, tasks) {
> if (!task.limits().empty()) {
>   return true;
> }
>   }
> 
>   return false;
> };
> 
> Option> executorLimits;
> if (limitsAreSet(tasks)) {
>   executorLimitsResult = Map(
>   {"cpus": Value::Scalar(),
>"mem":  Value::Scalar()});
>   foreach (const TaskInfo& _task, tasks) {
> if (_task.limits().count("cpus")) {
>   executorLimitsResult.at("cpus") += _task.limits().at("cpus");
> } else {
>   Option taskCpus =
> Resources(_task.resources()).get("cpus");
>   if (taskCpus.isSome()) {
> executorLimitsResult.at("cpus") += taskCpus.get();
>   }
> }
> 
> if 

Re: Review Request 71858: 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 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?
> 
> Qian Zhang wrote:
> 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 :-)
> 
> Greg Mann wrote:
> > 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
> 
> Personally, that approach sounds better to me. I might be missing 
> something, but this seems like a classic use case for the `Option` type.
> 
> Qian Zhang wrote:
> I think the classic use case for the `Option` type is like:
> ```
> Option var;
> if (...) {
>   var = 
> }
> ```
> 
> But here with the `Option` type, the code will be something like:
> ```
> Option executorLimits;
> Map executorLimits_;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits_.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits_.insert({"mem", executorMemLimit.get()});
> }
> 
> if (!executorLimits_.empty()) {
>  executorLimits = executorLimits_;
> }
> 
> ```
> 
> The above code is a bit redundant to me, I think the code like the below 
> is better:
> ```
> Map executorLimits;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits.insert({"mem", executorMemLimit.get()});
> }
> ```
> 
> Greg Mann wrote:
> Ah, this is related to my other comment about refactoring L3140 to L3201 
> :)
> 
> If we used an `Option` and refactored as I proposed, and if we will 
> always set the CPU and mem hard limits together (i.e. never set just CPU hard 
> limits but not mem hard limits), then we could do something like:
> 
> ```
> foreach (const TaskInfo& task, tasks) {
> if (!task.limits().empty()) {
>   return true;
> }
>   }
> 
>   return false;
> };
> 
> Option> executorLimits;
> if (limitsAreSet(tasks)) {
>   executorLimitsResult = Map(
>   {"cpus": Value::Scalar(),
>"mem":  Value::Scalar()});
>   foreach (const TaskInfo& _task, tasks) {
> if (_task.limits().count("cpus")) {
>   executorLimitsResult.at("cpus") += _task.limits().at("cpus");
> } else {
>   Option taskCpus =
> Resources(_task.resources()).get("cpus");
>   if (taskCpus.isSome()) {
> executorLimitsResult.at("cpus") += taskCpus.get();
>   }
> }
> 
> if 

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

[Greg Mann]

> On Feb. 28, 2020, 5:57 p.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3262 (patched)
> > 
> >
> > We have validation which ensures that executor CPU and mem are always 
> > greater than or equal to the minimum values, so I think this could be a 
> > `CHECK()`?
> > 
> > 
> > https://github.com/apache/mesos/blob/998aee66bfedd1fe15bb1e1fc43a637fe91662a5/src/master/validation.cpp#L1842-L1859
> 
> Qian Zhang wrote:
> Unfortunately, in `task::internal::validateExecutor()`, if there is no 
> CPU or memory in executor resources, we will just log a warning message but 
> not return an error.
> 
> 
> https://github.com/apache/mesos/blob/1.9.0/src/master/validation.cpp#L1638-L1648

Ah shoot, too bad :'(


- Greg


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


On Feb. 25, 2020, 1:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Feb. 25, 2020, 1:46 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
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Qian Zhang]

> On Feb. 29, 2020, 1:57 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3262 (patched)
> > 
> >
> > We have validation which ensures that executor CPU and mem are always 
> > greater than or equal to the minimum values, so I think this could be a 
> > `CHECK()`?
> > 
> > 
> > https://github.com/apache/mesos/blob/998aee66bfedd1fe15bb1e1fc43a637fe91662a5/src/master/validation.cpp#L1842-L1859

Unfortunately, in `task::internal::validateExecutor()`, if there is no CPU or 
memory in executor resources, we will just log a warning message but not return 
an error.

https://github.com/apache/mesos/blob/1.9.0/src/master/validation.cpp#L1638-L1648


- Qian


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


On Feb. 25, 2020, 9:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Feb. 25, 2020, 9:46 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
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Qian Zhang]

> On Feb. 29, 2020, 1:55 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3201-3204 (original), 3285-3291 (patched)
> > 
> >
> > I was just thinking about this more... IIUC, we actually want to always 
> > set limits for executors right now, don't we? We previously discussed that 
> > we have several cases to handle (default, command, and custom executors), 
> > and I believe in the case of custom executors, we need to be sure that 
> > limits are always set. Wouldn't the current code allow a custom executor to 
> > be launched with no resource constraints if the framework specifies 
> > `shared_cgroups==false` and does not set limits in the tasks?

> Wouldn't the current code allow a custom executor to be launched with no 
> resource constraints if the framework specifies shared_cgroups==false and 
> does not set limits in the tasks?

In that case, for memory subsystem we will set custom executor's limit to its 
request which is the sum of all its task's request, so custom executor will 
still be constrained. For CPU subsystem, it depends on the 
`--cgroups_enable_cfs` flag, if this flag is true, then similar with memory 
subsytem we will set custom executor's CFS quota to its request which is the 
sum of all its task's request, if this flag is false, then yes the custom 
executor will not be constrained which makes sense because its tasks need to be 
unconstrained as well (since they have no limit set).


- Qian


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


On Feb. 25, 2020, 9:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Feb. 25, 2020, 9:46 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
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Qian Zhang]

> On Feb. 29, 2020, 1:38 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Lines 3201-3204 (original), 3285-3291 (patched)
> > 
> >
> > So it looks like here, we will only set limits for the executor if they 
> > are explicitly set for its tasks. This is different than what we intend to 
> > do for task containers, since in that case we will set their limits equal 
> > to their requests if `share_cgroups==true` (modulo the value of the agent's 
> > `--cgroups_enable_cfs` flag).
> > 
> > Should we do the same for the executor container?

> since in that case we will set their limits equal to their requests if 
> share_cgroups==true (modulo the value of the agent's --cgroups_enable_cfs 
> flag).

Did you mean `share_cgroups` is true or false? I think if it is true, we will 
neither set soft limit nor the hard limit for the individual task since it does 
not have its own cgroups.


I think we already do the similar for the executor container, I mean if no 
tasks have limits set, we will not set executor limit here, but we will always 
set executor resource requests to the sum of all its task's resource requests:
```
*executorInfo_.mutable_resources() =
  Resources(executorInfo.resources()) + totalTaskResources;
```
So in CPU subsystem of cgroup isolator, we will set executor container's CFS 
quota to its resource request if `--cgroups_enable_cfs` is true.


- Qian


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


On Feb. 25, 2020, 9:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Feb. 25, 2020, 9:46 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
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71858: 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?
> 
> Qian Zhang wrote:
> 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 :-)
> 
> Greg Mann wrote:
> > 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
> 
> Personally, that approach sounds better to me. I might be missing 
> something, but this seems like a classic use case for the `Option` type.
> 
> Qian Zhang wrote:
> I think the classic use case for the `Option` type is like:
> ```
> Option var;
> if (...) {
>   var = 
> }
> ```
> 
> But here with the `Option` type, the code will be something like:
> ```
> Option executorLimits;
> Map executorLimits_;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits_.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits_.insert({"mem", executorMemLimit.get()});
> }
> 
> if (!executorLimits_.empty()) {
>  executorLimits = executorLimits_;
> }
> 
> ```
> 
> The above code is a bit redundant to me, I think the code like the below 
> is better:
> ```
> Map executorLimits;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits.insert({"mem", executorMemLimit.get()});
> }
> ```
> 
> Greg Mann wrote:
> Ah, this is related to my other comment about refactoring L3140 to L3201 
> :)
> 
> If we used an `Option` and refactored as I proposed, and if we will 
> always set the CPU and mem hard limits together (i.e. never set just CPU hard 
> limits but not mem hard limits), then we could do something like:
> 
> ```
> foreach (const TaskInfo& task, tasks) {
> if (!task.limits().empty()) {
>   return true;
> }
>   }
> 
>   return false;
> };
> 
> Option> executorLimits;
> if (limitsAreSet(tasks)) {
>   executorLimitsResult = Map(
>   {"cpus": Value::Scalar(),
>"mem":  Value::Scalar()});
>   foreach (const TaskInfo& _task, tasks) {
> if (_task.limits().count("cpus")) {
>   executorLimitsResult.at("cpus") += _task.limits().at("cpus");
> } else {
>   Option taskCpus =
> Resources(_task.resources()).get("cpus");
>   if (taskCpus.isSome()) {
> executorLimitsResult.at("cpus") += taskCpus.get();
>   }
> }
> 
> if 

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

[Greg Mann]

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




src/slave/slave.cpp
Lines 3262 (patched)


We have validation which ensures that executor CPU and mem are always 
greater than or equal to the minimum values, so I think this could be a 
`CHECK()`?


https://github.com/apache/mesos/blob/998aee66bfedd1fe15bb1e1fc43a637fe91662a5/src/master/validation.cpp#L1842-L1859


- Greg Mann


On Feb. 25, 2020, 1:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Feb. 25, 2020, 1:46 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
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Greg Mann]

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




src/slave/slave.cpp
Lines 3201-3204 (original), 3285-3291 (patched)


I was just thinking about this more... IIUC, we actually want to always set 
limits for executors right now, don't we? We previously discussed that we have 
several cases to handle (default, command, and custom executors), and I believe 
in the case of custom executors, we need to be sure that limits are always set. 
Wouldn't the current code allow a custom executor to be launched with no 
resource constraints if the framework specifies `shared_cgroups==false` and 
does not set limits in the tasks?


- Greg Mann


On Feb. 25, 2020, 1:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Feb. 25, 2020, 1:46 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
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71858: 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 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?
> 
> Qian Zhang wrote:
> 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 :-)
> 
> Greg Mann wrote:
> > 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
> 
> Personally, that approach sounds better to me. I might be missing 
> something, but this seems like a classic use case for the `Option` type.
> 
> Qian Zhang wrote:
> I think the classic use case for the `Option` type is like:
> ```
> Option var;
> if (...) {
>   var = 
> }
> ```
> 
> But here with the `Option` type, the code will be something like:
> ```
> Option executorLimits;
> Map executorLimits_;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits_.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits_.insert({"mem", executorMemLimit.get()});
> }
> 
> if (!executorLimits_.empty()) {
>  executorLimits = executorLimits_;
> }
> 
> ```
> 
> The above code is a bit redundant to me, I think the code like the below 
> is better:
> ```
> Map executorLimits;
> 
> if (executorCpuLimit.isSome()) {
>   executorLimits.insert({"cpus", executorCpuLimit.get()});
> }
> 
> if (executorMemLimit.isSome()) {
>   executorLimits.insert({"mem", executorMemLimit.get()});
> }
> ```
> 
> Greg Mann wrote:
> Ah, this is related to my other comment about refactoring L3140 to L3201 
> :)
> 
> If we used an `Option` and refactored as I proposed, and if we will 
> always set the CPU and mem hard limits together (i.e. never set just CPU hard 
> limits but not mem hard limits), then we could do something like:
> 
> ```
> foreach (const TaskInfo& task, tasks) {
> if (!task.limits().empty()) {
>   return true;
> }
>   }
> 
>   return false;
> };
> 
> Option> executorLimits;
> if (limitsAreSet(tasks)) {
>   executorLimitsResult = Map(
>   {"cpus": Value::Scalar(),
>"mem":  Value::Scalar()});
>   foreach (const TaskInfo& _task, tasks) {
> if (_task.limits().count("cpus")) {
>   executorLimitsResult.at("cpus") += _task.limits().at("cpus");
> } else {
>   Option taskCpus =
> Resources(_task.resources()).get("cpus");
>   if (taskCpus.isSome()) {
> executorLimitsResult.at("cpus") += taskCpus.get();
>   }
> }
> 
> if 

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

[Greg Mann]

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




src/slave/slave.cpp
Lines 3201-3204 (original), 3285-3291 (patched)


So it looks like here, we will only set limits for the executor if they are 
explicitly set for its tasks. This is different than what we intend to do for 
task containers, since in that case we will set their limits equal to their 
requests if `share_cgroups==true` (modulo the value of the agent's 
`--cgroups_enable_cfs` flag).

Should we do the same for the executor container?


- Greg Mann


On Feb. 25, 2020, 1:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71858/
> ---
> 
> (Updated Feb. 25, 2020, 1:46 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
> ---
> 
> Set resource limits when launching executor container.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
>   src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 
> 
> 
> Diff: https://reviews.apache.org/r/71858/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71858: 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).
> 
> Qian Zhang wrote:
> 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.
> 
> Greg Mann wrote:
> > 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 )
> 
> That works when the executor is first launched, but will it be updated 
> when additional task groups are sent to the same executor?
> 
> > 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.
> 
> If t1 has a memory limit specified, then don't we want to set the CFS 
> quota regardless of whether or not the '--cgroups_enable_cfs' flag is set?
> 
> Qian Zhang wrote:
> > That works when the executor is first launched, but will it be updated 
> when additional task groups are sent to the same executor?
> 
> Yes, see https://reviews.apache.org/r/71952/ for details.
> 
> > If t1 has a memory limit specified, then don't we want to set the CFS 
> quota regardless of whether or not the '--cgroups_enable_cfs' flag is set?
> 
> I do not think we want to do it. In that case, there is no CPU limit 
> specified for both t1 and t2 (which means user does not want CPU hard limit 
> set) and CFS quota is disabled via the `--cgroups_enable_cfs` flag, so why do 
> we want to set CFS quota when user does not want it?
> 
> For memory limit, it is a bit different, the original behavior is we 
> always set both soft limit and hard limit to the memory allocated to the task 
> (i.e. memory request) when launching a container and there is no agent flag 
> to control it. So to keep backward compatibility, we want to set memory hard 
> limit even there is no memory limit specified in the task (i.e., set hard 
> limit to memory request). And what we want to change here is, setting hard 
> limit to a value which is larger than soft limit if memory limit is specified 
> in the task.
> 
> Greg Mann wrote:
> In the design doc, the table outlining different states 

Re: Review Request 71858: 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?
> 
> 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).
> 
> Qian Zhang wrote:
> 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.
> 
> Greg Mann wrote:
> > 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 )
> 
> That works when the executor is first launched, but will it be updated 
> when additional task groups are sent to the same executor?
> 
> > 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.
> 
> If t1 has a memory limit specified, then don't we want to set the CFS 
> quota regardless of whether or not the '--cgroups_enable_cfs' flag is set?
> 
> Qian Zhang wrote:
> > That works when the executor is first launched, but will it be updated 
> when additional task groups are sent to the same executor?
> 
> Yes, see https://reviews.apache.org/r/71952/ for details.
> 
> > If t1 has a memory limit specified, then don't we want to set the CFS 
> quota regardless of whether or not the '--cgroups_enable_cfs' flag is set?
> 
> I do not think we want to do it. In that case, there is no CPU limit 
> specified for both t1 and t2 (which means user does not want CPU hard limit 
> set) and CFS quota is disabled via the `--cgroups_enable_cfs` flag, so why do 
> we want to set CFS quota when user does not want it?
> 
> For memory limit, it is a bit different, the original behavior is we 
> always set both soft limit and hard limit to the memory allocated to the task 
> (i.e. memory request) when launching a container and there is no agent flag 
> to control it. So to keep backward compatibility, we want to set memory hard 
> limit even there is no memory limit specified in the task (i.e., set hard 
> limit to memory request). And what we want to change here is, setting hard 
> limit to a value which is larger than soft limit if memory limit is specified 
> in the task.
> 
> Greg Mann wrote:
> In the design doc, the table outlining different states 

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

[Qian Zhang]

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

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


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Minor change.


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


Repository: mesos


Description
---

Set resource limits when launching executor container.


Diffs (updated)
-

  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 


Diff: https://reviews.apache.org/r/71858/diff/10/

Changes: https://reviews.apache.org/r/71858/diff/9-10/


Testing
---


Thanks,

Qian Zhang

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

[Qian Zhang]

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

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


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Accommodated infinite value.


Summary (updated)
-

Set resource limits when launching executor container.


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


Repository: mesos


Description (updated)
---

Set resource limits when launching executor container.


Diffs (updated)
-

  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 


Diff: https://reviews.apache.org/r/71858/diff/9/

Changes: https://reviews.apache.org/r/71858/diff/8-9/


Testing
---


Thanks,

Qian Zhang