Re: Review Request 66649: Added pb2gen.sh for generating python protobuf bindings.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66649]

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 June 7, 2018, 1:12 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66649/
> ---
> 
> (Updated June 7, 2018, 1:12 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current state of support for python in protoc has two serious
> issues:
> 1. The `__init__.py` files necessary to mark a directory as a python
> package aren't generated.
> 2. The import paths in each of the generated .py files do not reflect
> the `--python_path` option passed to `protoc`.
> 
> This results in incomplete code generated, preventing it from being used
> out of the box. To address this issue, we're adding a `pb2gen.sh` script
> to do end-to-end code generation for python protobuf bindings: the
> script generates the bindings based on what's in the `include` dir, then
> postprocesses the generated code to add proper import paths and the
> `__init__.py` files.
> 
> 
> Diffs
> -
> 
>   src/python/.gitignore fee529dccf57fd24036733f4b470b7b9865a918c 
>   src/python/lib/pb2gen.sh PRE-CREATION 
>   src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
>   support/pylint.config af25dd90cb2d467c688ea4b060dc4640040a068b 
> 
> 
> Diff: https://reviews.apache.org/r/66649/diff/6/
> 
> 
> Testing
> ---
> 
> 1. under `src/python/lib`, run `pb2gen.sh`
> 2. initialize and activate virtualenv: `virtualenv env && . env/bin/activate`
> 3. install reqs: `pip install -r requirements.in`
> 4. try to import modules from generated python code: `python -c 'from 
> mesos.pb2.mesos.v1.master import master_pb2'`
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 66649: Added pb2gen.sh for generating python protobuf bindings.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66649 was successfully built and tested.

Reviews applied: `['66649']`

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

- Mesos Reviewbot Windows


On June 6, 2018, 6:12 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66649/
> ---
> 
> (Updated June 6, 2018, 6:12 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current state of support for python in protoc has two serious
> issues:
> 1. The `__init__.py` files necessary to mark a directory as a python
> package aren't generated.
> 2. The import paths in each of the generated .py files do not reflect
> the `--python_path` option passed to `protoc`.
> 
> This results in incomplete code generated, preventing it from being used
> out of the box. To address this issue, we're adding a `pb2gen.sh` script
> to do end-to-end code generation for python protobuf bindings: the
> script generates the bindings based on what's in the `include` dir, then
> postprocesses the generated code to add proper import paths and the
> `__init__.py` files.
> 
> 
> Diffs
> -
> 
>   src/python/.gitignore fee529dccf57fd24036733f4b470b7b9865a918c 
>   src/python/lib/pb2gen.sh PRE-CREATION 
>   src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
>   support/pylint.config af25dd90cb2d467c688ea4b060dc4640040a068b 
> 
> 
> Diff: https://reviews.apache.org/r/66649/diff/6/
> 
> 
> Testing
> ---
> 
> 1. under `src/python/lib`, run `pb2gen.sh`
> 2. initialize and activate virtualenv: `virtualenv env && . env/bin/activate`
> 3. install reqs: `pip install -r requirements.in`
> 4. try to import modules from generated python code: `python -c 'from 
> mesos.pb2.mesos.v1.master import master_pb2'`
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 66649: Added pb2gen.sh for generating python protobuf bindings.

[Eric Chung]

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

(Updated June 7, 2018, 1:12 a.m.)


Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao Li.


Repository: mesos


Description
---

The current state of support for python in protoc has two serious
issues:
1. The `__init__.py` files necessary to mark a directory as a python
package aren't generated.
2. The import paths in each of the generated .py files do not reflect
the `--python_path` option passed to `protoc`.

This results in incomplete code generated, preventing it from being used
out of the box. To address this issue, we're adding a `pb2gen.sh` script
to do end-to-end code generation for python protobuf bindings: the
script generates the bindings based on what's in the `include` dir, then
postprocesses the generated code to add proper import paths and the
`__init__.py` files.


Diffs (updated)
-

  src/python/.gitignore fee529dccf57fd24036733f4b470b7b9865a918c 
  src/python/lib/pb2gen.sh PRE-CREATION 
  src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
  support/pylint.config af25dd90cb2d467c688ea4b060dc4640040a068b 


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

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


Testing
---

1. under `src/python/lib`, run `pb2gen.sh`
2. initialize and activate virtualenv: `virtualenv env && . env/bin/activate`
3. install reqs: `pip install -r requirements.in`
4. try to import modules from generated python code: `python -c 'from 
mesos.pb2.mesos.v1.master import master_pb2'`


Thanks,

Eric Chung

Re: Review Request 67421: Rewrote the `ROOT_BusyMountPoint` test to reflect updated behavior.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67264, 67423, 67421]

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 June 5, 2018, 5:08 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67421/
> ---
> 
> (Updated June 5, 2018, 5:08 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8830
> https://issues.apache.org/jira/browse/MESOS-8830
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current `ROOT_BusyMountPoint` test would fail because we added
> support for unmounting dangling mount points in directory to gc. This
> patch rewrote this test to reflect that after unmounting, the gc
> succeeded, directory was gone and metrics were correctly reported.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031 
> 
> 
> Diff: https://reviews.apache.org/r/67421/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67381: Added `cgroups/all` into CHANGELOG and upgrades.md.

[Gilbert Song]

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


Fix it, then Ship it!





docs/upgrades.md
Lines 451 (patched)


s/causes/allows/g?

Could you also emphasize that the default behavior does not change?


- Gilbert Song


On May 31, 2018, 8:28 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67381/
> ---
> 
> (Updated May 31, 2018, 8:28 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-7691
> https://issues.apache.org/jira/browse/MESOS-7691
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `cgroups/all` into CHANGELOG and upgrades.md.
> 
> 
> Diffs
> -
> 
>   CHANGELOG bdcf8e33b95f4bea04b64417aa73df44dd456afe 
>   docs/upgrades.md 18632edd20fcf943c6e7147aca3fec5b5521f14f 
> 
> 
> Diff: https://reviews.apache.org/r/67381/diff/2/
> 
> 
> Testing
> ---
> 
> Not a code change.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 67358: Added a test `CgroupsIsolatorTest.ROOT_CGROUPS_PERF_AutoLoadSubsystems`.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On May 30, 2018, 6:10 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67358/
> ---
> 
> (Updated May 30, 2018, 6:10 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-7691
> https://issues.apache.org/jira/browse/MESOS-7691
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `CgroupsIsolatorTest.ROOT_CGROUPS_PERF_AutoLoadSubsystems`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 231e9588c0d831c05a1d84f35f0f68105900789c 
> 
> 
> Diff: https://reviews.apache.org/r/67358/diff/2/
> 
> 
> Testing
> ---
> 
> Ran this test repeatedly
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 67343: Automatically loaded all the local enabled cgroups subsystems.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On May 31, 2018, 8:21 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67343/
> ---
> 
> (Updated May 31, 2018, 8:21 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-7691
> https://issues.apache.org/jira/browse/MESOS-7691
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When `cgroups/all` is specified in the agent flag `--isolation`, we
> will automatically load all the local enabled cgroups subsystems in
> the cgroups isolator with one exception: the `perf_event` subsystem,
> we will only automatically load it when the agent flag `--perf_events`
> is specified, otherwise it will be skipped.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 17a1a3762b2012ff875e4da9c9d4622514e48051 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 215e32461e851668247f9fae62aa656f5dd5e245 
> 
> 
> Diff: https://reviews.apache.org/r/67343/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 67342: Added `cgroups/all` into the agent flag `--isolation`.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On May 31, 2018, 8:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67342/
> ---
> 
> (Updated May 31, 2018, 8:20 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-7691
> https://issues.apache.org/jira/browse/MESOS-7691
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `cgroups/all` into the agent flag `--isolation`.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md e0aaf2c15023d1cb24e16b02f948b3d3848be57a 
>   src/slave/flags.cpp 23d9bb1ca9bc7451afae69f39c25605660612c2e 
> 
> 
> Diff: https://reviews.apache.org/r/67342/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 67476: Added guideline for contributions modifying Makefile.am.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67476]

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 June 6, 2018, 1:56 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67476/
> ---
> 
> (Updated June 6, 2018, 1:56 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While rare, contributions which modify any `Makefile.am` (usually
> because they add or remove files) tend to break the `distcheck` target
> as well as the cmake build system.
> 
> This patch adds a section so contributors are aware that they need to
> check that all build system and all make targets work when modifying
> `Makefile.am` files.
> 
> 
> Diffs
> -
> 
>   docs/advanced-contribution.md f95149120d39b9517ace3ec0ac37d82d3d3a77c4 
> 
> 
> Diff: https://reviews.apache.org/r/67476/diff/1/
> 
> 
> Testing
> ---
> 
> Not a code related change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67478: Addition to contributors.yaml in peparation for submitting an actual patch

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67478 was successfully built and tested.

Reviews applied: `['67478']`

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

- Mesos Reviewbot Windows


On June 6, 2018, 6:03 p.m., Kjetil Joergensen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67478/
> ---
> 
> (Updated June 6, 2018, 6:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addition to contributors.yaml in preparation for submitting an actual patch
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 24fc34b26 
> 
> 
> Diff: https://reviews.apache.org/r/67478/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kjetil Joergensen
> 
>

Re: Review Request 66652: Renamed local_puller to image_tar_puller.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66559, 66650, 66651, 66561, 66562, 66652]

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 June 6, 2018, 8:44 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66652/
> ---
> 
> (Updated June 6, 2018, 8:44 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed local_puller to image_tar_puller.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f86884de2beb946c8bfc2bb8260e09a9c98ce625 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 509be63635e21e48a62deaf7c545575d2d8221b3 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 3cb1a7ed251cf22c8609b39685da2d86b2770766 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> b703b827a9a00b1c335cd1773c4d3e048eb16d66 
> 
> 
> Diff: https://reviews.apache.org/r/66652/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 67478: Addition to contributors.yaml in peparation for submitting an actual patch

[Kjetil Joergensen]

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

Review request for mesos.


Repository: mesos


Description
---

Addition to contributors.yaml in preparation for submitting an actual patch


Diffs
-

  docs/contributors.yaml 24fc34b26 


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


Testing
---


Thanks,

Kjetil Joergensen

Re: Review Request 67421: Rewrote the `ROOT_BusyMountPoint` test to reflect updated behavior.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67421 was successfully built and tested.

Reviews applied: `['67264', '67423', '67421']`

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

- Mesos Reviewbot Windows


On June 5, 2018, 5:08 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67421/
> ---
> 
> (Updated June 5, 2018, 5:08 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8830
> https://issues.apache.org/jira/browse/MESOS-8830
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current `ROOT_BusyMountPoint` test would fail because we added
> support for unmounting dangling mount points in directory to gc. This
> patch rewrote this test to reflect that after unmounting, the gc
> succeeded, directory was gone and metrics were correctly reported.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031 
> 
> 
> Diff: https://reviews.apache.org/r/67421/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66561: Supported hdfs fetching in local puller.

[Gilbert Song]

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

(Updated June 6, 2018, 10:14 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Supported hdfs fetching in local puller.


Diffs (updated)
-

  docs/configuration/agent.md e0aaf2c15023d1cb24e16b02f948b3d3848be57a 
  src/hdfs/hdfs.hpp 716d13ff905e937f991f0a997e4d3cdca3b6e521 
  src/hdfs/hdfs.cpp 726925fbe2cbca1fbffb213eebb8de0cca815174 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
509be63635e21e48a62deaf7c545575d2d8221b3 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
d7d8987d493a37d20f32ddd254dc0c3b15159951 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
f6b8f394ee83d15b74bb0a100d768b9b32235734 
  src/slave/flags.cpp 23d9bb1ca9bc7451afae69f39c25605660612c2e 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 67464: Fixed `python3/mesos-style.py`.

[Armand Grillet]

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


Ship it!




Ship It!

- Armand Grillet


On June 5, 2018, 11:15 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67464/
> ---
> 
> (Updated June 5, 2018, 11:15 p.m.)
> 
> 
> Review request for mesos and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `cpplint.py` file was made to be single-source compatible with
> both Python 2 and Python 3, but `mesos-style.py` was erroneously
> changed to look for a copy of it under `support/python3`.
> 
> 
> Diffs
> -
> 
>   support/python3/mesos-style.py 6f9b9d4611c6501f8a2dbda4bfe08cffaadc6d86 
> 
> 
> Diff: https://reviews.apache.org/r/67464/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67476: Added guideline for contributions modifying Makefile.am.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67476 was successfully built and tested.

Reviews applied: `['67476']`

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

- Mesos Reviewbot Windows


On June 6, 2018, 1:56 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67476/
> ---
> 
> (Updated June 6, 2018, 1:56 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While rare, contributions which modify any `Makefile.am` (usually
> because they add or remove files) tend to break the `distcheck` target
> as well as the cmake build system.
> 
> This patch adds a section so contributors are aware that they need to
> check that all build system and all make targets work when modifying
> `Makefile.am` files.
> 
> 
> Diffs
> -
> 
>   docs/advanced-contribution.md f95149120d39b9517ace3ec0ac37d82d3d3a77c4 
> 
> 
> Diff: https://reviews.apache.org/r/67476/diff/1/
> 
> 
> Testing
> ---
> 
> Not a code related change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 66561: Supported hdfs fetching in local puller.

[Gilbert Song]

> On June 6, 2018, 8:57 a.m., Qian Zhang wrote:
> > docs/configuration/agent.md
> > Line 626 (original), 626 (patched)
> > 
> >
> > s/a HDFS/an HDFS/

+1


> On June 6, 2018, 8:57 a.m., Qian Zhang wrote:
> > docs/configuration/agent.md
> > Line 627 (original), 627 (patched)
> > 
> >
> > It seems "). Note " is missed between "" and "that".

good catch


> On June 6, 2018, 8:57 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp
> > Lines 107-112 (patched)
> > 
> >
> > Can we just merge these 4 lines into `return HDFS::parse(uri)`?

dang..my mistake


- Gilbert


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


On June 6, 2018, 1:43 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66561/
> ---
> 
> (Updated June 6, 2018, 1:43 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported hdfs fetching in local puller.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md e0aaf2c15023d1cb24e16b02f948b3d3848be57a 
>   src/hdfs/hdfs.hpp 716d13ff905e937f991f0a997e4d3cdca3b6e521 
>   src/hdfs/hdfs.cpp 726925fbe2cbca1fbffb213eebb8de0cca815174 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 509be63635e21e48a62deaf7c545575d2d8221b3 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> f6b8f394ee83d15b74bb0a100d768b9b32235734 
>   src/slave/flags.cpp 23d9bb1ca9bc7451afae69f39c25605660612c2e 
> 
> 
> Diff: https://reviews.apache.org/r/66561/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66561: Supported hdfs fetching in local puller.

[Gilbert Song]

> On April 18, 2018, 2:48 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp
> > Lines 133 (patched)
> > 
> >
> > In which case can `host.empty()` be `true`?

E.G., hdfs:///var/lib/mesos/store/docker/


> On April 18, 2018, 2:48 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp
> > Lines 232-235 (patched)
> > 
> >
> > Should we change `Option()` and `Option()` to `None()`?
> > 
> > Or we could change it to something like:
> > ```
> > URI uri = hdfsUri.get();
> > uri.set_path(paths::getImageArchiveTarPath(hdfsUri->path(), image));
> > ```

it wouldn't compile with None(). let's use the second option you provided.


> On April 18, 2018, 2:48 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp
> > Lines 39-40 (original), 39-41 (patched)
> > 
> >
> > 
> > [Image::Docker::name](https://github.com/apache/mesos/blob/1.5.0/include/mesos/v1/mesos.proto#L2696:L2700)
> >  can already represent docker registry, did you mean changing it in future?

I meant an optional URI, rephased it.


> On April 18, 2018, 2:48 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/docker/store.cpp
> > Lines 148 (patched)
> > 
> >
> > Just curious, I see this flag was introduced in Mesos long time ago, 
> > but I do not see this flag set in any Mesos code before, so this is the 
> > first time that we use it in Mesos?

yea, it was not consumed previously. the mesos fetcher rely on env var.


- Gilbert


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


On June 6, 2018, 1:43 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66561/
> ---
> 
> (Updated June 6, 2018, 1:43 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported hdfs fetching in local puller.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md e0aaf2c15023d1cb24e16b02f948b3d3848be57a 
>   src/hdfs/hdfs.hpp 716d13ff905e937f991f0a997e4d3cdca3b6e521 
>   src/hdfs/hdfs.cpp 726925fbe2cbca1fbffb213eebb8de0cca815174 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 509be63635e21e48a62deaf7c545575d2d8221b3 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> f6b8f394ee83d15b74bb0a100d768b9b32235734 
>   src/slave/flags.cpp 23d9bb1ca9bc7451afae69f39c25605660612c2e 
> 
> 
> Diff: https://reviews.apache.org/r/66561/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66562: Added test for local puller hdfs uri fetcher plugin.

[Qian Zhang]

> On April 18, 2018, 9:24 p.m., Qian Zhang wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp
> > Lines 493-517 (patched)
> > 
> >
> > Can you use raw string literal to avoid escaping?
> 
> Gilbert Song wrote:
> Considering all `mockHadoopScript` was written in this way. I would 
> suggest follow the style for now and refactor together in a separate patch.

Agree.


- Qian


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


On June 6, 2018, 4:43 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66562/
> ---
> 
> (Updated June 6, 2018, 4:43 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for local puller hdfs uri fetcher plugin.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 3cb1a7ed251cf22c8609b39685da2d86b2770766 
> 
> 
> Diff: https://reviews.apache.org/r/66562/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66561: Supported hdfs fetching in local puller.

[Qian Zhang]

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


Fix it, then Ship it!





docs/configuration/agent.md
Line 626 (original), 626 (patched)


s/a HDFS/an HDFS/



docs/configuration/agent.md
Line 627 (original), 627 (patched)


It seems "). Note " is missed between "" and "that".



src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp
Lines 107-112 (patched)


Can we just merge these 4 lines into `return HDFS::parse(uri)`?



src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp
Lines 128 (patched)


Suggest to change "Failed to parse URI: " to "Failed to parse the agent 
flag `--docker_registry`: "



src/slave/containerizer/mesos/provisioner/docker/puller.cpp
Lines 44 (patched)


s/either pulling from/pulling from either/



src/slave/flags.cpp
Line 205 (original), 205 (patched)


Ditto.



src/slave/flags.cpp
Lines 206 (patched)


Ditto.


- Qian Zhang


On June 6, 2018, 4:43 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66561/
> ---
> 
> (Updated June 6, 2018, 4:43 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported hdfs fetching in local puller.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md e0aaf2c15023d1cb24e16b02f948b3d3848be57a 
>   src/hdfs/hdfs.hpp 716d13ff905e937f991f0a997e4d3cdca3b6e521 
>   src/hdfs/hdfs.cpp 726925fbe2cbca1fbffb213eebb8de0cca815174 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 509be63635e21e48a62deaf7c545575d2d8221b3 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> f6b8f394ee83d15b74bb0a100d768b9b32235734 
>   src/slave/flags.cpp 23d9bb1ca9bc7451afae69f39c25605660612c2e 
> 
> 
> Diff: https://reviews.apache.org/r/66561/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67357 was successfully built and tested.

Reviews applied: `['67357']`

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

- Mesos Reviewbot Windows


On June 6, 2018, 1:45 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 1:45 p.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/4/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67472: Fixed CppLinter in python3/mesos-style.py to use correct linter.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67472]

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 June 6, 2018, 7:41 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67472/
> ---
> 
> (Updated June 6, 2018, 7:41 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed CppLinter in python3/mesos-style.py to use correct linter.
> 
> 
> Diffs
> -
> 
>   support/python3/mesos-style.py 6f9b9d4611c6501f8a2dbda4bfe08cffaadc6d86 
> 
> 
> Diff: https://reviews.apache.org/r/67472/diff/1/
> 
> 
> Testing
> ---
> 
> Created a commit updating one C++ file, then:
> 
> ```
> apache-mesos (MESOS-8979) $ git commit -m "Test."
> Congratulations! You have Python 3 installed correctly.
> Please start using the scripts in `support/python3`.
> Please also set the environment variable `MESOS_SUPPORT_PYTHON` to `3`
> so that the Git hooks use the Python 3 scripts.
> Checking 1 C++ file
> Done processing src/slave/constants.cpp
> Total errors found: 0
> Total errors found: 0
> No JavaScript files to lint
> No Python files to lint
> Congratulations! You have Python 3 installed correctly.
> Please start using the scripts in `support/python3`.
> Please also set the environment variable `MESOS_SUPPORT_PYTHON` to `3`
> so that the Git hooks use the Python 3 scripts.
> [MESOS-8979 b640bb182] Test.
>  1 file changed, 1 insertion(+)
> apache-mesos (MESOS-8979) $ export MESOS_SUPPORT_PYTHON=3
> apache-mesos (MESOS-8979) $ git commit --amend
> Checking 1 C++ file
> Done processing src/slave/constants.cpp
> Total errors found: 0
> Total errors found: 0
> No JavaScript files to lint
> No Python files to lint
> [MESOS-8979 3c596291f] Test.
>  Date: Wed Jun 6 09:38:59 2018 +0200
>  1 file changed, 1 insertion(+)
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 67425: [WIP] Implement a JWK Set parser converting JWKs into RSA keys.

[Alexander Rojas]

> On June 4, 2018, 4:04 p.m., Alexander Rojas wrote:
> > Before going into detail with this, last patchset we forgot to add certain 
> > files to the install target wich ended up breaking tarball generation. 
> > Mostly a failure of your reviewers (myself included). Since you are adding 
> > new files, could you please make sure it works with `make distcheck` and 
> > also make sure of updating the `cmake` build (that should be a rule of 
> > thumb whenever one modifies the `Makefile.am`).
> 
> Clement Michaud wrote:
> Yes, sure. I did not know I had to run those commands and did not get any 
> failure when not doing so. I will make sure to run them from now on. Sorry 
> for that.
> It would be good to add it in the test workflow though, don't you think?
> 
> Thanks.
> Clément.
> 
> Alexander Rojas wrote:
> No worries, I myself have messed up with it… so no need to be sorry. All 
> your reviewers and shepherd forgot about it. I would try to get that into the 
> contributing.

Done, you can help reviewing [r/67476/](https://reviews.apache.org/r/67476/)


- Alexander


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


On June 4, 2018, 3:27 p.m., Clement Michaud wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> ---
> 
> (Updated June 4, 2018, 3:27 p.m.)
> 
> 
> Review request for mesos and Alexander Rojas.
> 
> 
> Bugs: MESOS-8974
> https://issues.apache.org/jira/browse/MESOS-8974
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
> 
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
> 
> The parser only supports parsing of  RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819 
>   3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION 
>   3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67425/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Clement Michaud
> 
>

Review Request 67476: Added guideline for contributions modifying Makefile.am.

[Alexander Rojas]

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

Review request for mesos and Greg Mann.


Repository: mesos


Description
---

While rare, contributions which modify any `Makefile.am` (usually
because they add or remove files) tend to break the `distcheck` target
as well as the cmake build system.

This patch adds a section so contributors are aware that they need to
check that all build system and all make targets work when modifying
`Makefile.am` files.


Diffs
-

  docs/advanced-contribution.md f95149120d39b9517ace3ec0ac37d82d3d3a77c4 


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


Testing
---

Not a code related change.


Thanks,

Alexander Rojas

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Alexander Rojas]

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

(Updated June 6, 2018, 3:34 p.m.)


Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

A vulnerability in our JWT implementation allows an unauthenticated
remote attacker to execute to execute timing attacks [1].

This patch removes the vulnerability by adding a constant time
comparison of hashes, where the whole message is visited during
the comparison instead of returning at the first failure.

[1] https://codahale.com/a-lesson-in-timing-attacks/


Diffs (updated)
-

  3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 


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

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


Testing
---

```sh
make check
```


Thanks,

Alexander Rojas

Re: Review Request 67472: Fixed CppLinter in python3/mesos-style.py to use correct linter.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On June 6, 2018, 9:41 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67472/
> ---
> 
> (Updated June 6, 2018, 9:41 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed CppLinter in python3/mesos-style.py to use correct linter.
> 
> 
> Diffs
> -
> 
>   support/python3/mesos-style.py 6f9b9d4611c6501f8a2dbda4bfe08cffaadc6d86 
> 
> 
> Diff: https://reviews.apache.org/r/67472/diff/1/
> 
> 
> Testing
> ---
> 
> Created a commit updating one C++ file, then:
> 
> ```
> apache-mesos (MESOS-8979) $ git commit -m "Test."
> Congratulations! You have Python 3 installed correctly.
> Please start using the scripts in `support/python3`.
> Please also set the environment variable `MESOS_SUPPORT_PYTHON` to `3`
> so that the Git hooks use the Python 3 scripts.
> Checking 1 C++ file
> Done processing src/slave/constants.cpp
> Total errors found: 0
> Total errors found: 0
> No JavaScript files to lint
> No Python files to lint
> Congratulations! You have Python 3 installed correctly.
> Please start using the scripts in `support/python3`.
> Please also set the environment variable `MESOS_SUPPORT_PYTHON` to `3`
> so that the Git hooks use the Python 3 scripts.
> [MESOS-8979 b640bb182] Test.
>  1 file changed, 1 insertion(+)
> apache-mesos (MESOS-8979) $ export MESOS_SUPPORT_PYTHON=3
> apache-mesos (MESOS-8979) $ git commit --amend
> Checking 1 C++ file
> Done processing src/slave/constants.cpp
> Total errors found: 0
> Total errors found: 0
> No JavaScript files to lint
> No Python files to lint
> [MESOS-8979 3c596291f] Test.
>  Date: Wed Jun 6 09:38:59 2018 +0200
>  1 file changed, 1 insertion(+)
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Alexander Rukletsov]

> On June 6, 2018, 11:58 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 213 (patched)
> > 
> >
> > To avoid conversions and integral promotions, does it make sense to say 
> > here
> > ```
> > string::value_type valid = 0;
> > ```
> > ?
> 
> Benno Evers wrote:
> I would recommend against that, as it would turn `valid` back into a 
> signed type and even obfuscate that fact. As I said before, it's good 
> practice to only do bitwise operation on unsigned types.
> 
> For example (admittedly, its a bit far-fetched, but bear with me), 
> imagine a system using ones complement. Two different characters could xor to 
> produce a negative zero, and implementations are permitted to convert that to 
> positive zero, producing a false positive.

Discussed this offline with Benno and Alexander. If `char ^ char` produces `0`, 
during integer promotion it is padded with `0`s and `OR`d with unsigned 
correctly. If `char ^ char` does not produce `0`, it can be padded with either 
`1`s or `0`s, but we don't really care since the result of `OR` with unsigned 
will be non-zero. The code in the current version seems to be correct.


- Alexander


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


On June 6, 2018, 8:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 8:54 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/3/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Alexander Rojas]

> On June 6, 2018, 12:26 p.m., Mesos Reviewbot Windows wrote:
> > FAIL: Some of the unit tests failed. Please check the relevant logs.
> > 
> > Reviews applied: `['67357']`
> > 
> > Failed command: `Start-MesosCITesting`
> > 
> > All the build artifacts available at: 
> > http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357
> > 
> > Relevant logs:
> > 
> > - 
> > [mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stdout.log):
> > 
> > ```
> > [ RUN  ] SlaveTest.ResourceVersions
> > [   OK ] SlaveTest.ResourceVersions (214 ms)
> > [ RUN  ] SlaveTest.ReconfigurationPolicy
> > [   OK ] SlaveTest.ReconfigurationPolicy (317 ms)
> > [ RUN  ] SlaveTest.ResourceProviderReconciliation
> > [   OK ] SlaveTest.ResourceProviderReconciliation (358 ms)
> > [ RUN  ] SlaveTest.RunTaskResourceVersions
> > [   OK ] SlaveTest.RunTaskResourceVersions (306 ms)
> > [--] 83 tests from SlaveTest (70519 ms total)
> > 
> > [--] 3 tests from SlaveStateTest
> > [ RUN  ] SlaveStateTest.CheckpointString
> > [   OK ] SlaveStateTest.CheckpointString (4 ms)
> > [ RUN  ] SlaveStateTest.CheckpointProtobufMessage
> > [   OK ] SlaveStateTest.CheckpointProtobufMessage (9 ms)
> > [ RUN  ] SlaveStateTest.CheckpointRepeatedProtobufMessages
> > [   OK ] SlaveStateTest.CheckpointRepeatedProtobufMessages (10 ms)
> > [--] 3 tests from SlaveStateTest (25 ms total)
> > 
> > [--] 30 tests from SlaveRecoveryTest/0, where TypeParam = class 
> > mesos::internal::slave::MesosContainerizer
> > [ RUN  ] SlaveRecoveryTest/0.RecoverSlaveState
> > [   OK ] SlaveRecoveryTest/0.RecoverSlaveState (1573 ms)
> > [ RUN  ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager
> > [   OK ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager (3328 ms)
> > [ RUN  ] SlaveRecoveryTest/0.ReconnectExecutor
> > [   OK ] SlaveRecoveryTest/0.ReconnectExecutor (3707 ms)
> > [ RUN  ] SlaveRecoveryTest/0.ReconnectExecutorRetry
> > [   OK ] SlaveRecoveryTest/0.ReconnectExecutorRetry (1205 ms)
> > [ RUN  ] SlaveRecoveryTest/0.PingTimeoutDuringRecovery
> > ```
> > 
> > - 
> > [mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stderr.log):
> > 
> > ```
> > @   7FF71117D888  
> > std::invoke<,process::Future
> >  >,process::ProcessBase *>
> > @   7FF71119257B  
> > lambda::internal::Partial<,process::Future
> >  >,std::_Ph<1> 
> > >::invoke_expand<,std::tuple
> >  >,std::_Ph<1> >,st
> > @   7FF7110C08BA  ) > @   7FF7110F058C  
> > std::_Invoker_functor::_Call,process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *>
> > @   7FF711183EBC  
> > std::invoke,process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *>
> > @   7FF7110C9F21  
> > ),process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *
> > @   7FF711236416  process::ProcessBase 
> > *)>::CallableFn,process::Future
> >  >,std::_Ph<1> > >::operator(
> > @   7FF712C1A25D  process::ProcessBase *)>::operator(
> > @   7FF712ACB2F9  process::ProcessBase::consume
> > @   7FF712C738CA  process::DispatchEvent::consume
> > @   7FF70ECE7B07  process::ProcessBase::serve
> > @   7FF712AD93B0  process::ProcessManager::resume
> > @   7FF712C07371   ?? 
> > @   7FF712B2B130  
> > std::_Invoker_functor::_Call< >
> > @   7FF712B8B8E0  
> > std::invoke< >
> > @   7FF712B4076C  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Execute<0>
> > @   7FF712C5A60A  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Run
> > @   7FF712C45E78  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Go
> > @   7FF712C2C3CD  std::_Pad::_Call_func
> > @   7FFF9BE53428  _register_onexit_function
> > @   7FFF9BE53071  _register_onexit_function
> > @   7FFFB6391FE4  BaseThreadInitThunk
> > @   7FFFB69FF061  RtlUserThreadStart
> > ll containerizers
> > I0606 10:25:26.680230 18356 slave.cpp:7158] Recovering executors
> > I0606 10:25:26.680230 18356 slave.cpp:7182] Sending reconnect request to 
> > executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
> > 62cf792a-dc69-4e3c-b54f-d83f98fb9451- at executor(1)@192.10.1.5:55652
> > I0606 10:25:26.688225 22560 slave.cpp:4984] Received re-registration 
> > message from executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
> > 62cf792a-dc69-4e3c-b54f-d83f98fb9451-
> > I0606 10:25:26.691216 22888 slave.cpp:5901] No pings from master received 
> > within 75secs
> > F0606 10:25:26.692219 22888 slave.cpp:1249] Check failed: state == 
> > DISCONNECTED || state == RUNNING || state == TERMINATING RECOVERING
> > ```
> 
> Alexander Rukletsov wrote:
> WTF is this... Can you please check the JIRA and file an issue if it is a 
> new one?

Re: Review Request 67421: Rewrote the `ROOT_BusyMountPoint` test to reflect updated behavior.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67264, 67423, 67421]

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 June 6, 2018, 12:08 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67421/
> ---
> 
> (Updated June 6, 2018, 12:08 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8830
> https://issues.apache.org/jira/browse/MESOS-8830
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current `ROOT_BusyMountPoint` test would fail because we added
> support for unmounting dangling mount points in directory to gc. This
> patch rewrote this test to reflect that after unmounting, the gc
> succeeded, directory was gone and metrics were correctly reported.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031 
> 
> 
> Diff: https://reviews.apache.org/r/67421/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Benno Evers]

> On May 31, 2018, 4:56 p.m., Benno Evers wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 205 (patched)
> > 
> >
> > To be more precise: "which compares either none of the content or the 
> > whole content of the strings".
> > 
> > (but actually I'm not even sure a comment is needed at all, the 
> > function name kinda speaks for itself)
> 
> Alexander Rojas wrote:
> I agree but the shepherd requested it ;)

Hehe, can't argue with that :D


> On May 31, 2018, 4:56 p.m., Benno Evers wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 215 (patched)
> > 
> >
> > I think its safer to use unsigned types for bit manipulation, otherwise 
> > it's really easy to accidentally trigger undefined behaviour. (i.e. even 
> > here I'm not sure its safe off the top of my head, since the xor could flip 
> > the sign bit of the chars)

Hm, so `left[i] ^ right[i]` is still an xor of signed chars, but since its 
actually safe the way its used here, I guess its up to personal preference if 
you want to add an explicit cast.


- Benno


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


On June 6, 2018, 8:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 8:54 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/3/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Benno Evers]

> On June 6, 2018, 11:58 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 213 (patched)
> > 
> >
> > To avoid conversions and integral promotions, does it make sense to say 
> > here
> > ```
> > string::value_type valid = 0;
> > ```
> > ?

I would recommend against that, as it would turn `valid` back into a signed 
type and even obfuscate that fact. As I said before, it's good practice to only 
do bitwise operation on unsigned types.

For example (admittedly, its a bit far-fetched, but bear with me), imagine a 
system using ones complement. Two different characters could xor to produce a 
negative zero, and implementations are permitted to convert that to positive 
zero, producing a false positive.


- Benno


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


On June 6, 2018, 8:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 8:54 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/3/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Alexander Rukletsov]

> On June 6, 2018, 10:26 a.m., Mesos Reviewbot Windows wrote:
> > FAIL: Some of the unit tests failed. Please check the relevant logs.
> > 
> > Reviews applied: `['67357']`
> > 
> > Failed command: `Start-MesosCITesting`
> > 
> > All the build artifacts available at: 
> > http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357
> > 
> > Relevant logs:
> > 
> > - 
> > [mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stdout.log):
> > 
> > ```
> > [ RUN  ] SlaveTest.ResourceVersions
> > [   OK ] SlaveTest.ResourceVersions (214 ms)
> > [ RUN  ] SlaveTest.ReconfigurationPolicy
> > [   OK ] SlaveTest.ReconfigurationPolicy (317 ms)
> > [ RUN  ] SlaveTest.ResourceProviderReconciliation
> > [   OK ] SlaveTest.ResourceProviderReconciliation (358 ms)
> > [ RUN  ] SlaveTest.RunTaskResourceVersions
> > [   OK ] SlaveTest.RunTaskResourceVersions (306 ms)
> > [--] 83 tests from SlaveTest (70519 ms total)
> > 
> > [--] 3 tests from SlaveStateTest
> > [ RUN  ] SlaveStateTest.CheckpointString
> > [   OK ] SlaveStateTest.CheckpointString (4 ms)
> > [ RUN  ] SlaveStateTest.CheckpointProtobufMessage
> > [   OK ] SlaveStateTest.CheckpointProtobufMessage (9 ms)
> > [ RUN  ] SlaveStateTest.CheckpointRepeatedProtobufMessages
> > [   OK ] SlaveStateTest.CheckpointRepeatedProtobufMessages (10 ms)
> > [--] 3 tests from SlaveStateTest (25 ms total)
> > 
> > [--] 30 tests from SlaveRecoveryTest/0, where TypeParam = class 
> > mesos::internal::slave::MesosContainerizer
> > [ RUN  ] SlaveRecoveryTest/0.RecoverSlaveState
> > [   OK ] SlaveRecoveryTest/0.RecoverSlaveState (1573 ms)
> > [ RUN  ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager
> > [   OK ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager (3328 ms)
> > [ RUN  ] SlaveRecoveryTest/0.ReconnectExecutor
> > [   OK ] SlaveRecoveryTest/0.ReconnectExecutor (3707 ms)
> > [ RUN  ] SlaveRecoveryTest/0.ReconnectExecutorRetry
> > [   OK ] SlaveRecoveryTest/0.ReconnectExecutorRetry (1205 ms)
> > [ RUN  ] SlaveRecoveryTest/0.PingTimeoutDuringRecovery
> > ```
> > 
> > - 
> > [mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stderr.log):
> > 
> > ```
> > @   7FF71117D888  
> > std::invoke<,process::Future
> >  >,process::ProcessBase *>
> > @   7FF71119257B  
> > lambda::internal::Partial<,process::Future
> >  >,std::_Ph<1> 
> > >::invoke_expand<,std::tuple
> >  >,std::_Ph<1> >,st
> > @   7FF7110C08BA  ) > @   7FF7110F058C  
> > std::_Invoker_functor::_Call,process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *>
> > @   7FF711183EBC  
> > std::invoke,process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *>
> > @   7FF7110C9F21  
> > ),process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *
> > @   7FF711236416  process::ProcessBase 
> > *)>::CallableFn,process::Future
> >  >,std::_Ph<1> > >::operator(
> > @   7FF712C1A25D  process::ProcessBase *)>::operator(
> > @   7FF712ACB2F9  process::ProcessBase::consume
> > @   7FF712C738CA  process::DispatchEvent::consume
> > @   7FF70ECE7B07  process::ProcessBase::serve
> > @   7FF712AD93B0  process::ProcessManager::resume
> > @   7FF712C07371   ?? 
> > @   7FF712B2B130  
> > std::_Invoker_functor::_Call< >
> > @   7FF712B8B8E0  
> > std::invoke< >
> > @   7FF712B4076C  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Execute<0>
> > @   7FF712C5A60A  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Run
> > @   7FF712C45E78  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Go
> > @   7FF712C2C3CD  std::_Pad::_Call_func
> > @   7FFF9BE53428  _register_onexit_function
> > @   7FFF9BE53071  _register_onexit_function
> > @   7FFFB6391FE4  BaseThreadInitThunk
> > @   7FFFB69FF061  RtlUserThreadStart
> > ll containerizers
> > I0606 10:25:26.680230 18356 slave.cpp:7158] Recovering executors
> > I0606 10:25:26.680230 18356 slave.cpp:7182] Sending reconnect request to 
> > executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
> > 62cf792a-dc69-4e3c-b54f-d83f98fb9451- at executor(1)@192.10.1.5:55652
> > I0606 10:25:26.688225 22560 slave.cpp:4984] Received re-registration 
> > message from executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
> > 62cf792a-dc69-4e3c-b54f-d83f98fb9451-
> > I0606 10:25:26.691216 22888 slave.cpp:5901] No pings from master received 
> > within 75secs
> > F0606 10:25:26.692219 22888 slave.cpp:1249] Check failed: state == 
> > DISCONNECTED || state == RUNNING || state == TERMINATING RECOVERING
> > ```

WTF is this... Can you please check the JIRA and file an issue if it is a new 
one?


- Alexander

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['67357']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
[ RUN  ] SlaveTest.ResourceVersions
[   OK ] SlaveTest.ResourceVersions (214 ms)
[ RUN  ] SlaveTest.ReconfigurationPolicy
[   OK ] SlaveTest.ReconfigurationPolicy (317 ms)
[ RUN  ] SlaveTest.ResourceProviderReconciliation
[   OK ] SlaveTest.ResourceProviderReconciliation (358 ms)
[ RUN  ] SlaveTest.RunTaskResourceVersions
[   OK ] SlaveTest.RunTaskResourceVersions (306 ms)
[--] 83 tests from SlaveTest (70519 ms total)

[--] 3 tests from SlaveStateTest
[ RUN  ] SlaveStateTest.CheckpointString
[   OK ] SlaveStateTest.CheckpointString (4 ms)
[ RUN  ] SlaveStateTest.CheckpointProtobufMessage
[   OK ] SlaveStateTest.CheckpointProtobufMessage (9 ms)
[ RUN  ] SlaveStateTest.CheckpointRepeatedProtobufMessages
[   OK ] SlaveStateTest.CheckpointRepeatedProtobufMessages (10 ms)
[--] 3 tests from SlaveStateTest (25 ms total)

[--] 30 tests from SlaveRecoveryTest/0, where TypeParam = class 
mesos::internal::slave::MesosContainerizer
[ RUN  ] SlaveRecoveryTest/0.RecoverSlaveState
[   OK ] SlaveRecoveryTest/0.RecoverSlaveState (1573 ms)
[ RUN  ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager
[   OK ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager (3328 ms)
[ RUN  ] SlaveRecoveryTest/0.ReconnectExecutor
[   OK ] SlaveRecoveryTest/0.ReconnectExecutor (3707 ms)
[ RUN  ] SlaveRecoveryTest/0.ReconnectExecutorRetry
[   OK ] SlaveRecoveryTest/0.ReconnectExecutorRetry (1205 ms)
[ RUN  ] SlaveRecoveryTest/0.PingTimeoutDuringRecovery
```

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

```
@   7FF71117D888  
std::invoke<,process::Future
 >,process::ProcessBase *>
@   7FF71119257B  
lambda::internal::Partial<,process::Future
 >,std::_Ph<1> 
>::invoke_expand<,std::tuple
 >,std::_Ph<1> >,st
@   7FF7110C08BA  ),process::Future
 >,std::_Ph<1> >,process::ProcessBase *>
@   7FF711183EBC  
std::invoke,process::Future
 >,std::_Ph<1> >,process::ProcessBase *>
@   7FF7110C9F21  
),process::Future
 >,std::_Ph<1> >,process::ProcessBase *
@   7FF711236416  process::ProcessBase 
*)>::CallableFn,process::Future
 >,std::_Ph<1> > >::operator(
@   7FF712C1A25D  process::ProcessBase *)>::operator(
@   7FF712ACB2F9  process::ProcessBase::consume
@   7FF712C738CA  process::DispatchEvent::consume
@   7FF70ECE7B07  process::ProcessBase::serve
@   7FF712AD93B0  process::ProcessManager::resume
@   7FF712C07371   ?? 
@   7FF712B2B130  
std::_Invoker_functor::_Call< >
@   7FF712B8B8E0  std::invoke< 
>
@   7FF712B4076C  
std::_LaunchPad
 >,std::default_delete > > 
> >::_Execute<0>
@   7FF712C5A60A  
std::_LaunchPad
 >,std::default_delete > > 
> >::_Run
@   7FF712C45E78  
std::_LaunchPad
 >,std::default_delete > > 
> >::_Go
@   7FF712C2C3CD  std::_Pad::_Call_func
@   7FFF9BE53428  _register_onexit_function
@   7FFF9BE53071  _register_onexit_function
@   7FFFB6391FE4  BaseThreadInitThunk
@   7FFFB69FF061  RtlUserThreadStart
ll containerizers
I0606 10:25:26.680230 18356 slave.cpp:7158] Recovering executors
I0606 10:25:26.680230 18356 slave.cpp:7182] Sending reconnect request to 
executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
62cf792a-dc69-4e3c-b54f-d83f98fb9451- at executor(1)@192.10.1.5:55652
I0606 10:25:26.688225 22560 slave.cpp:4984] Received re-registration message 
from executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
62cf792a-dc69-4e3c-b54f-d83f98fb9451-
I0606 10:25:26.691216 22888 slave.cpp:5901] No pings from master received 
within 75secs
F0606 10:25:26.692219 22888 slave.cpp:1249] Check failed: state == DISCONNECTED 
|| state == RUNNING || state == TERMINATING RECOVERING
```

- Mesos Reviewbot Windows


On June 6, 2018, 8:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 8:54 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A 

Re: Review Request 66652: Renamed local_puller to image_tar_puller.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66652 was successfully built and tested.

Reviews applied: `['66559', '66650', '66651', '66561', '66562', '66652']`

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

- Mesos Reviewbot Windows


On June 6, 2018, 8:44 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66652/
> ---
> 
> (Updated June 6, 2018, 8:44 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed local_puller to image_tar_puller.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f86884de2beb946c8bfc2bb8260e09a9c98ce625 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 509be63635e21e48a62deaf7c545575d2d8221b3 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 3cb1a7ed251cf22c8609b39685da2d86b2770766 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> b703b827a9a00b1c335cd1773c4d3e048eb16d66 
> 
> 
> Diff: https://reviews.apache.org/r/66652/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 67463: Fixed type error when using `check_output` in Python 3 scripts.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67463]

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 June 5, 2018, 10:22 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67463/
> ---
> 
> (Updated June 5, 2018, 10:22 p.m.)
> 
> 
> Review request for mesos and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8979
> https://issues.apache.org/jira/browse/MESOS-8979
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed type error when using `check_output` in Python 3 scripts.
> 
> 
> Diffs
> -
> 
>   support/python3/generate-endpoint-help.py 
> 711d3d325acbc1fe9e96d65caaf089727f03904f 
>   support/python3/mesos-gtest-runner.py 
> 153ded2994daad6ffb610866d499603af5a53b0a 
>   support/python3/push-commits.py 82a700418341731833f165a24054f0879648ea92 
> 
> 
> Diff: https://reviews.apache.org/r/67463/diff/1/
> 
> 
> Testing
> ---
> 
> Removed part of the content of `push-commits.py` and run it using Python 3.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66650: Removed an invalid TODO in puller.cpp.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On June 6, 2018, 4:40 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66650/
> ---
> 
> (Updated June 6, 2018, 4:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an invalid TODO in puller.cpp.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d7d8987d493a37d20f32ddd254dc0c3b15159951 
> 
> 
> Diff: https://reviews.apache.org/r/66650/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 67223: Used move constructors for making CSI gRPC calls.

[Benjamin Bannier]

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


Ship it!




This patch needs to be rebased, but LGTM.


src/csi/client.cpp
Line 59 (original), 64 (patched)


Nit: Might as well rewrap like all other function bodies.



src/csi/client.cpp
Line 227 (original), 256 (patched)


Nit: Rewrap?


- Benjamin Bannier


On May 19, 2018, 1:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67223/
> ---
> 
> (Updated May 19, 2018, 1:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Zhitao Li.
> 
> 
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch takes the advantages of the moving constructors of the CSI
> request protobuf messages to avoid copying the messages.
> 
> 
> Diffs
> -
> 
>   src/csi/client.hpp 5d846740f72125efb3654e14d763bd24634367b9 
>   src/csi/client.cpp 559e8057eccce5bf758918c24e5ca8c561af6592 
>   src/resource_provider/storage/provider.cpp 
> 63b5d7e5f10d6ad02b5cd11b119def3b4abf4180 
>   src/tests/csi_client_tests.cpp f5b9eac38a4079cc2873ce2e2de24eaf315e0bc9 
> 
> 
> Diff: https://reviews.apache.org/r/67223/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67157: Refactored the gRPC client runtime wrapper in libprocess.

[Benjamin Bannier]

> On May 25, 2018, 5:12 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/grpc.cpp
> > Lines 70 (patched)
> > 
> >
> > I am slightly worried that we only `join` this thread on `terminate`. 
> > Could we make sure we `join` the thread whenever `looper` gets `reset` or 
> > goes out of scope -- we might be able to e.g., use a custom deleter for 
> > that if the lifetimes are guaranteed to be correct.

Maybe as a simpler solution, let's call `looper.reset()` here and add a 
destructor

Runtime::RuntimeProcess::~RuntimeProcess()
{
  CHECK(!looper);
}

to make sure on the language level (in addition to libprocess `Process` 
semantics) that we never leak this thread.


> On May 25, 2018, 5:12 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/grpc.cpp
> > Lines 76 (patched)
> > 
> >
> > This seems almost like the libprocess equivalent of a throwing 
> > destructor. Could we trigger this e.g., if we fail a test `ASSERT`?
> > 
> > Also see my comment regarding joining whenever `looper` goes out of 
> > scope.
> 
> Chun-Hung Hsiao wrote:
> Given that we never directly call `process::terminate` on a 
> `RuntimeProcess`, but only indirectly terminate the process through 
> `RuntimeProcess::terminate` which also sets `terminating`, this cannot fail 
> at any case, including failed test assertions. I could remove this if this 
> line looks weird to you.

Makes sense, let's leave the assertion in here.

Dropped


- Benjamin


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


On May 18, 2018, 12:23 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> ---
> 
> (Updated May 18, 2018, 12:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
>process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
>managing its lifecycle in `Runtime::Data`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/grpc.hpp 
> 321a46e19c69eafb24012bcef68bb8b0cc6aa436 
>   3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0 
> 
> 
> Diff: https://reviews.apache.org/r/67157/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67472: Fixed CppLinter in python3/mesos-style.py to use correct linter.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67472 was successfully built and tested.

Reviews applied: `['67472']`

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

- Mesos Reviewbot Windows


On June 6, 2018, 3:41 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67472/
> ---
> 
> (Updated June 6, 2018, 3:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed CppLinter in python3/mesos-style.py to use correct linter.
> 
> 
> Diffs
> -
> 
>   support/python3/mesos-style.py 6f9b9d4611c6501f8a2dbda4bfe08cffaadc6d86 
> 
> 
> Diff: https://reviews.apache.org/r/67472/diff/1/
> 
> 
> Testing
> ---
> 
> Created a commit updating one C++ file, then:
> 
> ```
> apache-mesos (MESOS-8979) $ git commit -m "Test."
> Congratulations! You have Python 3 installed correctly.
> Please start using the scripts in `support/python3`.
> Please also set the environment variable `MESOS_SUPPORT_PYTHON` to `3`
> so that the Git hooks use the Python 3 scripts.
> Checking 1 C++ file
> Done processing src/slave/constants.cpp
> Total errors found: 0
> Total errors found: 0
> No JavaScript files to lint
> No Python files to lint
> Congratulations! You have Python 3 installed correctly.
> Please start using the scripts in `support/python3`.
> Please also set the environment variable `MESOS_SUPPORT_PYTHON` to `3`
> so that the Git hooks use the Python 3 scripts.
> [MESOS-8979 b640bb182] Test.
>  1 file changed, 1 insertion(+)
> apache-mesos (MESOS-8979) $ export MESOS_SUPPORT_PYTHON=3
> apache-mesos (MESOS-8979) $ git commit --amend
> Checking 1 C++ file
> Done processing src/slave/constants.cpp
> Total errors found: 0
> Total errors found: 0
> No JavaScript files to lint
> No Python files to lint
> [MESOS-8979 3c596291f] Test.
>  Date: Wed Jun 6 09:38:59 2018 +0200
>  1 file changed, 1 insertion(+)
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 66652: Renamed local_puller to image_tar_puller.

[Gilbert Song]

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

(Updated June 6, 2018, 1:44 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Renamed local_puller to image_tar_puller.


Diffs (updated)
-

  src/CMakeLists.txt f86884de2beb946c8bfc2bb8260e09a9c98ce625 
  src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
509be63635e21e48a62deaf7c545575d2d8221b3 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
d7d8987d493a37d20f32ddd254dc0c3b15159951 
  src/tests/containerizer/provisioner_docker_tests.cpp 
3cb1a7ed251cf22c8609b39685da2d86b2770766 
  src/tests/containerizer/runtime_isolator_tests.cpp 
b703b827a9a00b1c335cd1773c4d3e048eb16d66 


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

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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 66561: Supported hdfs fetching in local puller.

[Gilbert Song]

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

(Updated June 6, 2018, 1:43 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Supported hdfs fetching in local puller.


Diffs (updated)
-

  docs/configuration/agent.md e0aaf2c15023d1cb24e16b02f948b3d3848be57a 
  src/hdfs/hdfs.hpp 716d13ff905e937f991f0a997e4d3cdca3b6e521 
  src/hdfs/hdfs.cpp 726925fbe2cbca1fbffb213eebb8de0cca815174 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
4d2e4973a0d6c99dd3447a158003b4b09e2ba477 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
509be63635e21e48a62deaf7c545575d2d8221b3 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
d7d8987d493a37d20f32ddd254dc0c3b15159951 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
f6b8f394ee83d15b74bb0a100d768b9b32235734 
  src/slave/flags.cpp 23d9bb1ca9bc7451afae69f39c25605660612c2e 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 66562: Added test for local puller hdfs uri fetcher plugin.

[Gilbert Song]

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

(Updated June 6, 2018, 1:43 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Added test for local puller hdfs uri fetcher plugin.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
3cb1a7ed251cf22c8609b39685da2d86b2770766 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 66651: Supported host and port in hdfs constructor.

[Gilbert Song]

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

(Updated June 6, 2018, 1:42 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Supported host and port in hdfs constructor.


Diffs (updated)
-

  src/uri/schemes/hdfs.hpp 46b9055cb5404f90445bbf5ee64354242e7b4ca6 


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

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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 66651: Supported host and port in hdfs constructor.

[Gilbert Song]

> On April 18, 2018, 5:51 a.m., Qian Zhang wrote:
> > Can you add a test in `uri_fetcher_tests.cpp` to verify fetching a remote 
> > file via Hadoop fetcher plugin?

for now, it is hard to find a long existing hdfs:// source to verify network 
connection reliability. I would say simulate in unit test and manual test it.


- Gilbert


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


On April 16, 2018, 10:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66651/
> ---
> 
> (Updated April 16, 2018, 10:32 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported host and port in hdfs constructor.
> 
> 
> Diffs
> -
> 
>   src/uri/schemes/hdfs.hpp 46b9055cb5404f90445bbf5ee64354242e7b4ca6 
> 
> 
> Diff: https://reviews.apache.org/r/66651/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66650: Removed an invalid TODO in puller.cpp.

[Gilbert Song]

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

(Updated June 6, 2018, 1:40 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Removed an invalid TODO in puller.cpp.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
d7d8987d493a37d20f32ddd254dc0c3b15159951 


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

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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 66559: Made agent flag '--hadoop_home' as optional.

[Gilbert Song]

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

(Updated June 6, 2018, 1:40 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Made agent flag '--hadoop_home' as optional.


Diffs (updated)
-

  docs/configuration/agent.md e0aaf2c15023d1cb24e16b02f948b3d3848be57a 
  docs/operator-http-api.md c0f20c00f27ac5c81d4b56f166ea4140b5305096 
  src/slave/containerizer/fetcher.cpp d6b42708f42fb3b6b2ef56bb4d6d4f998796a40b 
  src/slave/flags.hpp ae09e199a4d0b27c11addea76df709412a25ba04 
  src/slave/flags.cpp 23d9bb1ca9bc7451afae69f39c25605660612c2e 
  src/slave/http.cpp ba43086f745b0171f699e9ccf90a505f2c5d0f8c 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 66562: Added test for local puller hdfs uri fetcher plugin.

[Gilbert Song]

> On April 18, 2018, 6:24 a.m., Qian Zhang wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp
> > Lines 493-517 (patched)
> > 
> >
> > Can you use raw string literal to avoid escaping?

Considering all `mockHadoopScript` was written in this way. I would suggest 
follow the style for now and refactor together in a separate patch.


- Gilbert


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


On April 16, 2018, 10:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66562/
> ---
> 
> (Updated April 16, 2018, 10:32 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-8794
> https://issues.apache.org/jira/browse/MESOS-8794
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for local puller hdfs uri fetcher plugin.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c664ff807583d587d94b0ab797330d5d3daf7657 
> 
> 
> Diff: https://reviews.apache.org/r/66562/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Alexander Rojas]

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

(Updated June 6, 2018, 10:31 a.m.)


Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

A vulnerability in our JWT implementation allows an unauthenticated
remote attacker to execute to execute timing attacks [1].

This patch removes the vulnerability by adding a constant time
comparison of hashes, where the whole message is visited during
the comparison instead of returning at the first failure.

[1] https://codahale.com/a-lesson-in-timing-attacks/


Diffs (updated)
-

  3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 


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

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


Testing
---

```sh
make check
```


Thanks,

Alexander Rojas

Re: Review Request 67357: Added constant time comparison of JWT signatures.

[Alexander Rojas]

> On May 31, 2018, 6:56 p.m., Benno Evers wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 205 (patched)
> > 
> >
> > To be more precise: "which compares either none of the content or the 
> > whole content of the strings".
> > 
> > (but actually I'm not even sure a comment is needed at all, the 
> > function name kinda speaks for itself)

I agree but the shepherd requested it ;)


- Alexander


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


On May 31, 2018, 11:02 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated May 31, 2018, 11:02 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/2/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 67472: Fixed CppLinter in python3/mesos-style.py to use correct linter.

[Armand Grillet]

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

Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.


Repository: mesos


Description
---

Fixed CppLinter in python3/mesos-style.py to use correct linter.


Diffs
-

  support/python3/mesos-style.py 6f9b9d4611c6501f8a2dbda4bfe08cffaadc6d86 


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


Testing
---

Created a commit updating one C++ file, then:

```
apache-mesos (MESOS-8979) $ git commit -m "Test."
Congratulations! You have Python 3 installed correctly.
Please start using the scripts in `support/python3`.
Please also set the environment variable `MESOS_SUPPORT_PYTHON` to `3`
so that the Git hooks use the Python 3 scripts.
Checking 1 C++ file
Done processing src/slave/constants.cpp
Total errors found: 0
Total errors found: 0
No JavaScript files to lint
No Python files to lint
Congratulations! You have Python 3 installed correctly.
Please start using the scripts in `support/python3`.
Please also set the environment variable `MESOS_SUPPORT_PYTHON` to `3`
so that the Git hooks use the Python 3 scripts.
[MESOS-8979 b640bb182] Test.
 1 file changed, 1 insertion(+)
apache-mesos (MESOS-8979) $ export MESOS_SUPPORT_PYTHON=3
apache-mesos (MESOS-8979) $ git commit --amend
Checking 1 C++ file
Done processing src/slave/constants.cpp
Total errors found: 0
Total errors found: 0
No JavaScript files to lint
No Python files to lint
[MESOS-8979 3c596291f] Test.
 Date: Wed Jun 6 09:38:59 2018 +0200
 1 file changed, 1 insertion(+)
```


Thanks,

Armand Grillet

Re: Review Request 67464: Fixed `python3/mesos-style.py`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67464]

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 June 5, 2018, 11:15 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67464/
> ---
> 
> (Updated June 5, 2018, 11:15 p.m.)
> 
> 
> Review request for mesos and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `cpplint.py` file was made to be single-source compatible with
> both Python 2 and Python 3, but `mesos-style.py` was erroneously
> changed to look for a copy of it under `support/python3`.
> 
> 
> Diffs
> -
> 
>   support/python3/mesos-style.py 6f9b9d4611c6501f8a2dbda4bfe08cffaadc6d86 
> 
> 
> Diff: https://reviews.apache.org/r/67464/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>