date:20180531


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



Patch looks great!

Reviews applied: [67185]

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 May 31, 2018, 9:05 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67185/
> ---
> 
> (Updated May 31, 2018, 9:05 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added request_protobuf to mesos.http.
> 
> 
> Diffs
> -
> 
>   src/python/lib/mesos/constants.py PRE-CREATION 
>   src/python/lib/mesos/http.py 073c159dc6916fa5d7349a033db022d7175aef05 
>   src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
>   src/python/lib/tests/test_http.py 66dd6d7c6272d1828dd591829ca3543d3430f69b 
> 
> 
> Diff: https://reviews.apache.org/r/67185/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

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


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



FAIL: Failed to apply the dependent review: 67335.

Failed command: `python.exe .\support\python3\apply-reviews.py -n -r 67335`

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

Relevant logs:

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

```
error: patch failed: docs/configuration/agent.md:967
error: docs/configuration/agent.md: patch does not apply
```

- Mesos Reviewbot Windows


On June 1, 2018, 11:28 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67381/
> ---
> 
> (Updated June 1, 2018, 11:28 a.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 67381: Added `cgroups/all` into CHANGELOG and upgrades.md.


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

(Updated June 1, 2018, 11:28 a.m.)


Review request for mesos and Gilbert Song.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added `cgroups/all` into CHANGELOG and upgrades.md.


Diffs (updated)
-

  CHANGELOG bdcf8e33b95f4bea04b64417aa73df44dd456afe 
  docs/upgrades.md 18632edd20fcf943c6e7147aca3fec5b5521f14f 


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

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


Testing
---

Not a code change.


Thanks,

Qian Zhang

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


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

(Updated June 1, 2018, 11:21 a.m.)


Review request for mesos and Gilbert Song.


Changes
---

Addressed review comments.


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 (updated)
-

  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/

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


Testing
---

sudo make check


Thanks,

Qian Zhang

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


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

(Updated June 1, 2018, 11:20 a.m.)


Review request for mesos and Gilbert Song.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Added `cgroups/all` into the agent flag `--isolation`.


Diffs (updated)
-

  docs/configuration/agent.md e0aaf2c15023d1cb24e16b02f948b3d3848be57a 
  src/slave/flags.cpp 23d9bb1ca9bc7451afae69f39c25605660612c2e 


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

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


Testing
---

sudo make check


Thanks,

Qian Zhang

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



> On May 31, 2018, 8 a.m., Gilbert Song wrote:
> > Could we also update the mesos-containerizer.md and have a new file to 
> > introduce the semantic, since people may rely on that document as reference.

Created https://issues.apache.org/jira/browse/MESOS-8973 for that.


- Qian


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


On May 30, 2018, 9:04 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67342/
> ---
> 
> (Updated May 30, 2018, 9:04 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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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



> On May 31, 2018, 8:42 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 108 (patched)
> > 
> >
> > enabled cgroup subsystems?

I'd like to be consistent with the existing ones, like:
https://github.com/apache/mesos/blob/1.6.0/src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp#L122

I guess we do not need the word `cgroups` here because we are already in the 
cgroups isolator.


> On May 31, 2018, 8:42 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 134 (patched)
> > 
> >
> > this is not like happen though, but I am thinking if we should return 
> > an error and block the agent in this case?

As we discussed, we'd better to error out in this case.


> On May 31, 2018, 8:42 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 163 (patched)
> > 
> >
> > why not const ref?

My bad, thanks for catching this!


- Qian


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


On May 30, 2018, 9:05 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67343/
> ---
> 
> (Updated May 30, 2018, 9:05 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 
> 93197b05eb837d02c6c113264f20a33d6ed92d7f 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 6d663a5770418b7ffe2b3af50b9181ecde183c67 
> 
> 
> Diff: https://reviews.apache.org/r/67343/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 67137: Avoided leaking file descriptors in Mesos containerizer.


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



Patch looks great!

Reviews applied: [67398, 67137]

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 May 24, 2018, 11:13 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67137/
> ---
> 
> (Updated May 24, 2018, 11:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch explicitly closes not required file descriptors when
> forking a Mesos containerizer instance. We currently only pass on
> stdin, stout, and sterr.
> 
> While it would in principle be possible to open all file descriptors
> with `O_CLOEXEC`, this is not practical as
> 
> * `O_CLOEXEC` is not supported on BSDs (not Windows either, but file
>   descriptor leaks are prevented there in a different way), and
> * we might call thirdparty code outside of our control which does not
>   use e.g., `O_CLOEXEC` either (the case e.g., for leveldb).
> 
> Closing all file descriptors after the fork avoids leaks even in above
> scenarios.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> cec6558d0ac61bf0fec87d2e101e8f84730a765a 
> 
> 
> Diff: https://reviews.apache.org/r/67137/diff/9/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> I still need to confirm this patch in CI on a wider range of scenarios.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67065: Added a new stout utility header file which interfaces with libarchive.

2018-05-31 Thread Joseph Wu


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




3rdparty/stout/include/stout/archiver.hpp
Lines 20 (patched)


This definition should go into the CMake file(s):
```
# This is a definition that only has an effect on Windows and is necessary
# when building a static libarchive instead of a DLL.
target_compile_definitions(libarchive PUBLIC LIBARCHIVE_STATIC)
```



3rdparty/stout/include/stout/archiver.hpp
Lines 36-58 (patched)


We might not need to have a helper for error handling.  If you treat all 
`result <= ARCHIVE_WARN` as an error, you can get the roughly same behavior 
(minus the WARNING logs).



3rdparty/stout/include/stout/archiver.hpp
Lines 124-127 (patched)


This error check could be made more explicit:
```
if (result != ARCHIVE_OK) { ... }
```


- Joseph Wu


On May 30, 2018, 1:29 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67065/
> ---
> 
> (Updated May 30, 2018, 1:29 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This archiver utility can be invoked to extract a compressed or
> uncompressed archive with several different supported formats.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/CMakeLists.txt 24a1f0acbee8a464fcc5159cf41d7e93aa8148fc 
>   3rdparty/stout/Makefile.am ef22a02a8a11326c6af19eb11d79eb82ff7861da 
>   3rdparty/stout/include/Makefile.am e0097c438a62a9802d6e60c78493bf97333f20ed 
>   3rdparty/stout/include/stout/archiver.hpp PRE-CREATION 
>   3rdparty/stout/tests/CMakeLists.txt 
> 86111a8709cab709ba1bf844043d3a1da07b6241 
>   3rdparty/stout/tests/archiver_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67065/diff/5/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67395: Fixed socket creation bug in docker.cpp.


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



Patch looks great!

Reviews applied: [67395]

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 May 31, 2018, 2:59 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67395/
> ---
> 
> (Updated May 31, 2018, 2:59 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, the statement `int_fd socket = ::socket(...);` would
> implictly call the `WindowsFD(int crt)` constructor. Since that
> contstructor only accepts values of {0, 1, 2}, it would incorrectly
> mark the socket as invalid. The code has been changed to use the stout
> network functions, which properly construct the `int_fd`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp fc032367400119dc827657d0e6e859d18ebdbb16 
> 
> 
> Diff: https://reviews.apache.org/r/67395/diff/1/
> 
> 
> Testing
> ---
> 
> This bug was found when testing recovery in our Windows cluster and this 
> patch was confirmed to fix it.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67257: Fixed filters in test `ROOT_ReconcileDroppedOperation` for consistency.


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

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


Review request for mesos, Gaston Kleiman and Greg Mann.


Changes
---

Fixed a flakiness due to a race between offers and test teardown.


Repository: mesos


Description
---

Fixed filters in test `ROOT_ReconcileDroppedOperation` for consistency.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
04a75fc4d53d1b7211e64755097b06b1e68cc3b3 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67256: Added a unit test for CSI plugin RPC metrics.


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

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


Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan Schlicht.


Changes
---

Fixed a flakiness due to a race between offers and test teardown.


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


Repository: mesos


Description
---

This patch adds the `ROOT_CsiPluginRpcMetrics` test that issues a
`CREATE_VOLUME` followed by a `DESTROY_VOLUME`, which would fail due to
an out-of-band deletion of the actual volume.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
04a75fc4d53d1b7211e64755097b06b1e68cc3b3 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65666: Added a unit test for SLRP operation state metrics.


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

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


Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan Schlicht.


Changes
---

Fixed a flakiness due to a race between offers and test teardown.


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


Repository: mesos


Description
---

This patch adds the `ROOT_OperationStateMetrics` test that issues a
`CREATE_VOLUME` followed by two `DESTROY_VOLUME`s. The first one will
fail due to an out-of-band deletion of the actual volume, and the second
one will fail due to modifying the resource version.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
04a75fc4d53d1b7211e64755097b06b1e68cc3b3 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67371: Introduced a random sorter as an alternative to the DRF sorter.


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


Fix it, then Ship it!





src/master/allocator/sorter/random/sorter.hpp
Lines 101 (patched)


Document the complexity and determinism.



src/master/allocator/sorter/random/sorter.hpp
Lines 135 (patched)


s/share/sampling probablity/



src/master/allocator/sorter/random/sorter.hpp
Lines 233 (patched)


Not needed.



src/master/allocator/sorter/random/sorter.hpp
Lines 295-296 (patched)


invariant (2) no longer relevant.

`s/It is up to//`



src/master/allocator/sorter/random/sorter.hpp
Lines 397-404 (patched)


Not needed.



src/master/allocator/sorter/random/sorter.cpp
Lines 466 (patched)


The allocator currently calls `sort()` as it cycles through the 
roles/frameworks, this is unnecessarily expensive and would lead to nonideal 
random distribution. Ideally, we only need to sort once for each allocation 
cycle (once for each complete triple loop) and cycle through the randomized 
list. This would require changes to the sorter interface. We can do it in 
subsequent patches. Can you add a TODO note here?


- Meng Zhu


On May 29, 2018, 6 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67371/
> ---
> 
> (Updated May 29, 2018, 6 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-8936
> https://issues.apache.org/jira/browse/MESOS-8936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This sorter returns a weighted random shuffling of its clients
> upon each `sort()` request.
> 
> This implementation is a copy of the `DRFSorter` with share
> calculation logic (including the `dirty` bit) removed and an
> adjusted `sort()` implementation. Work needs to be done to
> reduce the amount of duplicated logic needed across sorter
> implementations, but it is left unaddressed here.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
>   src/master/allocator/sorter/random/sorter.hpp PRE-CREATION 
>   src/master/allocator/sorter/random/sorter.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67371/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests added in subsequent patch.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67303: Added documentation for resource provider and CSI plugin metrics.


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

(Updated May 31, 2018, 11:08 p.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Joerg Schad.


Changes
---

Addressed Jie's comments.


Repository: mesos


Description
---

Added documentation for resource provider and CSI plugin metrics.


Diffs (updated)
-

  docs/home.md 5471c70d4023f88adae2234aec267e4d6fea83df 
  docs/monitoring.md c20b99eb32a50830ea6f66b5bdbfcabe45fc6ecb 


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

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


Testing
---

Spell checked through VIM's `:set spell` and manually tested the links.


Thanks,

Chun-Hung Hsiao

Re: Review Request 67255: Added per-CSI-call RPC metrics for SLRP.


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

(Updated May 31, 2018, 11:06 p.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan Schlicht.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

For each CSI call, e.g., `csi.v0.Identity.Probe`, we the following
metrics for SLRP:
`csi_plugin/rpcs/csi.v0.Identity.Probe/pending`
`csi_plugin/rpcs/csi.v0.Identity.Probe/successes`
`csi_plugin/rpcs/csi.v0.Identity.Probe/errors`
`csi_plugin/rpcs/csi.v0.Identity.Probe/cancelled`

To add these per-CSI-call metrics, each method in `csi::v0::Client`,
e.g., `csi::v0::Client::Probe`, is changed to
`csi::v0::Client::call`, to make RPC calls based on the RPC enum
value. A `call` helper function in SLRP is also added to intercept CSI
calls and update the corresponding metrics.


Diffs (updated)
-

  src/csi/client.hpp 9d7019afb08168d18e5a78831a45af88a75bf809 
  src/csi/client.cpp a4ba1f12a79354f81b29924b19b119fad31a06b9 
  src/resource_provider/storage/provider.cpp 
2c7dd8d96915d68a7cdfcb870d5448bc25ea9011 
  src/tests/csi_client_tests.cpp d5993d617056e86ae5a1224258cc6e0741466166 


Diff: https://reviews.apache.org/r/67255/diff/9/

Changes: https://reviews.apache.org/r/67255/diff/8-9/


Testing
---

sudo make check

A unit test for the metrics will be introduced in the next patch.


Thanks,

Chun-Hung Hsiao

Re: Review Request 67375: Added the `RPC` enum and `RPCTraits` helper.


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

(Updated May 31, 2018, 11:05 p.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

To make it easy to enumerate all types of CSI RPC calls, the `RPC` enum
is introduced. The `RPCTraits` helper class can be used to determine the
request and response type of a particular RPC.


Diffs (updated)
-

  src/CMakeLists.txt f86884de2beb946c8bfc2bb8260e09a9c98ce625 
  src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
  src/csi/rpc.hpp PRE-CREATION 
  src/csi/rpc.cpp PRE-CREATION 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67394: White list fds that child processes can inherit in mesos containerizer.

2018-05-31 Thread Radhika Jandhyala via Review Board


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

(Updated May 31, 2018, 10:50 p.m.)


Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Jie Yu, and Li Li.


Repository: mesos


Description
---

White list fds that child processes can inherit in mesos containerizer.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
eac1d16f2388385fec04ff8f013ce0ebf4e97f0f 
  src/slave/containerizer/mesos/launcher.hpp 
f69d934d2e1a129e10df8c7f5c78723e832adc7d 
  src/slave/containerizer/mesos/launcher.cpp 
2fe47d368cb82a46328e1f636baa836272db244c 
  src/slave/containerizer/mesos/linux_launcher.hpp 
0ea9b875ae46cadea483bc8dd8bf4907fd324dc9 
  src/slave/containerizer/mesos/linux_launcher.cpp 
80e444501e429c1e1ae354abcd51f86430316ada 
  src/tests/containerizer/launcher.hpp a8e436f164b67d937ebcff35e084d3ca755c003c 
  src/tests/containerizer/launcher.cpp a92d9890f0931425d69ef8ce0896d081b8722079 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
01f2b38cfa67b144298c361e92170322864ac201 


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


Testing (updated)
---

All mesos tests on windows


Thanks,

Radhika Jandhyala

Re: Review Request 67389: Windows: Implemented Windows IOCP async backend.

2018-05-31 Thread Radhika Jandhyala via Review Board


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




3rdparty/libprocess/src/libwinio_impl.cpp
Lines 38 (patched)


$NCPUs sounds good.


- Radhika Jandhyala


On May 30, 2018, 6:54 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67389/
> ---
> 
> (Updated May 30, 2018, 6:54 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
> John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
> https://issues.apache.org/jira/browse/MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8671
> https://issues.apache.org/jira/browse/MESOS-8672
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Windows IOCP async backend, called libwinio, provides a single
> threaded event loop implementation that allows for async IO and timer
> events. libwinio wraps the native Win32 async functions using
> libprocess's primitives, which makes it easier to use and more type
> safe.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> cf443dffd0663ecf02b7efd6f7094175b94aae19 
>   3rdparty/libprocess/src/libwinio_impl.hpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_impl.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67389/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67066: Modified the fetcher to use libarchive and added associated tests.


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



Patch looks great!

Reviews applied: [67118, 67064, 67065, 67066]

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 May 30, 2018, 8:30 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67066/
> ---
> 
> (Updated May 30, 2018, 8:30 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Chun-Hung Hsiao, 
> Eric Mumau, Jie Yu, Joseph Wu, Li Li, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8064
> https://issues.apache.org/jira/browse/MESOS-8064
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the fetcher to use libarchive and added associated tests.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 4cff7273fdc9e3897074da4da7dad02483086c2d 
>   src/tests/fetcher_tests.cpp 8c353f2a30a6dd6338c91dad7ff76ff28d1005e8 
> 
> 
> Diff: https://reviews.apache.org/r/67066/diff/7/
> 
> 
> Testing
> ---
> 
> I've built on Windows with CMake and have run all tests, which all pass.
> I've also built on Linux with both the autotools build and cmake build and 
> have run all tests, which all pass.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 67370: Added a test for weighted shuffling.


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


Fix it, then Ship it!





src/tests/sorter_tests.cpp
Lines 70 (patched)


Put a comment about how we get the table?


- Meng Zhu


On May 29, 2018, 5:57 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67370/
> ---
> 
> (Updated May 29, 2018, 5:57 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-8936
> https://issues.apache.org/jira/browse/MESOS-8936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp da4e0f64a565af1d9458ff256ae0eafddd0a6b68 
> 
> 
> Diff: https://reviews.apache.org/r/67370/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67305: Moved the "Resource Provider" section to `resource-provider.md`.


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


Ship it!




Ship It!

- Jie Yu


On May 25, 2018, 1:24 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67305/
> ---
> 
> (Updated May 25, 2018, 1:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The concept of resource providers should have a documentation file
> separated from `csi.md`. This patch moves the "Resource Provider"
> section from `csi.md` to `resource-provider.md` as a start, and also for
> preventing dangling links from other documents.
> 
> 
> Diffs
> -
> 
>   docs/csi.md 9d06da0a4494a7e3bd86e28757846d2ee72efbf3 
>   docs/resource-provider.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67305/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67303: Added documentation for resource provider and CSI plugin metrics.


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


Fix it, then Ship it!




Thanks!


docs/monitoring.md
Lines 1771 (patched)


"that apply to the resources provided by a resource provider with the given 
type and name"



docs/monitoring.md
Lines 1813 (patched)


may vary



docs/monitoring.md
Lines 1814-1816 (patched)


the following is a comprehensive list of operation types and the 
corresponding resource providers that support them. Note that the name column 
is for the operation placeholder in the above metrics.


- Jie Yu


On May 25, 2018, 6:30 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67303/
> ---
> 
> (Updated May 25, 2018, 6:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for resource provider and CSI plugin metrics.
> 
> 
> Diffs
> -
> 
>   docs/home.md 5471c70d4023f88adae2234aec267e4d6fea83df 
>   docs/monitoring.md c20b99eb32a50830ea6f66b5bdbfcabe45fc6ecb 
> 
> 
> Diff: https://reviews.apache.org/r/67303/diff/3/
> 
> 
> Testing
> ---
> 
> Spell checked through VIM's `:set spell` and manually tested the links.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67369: Introduced a weighted shuffle algorithm implementation.


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


Ship it!




Ship It!

- Meng Zhu


On May 29, 2018, 5:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67369/
> ---
> 
> (Updated May 29, 2018, 5:55 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and Meng Zhu.
> 
> 
> Bugs: MESOS-8936
> https://issues.apache.org/jira/browse/MESOS-8936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will be used by the random sorter in order to support weights.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
>   src/master/allocator/sorter/random/utils.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67369/diff/1/
> 
> 
> Testing
> ---
> 
> Test added in subsequent patch.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67264: Unmounted any dangling persistent volume in gc paths.


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



PASS: Mesos patch 67264 was successfully built and tested.

Reviews applied: `['67264']`

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

- Mesos Reviewbot Windows


On May 31, 2018, 8:38 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67264/
> ---
> 
> (Updated May 31, 2018, 8:38 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
> ---
> 
> In various corner cases, agent may not get chance to properly unmount
> persistent volumes mounted inside an executor's sandbox. When GC later
> gets to these sandbox directories, permanent data loss can happen (see
> MESOS-8830).
> 
> This patch added some protection to unmount possible persistent
> volumes inside a path to gc, and skipped the path if unmount failed.
> 
> NOTE: this means agent will not garbage collect any path if it cannot
> read its own `mountinfo` table.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp afff54653e8e659d947ddbee6dc38ba2715f2a78 
>   src/slave/gc.hpp df40165bb8a23f065156bf6c5f354b143d88c088 
>   src/slave/gc.cpp 390b35e6d17d6614a73c9548decbf10739560106 
>   src/slave/gc_process.hpp 20374ad91820341282fdf18ecade60a020e26cea 
>   src/slave/main.cpp 646125344d590b28256d8ee684d7e51a90e82f23 
>   src/slave/paths.hpp 015896453410a33923eed07b3e676be19af62a48 
>   src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126 
>   src/tests/cluster.cpp b56212f6529a4d307e65797ad9bb34f2104fc832 
>   src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031 
>   src/tests/mesos.hpp 733344a2f07ebd9d841a55fb9bbfda2e3c1a1eb2 
>   src/tests/mesos.cpp d3c87c295429481c59d5a49398e289a4b84e4496 
>   src/tests/slave_tests.cpp 65d860594572b58a50a89358e31e97fd2a10bf08 
> 
> 
> Diff: https://reviews.apache.org/r/67264/diff/3/
> 
> 
> Testing
> ---
> 
> Tested with following procedures:
> 1. Start a test master and agent;
> 2. Created a persistent volume on agent through operator API;
> 3. Use `mesos-execute` to run a task;
> 4. Stop the agent;
> 5. Manually bind mount persistent volume path into a `volume` directory 
> inside the executor sandbox (to simulate a dangling mount in MESOS-8830);
> 6. Restart agent with `--gc_disk_headroom=1.0 --gc_delay=1secs` to force it 
> gc the path immediately.
> 
> With this fix, we observed that the dangling mount is automatically cleaned 
> up, and agent produces log line:
> ```
> W0523 06:00:04.001075 82745 gc.cpp:229] Unmounting dangling mount point 
> '/home/zhitao/mesos-workdir/slaves/b3eb3aff-d19d-45ff-8113-f0316462d3fa-S0/frameworks/b3eb3aff-d19d-45ff-8113-f0316462d3fa-/executors/test_id/runs/1cd3bd06-2632-4541-a708-80c7cd51c74b/volume'
>  of persistent volume '/home/zhitao/mesos-workdir/volumes/roles/role/id1' 
> inside garbage collected path 
> '/home/zhitao/mesos-workdir/slaves/b3eb3aff-d19d-45ff-8113-f0316462d3fa-S0'
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67328: Used italic fonts to denote placeholdes in metrics.


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


Ship it!




Ship It!

- Jie Yu


On May 25, 2018, 6:28 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67328/
> ---
> 
> (Updated May 25, 2018, 6:28 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used italic fonts to denote placeholdes in metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md c20b99eb32a50830ea6f66b5bdbfcabe45fc6ecb 
> 
> 
> Diff: https://reviews.apache.org/r/67328/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67258: Fixed flakiness for some `AgentResourceProviderConfigApiTest` tests.


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


Ship it!




Ship It!

- Jie Yu


On May 22, 2018, 11:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67258/
> ---
> 
> (Updated May 22, 2018, 11:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ROOT_Add`, `ROOT_Update` and `ROOT_Remove` tests mistakenly use
> `WillOnce` to declient unwanted offers. This patch changes them to
> `WillRepeatedly`.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_resource_provider_config_api_tests.cpp 
> 7d4d3b9d21c3a0c603f805f47c57a7fa67db9d08 
> 
> 
> Diff: https://reviews.apache.org/r/67258/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67257: Fixed filters in test `ROOT_ReconcileDroppedOperation` for consistency.


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


Ship it!




Ship It!

- Jie Yu


On May 22, 2018, 11:38 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67257/
> ---
> 
> (Updated May 22, 2018, 11:38 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed filters in test `ROOT_ReconcileDroppedOperation` for consistency.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 45cb38922f78941e82667a60b3b71ce220c9202f 
> 
> 
> Diff: https://reviews.apache.org/r/67257/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67256: Added a unit test for CSI plugin RPC metrics.


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


Ship it!




Ship It!

- Jie Yu


On May 25, 2018, 12:24 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67256/
> ---
> 
> (Updated May 25, 2018, 12:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8943
> https://issues.apache.org/jira/browse/MESOS-8943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `ROOT_CsiPluginRpcMetrics` test that issues a
> `CREATE_VOLUME` followed by a `DESTROY_VOLUME`, which would fail due to
> an out-of-band deletion of the actual volume.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 45cb38922f78941e82667a60b3b71ce220c9202f 
> 
> 
> Diff: https://reviews.apache.org/r/67256/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67354: Removed `os::getenv()` calls from `MesosExecutorDriver`.



> On May 31, 2018, 6:24 p.m., Ilya Pronin wrote:
> > src/exec/exec.cpp
> > Lines 643-646 (original), 652-655 (patched)
> > 
> >
> > We don't change executor logging flags in our tests, do we? It might be 
> > worth calling out in a comment that this constructor still loads those 
> > flags from environment variables.

I think ideally we should pass environment into `Flags.load()` here and 
entirely avoid reading environment if this c-tor is used.


- Alexander


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


On May 29, 2018, 2:20 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67354/
> ---
> 
> (Updated May 29, 2018, 2:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, haosdent 
> huang, Ilya Pronin, and James Peach.
> 
> 
> Bugs: MESOS-3475
> https://issues.apache.org/jira/browse/MESOS-3475
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds overloaded constructor for `MesosExecutorDriver` that
> accepts `environment` parameter and stores it in the class variable.
> This new constructor is needed to get rid of `os::getenv()` calls,
> so that `MesosExecutorDriver` can be used in tests that require
> thread safety.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor.hpp d14c0369f6731100d27092142b56f108f8881003 
>   src/exec/exec.cpp 65a671d7ce83a51087d290ba039d18deba6313c2 
> 
> 
> Diff: https://reviews.apache.org/r/67354/diff/1/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 67264: Unmounted any dangling persistent volume in gc paths.

2018-05-31 Thread Zhitao Li

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

(Updated May 31, 2018, 1:38 p.m.)

Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.

Changes
---

Jason's comments.

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

Repository: mesos

Description
---

In various corner cases, agent may not get chance to properly unmount
persistent volumes mounted inside an executor's sandbox. When GC later
gets to these sandbox directories, permanent data loss can happen (see
MESOS-8830).

This patch added some protection to unmount possible persistent
volumes inside a path to gc, and skipped the path if unmount failed.

NOTE: this means agent will not garbage collect any path if it cannot
read its own `mountinfo` table.

Diffs (updated)
-

src/local/local.cpp afff54653e8e659d947ddbee6dc38ba2715f2a78
src/slave/gc.hpp df40165bb8a23f065156bf6c5f354b143d88c088
src/slave/gc.cpp 390b35e6d17d6614a73c9548decbf10739560106
src/slave/gc_process.hpp 20374ad91820341282fdf18ecade60a020e26cea
src/slave/main.cpp 646125344d590b28256d8ee684d7e51a90e82f23
src/slave/paths.hpp 015896453410a33923eed07b3e676be19af62a48
src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126
src/tests/cluster.cpp b56212f6529a4d307e65797ad9bb34f2104fc832
src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031
src/tests/mesos.hpp 733344a2f07ebd9d841a55fb9bbfda2e3c1a1eb2
src/tests/mesos.cpp d3c87c295429481c59d5a49398e289a4b84e4496
src/tests/slave_tests.cpp 65d860594572b58a50a89358e31e97fd2a10bf08

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

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

Testing
---

Tested with following procedures:
1. Start a test master and agent;
2. Created a persistent volume on agent through operator API;
3. Use `mesos-execute` to run a task;
4. Stop the agent;
5. Manually bind mount persistent volume path into a `volume` directory inside
the executor sandbox (to simulate a dangling mount in MESOS-8830);
6. Restart agent with `--gc_disk_headroom=1.0 --gc_delay=1secs` to force it gc
the path immediately.

With this fix, we observed that the dangling mount is automatically cleaned up,
and agent produces log line:
```
W0523 06:00:04.001075 82745 gc.cpp:229] Unmounting dangling mount point
'/home/zhitao/mesos-workdir/slaves/b3eb3aff-d19d-45ff-8113-f0316462d3fa-S0/frameworks/b3eb3aff-d19d-45ff-8113-f0316462d3fa-/executors/test_id/runs/1cd3bd06-2632-4541-a708-80c7cd51c74b/volume'
of persistent volume '/home/zhitao/mesos-workdir/volumes/roles/role/id1'
inside garbage collected path
'/home/zhitao/mesos-workdir/slaves/b3eb3aff-d19d-45ff-8113-f0316462d3fa-S0'
```

Thanks,

Zhitao Li

Re: Review Request 67344: Made disk allocatable on their own.

2018-05-31 Thread Benjamin Mahler


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



Gaston is also working on making the allocation "minimums" configurable, e.g.

```
Probably want to have an AND by default to avoid MESOS-8935:
--allocation_minimums=cpus:0.01;mem:32

Currently we have effectively an OR:
--allocation_minimums=cpus:0.01,mem:32
```

We would probably want this default to be an AND of resources to avoid 
MESOS-8935. Once we have "storage" frameworks that only want disk, we would 
probably allow a per-framework override of the allocation minimums, requiring 
those special frameworks to specify their minimums.

Make sense?

- Benjamin Mahler


On May 28, 2018, 3:11 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67344/
> ---
> 
> (Updated May 28, 2018, 3:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8626
> https://issues.apache.org/jira/browse/MESOS-8626
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes it possible to offer `disk` resources in isolation
> which previously was only possible for `cpus` or `mem` resources.
> 
> While it is not possible to perform launch operations on `disk`
> resources alone, frameworks could still perform most other operations
> on them. With the introduction of resource providers we introduced
> operations which do not take effect immediately, but only after the
> master has received a status update on the operations success, so that
> with the changes here frameworks can now operate on `disk` resources
> independently of task launch operations.
> 
> Since resource provider resources are independent from agent-default
> resources, the full agent resources are not necessarily in the same
> role. Due to the way the default allocator takes roles into account,
> this could in the past have lead to situations where it was impossible
> to find suitable `cpus` or `mem` resources to make `disk` resources
> from some other role offerable at all. This problem also disappears if
> `disk` become offerable on their own.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> b558228290e5ae6dbcc1b8a6e1fe69db9fd5874c 
>   src/master/constants.hpp b31e5f027b44d371050cea9a4662a4b54127812f 
>   src/tests/hierarchical_allocator_tests.cpp 
> c97b2ba0884a7ded867c2d80e4749de54c89b5e4 
>   src/tests/persistent_volume_tests.cpp 
> 43f31b2e613666a989c205b8543ef1cde484d93c 
> 
> 
> Diff: https://reviews.apache.org/r/67344/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67393: Windows: Ported io_tests.cpp.


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



Patch looks great!

Reviews applied: [67384, 67385, 67386, 67387, 67388, 67389, 67390, 67391, 
67392, 67393]

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 May 30, 2018, 6:40 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67393/
> ---
> 
> (Updated May 30, 2018, 6:40 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
> John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-5815
> https://issues.apache.org/jira/browse/MESOS-5815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the IOCP backened implemented, io_tests.cpp is now ported to
> Windows. Since the tests require async pipe IO, the tests are only
> run if Mesos was compiled with the IOCP backened.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> 8c1353b442d5fc7b2e796a4d87f5b41389d4e1c3 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> de7272b3ef0f414f056509448856c6bbd86ef75c 
> 
> 
> Diff: https://reviews.apache.org/r/67393/diff/1/
> 
> 
> Testing
> ---
> 
> The entire chain was tested with `make check` on Linux. The tests were run on 
> Windows as well with `-DENABLE_LIBEVENT` and `-DENABLE_LIBWINIO`.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67255: Added per-CSI-call RPC metrics for SLRP.


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


Fix it, then Ship it!





src/csi/client.hpp
Lines 68 (patched)


nits: I'd just make this consistent with others :)

```
template <>
process::Future
Client::call(
const DeleteVolumeRequest& request);
```

Ditto below.



src/resource_provider/storage/provider.cpp
Lines 1776 (patched)


Does this work? Can you make it more verbose if that's your intention to 
log here.


- Jie Yu


On May 30, 2018, 3:01 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67255/
> ---
> 
> (Updated May 30, 2018, 3:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8943
> https://issues.apache.org/jira/browse/MESOS-8943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For each CSI call, e.g., `csi.v0.Identity.Probe`, we the following
> metrics for SLRP:
> `csi_plugin/rpcs/csi.v0.Identity.Probe/pending`
> `csi_plugin/rpcs/csi.v0.Identity.Probe/successes`
> `csi_plugin/rpcs/csi.v0.Identity.Probe/errors`
> `csi_plugin/rpcs/csi.v0.Identity.Probe/cancelled`
> 
> To add these per-CSI-call metrics, each method in `csi::v0::Client`,
> e.g., `csi::v0::Client::Probe`, is changed to
> `csi::v0::Client::call`, to make RPC calls based on the RPC enum
> value. A `call` helper function in SLRP is also added to intercept CSI
> calls and update the corresponding metrics.
> 
> 
> Diffs
> -
> 
>   src/csi/client.hpp 9d7019afb08168d18e5a78831a45af88a75bf809 
>   src/csi/client.cpp a4ba1f12a79354f81b29924b19b119fad31a06b9 
>   src/resource_provider/storage/provider.cpp 
> 63b5d7e5f10d6ad02b5cd11b119def3b4abf4180 
>   src/tests/csi_client_tests.cpp d5993d617056e86ae5a1224258cc6e0741466166 
> 
> 
> Diff: https://reviews.apache.org/r/67255/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> A unit test for the metrics will be introduced in the next patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67403: Handled race condition when removing maintenance windows.


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



PASS: Mesos patch 67403 was successfully built and tested.

Reviews applied: `['67403']`

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

- Mesos Reviewbot Windows


On May 31, 2018, 4:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67403/
> ---
> 
> (Updated May 31, 2018, 4:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-7966
> https://issues.apache.org/jira/browse/MESOS-7966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When executing the `Master::inverseOffers()` callback, it
> could happen that the maintenance window the reverse offer
> referred to was already removed by a concurrent call to
> to the maintenance endpoint of Mesos.
> 
> In this case, we must not send out a reverse offer, because
> having outstanding inverse offers for an agent without
> any scheduled maintenance window will lead to a crash in
> the allocator when attempting to remove this offer.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 
> 
> 
> Diff: https://reviews.apache.org/r/67403/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Set up the reproduction environment locally and ran `while :; python call.py; 
> done` for about a minute. (see linked ticket)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67369: Introduced a weighted shuffle algorithm implementation.


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




src/master/allocator/sorter/random/utils.hpp
Lines 49 (patched)


s/-/ - /



src/master/allocator/sorter/random/utils.hpp
Lines 53-59 (patched)


Why do we need to construct the random distribution every time? How about:

```
std::uniform_real_distribution<> dis(0.0, 1.0);
for (...) {
  ...
  keys[i] = 0.0 - std::pow(dis(urbg), (1.0 / weights[i]));
}
```



src/master/allocator/sorter/random/utils.hpp
Lines 75 (patched)


ditto


- Meng Zhu


On May 29, 2018, 5:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67369/
> ---
> 
> (Updated May 29, 2018, 5:55 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Greg Mann, and meng han.
> 
> 
> Bugs: MESOS-8936
> https://issues.apache.org/jira/browse/MESOS-8936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will be used by the random sorter in order to support weights.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
>   src/master/allocator/sorter/random/utils.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67369/diff/1/
> 
> 
> Testing
> ---
> 
> Test added in subsequent patch.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67375: Added the `RPC` enum and `RPCTraits` helper.


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


Ship it!





src/csi/rpc.cpp
Lines 31 (patched)


nits: I suggest we align the statements here using the same way:
```
return stream
  << Identity::service_full_name()
  << ".GetPluginInfo";
```


- Jie Yu


On May 30, 2018, 2:57 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67375/
> ---
> 
> (Updated May 30, 2018, 2:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8943
> https://issues.apache.org/jira/browse/MESOS-8943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make it easy to enumerate all types of CSI RPC calls, the `RPC` enum
> is introduced. The `RPCTraits` helper class can be used to determine the
> request and response type of a particular RPC.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f86884de2beb946c8bfc2bb8260e09a9c98ce625 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
>   src/csi/rpc.hpp PRE-CREATION 
>   src/csi/rpc.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67375/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67354: Removed `os::getenv()` calls from `MesosExecutorDriver`.

2018-05-31 Thread Ilya Pronin


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



Looks good to me! Just one comment about logging flags.


src/exec/exec.cpp
Lines 643-646 (original), 652-655 (patched)


We don't change executor logging flags in our tests, do we? It might be 
worth calling out in a comment that this constructor still loads those flags 
from environment variables.


- Ilya Pronin


On May 29, 2018, 7:20 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67354/
> ---
> 
> (Updated May 29, 2018, 7:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, haosdent 
> huang, Ilya Pronin, and James Peach.
> 
> 
> Bugs: MESOS-3475
> https://issues.apache.org/jira/browse/MESOS-3475
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds overloaded constructor for `MesosExecutorDriver` that
> accepts `environment` parameter and stores it in the class variable.
> This new constructor is needed to get rid of `os::getenv()` calls,
> so that `MesosExecutorDriver` can be used in tests that require
> thread safety.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor.hpp d14c0369f6731100d27092142b56f108f8881003 
>   src/exec/exec.cpp 65a671d7ce83a51087d290ba039d18deba6313c2 
> 
> 
> Diff: https://reviews.apache.org/r/67354/diff/1/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 67401: Removed undefined behaviour in libprocess due to order-of-evaluation bug.


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



PASS: Mesos patch 67401 was successfully built and tested.

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

- Mesos Reviewbot Windows


On May 31, 2018, 4:11 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67401/
> ---
> 
> (Updated May 31, 2018, 4:11 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8970
> https://issues.apache.org/jira/browse/MESOS-8970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up to C++17, the only ordering constraint on the evaluation of
> expressions between synchronization points was that function
> arguments shall be evaluated before calling a function.
> 
> This could lead to the situation where `std::move(futures)` could be
> called before `await(futures.values())`, leading to a function call
> on a moved-from object and thus undefined behaviour.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> e0d0ee4c8d0df66779c7ef296a1a18d988e889b8 
> 
> 
> Diff: https://reviews.apache.org/r/67401/diff/1/
> 
> 
> Testing
> ---
> 
> `./libprocess-tests`
> 
> Internal CI run including this fix w/o libprocess segfaults: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/3655
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

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


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



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 May 31, 2018, 3:55 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66649/
> ---
> 
> (Updated May 31, 2018, 3:55 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/5/
> 
> 
> 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 67185: Added request_protobuf to mesos.http.


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



PASS: Mesos patch 67185 was successfully built and tested.

Reviews applied: `['67185']`

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

- Mesos Reviewbot Windows


On May 31, 2018, 4:05 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67185/
> ---
> 
> (Updated May 31, 2018, 4:05 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added request_protobuf to mesos.http.
> 
> 
> Diffs
> -
> 
>   src/python/lib/mesos/constants.py PRE-CREATION 
>   src/python/lib/mesos/http.py 073c159dc6916fa5d7349a033db022d7175aef05 
>   src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
>   src/python/lib/tests/test_http.py 66dd6d7c6272d1828dd591829ca3543d3430f69b 
> 
> 
> Diff: https://reviews.apache.org/r/67185/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 67403: Handled race condition when removing maintenance windows.

2018-05-31 Thread Vinod Kone



> On May 31, 2018, 4:41 p.m., Vinod Kone wrote:
> > Can you add a unit test for this?
> 
> Benno Evers wrote:
> It's tricky because we need very precise control over the scheduling, and 
> I'm not sure our testing infrastructure provides it. But I'll look into it.

I see.  The bug is in the allocator, so you cannot use a mock allocator 
unfortunately for control. Consider pausing the clock to have better control in 
the test.


- Vinod


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


On May 31, 2018, 4:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67403/
> ---
> 
> (Updated May 31, 2018, 4:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-7966
> https://issues.apache.org/jira/browse/MESOS-7966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When executing the `Master::inverseOffers()` callback, it
> could happen that the maintenance window the reverse offer
> referred to was already removed by a concurrent call to
> to the maintenance endpoint of Mesos.
> 
> In this case, we must not send out a reverse offer, because
> having outstanding inverse offers for an agent without
> any scheduled maintenance window will lead to a crash in
> the allocator when attempting to remove this offer.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 
> 
> 
> Diff: https://reviews.apache.org/r/67403/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Set up the reproduction environment locally and ran `while :; python call.py; 
> done` for about a minute. (see linked ticket)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67401: Removed undefined behaviour in libprocess due to order-of-evaluation bug.

2018-05-31 Thread Benjamin Mahler


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


Ship it!




Thanks for fixing this for me! I remember hearing about this once before.

Can you std::move the vector into await? That way when await is updated to take 
an rvalue reference it will not need an additional copy?

- Benjamin Mahler


On May 31, 2018, 4:11 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67401/
> ---
> 
> (Updated May 31, 2018, 4:11 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8970
> https://issues.apache.org/jira/browse/MESOS-8970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up to C++17, the only ordering constraint on the evaluation of
> expressions between synchronization points was that function
> arguments shall be evaluated before calling a function.
> 
> This could lead to the situation where `std::move(futures)` could be
> called before `await(futures.values())`, leading to a function call
> on a moved-from object and thus undefined behaviour.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> e0d0ee4c8d0df66779c7ef296a1a18d988e889b8 
> 
> 
> Diff: https://reviews.apache.org/r/67401/diff/1/
> 
> 
> Testing
> ---
> 
> `./libprocess-tests`
> 
> Internal CI run including this fix w/o libprocess segfaults: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/3655
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67403: Handled race condition when removing maintenance windows.



> On May 31, 2018, 4:41 p.m., Vinod Kone wrote:
> > Can you add a unit test for this?

It's tricky because we need very precise control over the scheduling, and I'm 
not sure our testing infrastructure provides it. But I'll look into it.


- Benno


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


On May 31, 2018, 4:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67403/
> ---
> 
> (Updated May 31, 2018, 4:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-7966
> https://issues.apache.org/jira/browse/MESOS-7966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When executing the `Master::inverseOffers()` callback, it
> could happen that the maintenance window the reverse offer
> referred to was already removed by a concurrent call to
> to the maintenance endpoint of Mesos.
> 
> In this case, we must not send out a reverse offer, because
> having outstanding inverse offers for an agent without
> any scheduled maintenance window will lead to a crash in
> the allocator when attempting to remove this offer.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 
> 
> 
> Diff: https://reviews.apache.org/r/67403/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Set up the reproduction environment locally and ran `while :; python call.py; 
> done` for about a minute. (see linked ticket)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67403: Handled race condition when removing maintenance windows.

2018-05-31 Thread Vinod Kone



> On May 31, 2018, 4:41 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Line 9459 (original), 9459 (patched)
> > 
> >
> > not yours, but can you log framework id here too?

s/framework id/*framework/


> On May 31, 2018, 4:41 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 9470 (patched)
> > 
> >
> > log framework id?

s/framework id/*framework/


- Vinod


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


On May 31, 2018, 4:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67403/
> ---
> 
> (Updated May 31, 2018, 4:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-7966
> https://issues.apache.org/jira/browse/MESOS-7966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When executing the `Master::inverseOffers()` callback, it
> could happen that the maintenance window the reverse offer
> referred to was already removed by a concurrent call to
> to the maintenance endpoint of Mesos.
> 
> In this case, we must not send out a reverse offer, because
> having outstanding inverse offers for an agent without
> any scheduled maintenance window will lead to a crash in
> the allocator when attempting to remove this offer.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 
> 
> 
> Diff: https://reviews.apache.org/r/67403/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Set up the reproduction environment locally and ran `while :; python call.py; 
> done` for about a minute. (see linked ticket)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67403: Handled race condition when removing maintenance windows.

2018-05-31 Thread Vinod Kone


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



Can you add a unit test for this?


src/master/master.cpp
Line 9459 (original), 9459 (patched)


not yours, but can you log framework id here too?



src/master/master.cpp
Lines 9466 (patched)


s/allocator/master/ ? we care about master invariant here right?



src/master/master.cpp
Lines 9470 (patched)


log framework id?


- Vinod Kone


On May 31, 2018, 4:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67403/
> ---
> 
> (Updated May 31, 2018, 4:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-7966
> https://issues.apache.org/jira/browse/MESOS-7966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When executing the `Master::inverseOffers()` callback, it
> could happen that the maintenance window the reverse offer
> referred to was already removed by a concurrent call to
> to the maintenance endpoint of Mesos.
> 
> In this case, we must not send out a reverse offer, because
> having outstanding inverse offers for an agent without
> any scheduled maintenance window will lead to a crash in
> the allocator when attempting to remove this offer.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 
> 
> 
> Diff: https://reviews.apache.org/r/67403/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Set up the reproduction environment locally and ran `while :; python call.py; 
> done` for about a minute. (see linked ticket)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67354: Removed `os::getenv()` calls from `MesosExecutorDriver`.


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




include/mesos/executor.hpp
Lines 219-220 (patched)


Instead of copying the comment from above, please explain how this c-tor is 
differet from the one above. Maybe you can also add a note for the comment 
above that it can be unsafe to use it due to getenv calls.



src/exec/exec.cpp
Lines 636-638 (patched)


really? : )


- Alexander Rukletsov


On May 29, 2018, 2:20 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67354/
> ---
> 
> (Updated May 29, 2018, 2:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, haosdent 
> huang, Ilya Pronin, and James Peach.
> 
> 
> Bugs: MESOS-3475
> https://issues.apache.org/jira/browse/MESOS-3475
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds overloaded constructor for `MesosExecutorDriver` that
> accepts `environment` parameter and stores it in the class variable.
> This new constructor is needed to get rid of `os::getenv()` calls,
> so that `MesosExecutorDriver` can be used in tests that require
> thread safety.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor.hpp d14c0369f6731100d27092142b56f108f8881003 
>   src/exec/exec.cpp 65a671d7ce83a51087d290ba039d18deba6313c2 
> 
> 
> Diff: https://reviews.apache.org/r/67354/diff/1/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 67403: Handled race condition when removing maintenance windows.


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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

When executing the `Master::inverseOffers()` callback, it
could happen that the maintenance window the reverse offer
referred to was already removed by a concurrent call to
to the maintenance endpoint of Mesos.

In this case, we must not send out a reverse offer, because
having outstanding inverse offers for an agent without
any scheduled maintenance window will lead to a crash in
the allocator when attempting to remove this offer.


Diffs
-

  src/master/master.cpp ba3f8746ea393c8655fcd5ceaace099f68df0b19 


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


Testing
---

`make check`

Set up the reproduction environment locally and ran `while :; python call.py; 
done` for about a minute. (see linked ticket)


Thanks,

Benno Evers

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



> On May 30, 2018, 1:37 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Line 253 (original), 253-261 (patched)
> > 
> >
> > I suggest to exatract it into a funciton, say `compareConstantTime`.
> 
> Alexander Rojas wrote:
> `compareConstantTime` is not a good name, since in C++ _compare_ 
> functions always returns an integer which, if less than zero means left is 
> greater than right, greater than zero means right is greater than left and 0 
> means equality, see 
> [std::comapre](http://www.cplusplus.com/reference/string/string/compare/) and 
> [memcmp](http://www.cplusplus.com/reference/cstring/memcmp/) for examples.
> 
> I also don't think it makes a lot of sense to move this little snippet to 
> a new fuction given that is only used here. But I will do it anyway.

Good point about the name, Alexander.

Regarding a separate function, I think it is much easier to spot this snippet 
if someone needs something similar in the future. Plus small doing-one-thing 
with no side effects routines simplify reading the code significantly.


- Alexander


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


On May 31, 2018, 9: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, 9: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
> 
>

Re: Review Request 67401: Removed undefined behaviour in libprocess due to order-of-evaluation bug.


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

(Updated May 31, 2018, 4:11 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Summary (updated)
-

Removed undefined behaviour in libprocess due to order-of-evaluation bug.


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


Repository: mesos


Description
---

Up to C++17, the only ordering constraint on the evaluation of
expressions between synchronization points was that function
arguments shall be evaluated before calling a function.

This could lead to the situation where `std::move(futures)` could be
called before `await(futures.values())`, leading to a function call
on a moved-from object and thus undefined behaviour.


Diffs
-

  3rdparty/libprocess/src/metrics/metrics.cpp 
e0d0ee4c8d0df66779c7ef296a1a18d988e889b8 


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


Testing (updated)
---

`./libprocess-tests`

Internal CI run including this fix w/o libprocess segfaults: 
https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/3655


Thanks,

Benno Evers

Re: Review Request 67137: Avoided leaking file descriptors in Mesos containerizer.


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



PASS: Mesos patch 67137 was successfully built and tested.

Reviews applied: `['67398', '67137']`

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

- Mesos Reviewbot Windows


On May 24, 2018, 11:13 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67137/
> ---
> 
> (Updated May 24, 2018, 11:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch explicitly closes not required file descriptors when
> forking a Mesos containerizer instance. We currently only pass on
> stdin, stout, and sterr.
> 
> While it would in principle be possible to open all file descriptors
> with `O_CLOEXEC`, this is not practical as
> 
> * `O_CLOEXEC` is not supported on BSDs (not Windows either, but file
>   descriptor leaks are prevented there in a different way), and
> * we might call thirdparty code outside of our control which does not
>   use e.g., `O_CLOEXEC` either (the case e.g., for leveldb).
> 
> Closing all file descriptors after the fork avoids leaks even in above
> scenarios.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> cec6558d0ac61bf0fec87d2e101e8f84730a765a 
> 
> 
> Diff: https://reviews.apache.org/r/67137/diff/9/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> I still need to confirm this patch in CI on a wider range of scenarios.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67185: Added request_protobuf to mesos.http.

2018-05-31 Thread Eric Chung


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

(Updated May 31, 2018, 4:05 p.m.)


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


Repository: mesos


Description (updated)
---

Added request_protobuf to mesos.http.


Diffs (updated)
-

  src/python/lib/mesos/constants.py PRE-CREATION 
  src/python/lib/mesos/http.py 073c159dc6916fa5d7349a033db022d7175aef05 
  src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
  src/python/lib/tests/test_http.py 66dd6d7c6272d1828dd591829ca3543d3430f69b 


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

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


Testing
---


Thanks,

Eric Chung

Re: Review Request 67009: Added tests of resource provider registrar recovery.


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


Ship it!




Ship It!

- Chun-Hung Hsiao


On May 29, 2018, 8:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67009/
> ---
> 
> (Updated May 29, 2018, 8:54 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-8837
> https://issues.apache.org/jira/browse/MESOS-8837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests of resource provider registrar recovery.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_provider_manager_tests.cpp 
> 77a59e4d285b455154990f9b5cf80525f26583c9 
> 
> 
> Diff: https://reviews.apache.org/r/67009/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67401: Fixed segfault in libprocess due to order-of-evaluation bug.

2018-05-31 Thread Benjamin Bannier


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


Ship it!




Ship It!

- Benjamin Bannier


On May 31, 2018, 5:47 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67401/
> ---
> 
> (Updated May 31, 2018, 5:47 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8970
> https://issues.apache.org/jira/browse/MESOS-8970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up to C++17, the only ordering constraint on the evaluation of
> expressions between synchronization points was that function
> arguments shall be evaluated before calling a function.
> 
> This could lead to the situation where `std::move(futures)` could be
> called before `await(futures.values())`, leading to a function call
> on a moved-from object and thus undefined behaviour.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> e0d0ee4c8d0df66779c7ef296a1a18d988e889b8 
> 
> 
> Diff: https://reviews.apache.org/r/67401/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 67401: Fixed segfault in libprocess due to order-of-evaluation bug.


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


Ship it!




Ship It!

- Alexander Rukletsov


On May 31, 2018, 3:47 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67401/
> ---
> 
> (Updated May 31, 2018, 3:47 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8970
> https://issues.apache.org/jira/browse/MESOS-8970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Up to C++17, the only ordering constraint on the evaluation of
> expressions between synchronization points was that function
> arguments shall be evaluated before calling a function.
> 
> This could lead to the situation where `std::move(futures)` could be
> called before `await(futures.values())`, leading to a function call
> on a moved-from object and thus undefined behaviour.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> e0d0ee4c8d0df66779c7ef296a1a18d988e889b8 
> 
> 
> Diff: https://reviews.apache.org/r/67401/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

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

2018-05-31 Thread Eric Chung


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

(Updated May 31, 2018, 3:55 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 (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/5/

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


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 67398: Changed nested container tests to not use pipes for synchronization.

2018-05-31 Thread Benjamin Bannier


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




src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Line 429 (original), 454 (patched)


I still see this test failing in our internal CI, need to investigate what 
is amiss here.


- Benjamin Bannier


On May 31, 2018, 4:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated May 31, 2018, 4:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 67401: Fixed segfault in libprocess due to order-of-evaluation bug.


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

Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

Up to C++17, the only ordering constraint on the evaluation of
expressions between synchronization points was that function
arguments shall be evaluated before calling a function.

This could lead to the situation where `std::move(futures)` could be
called before `await(futures.values())`, leading to a function call
on a moved-from object and thus undefined behaviour.


Diffs
-

  3rdparty/libprocess/src/metrics/metrics.cpp 
e0d0ee4c8d0df66779c7ef296a1a18d988e889b8 


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


Testing
---


Thanks,

Benno Evers

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

2018-05-31 Thread Eric Chung


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

(Updated May 31, 2018, 3:14 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 (updated)
-

  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/4/

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


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

Review Request 67398: Changed nested container tests to not use pipes for synchronization.

2018-05-31 Thread Benjamin Bannier


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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Some tests of nested container functionality used pipes passed to
launched tasks to detect whether a task has actually started executing
the workload (`TASK_RUNNING` updates might be sent before the task
workload is actually started).

Once we avoid leaking unspecified file descriptors into forked
processes, this test setup will be broken. In this patch we replace
the use of pipes for synchronization with HTTP requests to an actor
running in the tests.


Diffs
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
6050e6ebed87249382d56aedb6d98d3cf9812bb9 


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


Testing
---

`sudo make check`


Thanks,

Benjamin Bannier

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


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



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 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
> 
>

Re: Review Request 67009: Added tests of resource provider registrar recovery.