Re: Review Request 72041: Updated default executor to call the `LaunchContainer` agent API.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [72161, 72162, 71858, 71884, 71885, 71886, 71943, 71944, 
71950, 71951, 71952, 71953, 71955, 71956, 71983, 72022, 72027, 72040, 72041]

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

Error:
..
KAGE_URL=\"\" -DPACKAGE=\"mesos\" -DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 
-DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 
-DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 
-DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 
-DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 
-DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 -DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 
-DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 -DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 
-DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 
-DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. 
-I../../../../3rdparty/stout  -I../../../../3rdparty/stout/include 
-I../boost-1.65.0 -I../elfio-3.2 -I../glog-0.4.0/src 
-I../googletest-release-1.8.0/googlemock/include 
-I../googletest-release-1.8.0/googletest/include 
-I../libarchive-3.3.2/libarchive -D__STDC_FORMAT_MACROS -I../picojson-1.3.0 
-I../protobuf-3.5.0/src -I../rapidjson-1.1.0/
 include  -I/usr/include/subversion-1 -I/usr/include/apr-1 
-I/usr/include/apr-1.0  -Wall -Wsign-compare -Wformat-security 
-fstack-protector-strong -fPIC -fPIE -g1 -O0 -Wno-unused-local-typedefs 
-std=c++11 -c -o stout_tests-ip_tests.o `test -f 'tests/ip_tests.cpp' || echo 
'../../../../3rdparty/stout/'`tests/ip_tests.cpp
g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.10.0\" -DPACKAGE_STRING=\"mesos\ 1.10.0\" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 
-DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 
-DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 
-DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. -I../../../../3rdparty/stout  
-I../../../../3rdparty/stout/include -I../boost-1.65.0 -I../elfio-3.2 
-I../glog-0.4.0/src -I../googletest-release-1.8.0/googlemock/include 
-I../googletest-rel
 ease-1.8.0/googletest/include -I../libarchive-3.3.2/libarchive 
-D__STDC_FORMAT_MACROS -I../picojson-1.3.0 -I../protobuf-3.5.0/src 
-I../rapidjson-1.1.0/include  -I/usr/include/subversion-1 -I/usr/include/apr-1 
-I/usr/include/apr-1.0   -Wall -Wsign-compare -Wformat-security 
-fstack-protector-strong -fPIC -fPIE -g1 -O0 -Wno-unused-local-typedefs 
-std=c++11 -c -o stout_tests-json_tests.o `test -f 'tests/json_tests.cpp' || 
echo '../../../../3rdparty/stout/'`tests/json_tests.cpp
g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.10.0\" -DPACKAGE_STRING=\"mesos\ 1.10.0\" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.10.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DENABLE_NVML=1 
-DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 
-DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 
-DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. -I../../../../3rdparty/stout  
-I../../../../3rdparty/stout/include -I../boost-1.65.0 -I../elfio-3.2 
-I../glog-0.4.0/src -I../googletest-release-1.8.0/googlemock/include 
-I../googletest-rel
 ease-1.8.0/googletest/include -I../libarchive-3.3.2/libarchive 
-D__STDC_FORMAT_MACROS -I../picojson-1.3.0 -I../protobuf-3.5.0/src 
-I../rapidjson-1.1.0/include  -I/usr/include/subversion-1 -I/usr/include/apr-1 
-I/usr/include/apr-1.0   -Wall -Wsign-compare -Wformat-security 
-fstack-protector-strong -fPIC -fPIE -g1 -O0 -Wno-unused-local-typedefs 
-std=c++11 -c -o stout_tests-jsonify_tests.o `test 

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 71950: Updated containerizer's `update()` method to handle resource limits.

[Qian Zhang]

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

(Updated March 4, 2020, 9:43 a.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
---

Updated containerizer's `update()` method to handle resource limits.


Diffs (updated)
-

  src/slave/containerizer/composing.hpp 
0f838fa63b02d38606a6c484f01d24952d569721 
  src/slave/containerizer/composing.cpp 
d854794fc4775fb8a05efc233d488a64b9ef620a 
  src/slave/containerizer/containerizer.hpp 
6a9ebed5e55ad1cfed6340479743ef2bb4c05dab 
  src/slave/containerizer/docker.hpp 0349f537cef9651427e0e3ed33fd693107e07aa0 
  src/slave/containerizer/docker.cpp 2a9b2ffcbd01ae916839ae43c8342285ac3e14a2 
  src/slave/containerizer/mesos/containerizer.hpp 
9e86ff43535c4be937baa238442e8f0db8857a27 
  src/slave/containerizer/mesos/containerizer.cpp 
d034c02c0f73a2745a063561430a1bce7b552420 
  src/tests/containerizer.hpp e05ce0323e032911d46d8682661ffe4463fdc276 
  src/tests/containerizer.cpp 3ac992f8599e8a48f428b506f5bdecc722050c6a 
  src/tests/containerizer/docker_containerizer_tests.cpp 
689a7220a09f2a58dffdf0dc9fd9f0548600be0e 
  src/tests/containerizer/mock_containerizer.hpp 
8700257a9347199ed6e32b8b6b2a58787d68af03 
  src/tests/master_draining_tests.cpp a91201e0bfb6f53f70dbdc4649cc344076ef474b 
  src/tests/master_tests.cpp d0f53b2139bf36ff15a27e438f058f4914df5caa 
  src/tests/mock_docker.hpp 4a2266fb1239cbc96c7df74b997212e3b3b01c75 
  src/tests/registrar_zookeeper_tests.cpp 
9d3ea5f194374fb3492fdfa8bd2efeef55fc4743 
  src/tests/scheduler_tests.cpp 299d3a06ca1fd55ef55dc9be323b98a4da3be31a 
  src/tests/slave_tests.cpp 92fa2996f0e39dded79aa372ddf2390b68b885a2 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72187: Fixed build on platforms not treating initializer_list as literal.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [72187]

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

- Mesos Reviewbot


On March 3, 2020, 8:14 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72187/
> ---
> 
> (Updated March 3, 2020, 8:14 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10056
> https://issues.apache.org/jira/browse/MESOS-10056
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed build on platforms not treating initializer_list as literal.
> 
> 
> Diffs
> -
> 
>   src/master/framework.cpp 7e46469dc727ea6f38ca6331b649b3c80e343171 
> 
> 
> Diff: https://reviews.apache.org/r/72187/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72187: Fixed build on platforms not treating initializer_list as literal.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 3, 2020, 8:14 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72187/
> ---
> 
> (Updated March 3, 2020, 8:14 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10056
> https://issues.apache.org/jira/browse/MESOS-10056
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed build on platforms not treating initializer_list as literal.
> 
> 
> Diffs
> -
> 
>   src/master/framework.cpp 7e46469dc727ea6f38ca6331b649b3c80e343171 
> 
> 
> Diff: https://reviews.apache.org/r/72187/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72187: Fixed build on platforms not treating initializer_list as literal.

[Andrei Sekretenko]

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

(Updated March 3, 2020, 8:14 p.m.)


Review request for mesos, Andrei Budnik and Benjamin Mahler.


Changes
---

Moved as a brace-init-list into `createObjectApprovers(...)` to avoid non-pod 
global static and other initializer_list lifetime issues.


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


Repository: mesos


Description
---

Fixed build on platforms not treating initializer_list as literal.


Diffs (updated)
-

  src/master/framework.cpp 7e46469dc727ea6f38ca6331b649b3c80e343171 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72122: Fixed `cgroups::create` for nested cgroups.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [72122, 71966, 71965, 72121]

Error:
2020-03-03 19:01:03 URL:https://reviews.apache.org/r/71965/diff/raw/ 
[12043/12043] -> "71965.patch" [1]
error: patch failed: 
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp:818
error: src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp: patch does 
not apply

- Mesos Reviewbot


On Feb. 12, 2020, 3:48 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72122/
> ---
> 
> (Updated Feb. 12, 2020, 3:48 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch modifies `cgroups::create` function to call
> `cloneCpusetCpusMems` for all absent nested cgroups along
> the path to a cgroup that is accepted as an argument of this function.
> For instance, if `cgroups::create` is called to create three
> non-existent cgroups recursively for the path `/a/b/c`, then
> `cloneCpusetCpusMems` is called to clone both `cpuset.cpus` and
> `cpuset.mems` for `/a` from its parent, then `/a/b` from `/a`,
> and so on down the path.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 73646c9eb39948192acedb67e3d2fb13acb14b30 
> 
> 
> Diff: https://reviews.apache.org/r/72122/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 72187: Fixed build on platforms not treating initializer_list as literal.

[Benjamin Mahler]

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


Ship it!




Isn't this a static non-POD (which we ban per the google style guide)? If so, 
then maybe use a pointer or make it a function?

- Benjamin Mahler


On March 3, 2020, 5:14 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72187/
> ---
> 
> (Updated March 3, 2020, 5:14 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10056
> https://issues.apache.org/jira/browse/MESOS-10056
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed build on platforms not treating initializer_list as literal.
> 
> 
> Diffs
> -
> 
>   src/master/framework.cpp 7e46469dc727ea6f38ca6331b649b3c80e343171 
> 
> 
> Diff: https://reviews.apache.org/r/72187/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 72185: Disabled RoleTest.RolesEndpointContainsConsumedQuota on Windows.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [72185]

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

- Mesos Reviewbot


On March 3, 2020, 5:10 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72185/
> ---
> 
> (Updated March 3, 2020, 5:10 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch temporarily disables this test on Windows due to the need
> to use "posix" launcher in the current implementation of the test.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp fb8122b4ca5a158e61a100bd91ec6fd752a06bab 
> 
> 
> Diff: https://reviews.apache.org/r/72185/diff/2/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh 
> --gtest_filter="*RoleTest.RolesEndpointContainsConsumedQuota*" 
> --gtest_break_on_failure --verbose` on Linux.
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Review Request 72187: Fixed build on platforms not treating initializer_list as literal.

[Andrei Sekretenko]

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

Review request for mesos, Andrei Budnik and Benjamin Mahler.


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


Repository: mesos


Description
---

Fixed build on platforms not treating initializer_list as literal.


Diffs
-

  src/master/framework.cpp 7e46469dc727ea6f38ca6331b649b3c80e343171 


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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 72185: Disabled RoleTest.RolesEndpointContainsConsumedQuota on Windows.

[Andrei Sekretenko]

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

(Updated March 3, 2020, 5:10 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added TODO for re-enabling.


Repository: mesos


Description
---

This patch temporarily disables this test on Windows due to the need
to use "posix" launcher in the current implementation of the test.


Diffs (updated)
-

  src/tests/role_tests.cpp fb8122b4ca5a158e61a100bd91ec6fd752a06bab 


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

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


Testing
---

`./bin/mesos-tests.sh 
--gtest_filter="*RoleTest.RolesEndpointContainsConsumedQuota*" 
--gtest_break_on_failure --verbose` on Linux.


Thanks,

Andrei Sekretenko

Re: Review Request 72185: Disabled RoleTest.RolesEndpointContainsConsumedQuota on Windows.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/tests/role_tests.cpp
Line 494 (original), 494 (patched)


should we add a TODO here saying why it's disabled and that we'd like to 
enable it?


- Benjamin Mahler


On March 3, 2020, 2:04 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72185/
> ---
> 
> (Updated March 3, 2020, 2:04 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch temporarily disables this test on Windows due to the need
> to use "posix" launcher in the current implementation of the test.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp fb8122b4ca5a158e61a100bd91ec6fd752a06bab 
> 
> 
> Diff: https://reviews.apache.org/r/72185/diff/1/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh 
> --gtest_filter="*RoleTest.RolesEndpointContainsConsumedQuota*" 
> --gtest_break_on_failure --verbose` on Linux.
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

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

[Qian Zhang]

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

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


- Qian


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


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

Re: Review Request 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
> 
>

Review Request 72185: Disabled RoleTest.RolesEndpointContainsConsumedQuota on Windows.

[Andrei Sekretenko]

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

Review request for mesos.


Repository: mesos


Description
---

This patch temporarily disables this test on Windows due to the need
to use "posix" launcher in the current implementation of the test.


Diffs
-

  src/tests/role_tests.cpp fb8122b4ca5a158e61a100bd91ec6fd752a06bab 


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


Testing
---

`./bin/mesos-tests.sh 
--gtest_filter="*RoleTest.RolesEndpointContainsConsumedQuota*" 
--gtest_break_on_failure --verbose` on Linux.


Thanks,

Andrei Sekretenko