Re: Review Request 66115: Added `mesos-tidy.py`, a modified version of `run-clang-tidy.py`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66114, 66115]

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

- Mesos Reviewbot


On March 16, 2018, 3:57 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66115/
> ---
> 
> (Updated March 16, 2018, 3:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `mesos-tidy.py`, a modified version of `run-clang-tidy.py`.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66115/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Benjamin Mahler]

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




src/master/allocator/mesos/hierarchical.cpp
Lines 1577-1590 (patched)


This function seems rather specific to the usage in this code, which makes 
it not very easy to intuit its behavior. How about:

```
// Returns the result of shrinking the provided resources down to the
// target scalar quantities. If a resource does not have a target
// quantity provided, it will not be shrunk.
//
// Note that some resources are indivisible (e.g. MOUNT volume) and
// may be excluded in entirety in order to acheive the target size.
//
// Note also that there may be more than 1 result that satisfies
// the target sizes (e.g. need to exclude 1 of 2 disks); this function
// will make a random choice in these cases.
auto shrinkResources = [](
const Resources& resources,
hashmap targetQuantities) {
  ...
};
```

Then the caller can omit resources if it wants to acheive a default target 
size of 0 (i.e. `filterNonExistScalar`):

```
Resources allocation = shrinkResources(allocation, targetQuantities);

allocation = allocation.filter([](const Resource& r) {
  return targetQuantites.contains(r.name());
});
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1595-1603 (patched)


This comment seems to belong in the interface now that it's a function, 
otherwise the user reading the documentation for shrinkResources does not know 
that the result can vary given the input.


- Benjamin Mahler


On March 13, 2018, 10:04 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated March 13, 2018, 10:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 0e8c2c4a52969448f99bd5f42252a84cc52b9271 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66115: Added `mesos-tidy.py`, a modified version of `run-clang-tidy.py`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66115 was successfully built and tested.

Reviews applied: `['66114', '66115']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66115

- Mesos Reviewbot Windows


On March 16, 2018, 10:57 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66115/
> ---
> 
> (Updated March 16, 2018, 10:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `mesos-tidy.py`, a modified version of `run-clang-tidy.py`.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66115/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 66123: Temporarily disabled some default executor tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65692, 65693, 65694, 65695, 65962, 66123]

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

- Mesos Reviewbot


On March 16, 2018, 12:54 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66123/
> ---
> 
> (Updated March 16, 2018, 12:54 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-8530 and MESOS-8666
> https://issues.apache.org/jira/browse/MESOS-8530
> https://issues.apache.org/jira/browse/MESOS-8666
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch temporarily disables some default executor tests that fail
> because Windows agents respond to `KILL_NESTED_CONTAINER` with a
> failure, even though they successfully execute the requested kill.
> 
> The tests should be re-enabled once MESOS-8666 is fixed.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 316ec7ed033b4c57fd06486574c31bba67771d8a 
> 
> 
> Diff: https://reviews.apache.org/r/66123/diff/2/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux =).
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66104: Fixed potential memory leak in the `volume/sandbox_path` isolator.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On March 15, 2018, 10:31 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> ---
> 
> (Updated March 15, 2018, 10:31 p.m.)
> 
> 
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
> 
> 
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
> 
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 
> 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 5801977e93bcb8f463a2635f73e763098f2aa97d 
> 
> 
> Diff: https://reviews.apache.org/r/66104/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 66034: Remount several proc filesystem entries as read-only.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On March 15, 2018, 6:24 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66034/
> ---
> 
> (Updated March 15, 2018, 6:24 p.m.)
> 
> 
> Review request for mesos, Eric Chung, Gilbert Song, Ian Downes, Jie Yu, James 
> Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8654
> https://issues.apache.org/jira/browse/MESOS-8654
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Several entries under the proc FS within Mesos containers need to be
> remounted as readonly for improved security reasons.
> 
> The list should include the important ones introduced by Systemd's
> `ProtectKernelTunables` option:
> 
> * `/proc/bus`
> * `/proc/fs`
> * `/proc/irq`
> * `/proc/sys`
> * `/proc/sysrq-trigger`
> 
> It is particularly necessary to remount `/proc/sysrq-trigger` as
> read-only. Otherwise, it would be possible for processes running in
> containers as `root` to perform privileged operations, such as host
> reboot.
> 
> Extra mount options should include `nosuid,noexec,nodev` (see also
> `mount(2)` for detailed explanations of the options).
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp ed26f80ef7315809a1df9f2c50b4fe3445810f8a 
> 
> 
> Diff: https://reviews.apache.org/r/66034/diff/1/
> 
> 
> Testing
> ---
> 
> The mount table of the container launched by the patched version of 
> `mesos-containerizer launch` include the entries listed below, with 
> `nosuid,noexec,nodev` mount options:
> ```
> $ sudo unshare -m -p -f /usr/local/libexec/mesos/mesos-containerizer launch 
> --launch_info="$(jq -c . launch_info.json)" --runtime_directory="$(pwd)"
> Marked '/' as rslave
> Prepared mount 
> '{"flags":20480,"source":"\/etc\/hostname","target":"\/home\/jlai\/containers\/rootfs\/etc\/hostname"}'
> Prepared mount 
> '{"flags":20480,"source":"\/etc\/hosts","target":"\/home\/jlai\/containers\/rootfs\/etc\/hosts"}'
> Prepared mount 
> '{"flags":20480,"source":"\/etc\/resolv.conf","target":"\/home\/jlai\/containers\/rootfs\/etc\/resolv.conf"}'
> Changing root to /home/jlai/containers/rootfs
> bash-4.4# findmnt -a
> TARGET  SOURCE  FSTYPE  OPTIONS
> /   alpine  overlay 
> rw,relatime,lowerdir=overlay/lower,upperdir=overlay/upper,workdir=overlay/work
> |-/etc/hostname /dev/dm-0[/etc/hostname]ext4
> rw,noatime,errors=panic,data=ordered
> |-/etc/hosts/dev/dm-0[/etc/hosts]   ext4
> rw,noatime,errors=panic,data=ordered
> |-/etc/resolv.conf  /dev/dm-0[/etc/resolv.conf] ext4
> rw,noatime,errors=panic,data=ordered
> |-/proc procproc
> rw,nosuid,nodev,noexec,relatime
> | |-/proc/bus   proc[/bus]  proc
> ro,nosuid,nodev,noexec,relatime
> | |-/proc/fsproc[/fs]   proc
> ro,nosuid,nodev,noexec,relatime
> | |-/proc/irq   proc[/irq]  proc
> ro,nosuid,nodev,noexec,relatime
> | |-/proc/sys   proc[/sys]  proc
> ro,nosuid,nodev,noexec,relatime
> | `-/proc/sysrq-trigger proc[/sysrq-trigger]proc
> ro,nosuid,nodev,noexec,relatime
> |-/sys  sysfs   sysfs   
> ro,nosuid,nodev,noexec,relatime
> `-/dev  tmpfs   tmpfs   
> rw,nosuid,noexec,mode=755
>   |-/dev/ptsdevpts  devpts  
> rw,nosuid,noexec,relatime,mode=600,ptmxmode=666
>   `-/dev/shmtmpfs   tmpfs   rw,nosuid,nodev
> ```
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65812: Added an overloaded version of `os::realpath` to stout

[Jie Yu]

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




3rdparty/stout/include/stout/os/posix/realpath.hpp
Lines 48 (patched)


Please add unit test for this.


- Jie Yu


On March 5, 2018, 7:29 a.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65812/
> ---
> 
> (Updated March 5, 2018, 7:29 a.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an overloaded version of `os::realpath` to stout.
> 
> The new `os::realpath` function is used for evaluating real path
> within a scoped root directory.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/realpath.hpp 
> 31352cefc5b8d0ccd9af8f6dabdec4a959fded32 
> 
> 
> Diff: https://reviews.apache.org/r/65812/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jie Yu]

> On March 16, 2018, 11:17 p.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 65 (patched)
> > 
> >
> > Can you add some tests for this? This warrents some tests. Also, please 
> > reach out to Andrew because this will also compile (maybe used) on windows.

if it's posix only for now, please move to posix only headers, or add an 
assertion failure if this method is used on windows.


- Jie


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


On March 2, 2018, 7:40 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated March 2, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jie Yu]

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




3rdparty/stout/include/stout/path.hpp
Lines 65 (patched)


Can you add some tests for this? This warrents some tests. Also, please 
reach out to Andrew because this will also compile (maybe used) on windows.


- Jie Yu


On March 2, 2018, 7:40 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated March 2, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 65811: Add `path::clean` to stout for normalizing path (for POSIX only now)

[Jie Yu]

> On March 2, 2018, 8:06 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 65 (patched)
> > 
> >
> > How about s/clean/normalize/?
> 
> Jason Lai wrote:
> Indeed I considered this option. And I also considered `normpath` as in 
> `os.path.normpath` in Python but ended up picking `path::clean` from the Go 
> counterpart (`filepath.Clean`) over `path::normalize`.
> That said, I'm open to `path::normalize` if we have more folks in favor 
> of it.

+1 on using `normalize`. `clean` sounds too general to me.


- Jie


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


On March 2, 2018, 7:40 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65811/
> ---
> 
> (Updated March 2, 2018, 7:40 p.m.)
> 
> 
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, 
> James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `path::clean` to stout for normalizing path (for POSIX only now).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/65811/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 66010: Windows: Switched to default CRT linkage.

[Andrew Schwartzmeyer]

> On March 16, 2018, 3:08 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 326-330 (original), 326-330 (patched)
> > 
> >
> > Looks like this variable can go away too.

Indeed! Thank you :) I can do that before I commit.


- Andrew


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


On March 13, 2018, 1:45 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66010/
> ---
> 
> (Updated March 13, 2018, 1:45 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8659
> https://issues.apache.org/jira/browse/MESOS-8659
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We previously attempted to manually override the CRT to be static
> everywhere. Not only did this emit warnings, it was also error-prone
> and unnecessary. We can, and should, just use the defaults, which is
> `/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
> to the CRT dynamically results in smaller libraries and executables,
> reduces linking time, and avoids bugs when sharing allocated memory
> across modules.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
> 
> 
> Diff: https://reviews.apache.org/r/66010/diff/2/
> 
> 
> Testing
> ---
> 
> NOTE: I checked with Alex Ionescu and since we are deploying on the same OS 
> versions as we're targetting, we don't need to worry about redistributing the 
> CRT.
> 
> If/when we start deploying on older OS versions, we can simply add 
> instructions on installing the correct CRT redistributable (or make it part 
> of the eventual installer).
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66123: Temporarily disabled some default executor tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66123 was successfully built and tested.

Reviews applied: `['65692', '65693', '65694', '65695', '65962', '66123']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66123

- Mesos Reviewbot Windows


On March 16, 2018, 7:54 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66123/
> ---
> 
> (Updated March 16, 2018, 7:54 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-8530 and MESOS-8666
> https://issues.apache.org/jira/browse/MESOS-8530
> https://issues.apache.org/jira/browse/MESOS-8666
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch temporarily disables some default executor tests that fail
> because Windows agents respond to `KILL_NESTED_CONTAINER` with a
> failure, even though they successfully execute the requested kill.
> 
> The tests should be re-enabled once MESOS-8666 is fixed.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 316ec7ed033b4c57fd06486574c31bba67771d8a 
> 
> 
> Diff: https://reviews.apache.org/r/66123/diff/2/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux =).
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66010: Windows: Switched to default CRT linkage.

[Joseph Wu]

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


Fix it, then Ship it!





cmake/CompilationConfigure.cmake
Lines 326-330 (original), 326-330 (patched)


Looks like this variable can go away too.


- Joseph Wu


On March 13, 2018, 1:45 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66010/
> ---
> 
> (Updated March 13, 2018, 1:45 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8659
> https://issues.apache.org/jira/browse/MESOS-8659
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We previously attempted to manually override the CRT to be static
> everywhere. Not only did this emit warnings, it was also error-prone
> and unnecessary. We can, and should, just use the defaults, which is
> `/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
> to the CRT dynamically results in smaller libraries and executables,
> reduces linking time, and avoids bugs when sharing allocated memory
> across modules.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
> 
> 
> Diff: https://reviews.apache.org/r/66010/diff/2/
> 
> 
> Testing
> ---
> 
> NOTE: I checked with Alex Ionescu and since we are deploying on the same OS 
> versions as we're targetting, we don't need to worry about redistributing the 
> CRT.
> 
> If/when we start deploying on older OS versions, we can simply add 
> instructions on installing the correct CRT redistributable (or make it part 
> of the eventual installer).
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Jie Yu]

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




include/mesos/mesos.proto
Lines 1967 (patched)


I'd suggest the following:
```
message GrowVolume {
  required Resource volume = 1;
  required Resource addition = 2;
}
```

Using `Resource` for `addition` make it clear that you need an offer 
containing the resources to be able to grow an volume. You cannot bindly grown 
a volume. The master will validate that `addition` is actually allocated to the 
framework.

The master will also need to validate that the `addition` Resource is 
compatible with `volume` in the operation.
1) If `volume` is a `ROOT` disk, `addition` has to be a `ROOT` disk too.
2) If `volume` is a `PATH` disk, the `addition` can either be a `PATH` disk 
with the same `Source`, or a `RAW` disk from the same resource provider and the 
same profile.
3) If `volume` is a `MOUNT` disk, the `addition` should be a `RAW` disk 
from the same resource provider and the same profile.

Given that, using a `Resource` for `addition` is more appropriate and 
future proof.


- Jie Yu


On March 16, 2018, 5:54 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 16, 2018, 5:54 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
>   include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66115: Added `mesos-tidy.py`, a modified version of `run-clang-tidy.py`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66114, 66115]

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

- Mesos Reviewbot


On March 16, 2018, 3:57 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66115/
> ---
> 
> (Updated March 16, 2018, 3:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66115/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 66009: CMake: Added `-Wno-unused-local-typedefs` to Boost interface.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 14, 2018, 4:11 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66009/
> ---
> 
> (Updated March 14, 2018, 4:11 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8658
> https://issues.apache.org/jira/browse/MESOS-8658
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As found in the Autotools builds, GCC and Clang will emit an used
> local typedef warning for headers including Boost. Since it is
> 3rdparty code, we disable this warning in its interface.
> 
> The versions were updated with the latest known scenario.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 
>   configure.ac 30fbadc32d1d96f719d45fa8067f975283c25507 
> 
> 
> Diff: https://reviews.apache.org/r/66009/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66076: Used raw string literal in protobuf tests to avoid escaping.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





3rdparty/stout/tests/protobuf_tests.cpp
Line 108 (original), 108 (patched)


Not sure what style is better. How about
```
string expected = R"~({
  ...
})~";
```
Or
```
string expceted =
  R"~({
...
  })~";
```
?


- Chun-Hung Hsiao


On March 15, 2018, 9:29 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66076/
> ---
> 
> (Updated March 15, 2018, 9:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used raw string literal in protobuf tests to avoid escaping.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.cpp 
> a0ef1d110204fe2868ac9b686da090d9e7b3d2a3 
> 
> 
> Diff: https://reviews.apache.org/r/66076/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 66123: Temporarily disabled some default executor tests.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On March 16, 2018, 12:54 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66123/
> ---
> 
> (Updated March 16, 2018, 12:54 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-8530 and MESOS-8666
> https://issues.apache.org/jira/browse/MESOS-8530
> https://issues.apache.org/jira/browse/MESOS-8666
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch temporarily disables some default executor tests that fail
> because Windows agents respond to `KILL_NESTED_CONTAINER` with a
> failure, even though they successfully execute the requested kill.
> 
> The tests should be re-enabled once MESOS-8666 is fixed.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 316ec7ed033b4c57fd06486574c31bba67771d8a 
> 
> 
> Diff: https://reviews.apache.org/r/66123/diff/2/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux =).
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66008: CMake: Enabled compiler warnings.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 14, 2018, 4:10 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66008/
> ---
> 
> (Updated March 14, 2018, 4:10 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8033 and MESOS-8658
> https://issues.apache.org/jira/browse/MESOS-8033
> https://issues.apache.org/jira/browse/MESOS-8658
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We had previously been using the default sets of warnings, but now we
> use the same warnings as on Autotools. This also means that we can
> safely turn on the treat-warnings-as-errors (at least for the Mesos
> code). However, doing so requires that we disable two common
> possible-loss-of-data warnings on Windows that are not part of the
> GNU/Clang default warnings.
> 
> Because `-Werror` semantics can be burdensome, it can be turned off at
> configuration time with `-DENABLE_WERROR=FALSE`. It is on by default.
> 
> This also replaces the use of `string(APPEND CMAKE_CXX_FLAGS)` with
> the canonical command `add_compile_options`. Although generally the
> use of `target_compile_options` is preferred, it would currently
> result in a lot more churn, and the build already supports setting
> these flags globally.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
>   docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9 
>   src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 
> 
> 
> Diff: https://reviews.apache.org/r/66008/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65994: Made the master forward operation status updates to the schedulers.

[Gaston Kleiman]

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

(Updated March 16, 2018, 2:07 p.m.)


Review request for mesos, Greg Mann and Zhitao Li.


Changes
---

Addressed Greg's feedback.


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


Repository: mesos


Description
---

Made the master forward operation status updates to the schedulers.


Diffs (updated)
-

  src/master/master.cpp aa35abc53bfc34e19d19a93328fb6552b64c05d7 


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

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


Testing
---

`sudo bin/mesos-tests.sh` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 66123: Temporarily disabled some default executor tests.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['65692', '65693', '65694', '65695', '65962', '66123']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66123

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66123/logs/mesos-tests-stdout.log):

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (114 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (1079 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (33 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (41 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (77 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (2477 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (2506 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (2451 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (2479 ms total)

[--] Global test environment tear-down
[==] 904 tests from 89 test cases ran. (439219 ms total)
[  PASSED  ] 903 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] MesosContainerizer/DefaultExecutorTest.CommitSuicideOnKillTask/0, 
where GetParam() = "mesos"

 1 FAILED TEST
  YOU HAVE 213 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66123/logs/mesos-tests-stderr.log):

```
I0316 20:58:57.324566  2148 master.cpp:10180] Updating the state of task 
f7279ce8-d7c2-48ca-8bb1-79df1ededcbe of framework 
3c6ca21e-46e3-44db-8001-0db708376236- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0316 20:58:57.324566  8240 slave.cpp:3879] Shutting down framework 
3c6ca21e-46e3-44db-8001-0db708376236-
I031I0316 20:58:56.567489 10044 exec.cpp:162] Version: 1.6.0
I0316 20:58:56.599504  5336 exec.cpp:236] Executor registered on agent 
3c6ca21e-46e3-44db-8001-0db708376236-S0
I0316 20:58:56.605597  6188 executor.cpp:176] Received SUBSCRIBED event
I0316 20:58:56.611496  6188 executor.cpp:180] Subscribed executor on 
win-bld-srv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0316 20:58:56.612504  6188 executor.cpp:176] Received LAUNCH event
I0316 20:58:56.616492  6188 executor.cpp:648] Starting task 
f7279ce8-d7c2-48ca-8bb1-79df1ededcbe
I0316 20:58:56.699538  6188 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0316 20:58:57.289636  6188 executor.cpp:661] Forked command at 13140
I0316 20:58:57.327533  1012 exec.cpp:445] Executor asked to shutdown
I0316 20:58:57.328531 13636 executor.cpp:176] Received SHUTDOWN event
I0316 20:58:57.329532 13636 executor.cpp:758] Shutting down
I0316 20:58:57.329532 13636 executor.cpp:868] Sending SIGTERM to process tree 
at pi6 20:58:57.324566  8240 slave.cpp:6572] Shutting down executor 
'f7279ce8-d7c2-48ca-8bb1-79df1ededcbe' of framework 
3c6ca21e-46e3-44db-8001-0db708376236- at executor(1)@10.3.1.8:52727
I0316 20:58:57.326531  8240 slave.cpp:925] Agent terminating
W0316 20:58:57.326531  8240 slave.cpp:3875] Ignoring shutdown framework 
3c6ca21e-46e3-44db-8001-0db708376236- because it is terminating
I0316 20:58:57.327533  2148 master.cpp:10279] Removing task 
f7279ce8-d7c2-48ca-8bb1-79df1ededcbe with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 3c6ca21e-46e3-44db-8001-0db708376236- on 
agent 3c6ca21e-46e3-44db-8001-0db708376236-S0 at slave(388)@10.3.1.8:51123 
(win-bld-srv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0316 20:58:57.329532  2148 master.cpp:1293] Agent 
3c6ca21e-46e3-44db-8001-0db708376236-S0 at slave(388)@10.3.1.8:51123 
(win-bld-srv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) 
disconnected
I0316 20:58:57.330545  2148 master.cpp:3263] Disconnecting agent 
3c6ca21e-46e3-44db-8001-0db708376236-S0 at slave(388)@10.3.1.8:51123 
(win-bld-srv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0316 20:58:57.330545  3716 hierarchical.cpp:344] Removed framework 
3c6ca21e-46e3-44db-8001-0db708376236-
I0316 20:58:57.330545  1296 containerizer.cpp

Re: Review Request 66057: Added a `createCallAcknowledgeOperationStatus()` test helper.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 16, 2018, 7:58 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66057/
> ---
> 
> (Updated March 16, 2018, 7:58 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Zhitao Li.
> 
> 
> Bugs: MESOS-8201
> https://issues.apache.org/jira/browse/MESOS-8201
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a `createCallAcknowledgeOperationStatus()` test helper.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 2c3d0c9350cd2223ff20c0797d1849d38c19 
> 
> 
> Diff: https://reviews.apache.org/r/66057/diff/2/
> 
> 
> Testing
> ---
> 
> Used by the tests added in https://reviews.apache.org/r/66060/.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66056: Updated `CREATE_VOLUME()` helper to allow specifying an operation ID.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 16, 2018, 7:57 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66056/
> ---
> 
> (Updated March 16, 2018, 7:57 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Zhitao Li.
> 
> 
> Bugs: MESOS-8201
> https://issues.apache.org/jira/browse/MESOS-8201
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `CREATE_VOLUME()` helper to allow specifying an operation ID.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 2c3d0c9350cd2223ff20c0797d1849d38c19 
> 
> 
> Diff: https://reviews.apache.org/r/66056/diff/1/
> 
> 
> Testing
> ---
> 
> Used by the tests added in https://reviews.apache.org/r/66060/.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65993: Added evolve functions for operation status updates.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 16, 2018, 7:56 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65993/
> ---
> 
> (Updated March 16, 2018, 7:56 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Zhitao Li.
> 
> 
> Bugs: MESOS-8189
> https://issues.apache.org/jira/browse/MESOS-8189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds evolve functions for `UpdateOperationStatusMessage` and
> for `OperationStatus`.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 77b7172b8ba51a236647334337be6749a03ae021 
>   src/internal/evolve.cpp 3ad5ebcc5ef489c59586875602f9c1c653aff805 
> 
> 
> Diff: https://reviews.apache.org/r/65993/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66007: CMake: Set C++11 as standard automatically.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 9, 2018, 2:37 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66007/
> ---
> 
> (Updated March 9, 2018, 2:37 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8033
> https://issues.apache.org/jira/browse/MESOS-8033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of setting the compiler option manually, we use the
> `CMAKE_CXX_STANDARD` variable to set the default for all targets. This
> automatically appends the correct flag for each compiler.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
> 
> 
> Diff: https://reviews.apache.org/r/66007/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 65994: Made the master forward operation status updates to the schedulers.

[Greg Mann]

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


Fix it, then Ship it!





src/master/master.cpp
Lines 8212-8214 (original), 8206-8208 (patched)


Nit: I wonder if we should move this above the `if 
(operation->info().has_id() && frameworkId.isSome())` clause, to improve 
readability slightly?

We have two mutually exclusive cases: either we forward the update to the 
framework, or we ACK it ourselves (or neither).

So, maybe it's a bit more readable if we do:
1) Update the operation internally
2) If has ID, forward
3) If doesn't have ID, ACK

WDYT?


- Greg Mann


On March 16, 2018, 7:57 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65994/
> ---
> 
> (Updated March 16, 2018, 7:57 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Zhitao Li.
> 
> 
> Bugs: MESOS-8189
> https://issues.apache.org/jira/browse/MESOS-8189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the master forward operation status updates to the schedulers.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 223ebf29ac4dd1dea9080e4bef4b2d4d064d847f 
> 
> 
> Diff: https://reviews.apache.org/r/65994/diff/3/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Review Request 66123: Temporarily disabled some default executor tests.

[Gaston Kleiman]

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

Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Bugs: MESOS-8530 and MESOS-8666
https://issues.apache.org/jira/browse/MESOS-8530
https://issues.apache.org/jira/browse/MESOS-8666


Repository: mesos


Description
---

This patch temporarily disables some default executor tests that fail
because Windows agents respond to `KILL_NESTED_CONTAINER` with a
failure, even though they successfully execute the requested kill.

The tests should be re-enabled once MESOS-8666 is fixed.


Diffs
-

  src/tests/default_executor_tests.cpp 316ec7ed033b4c57fd06486574c31bba67771d8a 


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


Testing
---

Tests still pass on GNU/Linux =).


Thanks,

Gaston Kleiman

Re: Review Request 65962: Avoided copying `Owned` pointers in the default executor.

[Gaston Kleiman]

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

(Updated March 16, 2018, 12:54 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Rebase.


Repository: mesos


Description
---

`Owned` pointers are copied in multiple places of the default executor.
This violates the semantic of owned pointers and works only because
`Owned` is currently implemented with `shared_ptr`, it would otherwise
lead to double-freeing the pointers.

This patch changes those places to use references to the original
`Owned` objects or raw pointers instead of copies.


Diffs (updated)
-

  src/launcher/default_executor.cpp 906836f3b8e0af79d7c61f90fd8a95f193b26e84 


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

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


Testing
---

`sudo bin/mesos-tests.sh --gtest_filter="*Default*"` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 65695: Made the default executor allow schedulers to retry task kills.

[Gaston Kleiman]

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

(Updated March 16, 2018, 12:53 p.m.)


Review request for mesos, Joseph Wu, Qian Zhang, and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

The default executor transitions a task to `TASK_KILLING` and marks its
child container as being killed before posting a `KILL` call to the
agent.

The executor ignores kill requests for containers that are marked as
being killed, and it doesn't remove this mark if the `KILL` call fails.
This means that it's possible for tasks to get stuck in a `TASK_KILLING`
state.

This patch makes the default executor remove the killing mark if a
`KILL` call fails. That way a scheduler can retry a kill.


Diffs (updated)
-

  src/launcher/default_executor.cpp 906836f3b8e0af79d7c61f90fd8a95f193b26e84 


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

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


Testing
---

`sudo bin/mesos-tests.sh` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 65694: Made the default executor's handling of kill escalations more robust.

[Gaston Kleiman]

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

(Updated March 16, 2018, 12:53 p.m.)


Review request for mesos, Joseph Wu, Qian Zhang, and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This patch makes the default executor retry SIGKILL escalations if the
executor is disconnected from the agent or the kill call fails.


Diffs (updated)
-

  src/launcher/default_executor.cpp 906836f3b8e0af79d7c61f90fd8a95f193b26e84 


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

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


Testing
---

`sudo bin/mesos-tests.sh` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 65693: Made the default executor fail kills if the response isn't "200 OK".

[Gaston Kleiman]

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

(Updated March 16, 2018, 12:53 p.m.)


Review request for mesos, Joseph Wu, Qian Zhang, and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

The default executor's `Future kill(const ContainerID&, int)`
method returns `Nothing()` if the agent responded to the
`KILL_NESTED_CONTAINER` call, regardless of the response.

This patch updates the method, so that it returns a failure if the
response is not "200 OK".


Diffs (updated)
-

  src/launcher/default_executor.cpp 906836f3b8e0af79d7c61f90fd8a95f193b26e84 


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

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


Testing
---

`sudo bin/mesos-tests.sh` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 66120: Updated an agent test to use mock garbage collector.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66120 was successfully built and tested.

Reviews applied: `['66118', '66119', '66120']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66120

- Mesos Reviewbot Windows


On March 16, 2018, 6:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66120/
> ---
> 
> (Updated March 16, 2018, 6:17 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test `RemoveExecutorUponFailedTaskGroupLaunch`
> currently depends on catching and modifying the input
> arguments of agent `__run()` function to mock the
> garbage collector behavior. Using mock garbage collector
> gives better readability and more functionality.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 2c3d0c9350cd2223ff20c0797d1849d38c19 
>   src/tests/mesos.cpp d82963195573dd9ed7d12a7708f64a236b28cdf1 
>   src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354 
> 
> 
> Diff: https://reviews.apache.org/r/66120/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65679: Removed direct unmock calls and added missing mock call expectations.

[Meng Zhu]

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

(Updated March 16, 2018, 12:07 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Patch updated. Thanks for the informative review.


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


Repository: mesos


Description
---

Directly invoking unmock calls in the test process can potentially
cause races with the real mock slave process. It is more robust to
dispatch the unmock calls to the real mock slave process.

Also added several mock expectations to avoid "uninteresting mock
call" test warnings.


Diffs (updated)
-

  src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354 


Diff: https://reviews.apache.org/r/65679/diff/5/

Changes: https://reviews.apache.org/r/65679/diff/4-5/


Testing
---

`./bin/mesos-tests.sh --gtest_filter=*SlaveTest* --gtest_repeat=-1 
--gtest_break_on_failure` runs forever :)


Thanks,

Meng Zhu

Re: Review Request 65679: Removed direct unmock calls and added missing mock call expectations.

[Meng Zhu]

> On March 14, 2018, 5:08 p.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Line 4982 (original), 5022 (patched)
> > 
> >
> > Is it guaranteed that this lambda will finish before 
> > `exitedExecutorMessage` is satisfied, so we can make sure that is function 
> > won't be called after the variables are destructed?
> > 
> > If not, we probably should do the following instead:
> > ```
> > Future unmocked___run = process::dispatch(slave.get()->pid, 
> > [&] {
> >   slave.get()->mock()->unmocked___run(
> > ...);
> > 
> >   return Nothing();
> > });
> > ```
> > And do `AWAIT_READY(unmocked___run)` at the end of the test.

Currently, it is guaranteed, as send `exitedExecutorMessage` is the last 
statement of the function. But I should add your suggestion to be future proof. 
But this piece of code is gone after I updated the test to use the mock 
authorizer. Dropping the issue.


> On March 14, 2018, 5:08 p.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 5179 (patched)
> > 
> >
> > Is it guaranteed that this lambda will finish before `executorShutdown` 
> > is satisfied, so we can make sure that is function won't be called after 
> > the variables are destructed?
> > 
> > If not, we need to do the same thing I suggested above.

Currently, it is guaranteed, but let's add that to be future proof.


- Meng


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


On March 16, 2018, 12:07 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> ---
> 
> (Updated March 16, 2018, 12:07 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8611
> https://issues.apache.org/jira/browse/MESOS-8611
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Directly invoking unmock calls in the test process can potentially
> cause races with the real mock slave process. It is more robust to
> dispatch the unmock calls to the real mock slave process.
> 
> Also added several mock expectations to avoid "uninteresting mock
> call" test warnings.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354 
> 
> 
> Diff: https://reviews.apache.org/r/65679/diff/5/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter=*SlaveTest* --gtest_repeat=-1 
> --gtest_break_on_failure` runs forever :)
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66104: Fixed potential memory leak in the `volume/sandbox_path` isolator.

[Jie Yu]

> On March 16, 2018, 6:47 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
> > Lines 404 (patched)
> > 
> >
> > Let's not have this CHECK here. It's possible that cleanup is called 
> > multiple times (caller assumes that this function is idempotent).

Correction, cleanup won't be called multiple times for a containre, but might 
be called for an unknown container.


- Jie


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


On March 15, 2018, 10:31 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> ---
> 
> (Updated March 15, 2018, 10:31 p.m.)
> 
> 
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
> 
> 
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
> 
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 
> 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 5801977e93bcb8f463a2635f73e763098f2aa97d 
> 
> 
> Diff: https://reviews.apache.org/r/66104/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 66104: Fixed potential memory leak in the `volume/sandbox_path` isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
Lines 404 (patched)


Let's not have this CHECK here. It's possible that cleanup is called 
multiple times (caller assumes that this function is idempotent).


- Jie Yu


On March 15, 2018, 10:31 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> ---
> 
> (Updated March 15, 2018, 10:31 p.m.)
> 
> 
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
> 
> 
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
> 
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 
> 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 5801977e93bcb8f463a2635f73e763098f2aa97d 
> 
> 
> Diff: https://reviews.apache.org/r/66104/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Review Request 66120: Updated an agent test to use mock garbage collector.

[Meng Zhu]

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

Review request for mesos and Greg Mann.


Repository: mesos


Description
---

The test `RemoveExecutorUponFailedTaskGroupLaunch`
currently depends on catching and modifying the input
arguments of agent `__run()` function to mock the
garbage collector behavior. Using mock garbage collector
gives better readability and more functionality.


Diffs
-

  src/tests/mesos.hpp 2c3d0c9350cd2223ff20c0797d1849d38c19 
  src/tests/mesos.cpp d82963195573dd9ed7d12a7708f64a236b28cdf1 
  src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354 


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


Testing
---

make check


Thanks,

Meng Zhu

Review Request 66118: Updated an agent test to use mock authorizer.

[Meng Zhu]

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

Review request for mesos and Greg Mann.


Repository: mesos


Description
---

The test `RemoveExecutorUponFailedTaskAuthorization`
currently depends on catching and modifying the input
arguments of agent `_run()` function to mock the
authorizer behavior. Using mock authorizer gives better
readability and more functionality.


Diffs
-

  src/tests/mesos.hpp 2c3d0c9350cd2223ff20c0797d1849d38c19 
  src/tests/mesos.cpp d82963195573dd9ed7d12a7708f64a236b28cdf1 
  src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354 


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


Testing
---

make check


Thanks,

Meng Zhu

Review Request 66119: Added a mock sandbox garbage collector.

[Meng Zhu]

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

Review request for mesos and Greg Mann.


Repository: mesos


Description
---

Added a mock sandbox garbage collector.


Diffs
-

  src/tests/mesos.hpp 2c3d0c9350cd2223ff20c0797d1849d38c19 
  src/tests/mesos.cpp d82963195573dd9ed7d12a7708f64a236b28cdf1 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 66052: Added new operator API to grow and shrink persistent volume.

[Zhitao Li]

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

(Updated March 16, 2018, 10:54 a.m.)


Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.


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


Repository: mesos


Description
---

Added new operator API to grow and shrink persistent volume.


Diffs (updated)
-

  include/mesos/master/master.proto 6622e1b82861f380346f505eca326f7174dd8bd6 
  include/mesos/v1/master/master.proto 6034bd5af8ad764f625f9fe80be08b48707bbadb 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

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

(Updated March 16, 2018, 10:54 a.m.)


Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.


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


Repository: mesos


Description
---

Added offer operation to grow and shrink persistent volumes.


Diffs (updated)
-

  include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
  include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Chun-Hung Hsiao]

> On March 15, 2018, 7:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > I'm thinking that, instead of asking the framework to craft the freed 
> > disk, we could leave it to the agent/RP so they have the freedom to 
> > transform the freed disk to an appropriate type of disk resource (although 
> > for now it will be the same as the original volume except in size and 
> > persistence). So how about having a `Scalar target` instead, and we can 
> > implement it through `Resources::shrink()`?
> 
> Zhitao Li wrote:
> I suggest we do not look at how `Resources` class implements various 
> helpers when designing public API, but rather think about how to make the API 
> easy, straightforward and consistent for the users (here it would be 
> framework authors).
> 
> Initially, I chose this format because it is more symetrical to 
> `RESERVE`/`UNRESERVE` and could be chained together in one operation. 
> However, after the decision to make new API non-chainable (and 
> non-speculative), that argument seems weaker now.
> 
> If we go with `target` format, I'd rather make the target as a full 
> `Resource` object and the API will look like:
> 
> ```
> message SHRINK_VOLUME {
>   required Resource original = 1; // original volume resource
>   required Resource target = 2;  // target volume resource
> }
> ```
> 
> Framework author can just make a copy of `original` and change the scalar 
> quantity.
> 
> Eventually, we might be able to mark `original` optional or drop it in 
> the API.
> 
> What do you think?

Yeah you're right about not making API decision based on utility functions.

Dropping `original` doesn't sound a good idea to me for the following reasons:
1. Consistency-wise, it would be different from other non-speculative calls 
such as `CREATE_VOLUME` or `DESTROY_VOLUME` (NOTE: I'm not talking about the 
`CREATE_VOLUMES` or `DESTROY_VOLUMES` operator calls), where we specify the 
source resource and let the operation performer to craft the converted.results.
2. Implementation-wise, the master and agent still need to find the original 
resource to construct and apply the resource conversion. If that doesn't come 
from the request, we need to do an extra search to find the source, which I 
don't feel necessary.

For `target` scalar vs resource, we could think about what are the restrictions 
we'd like to enforce. If we want to make sure that the resulting shrunk 
resource must look exactly the same and have no other metadata change, then I'm 
fine with making it a `Reaource`. If we want to have minimal restrictions and 
give the operation performer some freedom to modify the resoure (for example, 
make `taeget` a reference size or upper limit and thus the agent can shrink the 
volume as small as possible), then I feel a scalar looks better. FYI, in 
`CREATE_VOLUME`, we specify a `source` and a target type (`MOUNT` or `PATH`) 
only instead of the resulting resource, because the reaource provider will fill 
in extra information such as `id` and `metadata`.


- Chun-Hung


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


On March 13, 2018, 10:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 13, 2018, 10:48 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
>   include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Zhitao Li]

> On March 15, 2018, 12:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1920-1921 (patched)
> > 
> >
> > As we discussed, let's do `s/PERSISTENT_//`, as well as 
> > `s/Persistent//` and `s/persistent_//` below.

Will do.


> On March 15, 2018, 12:10 p.m., Chun-Hung Hsiao wrote:
> > include/mesos/mesos.proto
> > Lines 1975 (patched)
> > 
> >
> > I'm thinking that, instead of asking the framework to craft the freed 
> > disk, we could leave it to the agent/RP so they have the freedom to 
> > transform the freed disk to an appropriate type of disk resource (although 
> > for now it will be the same as the original volume except in size and 
> > persistence). So how about having a `Scalar target` instead, and we can 
> > implement it through `Resources::shrink()`?

I suggest we do not look at how `Resources` class implements various helpers 
when designing public API, but rather think about how to make the API easy, 
straightforward and consistent for the users (here it would be framework 
authors).

Initially, I chose this format because it is more symetrical to 
`RESERVE`/`UNRESERVE` and could be chained together in one operation. However, 
after the decision to make new API non-chainable (and non-speculative), that 
argument seems weaker now.

If we go with `target` format, I'd rather make the target as a full `Resource` 
object and the API will look like:

```
message SHRINK_VOLUME {
  required Resource original = 1; // original volume resource
  required Resource target = 2;  // target volume resource
}
```

Framework author can just make a copy of `original` and change the scalar 
quantity.

Eventually, we might be able to mark `original` optional or drop it in the API.

What do you think?


- Zhitao


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


On March 13, 2018, 3:48 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 13, 2018, 3:48 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Gaston Kleiman.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e6ba3746456c9241ceaefac39200f68562dd5cb9 
>   include/mesos/v1/mesos.proto 30d4d35e865db2af1ba85b12e2b5b0e499ef8de8 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66115: Added `mesos-tidy.py`, a modified version of `run-clang-tidy.py`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66115 was successfully built and tested.

Reviews applied: `['66114', '66115']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66115

- Mesos Reviewbot Windows


On March 16, 2018, 10:57 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66115/
> ---
> 
> (Updated March 16, 2018, 10:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66115/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 66001: MESOS-6575: Add soft limit and kill to disk/xfs.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66001]

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

- Mesos Reviewbot


On March 16, 2018, 10:43 a.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66001/
> ---
> 
> (Updated March 16, 2018, 10:43 a.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-6575
> https://issues.apache.org/jira/browse/MESOS-6575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> New Flags for disk/xfs isolator
> - This patch adds a number of flags to handle switching the limit in the
>   `disk/xfs` isolator to allow apps to go over their limit and for mesos
> to kill them if they have gone over their limit.
> 
> New Flags:
> - xfs_disk_hard_limit_offset_pct - Use the `disk` as the soft limit and
>   set the hard limit to be some percentage above the soft limit.
> Allowing
>   for containers to surpass a desired allocation and making them
> killable.
> - xfs_disk_hard_limit_offset - Use the `disk` as the soft limit and set
>   the hard limit to some number of bytes specified above the
>   applications to be a soft limit instead of a hard limit.
> - xfs_kill_after_grace_period - This will kill tasks if they breach the
>   grace period configured using `xfs_quota -x -c "timer -p "`
> - xfs_kill_check_interval - The frequency with which a container will be
>   checked for soft limit violations.
> 
> Functionality
> - Add head room to the hard limit as a percentage or specified amount
>   for each container. This is specified at a flag level and not
>   customizable on a per container basis.
> - Provide the ability for an application to be killed after the
>   configured grace period for projects is violated.
> - Add an interval between which the watcher will check for violations.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 07e68a777aefba4dd35066f2eb207bba7f199d83 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 8d9f8f846866f9de377c59cb7fb311041283ba70 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> e034133629a9c1cf58b776f8da2a93421332cee0 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
> 2708524add1ff693b616d4fb241c4a0a3070520b 
>   src/slave/flags.hpp 0c67bf214ceb93ae7ff088bec2648fa26ddac59e 
>   src/slave/flags.cpp 962b07c1d701f4ab819b14730fbc116b981433bb 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe 
> 
> 
> Diff: https://reviews.apache.org/r/66001/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 66001: MESOS-6575: Add soft limit and kill to disk/xfs.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66001 was successfully built and tested.

Reviews applied: `['66001']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66001

- Mesos Reviewbot Windows


On March 16, 2018, 10:43 a.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66001/
> ---
> 
> (Updated March 16, 2018, 10:43 a.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-6575
> https://issues.apache.org/jira/browse/MESOS-6575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> New Flags for disk/xfs isolator
> - This patch adds a number of flags to handle switching the limit in the
>   `disk/xfs` isolator to allow apps to go over their limit and for mesos
> to kill them if they have gone over their limit.
> 
> New Flags:
> - xfs_disk_hard_limit_offset_pct - Use the `disk` as the soft limit and
>   set the hard limit to be some percentage above the soft limit.
> Allowing
>   for containers to surpass a desired allocation and making them
> killable.
> - xfs_disk_hard_limit_offset - Use the `disk` as the soft limit and set
>   the hard limit to some number of bytes specified above the
>   applications to be a soft limit instead of a hard limit.
> - xfs_kill_after_grace_period - This will kill tasks if they breach the
>   grace period configured using `xfs_quota -x -c "timer -p "`
> - xfs_kill_check_interval - The frequency with which a container will be
>   checked for soft limit violations.
> 
> Functionality
> - Add head room to the hard limit as a percentage or specified amount
>   for each container. This is specified at a flag level and not
>   customizable on a per container basis.
> - Provide the ability for an application to be killed after the
>   configured grace period for projects is violated.
> - Add an interval between which the watcher will check for violations.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 07e68a777aefba4dd35066f2eb207bba7f199d83 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 8d9f8f846866f9de377c59cb7fb311041283ba70 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> e034133629a9c1cf58b776f8da2a93421332cee0 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
> 2708524add1ff693b616d4fb241c4a0a3070520b 
>   src/slave/flags.hpp 0c67bf214ceb93ae7ff088bec2648fa26ddac59e 
>   src/slave/flags.cpp 962b07c1d701f4ab819b14730fbc116b981433bb 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe 
> 
> 
> Diff: https://reviews.apache.org/r/66001/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Review Request 66114: Added supporting scripts for Mesos LLVM Tools.

[Michael Park]

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

Review request for mesos, Benjamin Bannier and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/llvm/README.md PRE-CREATION 
  support/llvm/install.sh PRE-CREATION 


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


Testing
---


Thanks,

Michael Park

Review Request 66115: Added `mesos-tidy.py`, a modified version of `run-clang-tidy.py`.

[Michael Park]

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

Review request for mesos, Benjamin Bannier and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/mesos-tidy.py PRE-CREATION 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 66001: MESOS-6575: Add soft limit and kill to disk/xfs.

[Harold Dost]

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

(Updated March 16, 2018, 10:43 a.m.)


Review request for mesos and James Peach.


Changes
---

Fixed the tests, how do I actually go about running in them in the Review bot?


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


Repository: mesos


Description
---

New Flags for disk/xfs isolator
- This patch adds a number of flags to handle switching the limit in the
  `disk/xfs` isolator to allow apps to go over their limit and for mesos
to kill them if they have gone over their limit.

New Flags:
- xfs_disk_hard_limit_offset_pct - Use the `disk` as the soft limit and
  set the hard limit to be some percentage above the soft limit.
Allowing
  for containers to surpass a desired allocation and making them
killable.
- xfs_disk_hard_limit_offset - Use the `disk` as the soft limit and set
  the hard limit to some number of bytes specified above the
  applications to be a soft limit instead of a hard limit.
- xfs_kill_after_grace_period - This will kill tasks if they breach the
  grace period configured using `xfs_quota -x -c "timer -p "`
- xfs_kill_check_interval - The frequency with which a container will be
  checked for soft limit violations.

Functionality
- Add head room to the hard limit as a percentage or specified amount
  for each container. This is specified at a flag level and not
  customizable on a per container basis.
- Provide the ability for an application to be killed after the
  configured grace period for projects is violated.
- Add an interval between which the watcher will check for violations.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
07e68a777aefba4dd35066f2eb207bba7f199d83 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
8d9f8f846866f9de377c59cb7fb311041283ba70 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
e034133629a9c1cf58b776f8da2a93421332cee0 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
2708524add1ff693b616d4fb241c4a0a3070520b 
  src/slave/flags.hpp 0c67bf214ceb93ae7ff088bec2648fa26ddac59e 
  src/slave/flags.cpp 962b07c1d701f4ab819b14730fbc116b981433bb 
  src/tests/containerizer/xfs_quota_tests.cpp 
64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe 


Diff: https://reviews.apache.org/r/66001/diff/7/

Changes: https://reviews.apache.org/r/66001/diff/6-7/


Testing
---


Thanks,

Harold Dost