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

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65994 was successfully built and tested.

Reviews applied: `['65300', '64618', '65993', '65994']`

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

- Mesos Reviewbot Windows


On March 8, 2018, 10:07 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65994/
> ---
> 
> (Updated March 8, 2018, 10:07 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> 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/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65919: Added a test `TaskValidationTest.TaskSettingDockerContainerName`.

[Gilbert Song]

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


Fix it, then Ship it!





src/tests/master_validation_tests.cpp
Lines 2831 (patched)


s/TaskSettingDockerContainerName/TasksettingDockerParameterName/g?


- Gilbert Song


On March 6, 2018, 12:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65919/
> ---
> 
> (Updated March 6, 2018, 12:41 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8497
> https://issues.apache.org/jira/browse/MESOS-8497
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `TaskValidationTest.TaskSettingDockerContainerName`.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> 77ec7fb37c9b9c168013d1d81ca862617682af6d 
> 
> 
> Diff: https://reviews.apache.org/r/65919/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65918: Made sure no `name` parameter exists in container's Docker info.

[Gilbert Song]

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


Fix it, then Ship it!





src/common/validation.cpp
Lines 283 (patched)


small nits:
s/Docker info/DockerInfo/g?


- Gilbert Song


On March 6, 2018, 12:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65918/
> ---
> 
> (Updated March 6, 2018, 12:40 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8497
> https://issues.apache.org/jira/browse/MESOS-8497
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Setting `name` parameter in container's Docker info will cause Docker
> containerizer cannot recognize the created Docker container, see
> MESOS-8497 for details. So this patch disallows that during validation
> stage.
> 
> 
> Diffs
> -
> 
>   src/common/validation.cpp bad9ea8a712162ba24a9a8ada02fe6cbe48f5c80 
> 
> 
> Diff: https://reviews.apache.org/r/65918/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65919: Added a test `TaskValidationTest.TaskSettingDockerContainerName`.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 6, 2018, 12:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65919/
> ---
> 
> (Updated March 6, 2018, 12:41 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8497
> https://issues.apache.org/jira/browse/MESOS-8497
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `TaskValidationTest.TaskSettingDockerContainerName`.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> 77ec7fb37c9b9c168013d1d81ca862617682af6d 
> 
> 
> Diff: https://reviews.apache.org/r/65919/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65917: Added a test `TaskValidationTest.TaskMissingDockerInfo`.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 5, 2018, 11:36 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65917/
> ---
> 
> (Updated March 5, 2018, 11:36 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8640
> https://issues.apache.org/jira/browse/MESOS-8640
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `TaskValidationTest.TaskMissingDockerInfo`.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> 77ec7fb37c9b9c168013d1d81ca862617682af6d 
> 
> 
> Diff: https://reviews.apache.org/r/65917/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66034 was successfully built and tested.

Reviews applied: `['66034']`

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

- Mesos Reviewbot Windows


On March 13, 2018, 1:29 a.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66034/
> ---
> 
> (Updated March 13, 2018, 1:29 a.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 above, with 
> `nosuid,noexec,nodev` mount points.
> ```
> $ 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 66030: Fixed perf stat output parsing.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66030]

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 12, 2018, 5:03 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66030/
> ---
> 
> (Updated March 12, 2018, 5:03 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-8664
> https://issues.apache.org/jira/browse/MESOS-8664
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a workaround for a bug in 'perf stat'
> (https://lkml.org/lkml/2018/3/6/22) and fixes handling of nameless
> counters.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040 
>   src/tests/containerizer/perf_tests.cpp 
> 7176240bd9caff1efc8b213fe7ae7faae59c0e3e 
> 
> 
> Diff: https://reviews.apache.org/r/66030/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test cases. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 65916: Set Docker info for the test `TaskUsesDockerContainerInfo`.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 5, 2018, 11:36 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65916/
> ---
> 
> (Updated March 5, 2018, 11:36 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8640
> https://issues.apache.org/jira/browse/MESOS-8640
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker info is required when container's type is DOCKER, it is enforced
> in `mesos::internal::common::validation:validateContainerInfo()`. So in
> this test, we have to set Docker info to pass the validation.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> 77ec7fb37c9b9c168013d1d81ca862617682af6d 
> 
> 
> Diff: https://reviews.apache.org/r/65916/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65915: Validated Docker info exists when container's type is DOCKER.

[Gilbert Song]

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


Fix it, then Ship it!





src/common/validation.cpp
Lines 273 (patched)


DockerInfo 'docker' is not set for DOCKER typed ContainerInfo.


- Gilbert Song


On March 5, 2018, 11:34 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65915/
> ---
> 
> (Updated March 5, 2018, 11:34 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8640
> https://issues.apache.org/jira/browse/MESOS-8640
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validated Docker info exists when container's type is DOCKER.
> 
> 
> Diffs
> -
> 
>   src/common/validation.cpp bad9ea8a712162ba24a9a8ada02fe6cbe48f5c80 
> 
> 
> Diff: https://reviews.apache.org/r/65915/diff/1/
> 
> 
> Testing
> ---
> 
> This patch will make the test 
> `TaskGroupValidationTest.TaskUsesDockerContainerInfo` fails, I fix it in a 
> subsequent patch: https://reviews.apache.org/r/65916
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Chun-Hung Hsiao]

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




3rdparty/stout/include/stout/protobuf.hpp
Lines 807-810 (patched)


How about making this a lambda inside `protobuf()` so we don't expose this?



3rdparty/stout/include/stout/protobuf.hpp
Lines 1018-1059 (patched)


How about:
```
JSON::Value key = value_for_field(entry, key_field);

if (key.is()) {
  name = key.as().value;
} else {
  name = jsonify(key);
}
```


- Chun-Hung Hsiao


On March 12, 2018, 7:15 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated March 12, 2018, 7:15 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, 
> and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with 3.0.0+ protobuf compiler, see the following discussion for
> details:
> 
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> 
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228. This patch adds map support in the json <-> protobuf
> conversion in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 66026: Updated `ProtobufTest.JSON` for parsing JSON::String to bools.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





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


Would you mind using the C++-style string literals to avoid the escaping, 
like we do here? 
https://github.com/apache/mesos/blob/master/src/tests/storage_local_resource_provider_tests.cpp#L178

It would introduce inconsistency between this test and the remaining tests 
in this file. So it's fine if you don't want to do this.



3rdparty/stout/tests/protobuf_tests.cpp
Lines 141 (patched)


Maybe `s/_expected/accepted/` to avoid naming confusion since we aren't 
expecting something equals to this string?



3rdparty/stout/tests/protobuf_tests.cpp
Lines 145-156 (patched)


We probably want to make `d`, `f`, `repeated_double` and `repeated_float` 
strings as well.


- Chun-Hung Hsiao


On March 12, 2018, 1:30 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66026/
> ---
> 
> (Updated March 12, 2018, 1:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8656
> https://issues.apache.org/jira/browse/MESOS-8656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `ProtobufTest.JSON` for parsing JSON::String to bools
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.cpp 
> be35ad0d1e16501df67752a1818f79751419a43d 
> 
> 
> Diff: https://reviews.apache.org/r/66026/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Jason Lai]

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

(Updated March 13, 2018, 1:29 a.m.)


Review request for mesos, Eric Chung, Gilbert Song, Ian Downes, Jie Yu, James 
Peach, and Zhitao Li.


Changes
---

Updated description.


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


Repository: mesos


Description (updated)
---

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 above, with 
`nosuid,noexec,nodev` mount points.
```
$ 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

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

[Jason Lai]

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

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 users 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 above, with 
`nosuid,noexec,nodev` mount points.
```
$ 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 66030: Fixed perf stat output parsing.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66030 was successfully built and tested.

Reviews applied: `['66030']`

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

- Mesos Reviewbot Windows


On March 13, 2018, 12:03 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66030/
> ---
> 
> (Updated March 13, 2018, 12:03 a.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-8664
> https://issues.apache.org/jira/browse/MESOS-8664
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a workaround for a bug in 'perf stat'
> (https://lkml.org/lkml/2018/3/6/22) and fixes handling of nameless
> counters.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040 
>   src/tests/containerizer/perf_tests.cpp 
> 7176240bd9caff1efc8b213fe7ae7faae59c0e3e 
> 
> 
> Diff: https://reviews.apache.org/r/66030/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test cases. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 66025: Converted `JSON::String` to bool and integers.

[Chun-Hung Hsiao]

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




3rdparty/stout/include/stout/protobuf.hpp
Lines 454-538 (patched)


As Ben mentioned, this is to match the "Notes" column of Google's JSON 
apping table ( 
https://developers.google.com/protocol-buffers/docs/proto3#json), except that 
the documentation missed the note that string values are also accepted for 
boolean fields.

Therefore, we probably should also include string -> float/double 
conversion. Given this, I'd suggest that we do the following instead to further 
simplify the logic:

```
case ...: // All 32-bit and 64-bit ints, float an double
case ...: {
  Try number = JSON::parse(string.value);

  if (number.isError()) {
return Error(...);
  }

  return operator()(number);
}
case TYPE_BOOL: {
  Try boolean = JSON::parse(string.value);
  
  if (boolean.isError()) {
return Error(...);
  }
  
  return operator()(boolean);
}
```


- Chun-Hung Hsiao


On March 12, 2018, 1:28 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66025/
> ---
> 
> (Updated March 12, 2018, 1:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8656
> https://issues.apache.org/jira/browse/MESOS-8656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previsouly when converting a JSON to a protobuf message in stout, we
> cannot handle the fields like below which are actually valid.
>   "int32": "-2147483647"
>   "int64": "-9223372036854775807"
>   "bool": "true"
> The conversion will fail with an error like "Not expecting a JSON string
> for field 'int32'".
> 
> So in this patch, `Try operator()(const JSON::String& string)`
> was enhanced to be able to convert `JSON::String` to bool and integers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 
> 
> 
> Diff: https://reviews.apache.org/r/66025/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65987: Allow nested containers in pods to have separate namespaces(Ref: MESOS-8534).

[Sagar Patwardhan]

> On March 9, 2018, 11:40 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Line 389 (original), 389-397 (patched)
> > 
> >
> > Instead of relying on an additional checkpoint file, I think we can 
> > infer if a nested container joins its parent container's network or not by 
> > first listing all entries in `rootDir`, and then recover those known 
> > containers, and then cleanup unknown orphans. Something like the following:
> > 
> > ```
> > // Build ContainerID -> ContainerState mapping
> > // List rootDir
> > // For each entry, find the corresponding ContainerState state (state 
> > is optional)
> > // Call _recover(containerId, state), if containerId is nested, set 
> > joinsParentsNetwork to false.
> > // Cleanup unknown orphans (not in container state mapping or orphans 
> > map)
> > ```

This is a good idea.


> On March 9, 2018, 11:40 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Lines 1269-1285 (patched)
> > 
> >
> > I don't think we need to checkpoint this additional information. You 
> > should be able to infer if a nested container has its own network namespace 
> > or not by checking if there's a container directory under 
> > `cni::paths::ROOT_DIR`. See 
> > src/slave/containerizer/mesos/isolators/network/cni/paths.hpp

Yeah, you are right. Did not consider this! :(


> On March 9, 2018, 11:40 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Lines 1488 (patched)
> > 
> >
> > What about top level containers? I think `joinParentsNetwork` should be 
> > orthogonol to whether it is nested or not. Top level container can join its 
> > parent's network (agent) too.
> > 
> > If you think about that way, here we should do
> > ```
> > if (isNestedContainer && joinsParentsNetwork)
> > ```

hmm.. did not think about it.


- Sagar


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


On March 8, 2018, 5:07 p.m., Sagar Patwardhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65987/
> ---
> 
> (Updated March 8, 2018, 5:07 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-8534
> https://issues.apache.org/jira/browse/MESOS-8534
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Continued from https://github.com/apache/mesos/pull/263
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 1d01915c2db66e54ed68a3dbaa12ea061ca5f6b2 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 896656987012b3ffe5008ce6873c9a5249c058de 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
> 7678a7c81c3cdb27410c1f066021eb34bd02a83f 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
> f9056c90f1075cb19c4f554e7d5b629561d06035 
> 
> 
> Diff: https://reviews.apache.org/r/65987/diff/1/
> 
> 
> Testing
> ---
> 
> Manually tested.
> 
> Working on unit tests.
> 
> 
> Thanks,
> 
> Sagar Patwardhan
> 
>

Re: Review Request 66016: Refactored resource allocation logic.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66016']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

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

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (37 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (45 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (84 ms 
total)

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

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

[--] Global test environment tear-down
[==] 916 tests from 91 test cases ran. (468263 ms total)
[  PASSED  ] 915 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SlaveTest.RestartSlaveRequireExecutorAuthentication

 1 FAILED TEST
  YOU HAVE 210 DISABLED TESTS

```

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

```
I0313 00:14:38.126778  3456 master.cpp:10245] Updating the state of task 
7585aff6-5367-47ae-8720-874fc68ad489 of framework 
32e127f4-6416-43b0-8dda-e21df6d56b0c- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0313 00:14:38.126778 10300 slave.cpp:3878] Shutting down framework 
32e127f4-6416-43b0-8dda-e21df6d56b0c-
I0313 00:14:38.127776 10300 slave.cpp:6571] Shutting down executor 
'7585aff6-5367-47ae-8720-874fc68ad489' of framework 
32e127f4-6416-43b0-8dda-e21df6d56b0c-I0313 00:14:37.422773   992 exec.cpp:162] 
Version: 1.6.0
I0313 00:14:37.450773  5208 exec.cpp:236] Executor registered on agent 
32e127f4-6416-43b0-8dda-e21df6d56b0c-S0
I0313 00:14:37.455775  3568 executor.cpp:176] Received SUBSCRIBED event
I0313 00:14:37.460777  3568 executor.cpp:180] Subscribed executor on 
build-srv-04.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0313 00:14:37.461776  3568 executor.cpp:176] Received LAUNCH event
I0313 00:14:37.466778  3568 executor.cpp:648] Starting task 
7585aff6-5367-47ae-8720-874fc68ad489
I0313 00:14:37.548775  3568 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0313 00:14:38.093776  3568 executor.cpp:661] Forked command at 10460
I0313 00:14:38.131774  7948 exec.cpp:445] Executor asked to shutdown
I0313 00:14:38.131774  3568 executor.cpp:176] Received SHUTDOWN event
I0313 00:14:38.132773  3568 executor.cpp:758] Shutting down
I0313 00:14:38.132773  3568 executor.cpp:868] Sending SIGTERM to process tree 
at pid  at executor(1)@10.3.1.5:56110
I0313 00:14:38.129777 10300 slave.cpp:924] Agent terminating
W0313 00:14:38.129777 10300 slave.cpp:3874] Ignoring shutdown framework 
32e127f4-6416-43b0-8dda-e21df6d56b0c- because it is terminating
I0313 00:14:38.129777  3456 master.cpp:10344] Removing task 
7585aff6-5367-47ae-8720-874fc68ad489 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 32e127f4-6416-43b0-8dda-e21df6d56b0c- on 
agent 32e127f4-6416-43b0-8dda-e21df6d56b0c-S0 at slave(398)@10.3.1.5:56089 
(build-srv-04.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0313 00:14:38.132773  5756 containerizer.cpp:2338] Destroying container 
cbf5b054-c117-4d63-9f04-3cc549b01d8a in RUNNING state
I0313 00:14:38.132773  5756 containerizer.cpp:2952] Transitioning the state of 
container cbf5b054-c117-4d63-9f04-3cc549b01d8a from RUNNING to DESTROYING
I0313 00:14:38.132773  3456 master.cpp:1288] Agent 
32e127f4-6416-43b0-8dda-e21df6d56b0c-S0 at slave(398)@10.3.1.5:56089 
(build-srv-04.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0313 00:14:38.133774  8204 hierarchical.cpp:344] Removed framework 
32e127f4-6416-43b0-8dda-e21df6d56b0c-
I0313 00:14:38.133774  5756 launcher.cpp:156] Asked to 

Re: Review Request 66016: Refactored resource allocation logic.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 12, 2018, 11:07 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66016/
> ---
> 
> (Updated March 12, 2018, 11:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current allocation logic maintains several states
> such as allocated-reservation, unsatisfied-quota and etc.
> This patch refactors the logic so that we only need to
> maintain a per-quota-role map to track role consumed quota.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5d30d1d4d4bbb5431745f61b5318b39c5c3a7117 
> 
> 
> Diff: https://reviews.apache.org/r/66016/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66031: Avoided copying when possible in Option::getOrElse.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On March 12, 2018, 5:08 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66031/
> ---
> 
> (Updated March 12, 2018, 5:08 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This mimics the behavior of std::optional::value_or.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/option.hpp 
> 72403e770afbb59bd1c58b621c2bc566de3cd979 
> 
> 
> Diff: https://reviews.apache.org/r/66031/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 66031: Avoided copying when possible in Option::getOrElse.

[Benjamin Mahler]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

This mimics the behavior of std::optional::value_or.


Diffs
-

  3rdparty/stout/include/stout/option.hpp 
72403e770afbb59bd1c58b621c2bc566de3cd979 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 66030: Fixed perf stat output parsing.

[Ilya Pronin]

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

Review request for mesos and James Peach.


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


Repository: mesos


Description
---

This change adds a workaround for a bug in 'perf stat'
(https://lkml.org/lkml/2018/3/6/22) and fixes handling of nameless
counters.


Diffs
-

  src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040 
  src/tests/containerizer/perf_tests.cpp 
7176240bd9caff1efc8b213fe7ae7faae59c0e3e 


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


Testing
---

Added new test cases. Ran `sudo make check`.


Thanks,

Ilya Pronin

Re: Review Request 66016: Refactored resource allocation logic.

[Meng Zhu]

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

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


Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael Park, 
and Till Toenshoff.


Changes
---

Thanks for the review!


Repository: mesos


Description
---

The current allocation logic maintains several states
such as allocated-reservation, unsatisfied-quota and etc.
This patch refactors the logic so that we only need to
maintain a per-quota-role map to track role consumed quota.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
5d30d1d4d4bbb5431745f61b5318b39c5c3a7117 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 66016: Refactored resource allocation logic.

[Meng Zhu]

> On March 12, 2018, 3:29 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1610-1612 (patched)
> > 
> >
> > Hm.. as an aside, looks like `Option::getOrElse` could be improved  to 
> > avoid copying in cases like this?
> > 
> > E.g.
> > 
> > ```
> >   T&& getOrElse(T&& _t) && { return std::move(isNone() ? _t : t); }
> >   T&& getOrElse(T&& _t) const && { return std::move(isNone() ? _t : t); 
> > }
> > ```

Sounds good. Will add this in a follow up patch.


> On March 12, 2018, 3:29 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1616 (patched)
> > 
> >
> > This should probably be renamed to s/Resources/ScalarQuantities/ to be 
> > consistent with your other naming? Want to do that in a separate patch?

Will do.


> On March 12, 2018, 3:29 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2002-2003 (original), 1972-1973 (patched)
> > 
> >
> > Why isn't this:
> > 
> > ```
> > rolesConsumedQuotaScalarQuantites[role] +=
> >   newQuotaAllocationScalarQuantities;
> > ```
> > 
> > It's not obvious to me, are the two equivalent? The one included in the 
> > patch seems to be including resources that aren't part of the quota 
> > guarantee? Seems weird?

Both produce the correct result (version 1 of this patch actaully uses this 
formula). But I think `+= allocatedUnreserved` makes more sense. The way we 
calculate `rolesConsumedQuotaScalarQuantites` in the first place is by summing 
`reservations + allocation - allocated reservations` i.e. 
`quota.contains(rolesConsumedQuotaScalarQuantites)` was never true.

I have plan to persist `rolesConsumedQuotaScalarQuantites` for quota role 
across iterations. Consider a role updated with a new quota. `+= 
newQuotaAllocationScalarQuantities` would need to recalculate 
`rolesConsumedQuotaScalarQuantites` but `+= allocatedUnreserved` would be fine.

In a more general sense, each role has some (default) quota for all resources, 
consumed resources that only has default quota should still be counted as 
consumed quota.


- Meng


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


On March 12, 2018, 4:07 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66016/
> ---
> 
> (Updated March 12, 2018, 4:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current allocation logic maintains several states
> such as allocated-reservation, unsatisfied-quota and etc.
> This patch refactors the logic so that we only need to
> maintain a per-quota-role map to track role consumed quota.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5d30d1d4d4bbb5431745f61b5318b39c5c3a7117 
> 
> 
> Diff: https://reviews.apache.org/r/66016/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66016: Refactored resource allocation logic.

[Benjamin Mahler]

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



Good cleanup thanks! Just two issues below that I would like to resolve prior 
to shipping.


src/master/allocator/mesos/hierarchical.cpp
Lines 1610-1612 (patched)


Hm.. as an aside, looks like `Option::getOrElse` could be improved  to 
avoid copying in cases like this?

E.g.

```
  T&& getOrElse(T&& _t) && { return std::move(isNone() ? _t : t); }
  T&& getOrElse(T&& _t) const && { return std::move(isNone() ? _t : t); }
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1616 (patched)


This should probably be renamed to s/Resources/ScalarQuantities/ to be 
consistent with your other naming? Want to do that in a separate patch?



src/master/allocator/mesos/hierarchical.cpp
Lines 2002-2003 (original), 1972-1973 (patched)


Why isn't this:

```
rolesConsumedQuotaScalarQuantites[role] +=
  newQuotaAllocationScalarQuantities;
```

It's not obvious to me, are the two equivalent? The one included in the 
patch seems to be including resources that aren't part of the quota guarantee? 
Seems weird?



src/master/allocator/mesos/hierarchical.cpp
Lines 2012-2014 (original), 1975-1977 (patched)


Not a new problem, but looks like a comment is warranted here, I couldn't 
easily figure out why one subtracts new quota allocation and the other 
subtracts unreserved allocation.


- Benjamin Mahler


On March 10, 2018, 12:44 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66016/
> ---
> 
> (Updated March 10, 2018, 12:44 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current allocation logic maintains several states
> such as allocated-reservation, unsatisfied-quota and etc.
> This patch refactors the logic so that we only need to
> maintain a per-quota-role map to track role consumed quota.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5d30d1d4d4bbb5431745f61b5318b39c5c3a7117 
> 
> 
> Diff: https://reviews.apache.org/r/66016/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66025: Converted `JSON::String` to bool and integers.

[Benjamin Mahler]

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



Can we clarify in the commit message that we're doing this to match Google's 
json -> protobuf behavior?

- Benjamin Mahler


On March 12, 2018, 1:28 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66025/
> ---
> 
> (Updated March 12, 2018, 1:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8656
> https://issues.apache.org/jira/browse/MESOS-8656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previsouly when converting a JSON to a protobuf message in stout, we
> cannot handle the fields like below which are actually valid.
>   "int32": "-2147483647"
>   "int64": "-9223372036854775807"
>   "bool": "true"
> The conversion will fail with an error like "Not expecting a JSON string
> for field 'int32'".
> 
> So in this patch, `Try operator()(const JSON::String& string)`
> was enhanced to be able to convert `JSON::String` to bool and integers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 
> 
> 
> Diff: https://reviews.apache.org/r/66025/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63992: Handled incorrect offer operation IDs in master's ACCEPT handler.

[Greg Mann]

> On March 7, 2018, 7:21 p.m., Gaston Kleiman wrote:
> > src/master/master.cpp
> > Line 4136 (original), 4184 (patched)
> > 
> >
> > If the operation has an operation ID, we'll always land on this 
> > `continue` even if the validations/checks pass.
> > 
> > This means that line #4187 will be skipped, and operations with an 
> > operatin ID won't be added to the object passed to the `_accept` 
> > continuation.

Ah thank you; this should be removed.


- Greg


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


On March 2, 2018, 10:52 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63992/
> ---
> 
> (Updated March 2, 2018, 10:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8190
> https://issues.apache.org/jira/browse/MESOS-8190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds code to send operation status updates in the master's
> ACCEPT call handler. In cases where operations are dropped and in
> cases where offer operation IDs are set when they should not be, the
> master will send operation status updates for the dropped operations.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 9cea7bb6a6ee8bf5f4226d07111bcfa6f5d3a88c 
> 
> 
> Diff: https://reviews.apache.org/r/63992/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 65879: Added a new test for validation of offer operation IDs.

[Greg Mann]

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

Review request for mesos.


Repository: mesos


Description
---

This patch adds a new test,
'SchedulerTest.OfferOperationFeedbackValidation',
to verify that task and offer operation status updates are correctly
sent when the 'id' field is set incorrectly on an operation.


Testing
---


Thanks,

Greg Mann

Re: Review Request 65995: Declined unwanted offers in `RetryOperationStatusUpdate*` SLRP tests.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On March 8, 2018, 11:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65995/
> ---
> 
> (Updated March 8, 2018, 11:39 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The two SLRP tests assume that SLRP will send out a RAW resource in its
> first `UPDATE_STATE` message, and expect that the test framework would
> receive an offer containing the RAW resource in its first offer. However
> this behavior is not guaranteed and should not be relied on. This patch
> makes the tests decline unwanted offers by default so they no longer
> rely on SLRP's internal behavior.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> c2fcbf32936ef1cb6fd313eac7e3c9c5de99c1fc 
> 
> 
> Diff: https://reviews.apache.org/r/65995/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> Ran the two tests in repitition.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66012: CMake: Split `CMAKE_FORWARD_ARGS` into `C` and `CXX` versions.

[John Kordich via Review Board]

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


Ship it!




I'm fine with these changes, but the description can be improved a bit I think. 
I know that this change relates to some of the later patches, so maybe mention 
that several of the fixes are built on top of this one (I guess that's implied, 
though).  Maybe mention that this allows for granularity between C/CPP cmake 
flags, even though it's rather clear from the code.

- John Kordich


On March 9, 2018, 10:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66012/
> ---
> 
> (Updated March 9, 2018, 10:39 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By passing `CMAKE_C_FORWARD_ARGS` to C libraries, e.g. curl, libapr,
> libevent, and zlib, we avoid CMake warning us about set but unused
> variables (since they do not use the `CXX` variables).
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 
> 
> 
> Diff: https://reviews.apache.org/r/66012/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66011: Windows: Set 3rdparty libraries to link to CRT dynamically.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On March 9, 2018, 10:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66011/
> ---
> 
> (Updated March 9, 2018, 10:39 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
> ---
> 
> This also fixes a bug where the `libevent` build on Windows would add
> `''` to the compiler flags, which is an unrecognized flag, and sets
> the names of our `CMAKE_FORWARD_ARGS` variables consistently (some
> were missing the `FORWARD` part).
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 
> 
> 
> Diff: https://reviews.apache.org/r/66011/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66013: Windows: Made ZooKeeper use default CRT linking.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On March 9, 2018, 10:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66013/
> ---
> 
> (Updated March 9, 2018, 10:39 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
> ---
> 
> Also deleted extraneous patch files.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 
>   3rdparty/zookeeper-06d3f3f.patch 7f66875c7ef8bea3cc077d064b90e056c706487f 
>   3rdparty/zookeeper-3.4.8.patch c532a696f8bf3373b7341ddf32dcf9c528ee4412 
>   3rdparty/zookeeper-3.5.2-alpha.patch 
> 6d5db536437b22a2903c1fa3348613e12fd35df0 
> 
> 
> Diff: https://reviews.apache.org/r/66013/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On March 9, 2018, 10:39 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66010/
> ---
> 
> (Updated March 9, 2018, 10:39 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/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On March 9, 2018, 10:38 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66009/
> ---
> 
> (Updated March 9, 2018, 10:38 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.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 61dc788edf96ca6e8e3ee736eb232c0c118e6191 
> 
> 
> Diff: https://reviews.apache.org/r/66009/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On March 9, 2018, 10: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, 10: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 65350: Modified `cgroups::prepare` to check nested cgroups support only once.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65350]

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 12, 2018, 2:50 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65350/
> ---
> 
> (Updated March 12, 2018, 2:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gilbert Song, Jie Yu, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-8489
> https://issues.apache.org/jira/browse/MESOS-8489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch modifies `cgroup::prepare` function to save the results of
> a test for a nested cgroups support in a hashmap to reuse it in the
> future.
> 
> Besides optimization, this patch fixes flaky
> `LinuxCapabilitiesIsolatorFlagsTest.ROOT_IsolatorFlags` test. There
> was a race condition between two starting agents, where the first
> agent could detect `test` orphaned container while iterating over
> `/sys/fs/cgroup/freezer/mesos/` cgroup, because the second agent
> is calling `cgroup::prepare` during its initialization. This patch
> fixes the test, because it won't create a `test` cgroup in
> `cgroups::prepare` as this test has already been passed during the
> initialization of the first agent and the result is stored in hashmap.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 682b288da8d56a623ef32cf6f0beecd9fc327622 
> 
> 
> Diff: https://reviews.apache.org/r/65350/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65350: Modified `cgroups::prepare` to check nested cgroups support only once.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65350 was successfully built and tested.

Reviews applied: `['65350']`

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

- Mesos Reviewbot Windows


On Jan. 26, 2018, 2:17 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65350/
> ---
> 
> (Updated Jan. 26, 2018, 2:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-8489
> https://issues.apache.org/jira/browse/MESOS-8489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch modifies `cgroup::prepare` function to save the results of
> a test for a nested cgroups support in a hashmap to reuse it in the
> future.
> 
> Besides optimization, this patch fixes flaky
> `LinuxCapabilitiesIsolatorFlagsTest.ROOT_IsolatorFlags` test. There
> was a race condition between two starting agents, where the first
> agent could detect `test` orphaned container while iterating over
> `/sys/fs/cgroup/freezer/mesos/` cgroup, because the second agent
> is calling `cgroup::prepare` during its initialization. This patch
> fixes the test, because it won't create a `test` cgroup in
> `cgroups::prepare` as this test has already been passed during the
> initialization of the first agent and the result is stored in hashmap.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 682b288da8d56a623ef32cf6f0beecd9fc327622 
> 
> 
> Diff: https://reviews.apache.org/r/65350/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65311: Added the ObjectApprovers to which unifies authorization logic.

[Alexander Rojas]

> On March 9, 2018, 11:16 p.m., Greg Mann wrote:
> > src/common/http.hpp
> > Lines 198 (patched)
> > 
> >
> > See the comment I left on https://reviews.apache.org/r/65313/ - I 
> > wonder if `action` really needs to be a template parameter here?

Strictly speaking it doesn't need to be a template, but then overrides like:

```c++
template <>
inline bool ObjectApprovers::approved(
const Resource& resource)
{
  // ...
}
```

Would have need to be dispatched to helper functions or handler in big ifs in 
`approved()`.

The end result would be, you just move the part hard to read from one side to 
the next.

My personal opinion is that the helper in the agent cleanup is more readable an 
a better price to pay than a gigantic switch here.


- Alexander


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


On March 8, 2018, 1:14 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65311/
> ---
> 
> (Updated March 8, 2018, 1:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the class `ObjectApprovers` which unifies the different
> mechanisms used in Mesos in order to perform authorization.
> 
> This new class will supersede the `Acceptor` interfaces and their
> logic.
> 
> Follow up patches will make use of this interface and remove
> deprecated code.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
>   src/common/http.cpp 8d3a4ad63bd74e0313082bad3d89c21fbf54f256 
> 
> 
> Diff: https://reviews.apache.org/r/65311/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 65314: Removed code which is not used.

[Alexander Rojas]

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

(Updated March 12, 2018, 3:05 p.m.)


Review request for mesos, Benjamin Hindman and Greg Mann.


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


Repository: mesos


Description (updated)
---

The introduction of the `ObjectApprovers` abstraction rendered the
`AuthorizationAcceptor` class obsolete. After the refactor of the code
the acceptor class was no longer used, nor were the helper functions
built around it.

This patch removes that obsolete code.


Diffs
-

  src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
  src/common/http.cpp 8d3a4ad63bd74e0313082bad3d89c21fbf54f256 


Diff: https://reviews.apache.org/r/65314/diff/8/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 66022: Based website-related Docker images on ubuntu16.

[Benjamin Bannier]

> On March 12, 2018, 12:51 p.m., Alexander Rukletsov wrote:
> > site/Dockerfile
> > Lines 12 (patched)
> > 
> >
> > You likely want to add `--no-install-recommends` here as well, right?

I definitely want to, and now actually do.


- Benjamin


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


On March 12, 2018, 2:29 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66022/
> ---
> 
> (Updated March 12, 2018, 2:29 p.m.)
> 
> 
> Review request for mesos, Armand Grillet and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We require a recent ruby version in order to build current version of
> native ruby extensions required by the website generator setup.
> 
> Instead of manually installing non-upstream ruby versions with the
> existing centos7-based website images, this patch changes the images
> to be based on ubuntu16 instead which already contains a recent enough
> ruby versions in the upstream repositories.
> 
> We also change the images to reuse the existing mesos-build images
> instead of duplicating the image setup for these parts.
> 
> 
> Diffs
> -
> 
>   site/Dockerfile 2dd9e288df51b50ce3553045c58be978ce0f3e8e 
>   support/mesos-website/Dockerfile 4156eb4112060332bc62ab14d50a685cc7ee0348 
> 
> 
> Diff: https://reviews.apache.org/r/66022/diff/2/
> 
> 
> Testing
> ---
> 
> Manually built the dev and jenkin images and verified that they contained all 
> required tools.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66022: Based website-related Docker images on ubuntu16.

[Benjamin Bannier]

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

(Updated March 12, 2018, 2:29 p.m.)


Review request for mesos, Armand Grillet and Vinod Kone.


Changes
---

Consistently used `--no-install-recommends` with installation.


Repository: mesos


Description
---

We require a recent ruby version in order to build current version of
native ruby extensions required by the website generator setup.

Instead of manually installing non-upstream ruby versions with the
existing centos7-based website images, this patch changes the images
to be based on ubuntu16 instead which already contains a recent enough
ruby versions in the upstream repositories.

We also change the images to reuse the existing mesos-build images
instead of duplicating the image setup for these parts.


Diffs (updated)
-

  site/Dockerfile 2dd9e288df51b50ce3553045c58be978ce0f3e8e 
  support/mesos-website/Dockerfile 4156eb4112060332bc62ab14d50a685cc7ee0348 


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

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


Testing
---

Manually built the dev and jenkin images and verified that they contained all 
required tools.


Thanks,

Benjamin Bannier

Re: Review Request 66000: Update enforce_container_disk_quota documentation to include disk/xfs.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66000]

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 12, 2018, 8:28 a.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66000/
> ---
> 
> (Updated March 12, 2018, 8:28 a.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update enforce_container_disk_quota documentation to include disk/xfs.
> 
> 
> Diffs
> -
> 
>   src/slave/flags.cpp 943aaaf58b5f36555f0902019b8c5c6522ab7afc 
> 
> 
> Diff: https://reviews.apache.org/r/66000/diff/1/
> 
> 
> Testing
> ---
> 
> None needed, since this is just a documentation update
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 65906: Updated site's middleman to latest 3.x release.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On March 10, 2018, 5:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65906/
> ---
> 
> (Updated March 10, 2018, 5:49 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Benjamin Mahler, Greg Mann, and 
> haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated site's middleman to latest 3.x release.
> 
> 
> Diffs
> -
> 
>   site/Gemfile 877fe914a9787c140848fdf9958571fec5fa58ff 
>   site/Gemfile.lock 909f3f3badeaa47c80929e243ce36307766edee4 
> 
> 
> Diff: https://reviews.apache.org/r/65906/diff/3/
> 
> 
> Testing
> ---
> 
> Locally tested the generated website: `bundle install && bundle exec rake && 
> bundle exec rake dev`.
> 
> The only manually updated file was `site/Gemfile`; `site/Gemfile.lock` was 
> updated automatically by `bundler`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66022: Based website-related Docker images on ubuntu16.

[Alexander Rukletsov]

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


Ship it!





site/Dockerfile
Lines 12 (patched)


You likely want to add `--no-install-recommends` here as well, right?


- Alexander Rukletsov


On March 10, 2018, 5:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66022/
> ---
> 
> (Updated March 10, 2018, 5:49 p.m.)
> 
> 
> Review request for mesos, Armand Grillet and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We require a recent ruby version in order to build current version of
> native ruby extensions required by the website generator setup.
> 
> Instead of manually installing non-upstream ruby versions with the
> existing centos7-based website images, this patch changes the images
> to be based on ubuntu16 instead which already contains a recent enough
> ruby versions in the upstream repositories.
> 
> We also change the images to reuse the existing mesos-build images
> instead of duplicating the image setup for these parts.
> 
> 
> Diffs
> -
> 
>   site/Dockerfile 2dd9e288df51b50ce3553045c58be978ce0f3e8e 
>   support/mesos-website/Dockerfile 4156eb4112060332bc62ab14d50a685cc7ee0348 
> 
> 
> Diff: https://reviews.apache.org/r/66022/diff/1/
> 
> 
> Testing
> ---
> 
> Manually built the dev and jenkin images and verified that they contained all 
> required tools.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65591: Explicitly tracked resource providers in master.

[Benjamin Bannier]

> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Line 7722 (original), 7680-7681 (patched)
> > 
> >
> > Perhaps we also want to rescind offers when 
> > `message.has_resource_providers()` is false? In that case, the agent has 
> > told us that it has no registered RPs, so offers with RP resources would 
> > not be valid?
> 
> Benjamin Bannier wrote:
> I do not think that is true. Only when the `resource_providers` field is 
> set does the agent send any resource provider information. The field could 
> contain information on zero or more resource providers in its `providers` 
> field, but we do not distinguish that here, exactly as you suggest.
> 
> Dropping.
> 
> Greg Mann wrote:
> IIUC, the agent always sends info on all registered RPs in each 
> UpdateSlaveMessage. So, if the master receives an UpdateSlaveMessage which 
> indicates that no RPs are currently registered on an agent, doesn't that mean 
> that offers for that agent with RP resources are currently invalid?

Yes, the offers will be rescinded below.


> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 10572-10574 (original), 10534-10536 (patched)
> > 
> >
> > Are you sure this will always be true? Could a framework send us a 
> > well-formed, but unknown RP ID in a message and make this false?
> 
> Benjamin Bannier wrote:
> This is an important invariant of the current implementation, see 
> https://issues.apache.org/jira/browse/MESOS-8321.
> 
> Dropping.
> 
> Greg Mann wrote:
> Are we sure that invariant holds in this case? The resource provider ID 
> here is framework-supplied, and I don't think we validate that it's present 
> in the master state before we hit this code.

On the accept path the master will verify that (i) it knows about the offer 
(https://github.com/apache/mesos/blob/843e5e85939d848b0898753c9d7542ecc997135c/src/master/master.cpp#L3906-L3924),
 and (ii) that the operations are consistent with the originally offered 
resources 
(https://github.com/apache/mesos/blob/843e5e85939d848b0898753c9d7542ecc997135c/src/master/master.cpp#L4530-L4544).

For a resource provider to be unknown to the master here it would either have 
to be explicitly removed by the master, or be made up by the framework. In the 
first case the offer would be invalid, in the second the operation consistency 
check would fail.


- Benjamin


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


On March 7, 2018, 12:36 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> ---
> 
> (Updated March 7, 2018, 12:36 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8382
> https://issues.apache.org/jira/browse/MESOS-8382
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds explicit tracking of resource providers to the master
> process. While we already had explicitly send resource provider
> information in e.g., `UpdateSlaveMessage`, we only stored that
> information aggregated over the full agent in the master up to now.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 9c5fb97afb58f98013b79f3cbaea7dacc3603271 
>   src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65591/diff/8/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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



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 9, 2018, 10:34 p.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66001/
> ---
> 
> (Updated March 9, 2018, 10:34 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> New Flags for disk/xfs isolator
> - xfs_disk_hard_limit_offset_pct - creates a gap between
> - xfs_disk_hard_limit_offset - overrides _pct creates an offset between
>   soft and hard limits of a container.
> - xfs_kill_after_grace_period - will cause a container to be killed if
>   the soft limit has been breached after the `xfs_quota` configured
>   grace period has ended.
> - xfs_kill_check_interval - Period of delay between which the isolator
>   will check for violations of the soft limit.
> 
> 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 943aaaf58b5f36555f0902019b8c5c6522ab7afc 
> 
> 
> Diff: https://reviews.apache.org/r/66001/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Re: Review Request 66000: Update enforce_container_disk_quota documentation to include disk/xfs.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66000']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

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

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

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

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

[--] Global test environment tear-down
[==] 916 tests from 91 test cases ran. (479030 ms total)
[  PASSED  ] 915 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CommandExecutorCheckTest.CommandCheckTimeout

 1 FAILED TEST
  YOU HAVE 210 DISABLED TESTS

```

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

```
I0312 09:32:58.398156  2368 slave.cpp:3878] Shutting down framework 
be3975be-cd38-4964-a044-55d4bad06ae6-
I0312 09:32:58.398156  3256 masI0312 09:32:57.682153  4136 exec.cpp:162] 
Version: 1.6.0
I0312 09:32:57.717155  1796 exec.cpp:236] Executor registered on agent 
be3975be-cd38-4964-a044-55d4bad06ae6-S0
I0312 09:32:57.722156   588 executor.cpp:176] Received SUBSCRIBED event
I0312 09:32:57.727156   588 executor.cpp:180] Subscribed executor on 
build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0312 09:32:57.729156  1796 executor.cpp:176] Received LAUNCH event
I0312 09:32:57.732177  1796 executor.cpp:648] Starting task 
3e420232-31de-4648-b4b4-1252920c0a9e
I0312 09:32:57.816180  1796 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0312 09:32:58.359159  1796 executor.cpp:661] Forked command at 8896
I0312 09:32:58.401181   588 exec.cpp:445] Executor asked to shutdown
I0312 09:32:58.403156  1796 executor.cpp:176] Received SHUTDOWN event
I0312 09:32:58.404158  1796 executor.cpp:758] Shutting down
I0312 09:32:58.404158  1796 executor.cpp:868] Sending SIGTERM to process tree 
at pid 8ter.cpp:10214] Updating the state of task 
3e420232-31de-4648-b4b4-1252920c0a9e of framework 
be3975be-cd38-4964-a044-55d4bad06ae6- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0312 09:32:58.398156  2368 slave.cpp:6571] Shutting down executor 
'3e420232-31de-4648-b4b4-1252920c0a9e' of framework 
be3975be-cd38-4964-a044-55d4bad06ae6- at executor(1)@10.3.1.11:49204
I0312 09:32:58.400156  8644 slave.cpp:924] Agent terminating
W0312 09:32:58.401181  8644 slave.cpp:3874] Ignoring shutdown framework 
be3975be-cd38-4964-a044-55d4bad06ae6- because it is terminating
I0312 09:32:58.401181  3256 master.cpp:10313] Removing task 
3e420232-31de-4648-b4b4-1252920c0a9e with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework be3975be-cd38-4964-a044-55d4bad06ae6- on 
agent be3975be-cd38-4964-a044-55d4bad06ae6-S0 at slave(398)@10.3.1.11:49183 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0312 09:32:58.405156  6640 containerizer.cpp:2338] Destroying container 
3927526f-5e88-428d-b318-2e4344e8bf1e in RUNNING state
I0312 09:32:58.405156  3256 master.cpp:1288] Agent 
be3975be-cd38-4964-a044-55d4bad06ae6-S0 at slave(398)@10.3.1.11:49183 
(build-srv-03.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0312 09:32:58.406154  8688 hierarchical.cpp:344] Removed framework 
be3975be-cd38-4964-a044-55d4bad06ae6-
I0312 09:32:58.406154  6640 containerizer.cpp:2952] Transitioning the state of 
container 3927526f-5e88-428d-b318-2e4344e8bf1e from RUNNING to DESTROYING
I0312 09:32:58.406154  3256 master.cpp:3258] Disconnecting 

Re: Review Request 66000: Update enforce_container_disk_quota documentation to include disk/xfs.

[Harold Dost]

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

(Updated March 12, 2018, 8:28 a.m.)


Review request for mesos and James Peach.


Repository: mesos


Description
---

Update enforce_container_disk_quota documentation to include disk/xfs.


Diffs
-

  src/slave/flags.cpp 943aaaf58b5f36555f0902019b8c5c6522ab7afc 


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


Testing (updated)
---

None needed, since this is just a documentation update


Thanks,

Harold Dost

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 391-487 (patched)
> > 
> >
> > It looks like we can avoid this logic since protobuf's JSON conversion 
> > allows the protobuf key types (bool, integers, strings) to be converted 
> > from JSON strings. This means we could just recurse for both key and value 
> > here:
> > 
> > ```
> > // Some comment explaining what map is equivalent to with
> > // a reference to the google documentation.
> > google::protobuf::Message* entry =
> >   reflection->AddMessage(message, field);
> > 
> > const google::protobuf::FieldDescriptor* key_field =
> >   entry->GetDescriptor()->FindFieldByNumber(1);
> >   
> > // Maybe passing just 'key' works due to implicit 
> > construction?
> > // TODO(...): This copies the key, consider avoiding this.
> > Try apply =
> >   boost::apply_visitor(Parser(entry, key_field), 
> > JSON::String(key));
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> >   
> > const google::protobuf::FieldDescriptor* value_field =
> >   entry->GetDescriptor()->FindFieldByNumber(2);
> >   
> > Try apply =
> >   boost::apply_visitor(Parser(entry, value_field), value);
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> > ```
> > 
> > Now, to make this simplification work, we need to update our JSON 
> > conversion in a separate patch to allow bools and integers to be parsed 
> > from JSON strings to match google's logic for conversion:
> > 
> > 
> > https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203
> > 
> > Documentation here: 
> > https://developers.google.com/protocol-buffers/docs/proto3#json
> 
> Qian Zhang wrote:
> > // Maybe passing just 'key' works due to implicit construction?
> 
> We cannot pass `key` or even `JSON::String` to `boost::apply_visitor()` 
> because it cannot pass compilation:
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'std::__1::basic_string'
> ```
> or
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'JSON::String'
> ```
> That is because `std::string` and `JSON::String` do not have a member 
> `apply_visitor`, so I think we have to use `JSON::Value` like this:
> ```
> google::protobuf::Message* entry =
>   reflection->AddMessage(message, field);
> 
> const google::protobuf::FieldDescriptor* key_field =
>   entry->GetDescriptor()->FindFieldByNumber(1);
> 
> JSON::Value key(name);
> 
> Try apply =
>   boost::apply_visitor(Parser(entry, key_field), key);
> 
> if (apply.isError()) {
>   return Error(apply.error());
> }
> 
> const google::protobuf::FieldDescriptor* value_field =
>   entry->GetDescriptor()->FindFieldByNumber(2);
> 
> apply = boost::apply_visitor(Parser(entry, value_field), 
> value);
> if (apply.isError()) {
>   return Error(apply.error());
> }
> ```
> 
> > we need to update our JSON conversion in a separate patch to allow 
> bools and integers to be parsed from JSON strings to match google's logic for 
> conversion
> 
> Did you mean we should improve the method below by adding more cases for 
> converting `JSON::String` to bools and integers?
> ```
>   Try operator()(const JSON::String& string) const
>   {
> switch (field->type()) {
>   case google::protobuf::FieldDescriptor::TYPE_STRING:
>   ...
> ```
> If so, then that means moving the code here (see below) into the above 
> method. But I think those code are specific for map support, however 
> `Try operator()(const JSON::String& string) const` is a generic 
> method for JSON string conversion, so I would still like to keep those code 
> as where they are now in this patch (i.e., the code path to handle map).
> ```
>   case google::protobuf::FieldDescriptor::TYPE_INT64:
>   case google::protobuf::FieldDescriptor::TYPE_SINT64:
>   case google::protobuf::FieldDescriptor::TYPE_SFIXED64:
>   {
> 

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

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

(Updated March 12, 2018, 3:15 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, and 
Zhitao Li.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with 3.0.0+ protobuf compiler, see the following discussion for
details:

https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4

We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228. This patch adds map support in the json <-> protobuf
conversion in stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 


Diff: https://reviews.apache.org/r/59987/diff/8/

Changes: https://reviews.apache.org/r/59987/diff/7-8/


Testing
---


Thanks,

Qian Zhang