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

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 1069 (patched)


Will we have the issue mentioned here: 
https://github.com/apache/mesos/blob/1.9.0/src/common/protobuf_utils.cpp#L1058:L1063
 ?


- Qian Zhang


On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> ---
> 
> (Updated Feb. 14, 2020, 1:19 a.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
> disabled `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/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Qian Zhang]

> On March 8, 2020, 10:29 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 743-748 (original), 758-762 (patched)
> > 
> >
> > I think this is for the case: Agent is started with a cgroup subsystem 
> > specified (like `--isolation=cgroups/cpu`) and a default executor is 
> > launched to run a task group with `share_cgroups==true`, now agent is 
> > restarted with two cgroup subsystems (like 
> > `--isolation=cgroups/cpu,cgroups/mem`) and another task group with 
> > `share_cgroups==true` is launched by the same default executor. For the 
> > nested containers corresponding to the second task group, we should NOT 
> > assign their pids for the subsystem `cgroups/mem` since the default 
> > executor does not have cgroup created under it.
> > 
> > So I think we should not change the log message here.
> 
> Andrei Budnik wrote:
> After introducing the support for nested cgroups, a `cgroup` variable 
> might refer to a nested container's cgroup rather than a root container's 
> cgroup. If a nested cgroup is lost for some reason, we shouldn't claim that a 
> parent cgroup doesn't have the cgroup hierarchy because it can actually 
> exist. In fact, a nested cgroup might be missing by 2 reasons: a) parent 
> cgroups is absent (due to the reason you've described) b) the parent cgroup 
> is here, but a nested cgroup is absent. I decided to update the log message 
> in order to make it more generic.

> In fact, a nested cgroup might be missing by 2 reasons: a) parent cgroups is 
> absent (due to the reason you've described) b) the parent cgroup is here, but 
> a nested cgroup is absent.

For a) it is correct to log an INFO message just like what we did before, but 
for b) is it OK just log an INFO message? In this case there must be something 
wrong, i.e., we can find the `Info` struct for the nested container, but its 
own cgroup does not exist somehow, we should return an Error in this case, 
right?

So basically in this method we still need to know if the nested container has 
`share_cgroup` as true or false, if it is true but its parent container's 
cgroup does not exist, that is a), we should just log an INFO message exactly 
like before. And for the other cases (the nested container whose `share_cgroup` 
as false and the top-level container), we should just go ahead with 
`cgroups::assign()`, if the cgroup does not exist, `cgroups::assign()` will 
give us reasonable error.


Maybe we could change
```
if (containerId.has_parent() && !cgroups::exists(hierarchy, info->cgroup)) {
```
to:
```
if (containerId.has_parent() && containerId != info->containerId && 
!cgroups::exists(hierarchy, info->cgroup)) {
```

In the above way, we know it is a nested container sharing its parent 
container's cgroups but the parent container's cgroup does not exist for the 
reason that I described above, so we just log the original INFO message. HDYT?


- Qian


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


On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> ---
> 
> (Updated Feb. 14, 2020, 1:19 a.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
> disabled `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/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

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

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 1069 (patched)


Will we have the issue mentioned here: 
https://github.com/apache/mesos/blob/1.9.0/src/common/protobuf_utils.cpp#L1058:L1063
 ?


- Qian Zhang


On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> ---
> 
> (Updated Feb. 14, 2020, 1:19 a.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
> disabled `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/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 72222: Added tests for agent validation of shared cgroups.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [72161, 72162, 71858, 71884, 71885, 71886, 71943, 71944, 
71950, 71951, 71952, 71953, 71955, 72216, 72217, 72221, 7]

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_7"]

Error:
..
rty_la-mount.lo slave/containerizer/mesos/libmesos_no_3rdparty_la-paths.lo 
slave/containerizer/mesos/provisioner/appc/libmesos_no_3rdparty_la-cache.lo 
slave/containerizer/mesos/provisioner/appc/libmesos_no_3rdparty_la-fetcher.lo 
slave/containerizer/mesos/provisioner/appc/libmesos_no_3rdparty_la-paths.lo 
slave/containerizer/mesos/provisioner/appc/libmesos_no_3rdparty_la-store.lo 
slave/containerizer/mesos/provisioner/libmesos_no_3rdparty_la-backend.lo 
slave/containerizer/mesos/provisioner/backends/libmesos_no_3rdparty_la-copy.lo 
slave/containerizer/mesos/provisioner/docker/libmesos_no_3rdparty_la-image_tar_puller.lo
 
slave/containerizer/mesos/provisioner/docker/libmesos_no_3rdparty_la-metadata_manager.lo
 slave/containerizer/mesos/provisioner/docker/libmesos_no_3rdparty_la-paths.lo 
slave/containerizer/mesos/provisioner/docker/libmesos_no_3rdparty_la-puller.lo 
slave/containerizer/mesos/provisioner/docker/libmesos_no_3rdparty_la-registry_puller.lo
 slave/containerizer/mesos/provisioner/dock
 er/libmesos_no_3rdparty_la-store.lo 
slave/containerizer/mesos/provisioner/libmesos_no_3rdparty_la-paths.lo 
slave/containerizer/mesos/provisioner/libmesos_no_3rdparty_la-provisioner.lo 
slave/containerizer/mesos/provisioner/libmesos_no_3rdparty_la-store.lo 
slave/containerizer/mesos/provisioner/libmesos_no_3rdparty_la-utils.lo 
slave/containerizer/mesos/libmesos_no_3rdparty_la-utils.lo 
slave/libmesos_no_3rdparty_la-flags.lo slave/libmesos_no_3rdparty_la-gc.lo 
slave/libmesos_no_3rdparty_la-http.lo slave/libmesos_no_3rdparty_la-metrics.lo 
slave/libmesos_no_3rdparty_la-paths.lo 
slave/libmesos_no_3rdparty_la-qos_controller.lo 
slave/qos_controllers/libmesos_no_3rdparty_la-noop.lo 
slave/libmesos_no_3rdparty_la-resource_estimator.lo 
slave/resource_estimators/libmesos_no_3rdparty_la-noop.lo 
slave/libmesos_no_3rdparty_la-slave.lo slave/libmesos_no_3rdparty_la-state.lo 
slave/libmesos_no_3rdparty_la-task_status_update_manager.lo 
slave/libmesos_no_3rdparty_la-validation.lo slave/volume_gid_manager/
 libmesos_no_3rdparty_la-volume_gid_manager.lo 
status_update_manager/libmesos_no_3rdparty_la-operation.lo 
uri/libmesos_no_3rdparty_la-fetcher.lo 
uri/fetchers/libmesos_no_3rdparty_la-copy.lo 
uri/fetchers/libmesos_no_3rdparty_la-curl.lo 
uri/fetchers/libmesos_no_3rdparty_la-docker.lo 
uri/fetchers/libmesos_no_3rdparty_la-hadoop.lo 
uri/libmesos_no_3rdparty_la-utils.lo usage/libmesos_no_3rdparty_la-usage.lo 
v1/libmesos_no_3rdparty_la-attributes.lo v1/libmesos_no_3rdparty_la-mesos.lo 
v1/libmesos_no_3rdparty_la-resources.lo v1/libmesos_no_3rdparty_la-values.lo 
version/libmesos_no_3rdparty_la-version.lo 
watcher/libmesos_no_3rdparty_la-whitelist_watcher.lo 
zookeeper/libmesos_no_3rdparty_la-authentication.lo 
zookeeper/libmesos_no_3rdparty_la-contender.lo 
zookeeper/libmesos_no_3rdparty_la-detector.lo 
zookeeper/libmesos_no_3rdparty_la-group.lo 
zookeeper/libmesos_no_3rdparty_la-zookeeper.lo 
linux/libmesos_no_3rdparty_la-capabilities.lo 
linux/libmesos_no_3rdparty_la-cgroups.lo linux/libmesos_no_3rd
 party_la-fs.lo linux/libmesos_no_3rdparty_la-ldcache.lo 
linux/libmesos_no_3rdparty_la-ldd.lo linux/libmesos_no_3rdparty_la-ns.lo 
linux/libmesos_no_3rdparty_la-perf.lo linux/libmesos_no_3rdparty_la-systemd.lo 
slave/containerizer/mesos/libmesos_no_3rdparty_la-linux_launcher.lo 
slave/containerizer/mesos/isolators/appc/libmesos_no_3rdparty_la-runtime.lo 
slave/containerizer/mesos/isolators/cgroups/libmesos_no_3rdparty_la-cgroups.lo 
slave/containerizer/mesos/isolators/cgroups/libmesos_no_3rdparty_la-subsystem.lo
 
slave/containerizer/mesos/isolators/cgroups/subsystems/libmesos_no_3rdparty_la-blkio.lo
 
slave/containerizer/mesos/isolators/cgroups/subsystems/libmesos_no_3rdparty_la-cpu.lo
 
slave/containerizer/mesos/isolators/cgroups/subsystems/libmesos_no_3rdparty_la-cpuacct.lo
 
slave/containerizer/mesos/isolators/cgroups/subsystems/libmesos_no_3rdparty_la-cpuset.lo
 
slave/containerizer/mesos/isolators/cgroups/subsystems/libmesos_no_3rdparty_la-devices.lo
 slave/containerizer/mesos/isolators/cgroup
 s/subsystems/libmesos_no_3rdparty_la-hugetlb.lo 
slave/containerizer/mesos/isolators/cgroups/subsystems/libmesos_no_3rdparty_la-memory.lo
 
slave/containerizer/mesos/isolators/cgroups/subsystems/libmesos_no_3rdparty_la-net_cls.lo
 

Re: Review Request 72216: Added master validation for task resource limits and shared cgroups.

[Greg Mann]

> On March 10, 2020, 1:06 p.m., Andrei Budnik wrote:
> > src/master/validation.cpp
> > Lines 1567 (patched)
> > 
> >
> > `if (limits.size() > cpuCount + memCount)` might be slightly easier to 
> > understand.

Good call; done.


- Greg


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


On March 11, 2020, 7:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72216/
> ---
> 
> (Updated March 11, 2020, 7:10 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Qian Zhang.
> 
> 
> Bugs: MESOS-10045
> https://issues.apache.org/jira/browse/MESOS-10045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added master validation for task resource limits and shared cgroups.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2 
> 
> 
> Diff: https://reviews.apache.org/r/72216/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72216: Added master validation for task resource limits and shared cgroups.

[Greg Mann]

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

(Updated March 11, 2020, 7:10 p.m.)


Review request for mesos, Andrei Budnik and Qian Zhang.


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


Repository: mesos


Description
---

Added master validation for task resource limits and shared cgroups.


Diffs (updated)
-

  src/master/validation.cpp 084f281eadd65cb8ae0a19b7b7797dc71ccebdd2 


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

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


Testing
---


Thanks,

Greg Mann

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

[Andrei Budnik]

> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 779-785 (original), 797-803 (patched)
> > 
> >
> > This logic is correct for root container since we will return an 
> > `Unknown container` failure for it if `infos` does not contain it, but it 
> > seems not correct for the nested container whose `share_cgroup` is false, 
> > for such container, if `infos` does not contain it, we will always return a 
> > pending future but never return the `unknown container` failure, that might 
> > hide some errors in our code. I think we should also return an `Unknown 
> > container` failure for the nested container whose `share_cgroup` is false 
> > and somehow `infos` does not contain it. HDYT?
> 
> Andrei Budnik wrote:
> That's a good point!
> Unfortunately, `share_cgroup` flag is unknown for terminated containers. 
> A terminated nested container with `share_cgroup=false` is indistinguishable 
> from a running container with `share_cgroup=true`, because they both are not 
> presented in the `infos`.
> We can keep `Info` for all existing containers and add `share_cgroup` 
> field to it.
> 
> Andrei Budnik wrote:
> On second thought, we won't be able to create `Info` for nested 
> containers with shared cgroups during the recovery. They're invisible to the 
> cgroups isolator.
> 
> Andrei Budnik wrote:
> ```
> This logic is correct for root container since we will return an Unknown 
> container failure for it if infos does not contain it, but it seems not 
> correct for the nested container whose share_cgroup is false, for such 
> container, if infos does not contain it, we will always return a pending 
> future but never return the unknown container failure, that might hide some 
> errors in our code.
> ```
> 
> The previous version was prone to the problem you have described above: 
> we could return a pending future for a non-existent nested container. This 
> code change neither introduces new problems nor fixes the existing problem.
> 
> Qian Zhang wrote:
> Could you please add a `TODO` in this code to mention that ideally we 
> should return an unknown container failure for the nested container whose 
> `share_cgroups` is false but `infos` does not contain it?
> 
> Or how about we add a new parameter `const ContainerConfig& 
> containerConfig` to the `watch()` method of the isolator interface, and with 
> such parameter in `watch()` method we will clearly know if the container's 
> `share_cgroups` is true or false, if it is false and the container does not 
> exist in `infos`, we can just return an unknown container failure.
> 
> Andrei Budnik wrote:
> If a container shares cgroups with its parent, then the cgroups isolator 
> should not be in charge of this container. So, returning "Unknown" seems 
> totally fine for me, and I don't think we need a comment either.

> ideally we should return an unknown container failure for the nested 
> container whose share_cgroups is false but infos does not contain it

I think the only reason for this to happen is a bug in our code. If this 
happens, then many other methods will be affected as well. For instance, 
`usage()` will return a failure if a running nested container with 
`share_cgroups=false` is unknown to the cgroups isolator. So I'm not sure if we 
should detect or think about the potential bug in this particular method.


- Andrei


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


On Фев. 13, 2020, 5:19 п.п., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> ---
> 
> (Updated Фев. 13, 2020, 5:19 п.п.)
> 
> 
> 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
> disabled `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/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik

[GitHub] [mesos] eLvErDe opened a new pull request #352: Typo in master.md

[GitBox]

eLvErDe opened a new pull request #352: Typo in master.md
URL: https://github.com/apache/mesos/pull/352
 
 
   


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 72222: Added tests for agent validation of shared cgroups.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [72161, 72162, 71858, 71884, 71885, 71886, 71943, 71944, 
71950, 71951, 71952, 71953, 71955, 72216, 72217, 72221, 7]

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_7"]

Error:
..
AGE_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/i
 nclude  -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 -f 

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/#review219885
---



Bad review!

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

Error:
2020-03-11 10:43:08 URL:https://reviews.apache.org/r/71956/diff/raw/ 
[7818/7818] -> "71956.patch" [1]
error: patch failed: src/tests/containerizer/cgroups_isolator_tests.cpp:50
error: src/tests/containerizer/cgroups_isolator_tests.cpp: patch does not apply

- Mesos Reviewbot


On March 5, 2020, 1:02 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72041/
> ---
> 
> (Updated March 5, 2020, 1:02 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10063
> https://issues.apache.org/jira/browse/MESOS-10063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated default executor to call the `LaunchContainer` agent API.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp b431d8f86a359d64f84ea1c5fae63ee6cbddba31 
> 
> 
> Diff: https://reviews.apache.org/r/72041/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72022: Set resource limits and OOM score adjustment in Docker executor.

[Qian Zhang]

> On March 6, 2020, 4:06 p.m., Greg Mann wrote:
> > src/docker/docker.cpp
> > Lines 685 (patched)
> > 
> >
> > Is it not possible for us to handle infinite limits here?

For infinite limits, we actually do not need to handle them, then the 
corresponding Docker options will not be set when we run `docker run` which 
means unlimited resource limits. I will add a comment here to explain this.


> On March 6, 2020, 4:06 p.m., Greg Mann wrote:
> > src/docker/docker.cpp
> > Line 663 (original), 714-716 (patched)
> > 
> >
> > Do we want to set the memory reservation here as well?

I do not think we want to do that because of backward compatibility.


- Qian


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


On March 11, 2020, 5:41 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72022/
> ---
> 
> (Updated March 11, 2020, 5:41 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10053
> https://issues.apache.org/jira/browse/MESOS-10053
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set resource limits and OOM score adjustment in Docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81 
>   src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd 
>   src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107 
> 
> 
> Diff: https://reviews.apache.org/r/72022/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72022: Set resource limits and OOM score adjustment in Docker executor.

[Qian Zhang]

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

(Updated March 11, 2020, 5:41 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Set resource limits and OOM score adjustment in Docker executor.


Diffs (updated)
-

  src/docker/docker.hpp b48d894ed59170849f4b4ac5e37842af6c421e81 
  src/docker/docker.cpp 04fb8d0ffbbfb9d10dd39a7b49a37f6e8c751bfd 
  src/docker/executor.cpp ebbbc0df043d1bf4b0712e4130a24abdfa5eb107 


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

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


Testing
---


Thanks,

Qian Zhang

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

[Qian Zhang]

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

(Updated March 11, 2020, 5:40 p.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Set container process's OOM score adjust.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
b12b73d8e0161d448075378765e77867521de04e 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
a311ab4495f71bedacd2e99c84c765f0e5fe99d3 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
dc6c7aa1c998c30c8b17db04a38e7a1e28a6a6c1 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
c62deec4b1cd749dba5fe71b901e0353806a0805 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
ac2e66b570bb84b43c4a3e8f19b40e5fcea71a4a 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
27d88e91fb784179effd54781f84000fe85c13eb 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
0896d37761a11f55ba4b866d235c3bd2b79dcfba 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
06531072f445d4ec978ebaf5ec5e4a2475517d05 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
ec2ce67e54387f26aa11c00d4c7f85f0807a127b 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
2c865aca35084a5db567b5f95c8c57bb6e1d5634 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp 
180afc936798c2fa4de0deef080276cf7cc94199 
  src/slave/containerizer/mesos/utils.hpp 
bfd07e28c78fc140e395ffccd11d65545bf007fc 
  src/slave/containerizer/mesos/utils.cpp 
d9964f0b0e8ad8a41e5f5490a1c28bba9a972ae0 


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

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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 71952: Set resource limits when updating executor container.

[Greg Mann]

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


Fix it, then Ship it!





src/slave/slave.hpp
Lines 782-784 (original), 782-785 (patched)


At this point, I think a comment for this helper would be useful, to 
explain why we have both `taskInfos` and `tasks` arguments. Maybe something 
like:

```
// This function is used to compute limits for executors
// before they are launched as well as when updating
// running executors, so we must accept both `TaskInfo`
// and `Task` types to handle both cases.
```



src/slave/slave.cpp
Line 9989 (original), 10049-10050 (patched)


Let's add a `CHECK_NOTNULL(task);` here.


- Greg Mann


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

Re: Review Request 71950: Updated containerizer's `update()` method to handle resource limits.

[Greg Mann]

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



Could you update the description of this patch with a note that not all 
callsites of `Containerizer::update` are updated here, since some involve 
significant related code changes? In particular, I noticed that a couple 
callsites are updated in https://reviews.apache.org/r/71952/, but it doesn't 
seem desirable to merge these two patches together.

- Greg Mann


On March 4, 2020, 1:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71950/
> ---
> 
> (Updated March 4, 2020, 1:43 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10050
> https://issues.apache.org/jira/browse/MESOS-10050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated containerizer's `update()` method to handle resource limits.
> 
> 
> Diffs
> -
> 
>   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/
> 
> 
> 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 71951: Added resource limits into the `Task` protobuf message.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 9, 2020, 2:05 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71951/
> ---
> 
> (Updated March 9, 2020, 2:05 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10050
> https://issues.apache.org/jira/browse/MESOS-10050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added resource limits into the `Task` protobuf message.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 40c45de371b12d84aeebcc91847336c4968592f2 
>   include/mesos/v1/mesos.proto 638763612ccb302f052eb30caf661984087314e1 
>   src/common/protobuf_utils.cpp b3057be7460100201066a354a1f621d100546233 
> 
> 
> Diff: https://reviews.apache.org/r/71951/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71943: Set container's `memory.limit_in_bytes` to its memory limit.

[Greg Mann]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Line 212 (original), 222-231 (patched)


I find this block a little unintuitive at first glance; perhaps a comment 
would help?

```
// Rather than trying to represent an infinite limit with
// the `Bytes` type, we represent the infinite case by
// setting `isInfiniteLimit` to `true` and letting
// `hardLimit` be NONE.
```



src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp
Lines 240 (patched)


Wrap at 80 characters.


- Greg Mann


On March 6, 2020, 8:33 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71943/
> ---
> 
> (Updated March 6, 2020, 8:33 a.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's `memory.limit_in_bytes` to its memory limit.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> 27d88e91fb784179effd54781f84000fe85c13eb 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> 0896d37761a11f55ba4b866d235c3bd2b79dcfba 
> 
> 
> Diff: https://reviews.apache.org/r/71943/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71955: Added a new parameter `resourceLimits` to the `createTask` methods.

[Greg Mann]

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




src/tests/mesos.hpp
Lines 105 (patched)


We always use fully-qualified names in headers; could you remove this 
`using` directive?


- Greg Mann


On March 10, 2020, 8:50 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71955/
> ---
> 
> (Updated March 10, 2020, 8:50 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Greg Mann.
> 
> 
> Bugs: MESOS-10047
> https://issues.apache.org/jira/browse/MESOS-10047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new parameter `resourceLimits` to the `createTask` methods.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 17fcaa3eff28e8139e6770629a5dfbb7c29289ec 
> 
> 
> Diff: https://reviews.apache.org/r/71955/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>