Re: Review Request 71197: Updated the `disk/du` disk isolator tests with rootfs cases.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71192, 71193, 71194, 71195, 71196, 71197]

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

Error:
..
libs/libprocess_la-gtest_constants.o
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.9.0\" "-DPACKAGE_STRING=\"mesos 1.9.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.9.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 -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/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.9.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
-I../boost-1.65.0 -I
 ../concurrentqueue-7b69a8f -I../elfio-3.2 -I../glog-0.4.0/src 
-I../grpc-1.10.0/include -I../http-parser-2.6.2 -I../libev-4.22 
-D__STDC_FORMAT_MACROS -I../picojson-1.3.0 -I../protobuf-3.5.0/src 
-I../rapidjson-1.1.0/include -I../../../3rdparty/libprocess/../stout/include 
-DLIBPROCESS_ALLOW_JEMALLOC -I/usr/include/subversion-1 -I/usr/include/apr-1 
-I/usr/include/apr-1.0 -Wall -Wsign-compare -Wformat-security -fstack-protector 
-fPIC -g1 -O0 -Wno-unused-local-typedefs -std=c++11 -c 
../../../3rdparty/libprocess/src/clock.cpp  -fPIC -DPIC -o 
src/.libs/libprocess_la-clock.o
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.9.0\" "-DPACKAGE_STRING=\"mesos 1.9.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.9.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 -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/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.9.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
-I../boost-1.65.0 -I
 ../concurrentqueue-7b69a8f -I../elfio-3.2 -I../glog-0.4.0/src 
-I../grpc-1.10.0/include -I../http-parser-2.6.2 -I../libev-4.22 
-D__STDC_FORMAT_MACROS -I../picojson-1.3.0 -I../protobuf-3.5.0/src 
-I../rapidjson-1.1.0/include -I../../../3rdparty/libprocess/../stout/include 
-DLIBPROCESS_ALLOW_JEMALLOC -I/usr/include/subversion-1 -I/usr/include/apr-1 
-I/usr/include/apr-1.0 -Wall -Wsign-compare -Wformat-security -fstack-protector 
-fPIC -g1 -O0 -Wno-unused-local-typedefs -std=c++11 -c 
../../../3rdparty/libprocess/src/authenticator_manager.cpp  -fPIC -DPIC -o 
src/.libs/libprocess_la-authenticator_manager.o
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.9.0\" "-DPACKAGE_STRING=\"mesos 1.9.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.9.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 -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/libprocess 
-DBUILD_DIR=\"/mesos/mesos-1.9.0/_build/3rdparty/libprocess\" 
-I../../../3rdparty/libprocess/include -I../../../3rdparty/libprocess/src 
-I../boost-1.65.0 -I
 ../concurrentqueue-7b69a8f -I../elfio-3.2 -I../glog-0.4.0/src 
-I../grpc-1.10.0/include -I../http-parser-2.6.2 

Re: Review Request 71230: Moved the sorter code under the `master/allocator/mesos/`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71230]

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

- Mesos Reviewbot


On Aug. 2, 2019, 12:30 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71230/
> ---
> 
> (Updated Aug. 2, 2019, 12:30 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The sorter interface is designed and used by the built-in
> hierarchical allocator. It is not intended to be used
> by other allocators. This patch moves it under the built-in
> mesos allocator folder for structural clarity.
> 
> 
> Diffs
> -
> 
>   docs/allocation-module.md 81f9d0c294eeb23f475c787da9f53f9c219bf6c3 
>   src/CMakeLists.txt e0200eb5e685cdac543defc8cda40b7452340c80 
>   src/Makefile.am d27d4b697cef034b44f89c198158a794ee52de4d 
>   src/local/local.cpp 68dc9fb0331e070ee10aee62e78381d76a52b7b0 
>   src/master/allocator/mesos/hierarchical.hpp 
> d2658bc8997202b17bac0f765bd4f522ebee350e 
>   src/master/allocator/sorter/drf/metrics.hpp  
>   src/master/allocator/sorter/drf/metrics.cpp 
> ece151c79b1ba7728b7ce338e74d2c682a1ef4f2 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 84c33ca49c5288d221f82eeb968d640461867da7 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 9367469132e426f0b4b66a80ad300c157fba6bf2 
>   src/master/allocator/sorter/random/sorter.hpp 
> ca687d4590387d5d788919f81fe32b43baba6b3c 
>   src/master/allocator/sorter/random/sorter.cpp 
> 9899cfd570607a60dbd7980d340a8e7d9d3e6df5 
>   src/master/allocator/sorter/random/utils.hpp  
>   src/master/allocator/sorter/sorter.hpp  
>   src/tests/sorter_tests.cpp 5cd2a64cb8c06f0d27c04bc0f40d347e6e61095e 
> 
> 
> Diff: https://reviews.apache.org/r/71230/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71194: Add `disk/xfs` isolator support for ephemeral volumes.

[James Peach]

> On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 335 (patched)
> > 
> >
> > According to this comment 
> > https://github.com/apache/mesos/blob/d8155f8125e38d145d280331146b934c2bb7c842/src/slave/containerizer/mesos/isolators/xfs/disk.cpp#L294-L301,
> >  this condition can only be met for containers that were launched before 
> > enabling XFS-isolator? If so, should we use `WARNING` here?

Yep, I agree that `WARNING` seems appropriate here.


> On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 425-426 (patched)
> > 
> >
> > Why doesn't `ephemeral_volumes` belonging to the `states` cover all 
> > directories? What is the case when we need this?

I'm not fully convinced that overlayfs directories are guaranteed to be in 
`ContainerState` at recovery time. For example, 
[here](https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L1073)
 we might find that we have an unrecoverable container what won't get a 
container state, and 
[here](https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L1060),
 we might find  the `ContainerState` erroneously missing the ephemeral volumes 
because the config could not be read.

So I felt that we should follow the same strategy as for sandboxes and treat 
the on-disk state as the source of truth. The consequences of leaking a project 
ID here are most likely that we would assign a project ID that is in use to a 
new task and that task could be immediately out of quota. This window should be 
small since the overlay backend is cleaned up immediately, but it's best to 
avoid the possibility.


> On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/provisioner/backends/overlay.cpp
> > Lines 17 (patched)
> > 
> >
> > Move it to the line after `#include "linux/fs.hpp"` (closer to the 
> > `#include "slave/containerizer/mesos/provisioner/constants.hpp"`)?

The counterpart header for the source file should come first since that helps 
to ensure that the header is self-contained. It's not really spelled out in the 
style guide, but the example does it:

https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#order-of-includes


- James


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


On Aug. 2, 2019, 2:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71194/
> ---
> 
> (Updated Aug. 2, 2019, 2:27 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for labeling ephemeral volumes with the sandbox XFS
> project ID. This makes changes to the container rootfs share the
> same disk quota as the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 646330c65b24aa28801ec99d7909db08a3e05c79 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 362e02172d2fd8e6e241fb6f5689f569ba74a0d1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> f2040cf36c601a13281a78ff844ebd41000a2d65 
> 
> 
> Diff: https://reviews.apache.org/r/71194/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71194: Add `disk/xfs` isolator support for ephemeral volumes.

[James Peach]

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

(Updated Aug. 2, 2019, 2:27 a.m.)


Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.


Changes
---

Addressed review feedback.


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


Repository: mesos


Description
---

Add support for labeling ephemeral volumes with the sandbox XFS
project ID. This makes changes to the container rootfs share the
same disk quota as the sandbox.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
646330c65b24aa28801ec99d7909db08a3e05c79 
  src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
362e02172d2fd8e6e241fb6f5689f569ba74a0d1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
f2040cf36c601a13281a78ff844ebd41000a2d65 


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

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


Testing
---

sudo make check (Frdora 30)


Thanks,

James Peach

Re: Review Request 71197: Updated the `disk/du` disk isolator tests with rootfs cases.

[James Peach]

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

(Updated Aug. 2, 2019, 1:40 a.m.)


Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.


Changes
---

Fixed the macOS build.


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


Repository: mesos


Description
---

Added the disk enforcement type parameter to the `disk/du` disk
isolator tests to verify rootfs ephemeral storage quota handling.


Diffs (updated)
-

  src/tests/disk_quota_tests.cpp cbb1ccff19ed9032298b40164e263ab3a0b0744d 


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

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


Testing
---

sudo make check (Frdora 30)


Thanks,

James Peach

Re: Review Request 71196: Updated the `disk/du` isolator to support rootfs checks.

[James Peach]

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

(Updated Aug. 2, 2019, 1:40 a.m.)


Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.


Changes
---

Addressed review feedback.


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


Repository: mesos


Description
---

Updated the `disk/du` isolator to support isolating rootfs ephemeral
volumes. We need to keep track of the ephemeral paths and start a `du`
check for each one of them, but otherwise treat them in the same way
as the sandbox path.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/posix/disk.hpp 
f513dfc55a6e680ca451586015f98f91e0b6abc5 
  src/slave/containerizer/mesos/isolators/posix/disk.cpp 
4869c72f77ea15978496f66997c63b07276d85f0 


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

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


Testing
---

sudo make check (Frdora 30)


Thanks,

James Peach

Re: Review Request 71196: Updated the `disk/du` isolator to support rootfs checks.

[James Peach]

> On Aug. 1, 2019, 4:06 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/isolators/posix/disk.cpp
> > Line 268 (original), 298 (patched)
> > 
> >
> > `info->directories.contains(path)`?

After applying your suggestion above, we can drop this change altogether. Now, 
the local `quota` hashmap is always consistent with the corresponding ephemeral 
directories, so we know that if there is no `quotas` entry then we should tear 
down the `info->paths` state.


> On Aug. 1, 2019, 4:06 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/isolators/posix/disk.cpp
> > Lines 411 (patched)
> > 
> >
> > can we move the computation of `currentUsage` inside the condition:
> > ```
> > if (flags.enforce_container_disk_quota && !isDiskSourceMount) {
> >   Bytes currentUsage = future.get();
> > 
> >   if (info->directories.contains(path)) {
> >   ...
> > }
> > ```
> > so that this isolator never evaluates `currentUsage` for `MOUNT`. Also, 
> > this code becomes a bit simpler.

I moved the ephemeral usage estimation to the `Info`, which makes this change 
cleaner.


- James


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


On July 30, 2019, 7:51 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71196/
> ---
> 
> (Updated July 30, 2019, 7:51 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the `disk/du` isolator to support isolating rootfs ephemeral
> volumes. We need to keep track of the ephemeral paths and start a `du`
> check for each one of them, but otherwise treat them in the same way
> as the sandbox path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> f513dfc55a6e680ca451586015f98f91e0b6abc5 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> 4869c72f77ea15978496f66997c63b07276d85f0 
> 
> 
> Diff: https://reviews.apache.org/r/71196/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 71230: Moved the sorter code under the `master/allocator/mesos/`.

[Meng Zhu]

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

Review request for mesos, Andrei Sekretenko and Benjamin Mahler.


Repository: mesos


Description
---

The sorter interface is designed and used by the built-in
hierarchical allocator. It is not intended to be used
by other allocators. This patch moves it under the built-in
mesos allocator folder for structural clarity.


Diffs
-

  docs/allocation-module.md 81f9d0c294eeb23f475c787da9f53f9c219bf6c3 
  src/CMakeLists.txt e0200eb5e685cdac543defc8cda40b7452340c80 
  src/Makefile.am d27d4b697cef034b44f89c198158a794ee52de4d 
  src/local/local.cpp 68dc9fb0331e070ee10aee62e78381d76a52b7b0 
  src/master/allocator/mesos/hierarchical.hpp 
d2658bc8997202b17bac0f765bd4f522ebee350e 
  src/master/allocator/sorter/drf/metrics.hpp  
  src/master/allocator/sorter/drf/metrics.cpp 
ece151c79b1ba7728b7ce338e74d2c682a1ef4f2 
  src/master/allocator/sorter/drf/sorter.hpp 
84c33ca49c5288d221f82eeb968d640461867da7 
  src/master/allocator/sorter/drf/sorter.cpp 
9367469132e426f0b4b66a80ad300c157fba6bf2 
  src/master/allocator/sorter/random/sorter.hpp 
ca687d4590387d5d788919f81fe32b43baba6b3c 
  src/master/allocator/sorter/random/sorter.cpp 
9899cfd570607a60dbd7980d340a8e7d9d3e6df5 
  src/master/allocator/sorter/random/utils.hpp  
  src/master/allocator/sorter/sorter.hpp  
  src/tests/sorter_tests.cpp 5cd2a64cb8c06f0d27c04bc0f40d347e6e61095e 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 71229: Fixed the webui roles table resource sorting.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71229]

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

- Mesos Reviewbot


On Aug. 1, 2019, 12:14 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71229/
> ---
> 
> (Updated Aug. 1, 2019, 12:14 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> All of the roles table resource columns were using the wrong
> keys for sorting.
> 
> 
> Diffs
> -
> 
>   src/webui/app/roles/roles.html b290587463a6ebc17bf645530247e46d4e3cff2a 
> 
> 
> Diff: https://reviews.apache.org/r/71229/diff/1/
> 
> 
> Testing
> ---
> 
> manual testing
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 71229: Fixed the webui roles table resource sorting.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Aug. 1, 2019, 7:14 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71229/
> ---
> 
> (Updated Aug. 1, 2019, 7:14 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> All of the roles table resource columns were using the wrong
> keys for sorting.
> 
> 
> Diffs
> -
> 
>   src/webui/app/roles/roles.html b290587463a6ebc17bf645530247e46d4e3cff2a 
> 
> 
> Diff: https://reviews.apache.org/r/71229/diff/1/
> 
> 
> Testing
> ---
> 
> manual testing
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 71229: Fixed the webui roles table resource sorting.

[Benjamin Mahler]

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

Review request for mesos, Andrei Sekretenko and Meng Zhu.


Repository: mesos


Description
---

All of the roles table resource columns were using the wrong
keys for sorting.


Diffs
-

  src/webui/app/roles/roles.html b290587463a6ebc17bf645530247e46d4e3cff2a 


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


Testing
---

manual testing


Thanks,

Benjamin Mahler

Re: Review Request 71196: Updated the `disk/du` isolator to support rootfs checks.

[Andrei Budnik]

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




src/slave/containerizer/mesos/isolators/posix/disk.cpp
Lines 277-294 (patched)


Can we delete this code in favour of

```
if (quotas.contains(info->sandbox)) {
  foreach (const string& path, info->directories) {
quotas[path] = quotas[info->sandbox];
  }
}
```
which should go after the very first loop (line #266).



src/slave/containerizer/mesos/isolators/posix/disk.cpp
Line 268 (original), 298 (patched)


`info->directories.contains(path)`?



src/slave/containerizer/mesos/isolators/posix/disk.cpp
Lines 411 (patched)


can we move the computation of `currentUsage` inside the condition:
```
if (flags.enforce_container_disk_quota && !isDiskSourceMount) {
  Bytes currentUsage = future.get();

  if (info->directories.contains(path)) {
  ...
}
```
so that this isolator never evaluates `currentUsage` for `MOUNT`. Also, 
this code becomes a bit simpler.



src/slave/containerizer/mesos/isolators/posix/disk.cpp
Lines 449 (patched)


Consider adding a comment describing why we skip here.


- Andrei Budnik


On Июль 30, 2019, 7:51 д.п., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71196/
> ---
> 
> (Updated Июль 30, 2019, 7:51 д.п.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the `disk/du` isolator to support isolating rootfs ephemeral
> volumes. We need to keep track of the ephemeral paths and start a `du`
> check for each one of them, but otherwise treat them in the same way
> as the sandbox path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> f513dfc55a6e680ca451586015f98f91e0b6abc5 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> 4869c72f77ea15978496f66997c63b07276d85f0 
> 
> 
> Diff: https://reviews.apache.org/r/71196/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71222: Added a test `VolumeSecretIsolatorCleanupTest.ROOT_FailInPreparing`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71201, 71221, 71222]

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

- Mesos Reviewbot


On Aug. 1, 2019, 7:18 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71222/
> ---
> 
> (Updated Aug. 1, 2019, 7:18 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-9893
> https://issues.apache.org/jira/browse/MESOS-9893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `VolumeSecretIsolatorCleanupTest.ROOT_FailInPreparing`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/volume_secret_isolator_tests.cpp 
> b6c43c3c3fcc61cd1bd03725cde78e58bcff801e 
> 
> 
> Diff: https://reviews.apache.org/r/71222/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 71194: Add `disk/xfs` isolator support for ephemeral volumes.

[Andrei Budnik]

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




src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 335 (patched)


According to this comment 
https://github.com/apache/mesos/blob/d8155f8125e38d145d280331146b934c2bb7c842/src/slave/containerizer/mesos/isolators/xfs/disk.cpp#L294-L301,
 this condition can only be met for containers that were launched before 
enabling XFS-isolator? If so, should we use `WARNING` here?



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 425-426 (patched)


Why doesn't `ephemeral_volumes` belonging to the `states` cover all 
directories? What is the case when we need this?



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp
Lines 17 (patched)


Move it to the line after `#include "linux/fs.hpp"` (closer to the 
`#include "slave/containerizer/mesos/provisioner/constants.hpp"`)?


- Andrei Budnik


On Июль 30, 2019, 7:52 д.п., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71194/
> ---
> 
> (Updated Июль 30, 2019, 7:52 д.п.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for labeling ephemeral volumes with the sandbox XFS
> project ID. This makes changes to the container rootfs share the
> same disk quota as the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 646330c65b24aa28801ec99d7909db08a3e05c79 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 362e02172d2fd8e6e241fb6f5689f569ba74a0d1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> f2040cf36c601a13281a78ff844ebd41000a2d65 
> 
> 
> Diff: https://reviews.apache.org/r/71194/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

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

- Mesos Reviewbot


On July 29, 2019, 8:56 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated July 29, 2019, 8:56 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70831: Fixed synchronization in MasterTest.UpdateSlaveMessageWithPendingOffers.

[Benjamin Bannier]

> On Aug. 1, 2019, 10:08 a.m., Chun-Hung Hsiao wrote:
> > src/tests/master_tests.cpp
> > Lines 9014 (patched)
> > 
> >
> > Alternatively, we can avoid this expectation and do the following:
> > ```
> > Future providerId = resourceProvider->process->id();
> > AWAIT_READY(providerId);
> > ```
> > Then s/`resourceProvider->process->info.id()`/`providerId.get()`/
> > 
> > I'm fine with either approach so please feel free to drop this.

Due to the way I reworked the way the `TestResourceProviderProcess` is used and 
it not exposing its `info` field anymore, this is actually what I now already 
do, and this patch does not apply anymore.

Dropping this issue and discarding the RR.


- Benjamin


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


On June 11, 2019, 2:38 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70831/
> ---
> 
> (Updated June 11, 2019, 2:38 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order for a resource provider to have been assigned and stored an
> assigned resource provider ID it needs to receive and process the
> corresponding resource provider `SUBSCRIBED` event.
> 
> This patch fixes `MasterTest.UpdateSlaveMessageWithPendingOffers` so we
> await the correct event. The test was previously awaiting an
> `UpdateSlaveMessage` which is only tangentially related, but does not
> guarantee correct event ordering.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e 
> 
> 
> Diff: https://reviews.apache.org/r/70831/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71224: Removed lossy `long` to `double` conversion in a test.

[Andrei Sekretenko]

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


Ship it!




The comment has been totally misleaing before - fixing it was a right thing.

- Andrei Sekretenko


On Aug. 1, 2019, 9:03 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71224/
> ---
> 
> (Updated Aug. 1, 2019, 9:03 a.m.)
> 
> 
> Review request for mesos and Andrei Sekretenko.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The code intends to construct a seconds value too large to fit into the
> internal representation in nanoseconds, but since the `long` value was
> passed to a function expecting a `double` the value will implicitly be
> converted. As the `long` value is large, `double` is not able to
> represent all possible `long` values which can lead to lossy
> conversions.
> 
> This is exactly what happened here. Recent clang has a warning to catch
> such issues at compile time if possible,
> 
> ```
> ../src/tests/master_tests.cpp:7194:34: warning: implicit conversion from
> 'long' to 'double' changes value from 9 to 1.0E+17
> [-Wimplicit-int-float-conversion]
>   framework.set_failover_timeout(9);
>    ^
> ```
> 
> This patch changes the value to `1e+17` (a `double` value) which removes
> the lossy conversion, and still accomplishes what the test required.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a 
> 
> 
> Diff: https://reviews.apache.org/r/71224/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71224: Removed lossy `long` to `double` conversion in a test.

[Benjamin Bannier]

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

(Updated Aug. 1, 2019, 11:03 a.m.)


Review request for mesos and Andrei Sekretenko.


Changes
---

Tweak existing comment as suggested by asekretenko offline


Repository: mesos


Description
---

The code intends to construct a seconds value too large to fit into the
internal representation in nanoseconds, but since the `long` value was
passed to a function expecting a `double` the value will implicitly be
converted. As the `long` value is large, `double` is not able to
represent all possible `long` values which can lead to lossy
conversions.

This is exactly what happened here. Recent clang has a warning to catch
such issues at compile time if possible,

```
../src/tests/master_tests.cpp:7194:34: warning: implicit conversion from
'long' to 'double' changes value from 9 to 1.0E+17
[-Wimplicit-int-float-conversion]
  framework.set_failover_timeout(9);
   ^
```

This patch changes the value to `1e+17` (a `double` value) which removes
the lossy conversion, and still accomplishes what the test required.


Diffs (updated)
-

  src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71197: Updated the `disk/du` disk isolator tests with rootfs cases.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71192, 71193, 71194, 71195, 71196, 71197]

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

- Mesos Reviewbot


On July 30, 2019, 7:51 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71197/
> ---
> 
> (Updated July 30, 2019, 7:51 a.m.)
> 
> 
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the disk enforcement type parameter to the `disk/du` disk
> isolator tests to verify rootfs ephemeral storage quota handling.
> 
> 
> Diffs
> -
> 
>   src/tests/disk_quota_tests.cpp cbb1ccff19ed9032298b40164e263ab3a0b0744d 
> 
> 
> Diff: https://reviews.apache.org/r/71197/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Frdora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 71224: Removed lossy `long` to `double` conversion in a test.

[Benjamin Bannier]

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

Review request for mesos and Andrei Sekretenko.


Repository: mesos


Description
---

The code intends to construct a seconds value to large to fit into the
internal representation in nanoseconds, but since the `long` value was
passed to a function expecting a `double` the value will implicitly be
converted. As the `long` value is large, `double` is not able to
represent all possible `long` values which can lead to lossy
conversions.

This is exactly what happened here. Recent clang has a warning to catch
such issues at compile time if possible,

```
../src/tests/master_tests.cpp:7194:34: warning: implicit conversion from
'long' to 'double' changes value from 9 to 1.0E+17
[-Wimplicit-int-float-conversion]
  framework.set_failover_timeout(9);
   ^
```

This patch changes the value to `1e+17` (a `double` value) which removes
the lossy conversion, and still accomplishes what the test required.


Diffs
-

  src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 70831: Fixed synchronization in MasterTest.UpdateSlaveMessageWithPendingOffers.

[Chun-Hung Hsiao]

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




src/tests/master_tests.cpp
Lines 9014 (patched)


Alternatively, we can avoid this expectation and do the following:
```
Future providerId = resourceProvider->process->id();
AWAIT_READY(providerId);
```
Then s/`resourceProvider->process->info.id()`/`providerId.get()`/

I'm fine with either approach so please feel free to drop this.


- Chun-Hung Hsiao


On June 11, 2019, 12:38 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70831/
> ---
> 
> (Updated June 11, 2019, 12:38 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order for a resource provider to have been assigned and stored an
> assigned resource provider ID it needs to receive and process the
> corresponding resource provider `SUBSCRIBED` event.
> 
> This patch fixes `MasterTest.UpdateSlaveMessageWithPendingOffers` so we
> await the correct event. The test was previously awaiting an
> `UpdateSlaveMessage` which is only tangentially related, but does not
> guarantee correct event ordering.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e 
> 
> 
> Diff: https://reviews.apache.org/r/70831/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70831: Fixed synchronization in MasterTest.UpdateSlaveMessageWithPendingOffers.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On June 11, 2019, 12:38 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70831/
> ---
> 
> (Updated June 11, 2019, 12:38 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order for a resource provider to have been assigned and stored an
> assigned resource provider ID it needs to receive and process the
> corresponding resource provider `SUBSCRIBED` event.
> 
> This patch fixes `MasterTest.UpdateSlaveMessageWithPendingOffers` so we
> await the correct event. The test was previously awaiting an
> `UpdateSlaveMessage` which is only tangentially related, but does not
> guarantee correct event ordering.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e 
> 
> 
> Diff: https://reviews.apache.org/r/70831/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71182: Stored last time a drain request was sent in the master.

[Benjamin Bannier]

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

(Updated Aug. 1, 2019, 9:52 a.m.)


Review request for mesos and Greg Mann.


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


Repository: mesos


Description (updated)
---

This information can be used to calculate an approximate time when
draining an agent would be finished, e.g., by comparing
`DrainConfig.max_grace_period` and the field
`estimated_drain_start_time` added here, both obtained from the master
API simultaneously. This information is necessarily approximate as the
agent might e.g., fail over before it has finished draining which would
reset the timeout; for that specific case the master would send the
drain request again so after some time the value calculated from the
master API would be in line with the expected true value.

WIP: drain start time in master


Diffs (updated)
-

  include/mesos/master/master.proto 07bf4e7194399ef660d7282275f25e1d4563e473 
  include/mesos/v1/master/master.proto 9bcef2f358b50fd63cdfdec5dcad742930de3ecc 
  src/common/protobuf_utils.cpp 7778e7f2475e9d6125d1c599715c91715f3654d3 
  src/master/http.cpp 765d5052ab6a8d731113f8922d20fb280b843003 
  src/master/master.hpp 8bdbac90b9000e9126a7fad375642dcf0a03378e 
  src/master/readonly_handler.cpp a8931153dc1b3f204c1b6437b972fe6607f900bd 
  src/tests/api_tests.cpp 641eb15153ddb85df322aa6a133ca8e4c6d6a510 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71183: Stored last time a drain request was sent in the agent.

[Benjamin Bannier]

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

(Updated Aug. 1, 2019, 9:52 a.m.)


Review request for mesos and Greg Mann.


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


Repository: mesos


Description (updated)
---

This information can be used to calculate an approximate time when
draining an agent would be finished, e.g., by comparing
`DrainConfig.max_grace_period` and the field
`estimated_drain_start_time` added here, both obtained from the agent
API simultaneously. This information is necessarily approximate as the
agent might e.g., fail over before it has finished draining which would
reset the timeout; for that specific case the master would send the
drain request again so after some time the value calculated from the
agent API would be in line with the expected true value.


Diffs (updated)
-

  include/mesos/agent/agent.proto 3cb622d5a334c8cc49b1df6bd85d7cbba7dad1b6 
  include/mesos/v1/agent/agent.proto 4324ad64bfd20c2cdba7f053b8b0c0a400363e95 
  src/slave/http.cpp d9f113d82424c468131a549efc32c7b2991e9fc7 
  src/slave/slave.hpp 556d8ea119ec0aec59b341e537cc1b75af72ea79 
  src/slave/slave.cpp 9c14784e140df86fe654d3f83be7ba541d30222f 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71148: Explicitly disabled periodic reconciliation for some provider tests.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/tests/storage_local_resource_provider_tests.cpp
Line 330 (original), 330 (patched)


Fits in one line.


- Chun-Hung Hsiao


On July 29, 2019, 8:56 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71148/
> ---
> 
> (Updated July 29, 2019, 8:56 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explicitly disabled periodic reconciliation for some provider tests.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71148/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71147: Update config factory to set resource provider reconciliation interval.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On July 29, 2019, 8:56 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71147/
> ---
> 
> (Updated July 29, 2019, 8:56 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update config factory to set resource provider reconciliation interval.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71147/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 71222: Added a test `VolumeSecretIsolatorCleanupTest.ROOT_FailInPreparing`.

[Qian Zhang]

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

Review request for mesos, Andrei Budnik and Gilbert Song.


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


Repository: mesos


Description
---

Added a test `VolumeSecretIsolatorCleanupTest.ROOT_FailInPreparing`.


Diffs
-

  src/tests/containerizer/volume_secret_isolator_tests.cpp 
b6c43c3c3fcc61cd1bd03725cde78e58bcff801e 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 71221: Moved const string `.secret` to paths.hpp.

[Qian Zhang]

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

Review request for mesos, Andrei Budnik and Gilbert Song.


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


Repository: mesos


Description
---

Moved const string `.secret` to paths.hpp.


Diffs
-

  src/slave/containerizer/mesos/isolators/volume/secret.cpp 
4bbcc7af0216a483d71b367c154c24500545a40b 
  src/slave/containerizer/mesos/paths.hpp 
c0033354bb07c68e4dda3cd49ca2d23164343e79 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 71214: Marked `SET_QUOTA` and `REMOVE_QUOTA` as deprecated.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71212, 71213, 71214]

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

- Mesos Reviewbot


On July 31, 2019, 1:04 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71214/
> ---
> 
> (Updated July 31, 2019, 1:04 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9600
> https://issues.apache.org/jira/browse/MESOS-9600
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These are deprecated in favor of `UPDATE_QUOTA`.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 07bf4e7194399ef660d7282275f25e1d4563e473 
>   include/mesos/v1/master/master.proto 
> 9bcef2f358b50fd63cdfdec5dcad742930de3ecc 
> 
> 
> Diff: https://reviews.apache.org/r/71214/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>