date:20170726

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

2017-07-26 Thread Mesos Reviewbot Windows


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



Patch looks great!

Reviews applied: [58939]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 27, 2017, 2:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58939/
> ---
> 
> (Updated July 27, 2017, 2:40 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: mesos-7374
> https://issues.apache.org/jira/browse/mesos-7374
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
> launcher is used when launching a mesos containerizer with an image
> under Linux. This prevents the executor from messing up with the host
> filesystem. The check is in `MesosContainerizerProcess::prepare()`
> after provisioning and before launching, since provisioning itself
> does not depend on the filesystem isolator.
> 
> Also checked that the 'filesystem/linux' is enabled and the 'linux'
> launcher is used when enabling the 'docker/runtime' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/containerizer/mesos/provisioner/store.cpp 
> cc5cc81e05f29bb0e11ffa13cdb8d63d4397114f 
> 
> 
> Diff: https://reviews.apache.org/r/58939/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on a simplified case of mesos-7374.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

2017-07-26 Thread Chun-Hung Hsiao


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

(Updated July 27, 2017, 2:40 a.m.)


Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.


Changes
---

Fixed a typo.


Bugs: mesos-7374
https://issues.apache.org/jira/browse/mesos-7374


Repository: mesos


Description
---

Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
launcher is used when launching a mesos containerizer with an image
under Linux. This prevents the executor from messing up with the host
filesystem. The check is in `MesosContainerizerProcess::prepare()`
after provisioning and before launching, since provisioning itself
does not depend on the filesystem isolator.

Also checked that the 'filesystem/linux' is enabled and the 'linux'
launcher is used when enabling the 'docker/runtime' isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
  src/slave/containerizer/mesos/provisioner/store.cpp 
cc5cc81e05f29bb0e11ffa13cdb8d63d4397114f 


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

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


Testing
---

sudo make check
Manually tested on a simplified case of mesos-7374.


Thanks,

Chun-Hung Hsiao

Re: Review Request 61157: Refactored ProcessManager::handle for future use with http::Server.

2017-07-26 Thread Mesos Reviewbot Windows


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



Bad patch!

Reviews applied: [61157, 61156, 61155, 61154, 61153, 61152, 61151, 61150, 
61149, 61148, 61147, 55324, 55323, 55322, 55321, 55320]

Failed command: python support/apply-reviews.py -n -r 55320

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 158, in apply_review
fetch_patch(options)
  File "support/apply-reviews.py", line 191, in fetch_patch
context=ssl_create_default_context())
  File "C:\Python27\lib\urllib2.py", line 154, in urlopen
return opener.open(url, data, timeout)
  File "C:\Python27\lib\urllib2.py", line 435, in open
response = meth(req, response)
  File "C:\Python27\lib\urllib2.py", line 548, in http_response
'http', request, response, code, msg, hdrs)
  File "C:\Python27\lib\urllib2.py", line 473, in error
return self._call_chain(*args)
  File "C:\Python27\lib\urllib2.py", line 407, in _call_chain
result = func(*args)
  File "C:\Python27\lib\urllib2.py", line 556, in http_error_default
raise HTTPError(req.get_full_url(), code, msg, hdrs, fp)
urllib2.HTTPError: HTTP Error 404: NOT FOUND

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/204/console

- Mesos Reviewbot Windows


On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61157/
> ---
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored ProcessManager::handle for future use with http::Server.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> b268cdad776a3ca2a87cbe60eb098bde2a70667c 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> ed11909a2a5e3214fa974bdea098f4057cea9666 
> 
> 
> Diff: https://reviews.apache.org/r/61157/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Review Request 61155: Added http::Server.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added http::Server.


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
f637999174d92a98208b5fc49a65f9929efb11a0 
  3rdparty/libprocess/src/http.cpp a4d71fb6c345d3c7a7611004830f6c2c0fbf6046 
  3rdparty/libprocess/src/tests/http_tests.cpp 
dde05f6a554fcb8c6c89e690bbdcd2bf509854d5 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61154: Added safe downcasts for Socket.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added safe downcasts for Socket.


Diffs
-

  3rdparty/libprocess/include/process/socket.hpp 
ae6154d5d142f65352e00f37b4e66d0b62fdc3c2 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61147: Added Future::onAbandoned and Future::isAbandoned.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added Future::onAbandoned and Future::isAbandoned.


Diffs
-

  3rdparty/libprocess/include/process/future.hpp 
cce950509f58022e79bb51a6e72ea1a005b9cb50 
  3rdparty/libprocess/include/process/queue.hpp 
ab08e30df742412f22a06202526f8b55350ed435 
  3rdparty/libprocess/src/http.cpp a4d71fb6c345d3c7a7611004830f6c2c0fbf6046 
  3rdparty/libprocess/src/tests/collect_tests.cpp 
155e0bb75cf723a0a6c29020f9f767e3ba3d7401 
  3rdparty/libprocess/src/tests/future_tests.cpp 
0c8725b9a5e64aaac6e3979e450a11e84f9bd45e 
  3rdparty/libprocess/src/tests/metrics_tests.cpp 
161ca0dc7aea526d450d71a80839d8cc075aaa31 
  3rdparty/libprocess/src/tests/process_tests.cpp 
ed11909a2a5e3214fa974bdea098f4057cea9666 
  3rdparty/libprocess/src/tests/shared_tests.cpp 
2a2ffe76b7b7ce016b559de7b5d3a28a06f422ef 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61149: Added Future::condition.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added Future::condition.


Diffs
-

  3rdparty/libprocess/include/process/future.hpp 
cce950509f58022e79bb51a6e72ea1a005b9cb50 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61148: Added Future::recover.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added Future::recover.


Diffs
-

  3rdparty/libprocess/include/process/future.hpp 
cce950509f58022e79bb51a6e72ea1a005b9cb50 
  3rdparty/libprocess/src/tests/future_tests.cpp 
0c8725b9a5e64aaac6e3979e450a11e84f9bd45e 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61157: Refactored ProcessManager::handle for future use with http::Server.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Refactored ProcessManager::handle for future use with http::Server.


Diffs
-

  3rdparty/libprocess/src/process.cpp b268cdad776a3ca2a87cbe60eb098bde2a70667c 
  3rdparty/libprocess/src/tests/process_tests.cpp 
ed11909a2a5e3214fa974bdea098f4057cea9666 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61150: Added Future::onAbandoned semantics to process::collect/await.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added Future::onAbandoned semantics to process::collect/await.


Diffs
-

  3rdparty/libprocess/include/process/collect.hpp 
fccf55a26a2ef61fa3b73d100a0741832e4dfa56 
  3rdparty/libprocess/src/tests/collect_tests.cpp 
155e0bb75cf723a0a6c29020f9f767e3ba3d7401 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61152: Added overload of process::await that takes and returns single future.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added overload of process::await that takes and returns single future.


Diffs
-

  3rdparty/libprocess/include/process/collect.hpp 
fccf55a26a2ef61fa3b73d100a0741832e4dfa56 
  3rdparty/libprocess/src/tests/collect_tests.cpp 
155e0bb75cf723a0a6c29020f9f767e3ba3d7401 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61153: Added discard happens-before relationship in process::await/collect.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added discard happens-before relationship in process::await/collect.


Diffs
-

  3rdparty/libprocess/include/process/collect.hpp 
fccf55a26a2ef61fa3b73d100a0741832e4dfa56 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61156: Factored out HttpProxy.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Factored out HttpProxy.


Diffs
-

  3rdparty/libprocess/Makefile.am 378a4340df56b1d84fbd46f89875ca56ca0a8997 
  3rdparty/libprocess/src/CMakeLists.txt 
f97291bd7cadcb440231619a8a2d1029a447ec27 
  3rdparty/libprocess/src/http_proxy.hpp PRE-CREATION 
  3rdparty/libprocess/src/http_proxy.cpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp b268cdad776a3ca2a87cbe60eb098bde2a70667c 
  3rdparty/libprocess/src/socket_manager.hpp PRE-CREATION 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 61151: Used Future::recover in Mesos.

2017-07-26 Thread Benjamin Hindman


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Used Future::recover in Mesos.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
64180944f8680828781168faa67417489abf1bc8 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 60440: Updated local development workflow of mesos website.

2017-07-26 Thread Mesos Reviewbot Windows


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



Patch looks great!

Reviews applied: [60438, 60439, 60440]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 27, 2017, 12:30 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60440/
> ---
> 
> (Updated July 27, 2017, 12:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the layout and scripts consistent with CI based automatic
> publishing of the website.
> 
> 
> Diffs
> -
> 
>   site/Dockerfile 230cfc779fe4f183d63cd99ef26dc540c68bff85 
>   site/README.md ebd3e6a0fea7ae0fe3b28719bcab28ee8f7c356c 
>   site/build.sh 06b1b32a5cdfaf2f9a69ce59339e0fd671e335de 
>   site/entrypoint.sh PRE-CREATION 
>   site/mesos-website-dev.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60440/diff/2/
> 
> 
> Testing
> ---
> 
> Tested by running the script locally.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60996: Adds documentation for LIBPROCESS_SSL_ECDH_CURVE environment variable.

2017-07-26 Thread Till Toenshoff



> On July 26, 2017, 11:08 p.m., Gastón Kleiman wrote:
> > docs/ssl.md
> > Lines 81 (patched)
> > 
> >
> > There was only one empty line before, do we need two?

True - thanks a bunch again :)


- Till


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


On July 21, 2017, 9:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60996/
> ---
> 
> (Updated July 21, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the adequate documentation entry for the new
> `LIBPROCESS_SSL_ECDH_CURVE` environment variable, which allow the
> configuration of ECDHE key exchange while establishing TLS sessions.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 811390f366c97b6d61cf9b3f188e3c399abb46cd 
> 
> 
> Diff: https://reviews.apache.org/r/60996/diff/2/
> 
> 
> Testing
> ---
> 
> non functional change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 60996: Adds documentation for LIBPROCESS_SSL_ECDH_CURVE environment variable.

2017-07-26 Thread Till Toenshoff


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




docs/ssl.md
Lines 81 (patched)


Indeed - thanks!


- Till Toenshoff


On July 21, 2017, 9:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60996/
> ---
> 
> (Updated July 21, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the adequate documentation entry for the new
> `LIBPROCESS_SSL_ECDH_CURVE` environment variable, which allow the
> configuration of ECDHE key exchange while establishing TLS sessions.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 811390f366c97b6d61cf9b3f188e3c399abb46cd 
> 
> 
> Diff: https://reviews.apache.org/r/60996/diff/2/
> 
> 
> Testing
> ---
> 
> non functional change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 60440: Updated local development workflow of mesos website.

2017-07-26 Thread Vinod Kone


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

(Updated July 27, 2017, 12:30 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent huang.


Changes
---

Updated comment and ran rake with bundle.


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


Repository: mesos


Description
---

Made the layout and scripts consistent with CI based automatic
publishing of the website.


Diffs (updated)
-

  site/Dockerfile 230cfc779fe4f183d63cd99ef26dc540c68bff85 
  site/README.md ebd3e6a0fea7ae0fe3b28719bcab28ee8f7c356c 
  site/build.sh 06b1b32a5cdfaf2f9a69ce59339e0fd671e335de 
  site/entrypoint.sh PRE-CREATION 
  site/mesos-website-dev.sh PRE-CREATION 


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

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


Testing
---

Tested by running the script locally.


Thanks,

Vinod Kone

Re: Review Request 60439: Added scripts to automate website publishing.

2017-07-26 Thread Vinod Kone



> On July 3, 2017, 4:47 p.m., haosdent huang wrote:
> > support/mesos-website/entrypoint.sh
> > Lines 32 (patched)
> > 
> >
> > Is it OK to run support/mesos-website/build.sh as root?

looks like you answered yourself based on the comment below. and no, this 
script should be run as non-root as commented in support/mesos-website.sh


- Vinod


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


On June 29, 2017, 9:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60439/
> ---
> 
> (Updated June 29, 2017, 9:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These scripts are expected to be run by ASF CI on any commit to
> the mesos repo. The directory layout is similar to tidybot CI job.
> 
> 
> Diffs
> -
> 
>   support/jenkins/websitebot.sh PRE-CREATION 
>   support/mesos-website.sh PRE-CREATION 
>   support/mesos-website/Dockerfile PRE-CREATION 
>   support/mesos-website/build.sh PRE-CREATION 
>   support/mesos-website/entrypoint.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60439/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running a CI job pointing to a branch containing this patch.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60440: Updated local development workflow of mesos website.

2017-07-26 Thread Vinod Kone



> On July 4, 2017, 12:10 p.m., Benjamin Bannier wrote:
> > I am confused why we need this at all at this point. With 
> > https://reviews.apache.org/r/60439/ we should be able to generate the site 
> > locally and look at its output. Executing the rake rules is already now 
> > pretty much possible (on macOS `rake` needs to be added to the `Gemfile`) 
> > if the needed tools are locally available, if not, we provide the 
> > container. We seem to add a lot of elaborate tooling to isolate us from 
> > something which is already an abstraction (i.e., docker around rake).
> > 
> > Can we get rid of this container completely and instead streamline the 
> > existing `support/website` and `site/Rakefile` toolings instead of adding 
> > another one?

See comment above.


- Vinod


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


On June 29, 2017, 9:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60440/
> ---
> 
> (Updated June 29, 2017, 9:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the layout and scripts consistent with CI based automatic
> publishing of the website.
> 
> 
> Diffs
> -
> 
>   site/Dockerfile 8ba0be0c28e924f7a2b89e6e5a3237deb3751a41 
>   site/README.md ebd3e6a0fea7ae0fe3b28719bcab28ee8f7c356c 
>   site/build.sh 11f15e15621c4d3db1472e88911787b9b3100f97 
>   site/entrypoint.sh PRE-CREATION 
>   site/mesos-website-dev.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60440/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running the script locally.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60439: Added scripts to automate website publishing.

2017-07-26 Thread Vinod Kone



> On July 3, 2017, 5:11 p.m., haosdent huang wrote:
> > Really sorry for the delay. LGTM, just want to sure is it possible to merge 
> > build.sh into entrypoint.sh

We need to keep this separate because build.sh needs to be run as non-root 
user, whereas entrypoint.sh need to be run as root user.


- Vinod


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


On June 29, 2017, 9:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60439/
> ---
> 
> (Updated June 29, 2017, 9:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These scripts are expected to be run by ASF CI on any commit to
> the mesos repo. The directory layout is similar to tidybot CI job.
> 
> 
> Diffs
> -
> 
>   support/jenkins/websitebot.sh PRE-CREATION 
>   support/mesos-website.sh PRE-CREATION 
>   support/mesos-website/Dockerfile PRE-CREATION 
>   support/mesos-website/build.sh PRE-CREATION 
>   support/mesos-website/entrypoint.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60439/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running a CI job pointing to a branch containing this patch.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60439: Added scripts to automate website publishing.

2017-07-26 Thread Vinod Kone


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

(Updated July 27, 2017, 12:28 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent huang.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

These scripts are expected to be run by ASF CI on any commit to
the mesos repo. The directory layout is similar to tidybot CI job.


Diffs (updated)
-

  support/jenkins/websitebot.sh PRE-CREATION 
  support/mesos-website.sh PRE-CREATION 
  support/mesos-website/Dockerfile PRE-CREATION 
  support/mesos-website/build.sh PRE-CREATION 
  support/mesos-website/entrypoint.sh PRE-CREATION 


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

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


Testing
---

Tested by running a CI job pointing to a branch containing this patch.


Thanks,

Vinod Kone

Re: Review Request 60439: Added scripts to automate website publishing.

2017-07-26 Thread Vinod Kone



> On July 4, 2017, 12:02 p.m., Benjamin Bannier wrote:
> > This is a great start, but I would prefer if we wouldn't directly mount an 
> > existing source directory into the container and bootstrap, configure and 
> > build in it (this is also broken for e.g., in-source builds).
> > 
> > What about instead modify mounting the Mesos source tree into the 
> > container, then inside the container cloning from it, and building in the 
> > clean checkout? This woudl not only minimize the risk of destructive 
> > side-effects outside the container, but also e.g., make sure that we do not 
> > by accident pick up uncommitted changes. This is the approach we also 
> > followed in `support/mesos-tidy`.
> > 
> > I left some first comments, but will look again once the handling of the 
> > source tree is under control.

Good point.

This script was originally intended to be used by CI and not by human users. 
The CI job does a clean checkout of mesos source and site repos, so there 
shouldn't be any problem with picking up unexpected uncommitted changes from 
mesos source repo. If this script will be eventually used by humans as well, as 
suggested in the next review, then yes I agree we need to be more careful. 
Although for dev workflow case, it might actually be preferrable that the 
website dev build picks up uncommitted changes while testing; otherwise users 
need to commit their code to see how it looks on website while dev/testing, 
which is not a great experience IMO. 

So I would like to punt on this for now until we figure out merging the dev and 
publish workflows.


> On July 4, 2017, 12:02 p.m., Benjamin Bannier wrote:
> > support/jenkins/websitebot.sh
> > Lines 31 (patched)
> > 
> >
> > If the source tree contains uncommitted changes, the output will not 
> > reproducibly correspond to this SHA1.
> > 
> > I'd suggest to instead clone the source tree instead.

see my comment above.


> On July 4, 2017, 12:02 p.m., Benjamin Bannier wrote:
> > support/mesos-website.sh
> > Lines 33 (patched)
> > 
> >
> > Not crucial here right now, but let's put this string into single 
> > quotes, see https://github.com/koalaman/shellcheck/wiki/SC2064.
> > 
> > Also please consider working on getting this image published in the 
> > Mesos dockerhub org so we can pull from there instead of having the rebuild 
> > the same image over and over again. See e.g., `support/mesos-tidy.sh` how 
> > we already do this for other images.

Yea, I very much liked to use a image hosted on dockerhub; but one thing at a 
time :)


> On July 4, 2017, 12:02 p.m., Benjamin Bannier wrote:
> > support/mesos-website/Dockerfile
> > Lines 1-11 (patched)
> > 
> >
> > We now have one Docker image to run tests in CI (via 
> > `docker_build.sh`), one to run clang-tidy checks (`mesos-tidy/Dockerfile`), 
> > and are adding another one  here to build the website.
> > 
> > We should try to reduce the number of these images or remove 
> > duplication, e.g., this image could be in principle implemented as a small 
> > addon on top of `mesos/mesos-tidy`
> > 
> > FROM mesos/mesos-tidy:latest
> > LABEL Description="This image is used for generating Mesos web 
> > site."
> > 
> > # Install ruby and doxygen tools ...

I agree. We'll fix it as part of 
https://issues.apache.org/jira/browse/MESOS-6984


- Vinod


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


On June 29, 2017, 9:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60439/
> ---
> 
> (Updated June 29, 2017, 9:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These scripts are expected to be run by ASF CI on any commit to
> the mesos repo. The directory layout is similar to tidybot CI job.
> 
> 
> Diffs
> -
> 
>   support/jenkins/websitebot.sh PRE-CREATION 
>   support/mesos-website.sh PRE-CREATION 
>   support/mesos-website/Dockerfile PRE-CREATION 
>   support/mesos-website/build.sh PRE-CREATION 
>   support/mesos-website/entrypoint.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60439/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running a CI job pointing to a branch containing this patch.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60440: Updated local development workflow of mesos website.

2017-07-26 Thread Vinod Kone



> On July 3, 2017, 5:15 p.m., haosdent huang wrote:
> > Should we make `support/mesos-website/entrypoint.sh` support a environment 
> > variable `MODE=dev` and then we `export MODE=dev` in 
> > `site/mesos-website-dev.sh` and just call `support/mesos-website.sh`? It 
> > would help to avoid write duplicate code in different places.

There are some differences between the publish and dev workflows:

-- "publish" Dockerfile needs deps that are needed to build mesos and generate 
endpoint doc in addition to generating website.
-- "dev" Dockerfile only needs deps for generating the website. 
-- In "dev" workflow the developer is expected to build mesos and generate 
endpoint docs outside the Dockefile; this is it allow fast iteration. For 
example if a developer is only making changes to docs or website pages, no need 
to re-build mesos and endpoint docs.
-- `docker run` command params are slightly different because dev workflow 
needs interactive mode and ports exposed
-- `build.sh` script is different for each workflow because of the first 2 
points.
-- `entrypoint.sh` script is different for each workflow because of the 
discrepancies between how Docker works on Linux and MacOS. 


I agree that we should ideally merge the two workflows to avoid code 
duplication or as Bannier suggested below to just get rid of Docker for dev 
workflow. I'm actually in favor of the latter unless there is a strong reason 
to use docker for the dev workflow. If we agree on that I can update this patch.


- Vinod


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


On June 29, 2017, 9:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60440/
> ---
> 
> (Updated June 29, 2017, 9:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the layout and scripts consistent with CI based automatic
> publishing of the website.
> 
> 
> Diffs
> -
> 
>   site/Dockerfile 8ba0be0c28e924f7a2b89e6e5a3237deb3751a41 
>   site/README.md ebd3e6a0fea7ae0fe3b28719bcab28ee8f7c356c 
>   site/build.sh 11f15e15621c4d3db1472e88911787b9b3100f97 
>   site/entrypoint.sh PRE-CREATION 
>   site/mesos-website-dev.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60440/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running the script locally.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60996: Adds documentation for LIBPROCESS_SSL_ECDH_CURVE environment variable.

2017-07-26 Thread Till Toenshoff



> On July 26, 2017, 11:12 p.m., Greg Mann wrote:
> > docs/ssl.md
> > Lines 78-79 (patched)
> > 
> >
> > Proposing a couple small tweaks for grammar/spelling:
> > 
> > ```
> > List of elliptic curves which should be used for ECDHE-based cipher 
> > suites, in preferred order. Available values depend on the OpenSSL version 
> > used. Default value `auto` allows OpenSSL to pick the curve automatically.
> > OpenSSL versions prior to `1.0.2` allow for the use of only one curve; 
> > in those cases, `auto` defaults to `prime256v1`.
> > ```

Thanks Greg - much appreciated -- will fix while committing.


- Till


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


On July 21, 2017, 9:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60996/
> ---
> 
> (Updated July 21, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the adequate documentation entry for the new
> `LIBPROCESS_SSL_ECDH_CURVE` environment variable, which allow the
> configuration of ECDHE key exchange while establishing TLS sessions.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 811390f366c97b6d61cf9b3f188e3c399abb46cd 
> 
> 
> Diff: https://reviews.apache.org/r/60996/diff/2/
> 
> 
> Testing
> ---
> 
> non functional change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 60438: Updated endpoint help generator script to work inside Docker.

2017-07-26 Thread Vinod Kone


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

(Updated July 27, 2017, 12:27 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent huang.


Changes
---

Added comment per BenM's suggestion. NNFR.


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


Repository: mesos


Description
---

Changed agent flags to make this script work inside Docker container.
This is needed because this script will be run as part of website
publishing process which runs on ASF CI inside Docker.


Diffs (updated)
-

  support/generate-endpoint-help.py 19cff57810863a5050db73647348c28858a9499e 


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

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


Testing
---

Tested by running a CI job pointing to a branch containing this patch.


Thanks,

Vinod Kone

Re: Review Request 60913: Adds support for OpenSSL's ECDH key exchange.

2017-07-26 Thread Till Toenshoff



> On July 19, 2017, 11:50 a.m., Jan-Philip Gehrcke wrote:
> > Hey! This looks great.
> > 
> > I have a few remarks.
> > 
> > 1) The test in this patch seems to prove that the patch introduces support 
> > for the _ephemeral_ elliptic curve Diffie-Hellman key exchange, usually 
> > abbreviated with ECDHE. Does this patch also add ECDH (non-ephemeral key 
> > exchange, where an ever-static ECDH public key is used) support? If it does 
> > not then I think this is good news :-) I'd appreciate a comment on that in 
> > the patch description. If this can be controlled via external configuration 
> > through the cipher suite string/list, then this is an appropriate answer.
> > 
> > 2) Even ECDHE can be used in two modes. Related to the first point we 
> > should stress in the patch description and also via a code comment that 
> > this patch explicitly disables an optimization called "ephemeral-static 
> > ECDH" via the SSL_OP_SINGLE_ECDH_USE option. I found 
> > https://forum.nginx.org/read.php?29,241610,249058 insightful, but also 
> > https://eprint.iacr.org/2011/633.pdf from which I'd like to quote:
> > 
> > "2. Use of ephemeral ECDH-based cipher suites (e.g., ECDHE-ECDSA and 
> > ECDHE-RSA)
> > in combination with the OpenSSL ephemeral-static ECDH optimisation. In 
> > such cipher
> > suites, and according to the TLS specification, a fresh ECDH public key 
> > should be
> > generated for each key exchange. However OpenSSL allows one-time 
> > generation of said
> > key when the TLS server is initialised, sharing it across an arbitrary 
> > number of key
> > exchanges thereafter."
> > 
> > 3) This patch contains a test that proves that RSA-signed key exchange 
> > works (i.e. ECDHE_RSA type cipher suites). I assume that this patch also 
> > allows for ECDSA-signed key exchange (i.e. ECDHE_ECDSA type cipher suites), 
> > but it just requires a corresponding private key type. I'd appreciate to 
> > know if you agree with that or not :-).
> > 
> > 4) I think we should adjust the commit message saying "Adds support for 
> > OpenSSL's ECDHE key exchange"

re 4: We commonly use past tense for the RR subject and present tense for the 
description. We also always terminate with a period. Both the summary and the 
description form the commit message as a whole. Hence it should be "Added 
support for OpenSSL's ECDHE key exchange."


- Till


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


On July 26, 2017, 9:25 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60913/
> ---
> 
> (Updated July 26, 2017, 9:25 a.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the configuration necesary so that the Elliptic Curve
> Diffie Hellman algorithm can be used for TLS key exchange if the
> OpenSSL version used supports it.
> 
> It also adds the ssl flag `LIBPROCESS_SSL_ECDH_CURVES` which allows
> for the specification of a specific elliptic curve (or set of curves).
> 
> Users will need to specify the TLS cipher suite that uses ECDH in order
> to enable the new key exchange. By default Mesos does not use any ECDH
> cipher suites.
> 
> Support for ephemeral ECDH public keys is the default, so that new
> public keys are generated for each exchange.
> 
> Note that in order to enable ECDSA ciphers an ECDSA is still necesary
> instead of the more traditionl RSA one.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
>   3rdparty/libprocess/src/openssl.hpp 
> 7ded2c74b2f92aacfa0f366bd27d5e0df2b8f25c 
>   3rdparty/libprocess/src/openssl.cpp 
> e6f17e4591f573186e1dc9697e1e7b60a841fe4f 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 8a14dcb865dfab34fb4d0d51f42a28a913fb7ace 
> 
> 
> Diff: https://reviews.apache.org/r/60913/diff/7/
> 
> 
> Testing
> ---
> 
> ```shell
> make check
> ```
> 
> Launched Mesos with only ECDHE handshake ciphers enabled
> 
> ```shell
> LIBPROCESS_SSL_ENABLED=1 \
> LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
> LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
> LIBPROCESS_SSL_CIPHERS="ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA"
>  \
> ./bin/mesos-master.sh \
> --work_dir=/tmp/mesos/master \
> --log_dir=/tmp/mesos/master/log
> ```
> 
> Then in another shell:
> 
> ```shell
> http -v --verify=no https://${MESOS_MASTER_IP}:5050/state
> 
> #

Re: Review Request 61097: Added gRPC support in libprocess.

2017-07-26 Thread Chun-Hung Hsiao

> On July 26, 2017, 10:49 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/grpc.hpp
> > Lines 123 (patched)
> > 
> >
> > who is going to delete the lambda function?

This lambda will be retrieved and managed by a `shared_ptr` in 
`client::Runtime::Data::loop()`. When it will be deleted depends on if the call 
is successful. If so, the `shared_ptr` will be captured by another lambda that 
is dispatched to `client::Runtime::process`, and thus this lambda will be 
deleted after execution; otherwise, this lambda will be deleted at the end of 
the lifecycle of the `shared_ptr`. It is required that a `CompletionQueue` must 
be drained before getting destructed, so a regular termination process would 
call `CompletionQueue::Shutdown()` which makes `CompletionQueue::Next()` to 
return pending calls as failures in `client::Runtime::Data::loop()`, thus all 
allocated lambdas will eventually be deleted.

- Chun-Hung

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

On July 25, 2017, 9:13 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61097/
> ---
> 
> (Updated July 25, 2017, 9:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7810
> https://issues.apache.org/jira/browse/MESOS-7810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A gRPC client can use `process::grpc::call(...)` to send an asynchronous
> gRPC call and get a future for the response. The client needs to set up
> two data structures: a `Channel` which represents a connection to a gRPC
> server, and a `ClientRuntime` which maintains a `CompletionQueue` that
> keeps track of all pending asynchronous gRPC calls, and spawns a thread
> waiting for any response from the `CompletionQueue`. All gRPC calls
> using the same `ClientRuntime` would share the same thread.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/grpc.hpp PRE-CREATION 
>   3rdparty/libprocess/src/grpc.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61097/diff/2/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 60771: Implemented the 'SUBSCRIBE' call in the resource provider manager.

2017-07-26 Thread Jie Yu


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


Ship it!




Ship It!

- Jie Yu


On July 26, 2017, 9:35 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60771/
> ---
> 
> (Updated July 26, 2017, 9:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7780
> https://issues.apache.org/jira/browse/MESOS-7780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using HTTP, resource providers are expected to subscribe to the resource
> provider manager. The resource provider manager will assign a unique ID
> to subscribed resource providers and use events to communicate with
> them.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp a9bf258a0896bd519b99a689a0bfab64d665172f 
>   src/tests/resource_provider_http_api_tests.cpp 
> 46a3f37ba046201e9d79e8ba037da2cb37f5c31f 
> 
> 
> Diff: https://reviews.apache.org/r/60771/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

2017-07-26 Thread Mesos Reviewbot Windows


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



Patch looks great!

Reviews applied: [58939]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 26, 2017, 11:29 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58939/
> ---
> 
> (Updated July 26, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: mesos-7374
> https://issues.apache.org/jira/browse/mesos-7374
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
> launcher is used when launching a mesos containerizer with an image
> under Linux. This prevents the executor from messing up with the host
> filesystem. The check is in `MesosContainerizerProcess::prepare()`
> after provisioning and before launching, since provisioning itself
> does not depend on the filesystem isolator.
> 
> Also checked that the 'filesystem/linux' is enabled and the 'linux'
> launcher is used when enabling the 'docker/runtime' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/containerizer/mesos/provisioner/store.cpp 
> cc5cc81e05f29bb0e11ffa13cdb8d63d4397114f 
> 
> 
> Diff: https://reviews.apache.org/r/58939/diff/7/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on a simplified case of mesos-7374.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 60770: Added validation functions for resource provider calls.

2017-07-26 Thread Jie Yu


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


Ship it!




Ship It!

- Jie Yu


On July 26, 2017, 10:48 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60770/
> ---
> 
> (Updated July 26, 2017, 10:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7780
> https://issues.apache.org/jira/browse/MESOS-7780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation functions for resource provider calls.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 51b67428f823454665db695ba70a0dc896a7595c 
>   src/Makefile.am 8b91716432fb2f6210c12ed7edb312f49e57aa4c 
>   src/internal/devolve.hpp 214fcefe07950590ba021c1e0b3aaa3e64231f81 
>   src/internal/devolve.cpp 79c668ff86bb3b1fddc48bcd978f9d4fc026a415 
>   src/resource_provider/validation.hpp PRE-CREATION 
>   src/resource_provider/validation.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt f5fe5a6785e2d230707396cc42d9f824f2539dba 
>   src/tests/resource_provider_validation_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60770/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 60996: Adds documentation for LIBPROCESS_SSL_ECDH_CURVE environment variable.

2017-07-26 Thread Till Toenshoff



> On July 26, 2017, 11:08 p.m., Gastón Kleiman wrote:
> > docs/ssl.md
> > Lines 78 (patched)
> > 
> >
> > s/Valid values depends on OpenSSL version used/Valid values depend on 
> > the OpenSSL version used./

Indeed - commented on that earlier but missed the remaining "depends" -- thanks!


- Till


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


On July 21, 2017, 9:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60996/
> ---
> 
> (Updated July 21, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the adequate documentation entry for the new
> `LIBPROCESS_SSL_ECDH_CURVE` environment variable, which allow the
> configuration of ECDHE key exchange while establishing TLS sessions.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 811390f366c97b6d61cf9b3f188e3c399abb46cd 
> 
> 
> Diff: https://reviews.apache.org/r/60996/diff/2/
> 
> 
> Testing
> ---
> 
> non functional change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 60996: Adds documentation for LIBPROCESS_SSL_ECDH_CURVE environment variable.

2017-07-26 Thread Till Toenshoff



> On July 26, 2017, 11:08 p.m., Gastón Kleiman wrote:
> > Nit: AFAIK we try to write our commit messages in simple past.

The summary should be past tense, description present tense.


- Till


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


On July 21, 2017, 9:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60996/
> ---
> 
> (Updated July 21, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the adequate documentation entry for the new
> `LIBPROCESS_SSL_ECDH_CURVE` environment variable, which allow the
> configuration of ECDHE key exchange while establishing TLS sessions.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 811390f366c97b6d61cf9b3f188e3c399abb46cd 
> 
> 
> Diff: https://reviews.apache.org/r/60996/diff/2/
> 
> 
> Testing
> ---
> 
> non functional change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 60524: Stout: Made the `Duration` operators handle int overflows explicitly.

2017-07-26 Thread Gastón Kleiman


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

(Updated July 26, 2017, 11:31 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, and 
Vinod Kone.


Changes
---

Reordered the chain.

We haven't reached consensus on this patch yet, it is likely that I'll end up 
discarding it.


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


Repository: mesos


Description
---

Made the `Duration` arithmetic operators return `Duration::max()` if the
operation would result in an integer overflow, and `Duration::min()` if
it would result in an underflow.


Diffs (updated)
-

  3rdparty/stout/include/stout/duration.hpp 
b0cd77b833f6fbf752b4db820fd43b87e1d1e476 
  3rdparty/stout/tests/duration_tests.cpp 
59b08f14849a8db31f11fbd0b2e1248c99afd9dd 


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

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


Testing
---

Added new Stout tests and confirmed that the Mesos test suite still passes.


Thanks,

Gastón Kleiman

Re: Review Request 60525: Fixed the default filter used by the allocator.

2017-07-26 Thread Gastón Kleiman


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

(Updated July 26, 2017, 11:30 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, and 
Vinod Kone.


Changes
---

I had some offline discussions with Greg, BenM, and Vinod. We agreed to cap the 
filter duration to at most 365 days.


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


Repository: mesos


Description (updated)
---

If a framework accepts/refuses an offer using a very long filter, the
`HierarchicalAllocator` will use the default filter instead. Meaning
that it will filter the resources for only 5 seconds. This can happen
when a framework sets `Filter::refuse_seconds` to a number of seconds
larger than what fits in `Duration`.

This patch makes the hierarchical allocator cap the filter duration to
at most 365 days.


Diffs (updated)
-

  include/mesos/mesos.proto 41e42b4996235cbee26f580f4a7aa2daed166b7f 
  include/mesos/v1/mesos.proto 9de282f85b8d0ba91fa3de85acd5d0f4082a47d8 
  src/master/allocator/mesos/hierarchical.cpp 
f021c34ef11aac42026ba39c5a1b775794982035 


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

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


Testing
---

`make check`


Thanks,

Gastón Kleiman

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

2017-07-26 Thread Chun-Hung Hsiao


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

(Updated July 26, 2017, 11:29 p.m.)


Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.


Changes
---

Adderessed Gilbert's comments and rebased.


Bugs: mesos-7374
https://issues.apache.org/jira/browse/mesos-7374


Repository: mesos


Description
---

Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
launcher is used when launching a mesos containerizer with an image
under Linux. This prevents the executor from messing up with the host
filesystem. The check is in `MesosContainerizerProcess::prepare()`
after provisioning and before launching, since provisioning itself
does not depend on the filesystem isolator.

Also checked that the 'filesystem/linux' is enabled and the 'linux'
launcher is used when enabling the 'docker/runtime' isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
  src/slave/containerizer/mesos/provisioner/store.cpp 
cc5cc81e05f29bb0e11ffa13cdb8d63d4397114f 


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

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


Testing
---

sudo make check
Manually tested on a simplified case of mesos-7374.


Thanks,

Chun-Hung Hsiao

Re: Review Request 60721: Stout: Made `Duration::parse()` handle durations out of range.

2017-07-26 Thread Gastón Kleiman


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

(Updated July 26, 2017, 11:12 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, and 
Vinod Kone.


Changes
---

Reordered the chain, improved tests comments.


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


Repository: mesos


Description
---

Made `Duration:parse()` return an error if the argument is out of the
range that a `Duration` can represent.


Diffs (updated)
-

  3rdparty/stout/include/stout/duration.hpp 
b0cd77b833f6fbf752b4db820fd43b87e1d1e476 
  3rdparty/stout/tests/duration_tests.cpp 
59b08f14849a8db31f11fbd0b2e1248c99afd9dd 


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

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


Testing
---

Added a new expectation to an existing test and confirmed that tests still pass.


Thanks,

Gastón Kleiman

Re: Review Request 60996: Adds documentation for LIBPROCESS_SSL_ECDH_CURVE environment variable.

2017-07-26 Thread Greg Mann


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




docs/ssl.md
Lines 78-79 (patched)


Proposing a couple small tweaks for grammar/spelling:

```
List of elliptic curves which should be used for ECDHE-based cipher suites, 
in preferred order. Available values depend on the OpenSSL version used. 
Default value `auto` allows OpenSSL to pick the curve automatically.
OpenSSL versions prior to `1.0.2` allow for the use of only one curve; in 
those cases, `auto` defaults to `prime256v1`.
```


- Greg Mann


On July 21, 2017, 9:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60996/
> ---
> 
> (Updated July 21, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the adequate documentation entry for the new
> `LIBPROCESS_SSL_ECDH_CURVE` environment variable, which allow the
> configuration of ECDHE key exchange while establishing TLS sessions.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 811390f366c97b6d61cf9b3f188e3c399abb46cd 
> 
> 
> Diff: https://reviews.apache.org/r/60996/diff/2/
> 
> 
> Testing
> ---
> 
> non functional change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 60996: Adds documentation for LIBPROCESS_SSL_ECDH_CURVE environment variable.

2017-07-26 Thread Gastón Kleiman


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



Nit: AFAIK we try to write our commit messages in simple past.


docs/ssl.md
Lines 78 (patched)


s/Valid values depends on OpenSSL version used/Valid values depend on the 
OpenSSL version used./



docs/ssl.md
Lines 81 (patched)


There was only one empty line before, do we need two?


- Gastón Kleiman


On July 21, 2017, 9:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60996/
> ---
> 
> (Updated July 21, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the adequate documentation entry for the new
> `LIBPROCESS_SSL_ECDH_CURVE` environment variable, which allow the
> configuration of ECDHE key exchange while establishing TLS sessions.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 811390f366c97b6d61cf9b3f188e3c399abb46cd 
> 
> 
> Diff: https://reviews.apache.org/r/60996/diff/2/
> 
> 
> Testing
> ---
> 
> non functional change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 61097: Added gRPC support in libprocess.

2017-07-26 Thread Jie Yu

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

Good work! See my detailed comments. I'll do a second pass once the existing 
comments are addressed.

3rdparty/libprocess/include/process/grpc.hpp
Lines 49 (patched)

Let's comment on this class. One important thing to call out here is that: 
each instance of this class consists a separate completion queue. So if folks 
want parallelism or isolation, one can choose to launch multiple instances of 
this class.

Also, I'd rename this class to `Runtime` and put it under `client` 
namespace:
```
grpc::client::Runtime ...;
```

3rdparty/libprocess/include/process/grpc.hpp
Lines 52 (patched)

style nits: we usually put parathesis for constructor calls without 
arguments:
```
new Looper()
```

3rdparty/libprocess/include/process/grpc.hpp
Lines 55 (patched)

I would s/Looper/Data/

This is more than just a looper thread.

3rdparty/libprocess/include/process/grpc.hpp
Lines 62 (patched)

I would call this `looper`. s/thread/looper/

3rdparty/libprocess/include/process/grpc.hpp
Lines 93 (patched)

I think you can get rid of this struct if `call` is part of 
`client::Runtime`?

3rdparty/libprocess/include/process/grpc.hpp
Lines 101 (patched)

s/method/stub/

I'd also restructure this a bit (indentation for function paramters should 
be 4:
```
std::unique_ptr<::grpc::ClientAsyncResponseReader>(T::*stub)(
::grpc::ClientContext*,
const Request&,
::grpc::CompletionQueue*),
```

3rdparty/libprocess/include/process/grpc.hpp
Lines 106 (patched)

In fact, I would introduce a `terminated` field in `client::Runtime::Data`, 
and introduce a `wait` method for `client::Runtime`:
```
class Runtime
{
public:
  // The future will become ready when the runtime is terminated;
  Future wait()
  {
return data->terminating.future()
  .then(defer(process, [=]() {
// NOTE: This is a blocking call. However, the thread is
// guaranteed to be exiting, therefore the amount of time in
// blocking state is bounded (just like other syscalls we
// invoke).
looper->join();

return Nothing();
  }));
  }

  void terminate()
  {
// This will signal the looper thread to exit.
data->terminate.test_and_set();
  }

private:
  struct Data
  {
...
std::atomic_flag terminate;
Promise terminating;
  };
};
```

You probably want to use `AsyncNext` rather than `Next` so that the looper 
thread can be interrupted (by always checking `data->terminate`.

3rdparty/libprocess/include/process/grpc.hpp
Lines 112 (patched)

Can you add a TODO here to support allowing the caller to specify a timeout?

3rdparty/libprocess/include/process/grpc.hpp
Lines 123 (patched)

who is going to delete the lambda function?

3rdparty/libprocess/include/process/grpc.hpp
Lines 124 (patched)

I would comment on why you need to capture those field that are not used in 
the lambda function. Is it possible that compiler optimize it away?

3rdparty/libprocess/include/process/grpc.hpp
Lines 145-154 (patched)

Can we make this a member function of `Runtime`:
```
grpc::client::Runtime client;
client.call(channel, rpc, request);
```

3rdparty/libprocess/include/process/grpc.hpp
Lines 146 (patched)

no need for `inline` for template. template is always inlined.

3rdparty/libprocess/include/process/grpc.hpp
Lines 148 (patched)

I would use the name `Stub` here.
```
auto call(
const Channel& channel,
const Stub& stub,
const Request& request);
```

Also, is there a way to restrict that `Request` is a subclass of 
`protobuf::Message`?

- Jie Yu

On July 25, 2017, 9:13 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61097/

Re: Review Request 61063: Do not track files generated by stout autotools build setup.

2017-07-26 Thread Till Toenshoff


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


Ship it!




Ship It!

- Till Toenshoff


On July 23, 2017, 9:09 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61063/
> ---
> 
> (Updated July 23, 2017, 9:09 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Do not track files generated by stout autotools build setup.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/install-sh 6781b987bdbcbc23efe6bbe1654a1e3637b9af07 
> 
> 
> Diff: https://reviews.apache.org/r/61063/diff/1/
> 
> 
> Testing
> ---
> 
> Standalone build of `stout` causes only known build failures.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61064: Do not track files generated by libprocess autotools build setup.

2017-07-26 Thread Till Toenshoff


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


Ship it!




Ship It!

- Till Toenshoff


On July 23, 2017, 9:09 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61064/
> ---
> 
> (Updated July 23, 2017, 9:09 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Do not track files generated by libprocess autotools build setup.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/install-sh 6781b987bdbcbc23efe6bbe1654a1e3637b9af07 
> 
> 
> Diff: https://reviews.apache.org/r/61064/diff/1/
> 
> 
> Testing
> ---
> 
> Standalone build of `libprocess` causes only known build failures.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60996: Adds documentation for LIBPROCESS_SSL_ECDH_CURVE environment variable.

2017-07-26 Thread Till Toenshoff


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


Ship it!




Ship It!

- Till Toenshoff


On July 21, 2017, 9:30 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60996/
> ---
> 
> (Updated July 21, 2017, 9:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds the adequate documentation entry for the new
> `LIBPROCESS_SSL_ECDH_CURVE` environment variable, which allow the
> configuration of ECDHE key exchange while establishing TLS sessions.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 811390f366c97b6d61cf9b3f188e3c399abb46cd 
> 
> 
> Diff: https://reviews.apache.org/r/60996/diff/2/
> 
> 
> Testing
> ---
> 
> non functional change.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

2017-07-26 Thread Mesos Reviewbot Windows


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



Bad patch!

Reviews applied: [58939]

Failed command: python support/apply-reviews.py -n -r 58939

Error:
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:1109
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/202/console

- Mesos Reviewbot Windows


On May 9, 2017, 6:08 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58939/
> ---
> 
> (Updated May 9, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: mesos-7374
> https://issues.apache.org/jira/browse/mesos-7374
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
> launcher is used when launching a mesos containerizer with an image
> under Linux. This prevents the executor from messing up with the host
> filesystem. The check is in `MesosContainerizerProcess::prepare()`
> after provisioning and before launching, since provisioning itself
> does not depend on the filesystem isolator.
> 
> Also checked that the 'filesystem/linux' is enabled and the 'linux'
> launcher is used when enabling the 'docker/runtime' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 58ab74571fb14c6dbb1907151dc421f93e324bb5 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 2a6e0b179394e0485d2495ceb4bbbcb184af08fe 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> b47a6b5081a63ac474ac4634701b1a572eb58137 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4 
> 
> 
> Diff: https://reviews.apache.org/r/58939/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on a simplified case of mesos-7374.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

2017-07-26 Thread Chun-Hung Hsiao



> On July 20, 2017, 8:19 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1112-1126 (patched)
> > 
> >
> > I don't like the checks here, since we have the following case:
> > 
> > what if we have a task with volumes specified in its containerinfo but 
> > no image?
> > 
> > Let's add `filesystem/isolator` check at docker::store::create().

Based on our discussion, let's put the checks in `MesosContainerizer::create()`.


> On July 20, 2017, 8:19 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/runtime.cpp
> > Lines 70-79 (patched)
> > 
> >
> > Basically we dont add isolator dependencies inside of any isolator, nor 
> > the launcher since the launcher is supposed to be a component for 
> > containerizer.

Should I move this check into `MesosContainerizer::create()` or just remove it, 
since we already plan to check these two conditions when `--image_provider` is 
set?


- Chun-Hung


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


On May 9, 2017, 6:08 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58939/
> ---
> 
> (Updated May 9, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: mesos-7374
> https://issues.apache.org/jira/browse/mesos-7374
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
> launcher is used when launching a mesos containerizer with an image
> under Linux. This prevents the executor from messing up with the host
> filesystem. The check is in `MesosContainerizerProcess::prepare()`
> after provisioning and before launching, since provisioning itself
> does not depend on the filesystem isolator.
> 
> Also checked that the 'filesystem/linux' is enabled and the 'linux'
> launcher is used when enabling the 'docker/runtime' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 58ab74571fb14c6dbb1907151dc421f93e324bb5 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 2a6e0b179394e0485d2495ceb4bbbcb184af08fe 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> b47a6b5081a63ac474ac4634701b1a572eb58137 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4 
> 
> 
> Diff: https://reviews.apache.org/r/58939/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on a simplified case of mesos-7374.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 61089: Logged each container state transition.

2017-07-26 Thread Jiang Yan Xu


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

(Updated July 26, 2017, 12:18 p.m.)


Review request for mesos and Gilbert Song.


Changes
---

Review comments.


Repository: mesos


Description
---

Currently when container launch is blocked silently, it's hard to tell
which state it is in. Logging state transition helps with triaging.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
ea0691945d3450fcc336df03d9cf7b48afbd8b3d 
  src/slave/containerizer/mesos/containerizer.cpp 
9376d14d66f5dc7e91c7c0e9da253f5eb9347539 


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

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


Testing
---

make check


Thanks,

Jiang Yan Xu

Re: Review Request 61089: Logged each container state transition.

2017-07-26 Thread Jiang Yan Xu



> On July 26, 2017, 10:48 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 2599-2603 (patched)
> > 
> >
> > Would suggest we either
> > 1. add a check here: `CHECK(containers_.contains(containerId)`
> > 
> > or
> > 
> > 2. pass an `const Owned` pointer to transition() instead of 
> > the `ContainerID`.
> > 
> > Thoughts?

I would prefer the former. Thanks for the suggestion.

The problem with the latter is that 
- I'd like to log the `containerId` which is not in the `Container` but if I 
pass both, the API looks kind of stupid. Adding `containerId` to `Container` 
would probably overkill.
- Passing `Owned` to a method when you don't mean to pass on the ownership 
feels odd. Our pratices on `Owned` has always been murky but when possible I am 
sticking with the strict semantics.


- Jiang Yan


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


On July 24, 2017, 2:48 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61089/
> ---
> 
> (Updated July 24, 2017, 2:48 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently when container launch is blocked silently, it's hard to tell
> which state it is in. Logging state transition helps with triaging.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> ea0691945d3450fcc336df03d9cf7b48afbd8b3d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
> 
> 
> Diff: https://reviews.apache.org/r/61089/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 61123: Added regression test for host volume ownership issue.

2017-07-26 Thread Ilya Pronin



> On July 26, 2017, 7:08 p.m., Ilya Pronin wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 252-255 (patched)
> > 
> >
> > Why do we need Docker for this test? The problem should be reproducible 
> > without it.

I meant an image. Not Docker itself :)


- Ilya


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


On July 26, 2017, 12:05 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61123/
> ---
> 
> (Updated July 26, 2017, 12:05 a.m.)
> 
> 
> Review request for mesos, Ilya Pronin, Jie Yu, James Peach, Stephan Erb, 
> Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5187
> https://issues.apache.org/jira/browse/MESOS-5187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added regression test for host volume ownership issue.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 457311e23a0c848ef3ed69c8be4396e6a01f3b95 
> 
> 
> Diff: https://reviews.apache.org/r/61123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that this test failed if we don't have the bugfix patch.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61123: Added regression test for host volume ownership issue.

2017-07-26 Thread Ilya Pronin


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


Fix it, then Ship it!




LGTM! The comment is mostly a question.


src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Lines 252-255 (patched)


Why do we need Docker for this test? The problem should be reproducible 
without it.


- Ilya Pronin


On July 26, 2017, 12:05 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61123/
> ---
> 
> (Updated July 26, 2017, 12:05 a.m.)
> 
> 
> Review request for mesos, Ilya Pronin, Jie Yu, James Peach, Stephan Erb, 
> Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5187
> https://issues.apache.org/jira/browse/MESOS-5187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added regression test for host volume ownership issue.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 457311e23a0c848ef3ed69c8be4396e6a01f3b95 
> 
> 
> Diff: https://reviews.apache.org/r/61123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that this test failed if we don't have the bugfix patch.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61122: Fixed the host volume relative host path ownership.

2017-07-26 Thread Ilya Pronin


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


Fix it, then Ship it!




LGTM!


src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 438-440 (patched)


This can be shortened:
```c++
return ErrnoError("Failed to stat '" + containerConfig.directory() + "'");
```



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Line 487 (original), 502 (patched)


Do we call this a host volume as well? We're creating it in the container 
sandbox.


- Ilya Pronin


On July 26, 2017, 7:41 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61122/
> ---
> 
> (Updated July 26, 2017, 7:41 a.m.)
> 
> 
> Review request for mesos, Ilya Pronin, Jie Yu, James Peach, Stephan Erb, 
> Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5187
> https://issues.apache.org/jira/browse/MESOS-5187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This bugfix addresses the issue from MESOS-5178. Basically, the
> host volume ownership was not set correctly. This issue can be
> exposed if a framework user is non-root while the agent
> process runs as root. Then, the non-root user does not have
> permissions to write to this volume.
> 
> The correct solution should be giving permissions to corresponding
> users by leveraging supplementary groups. But we can still
> introduce a workaround in this patch by changing the ownership
> of this host volume to its sandbox's ownership.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> bf35b7f00d6e80672ffc27cfc3f3a2fd8de69a99 
> 
> 
> Diff: https://reviews.apache.org/r/61122/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61090: Store container reference in a variable if it will be used repeatedly.

2017-07-26 Thread Gilbert Song


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


Ship it!




Ship It!

- Gilbert Song


On July 24, 2017, 3:07 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61090/
> ---
> 
> (Updated July 24, 2017, 3:07 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The containerizer already does it in some places, this patch makes it
> more consistent.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
> 
> 
> Diff: https://reviews.apache.org/r/61090/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 61089: Logged each container state transition.

2017-07-26 Thread Gilbert Song


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




src/slave/containerizer/mesos/containerizer.cpp
Lines 2599-2603 (patched)


Would suggest we either
1. add a check here: `CHECK(containers_.contains(containerId)`

or

2. pass an `const Owned` pointer to transition() instead of the 
`ContainerID`.

Thoughts?


- Gilbert Song


On July 24, 2017, 2:48 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61089/
> ---
> 
> (Updated July 24, 2017, 2:48 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently when container launch is blocked silently, it's hard to tell
> which state it is in. Logging state transition helps with triaging.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> ea0691945d3450fcc336df03d9cf7b48afbd8b3d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
> 
> 
> Diff: https://reviews.apache.org/r/61089/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 61122: Fixed the host volume relative host path ownership.

2017-07-26 Thread James Peach


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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 440 (patched)


We probably should expose a `os::stat::stat()`, but this should be:
```
return ErrnoError("Failed to stat ...");
```



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 509 (patched)


Should this be `UID` and `GID`?


- James Peach


On July 26, 2017, 6:41 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61122/
> ---
> 
> (Updated July 26, 2017, 6:41 a.m.)
> 
> 
> Review request for mesos, Ilya Pronin, Jie Yu, James Peach, Stephan Erb, 
> Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5187
> https://issues.apache.org/jira/browse/MESOS-5187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This bugfix addresses the issue from MESOS-5178. Basically, the
> host volume ownership was not set correctly. This issue can be
> exposed if a framework user is non-root while the agent
> process runs as root. Then, the non-root user does not have
> permissions to write to this volume.
> 
> The correct solution should be giving permissions to corresponding
> users by leveraging supplementary groups. But we can still
> introduce a workaround in this patch by changing the ownership
> of this host volume to its sandbox's ownership.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> bf35b7f00d6e80672ffc27cfc3f3a2fd8de69a99 
> 
> 
> Diff: https://reviews.apache.org/r/61122/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61121: Added regression test for sandbox_path volume ownership issue.

2017-07-26 Thread James Peach


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


Fix it, then Ship it!





src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp
Lines 152 (patched)


s/whlie/while/



src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp
Lines 187 (patched)


Maybe simplify this (here and above):
```
ASSERT_SOME(os::chown("nobody", directory.get()));
```


- James Peach


On July 25, 2017, 11:15 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61121/
> ---
> 
> (Updated July 25, 2017, 11:15 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Ilya Pronin, Jie Yu, James Peach, Vinod 
> Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7830
> https://issues.apache.org/jira/browse/MESOS-7830
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added regression test for sandbox_path volume ownership issue.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
> 3228b9a7df0fcb7973b210a1d0c17a18869c73d4 
> 
> 
> Diff: https://reviews.apache.org/r/61121/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that this test failed if we don't have the bugfix patch.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60961: Added Prometheus support to the `/metrics/snapshot` endpoint.

2017-07-26 Thread Mesos Reviewbot Windows


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



Patch looks great!

Reviews applied: [61124, 60957, 61125, 60958, 60959, 61126, 61127, 60960, 60961]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 19, 2017, 8:59 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60961/
> ---
> 
> (Updated July 19, 2017, 8:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for using a `format=prometheus` query parameter
> to the `/metrics/snapshot` endpoint to emit metrics in native
> Prometheus format.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp 
> 9c32a88d851c884a5025edb6ea1e27939b484546 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> 7184aa4d0294c20466646c9aa61d90973eca22e1 
>   3rdparty/libprocess/src/tests/metrics_tests.cpp 
> 161ca0dc7aea526d450d71a80839d8cc075aaa31 
> 
> 
> Diff: https://reviews.apache.org/r/60961/diff/5/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26). Manual testing with Prometheus 2.0beta.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61137: Cleaned up style in example frameworks.

2017-07-26 Thread Mesos Reviewbot Windows


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



Patch looks great!

Reviews applied: [61110, 6, 61112, 61134, 61135, 61136, 61137]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 26, 2017, 2:06 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61137/
> ---
> 
> (Updated July 26, 2017, 2:06 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
>   src/examples/disk_full_framework.cpp 
> 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a 
>   src/examples/docker_no_executor_framework.cpp 
> 4a58f11fc8892f23ade1d8c872ab9b4fc580d478 
> 
> 
> Diff: https://reviews.apache.org/r/61137/diff/1/
> 
> 
> Testing
> ---
> 
> `$ make check`
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 61137: Cleaned up style in example frameworks.

2017-07-26 Thread Armand Grillet


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

Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
  src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a 
  src/examples/docker_no_executor_framework.cpp 
4a58f11fc8892f23ade1d8c872ab9b4fc580d478 


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


Testing
---

`$ make check`


Thanks,

Armand Grillet

Review Request 61136: Extracted strings into constants in docker no executor framework.

2017-07-26 Thread Armand Grillet


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

Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

Allows to distinguish the framework between different instances.


Diffs
-

  src/examples/docker_no_executor_framework.cpp 
4a58f11fc8892f23ade1d8c872ab9b4fc580d478 


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


Testing
---

`$ make check`


Thanks,

Armand Grillet

Review Request 61135: Extracted strings into constants in disk full framework.

2017-07-26 Thread Armand Grillet


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

Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a 


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


Testing
---

`$ make check`


Thanks,

Armand Grillet

Review Request 61134: Added name flag to disk full framework.

2017-07-26 Thread Armand Grillet


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

Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

Allows to distinguish the framework between different instances.


Diffs
-

  src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a 


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


Testing
---

`$ make check`


Thanks,

Armand Grillet

Re: Review Request 61112: Added flag executor_extra_uris to balloon framework.

2017-07-26 Thread Alexander Rukletsov



> On July 25, 2017, 7:08 p.m., Till Toenshoff wrote:
> > src/examples/balloon_framework.cpp
> > Lines 120-124 (patched)
> > 
> >
> > I would suggest to use a different approach here -- instead of allowing 
> > multiple URIs to be used for the executor artefact, lets simply go for 
> > using archives in those cases.
> > 
> > Now the issue preventing us to do so here is 
> > https://github.com/apache/mesos/blob/cd3380c4e9521b4b26f9030658816eee7a4b89a1/src/examples/balloon_framework.cpp#L493
> > 
> > Hence my suggestion is to add a new flag called e.g. 
> > `executor_executable` which can be used if we want the fetcher to make non 
> > archive downloads `chmod` into `+X`. Whenever that flag is not set, we can 
> > now allow archive downloads and achieve what we want without having to;
> > - trigger multiple downloads
> > - accept depending libraries to be `chmod` into `+X` which wont trigger 
> > failures but still is nasty

Another approach would be to delegate URI specification to the user. A similar 
approach can be seen in e.g. [1]. This way we will allow for both scenarios: 
multiple URIs and a single archive URI. The only disadvantage I see here is 
overcomplicating API for a fairly straightforward framework. Having said that, 
it seems to me a fair price, given we've already seen requests for specifying a 
command and an extra library.

[1] 
https://github.com/apache/mesos/blob/95fe8b367a94da6da0a580026519bf07a4f65ec7/src/cli/execute.cpp#L104


- Alexander


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


On July 25, 2017, 3:48 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61112/
> ---
> 
> (Updated July 25, 2017, 3:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
> 
> 
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows to set URIs that will be fetched and executed before
> running the executor.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
> 
> 
> Diff: https://reviews.apache.org/r/61112/diff/1/
> 
> 
> Testing
> ---
> 
> $ make check
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61111: Extracted strings into constants in balloon framework.

2017-07-26 Thread Armand Grillet


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

(Updated July 26, 2017, 1:16 p.m.)


Review request for mesos, Alexander Rukletsov and Alexander Rojas.


Changes
---

Resolved issues.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 


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

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


Testing
---

$ make check


Thanks,

Armand Grillet

Re: Review Request 60770: Added validation functions for resource provider calls.

2017-07-26 Thread Jan Schlicht


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

(Updated July 26, 2017, 12:48 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Used explicit namespaces in `devolve` to avoid lookup failures when using 
precompiled headers due to the newly introduced 
`mesos::internal::resource_provider` namespace.


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


Repository: mesos


Description
---

Added validation functions for resource provider calls.


Diffs (updated)
-

  src/CMakeLists.txt 51b67428f823454665db695ba70a0dc896a7595c 
  src/Makefile.am 8b91716432fb2f6210c12ed7edb312f49e57aa4c 
  src/internal/devolve.hpp 214fcefe07950590ba021c1e0b3aaa3e64231f81 
  src/internal/devolve.cpp 79c668ff86bb3b1fddc48bcd978f9d4fc026a415 
  src/resource_provider/validation.hpp PRE-CREATION 
  src/resource_provider/validation.cpp PRE-CREATION 
  src/tests/CMakeLists.txt f5fe5a6785e2d230707396cc42d9f824f2539dba 
  src/tests/resource_provider_validation_tests.cpp PRE-CREATION 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 60769: Added evolve functions for resource provider Event/Call.

2017-07-26 Thread Jan Schlicht


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

(Updated July 26, 2017, 12:46 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Used explicit namespaces to avoid lookup issues.


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


Repository: mesos


Description
---

Added evolve functions for resource provider Event/Call.


Diffs (updated)
-

  src/internal/evolve.hpp 42ead345e94bf68f793d1beb98f7cd8d50fdf994 
  src/internal/evolve.cpp 3ac55ac1f3e4be6130b1772e18325053da8c55c8 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 60771: Implemented the 'SUBSCRIBE' call in the resource provider manager.

2017-07-26 Thread Jan Schlicht


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

(Updated July 26, 2017, 11:35 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Re-added the `AgentEndpoint` test case. Added checks for response bodies.


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


Repository: mesos


Description
---

Using HTTP, resource providers are expected to subscribe to the resource
provider manager. The resource provider manager will assign a unique ID
to subscribed resource providers and use events to communicate with
them.


Diffs (updated)
-

  src/resource_provider/manager.cpp a9bf258a0896bd519b99a689a0bfab64d665172f 
  src/tests/resource_provider_http_api_tests.cpp 
46a3f37ba046201e9d79e8ba037da2cb37f5c31f 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 60913: Adds support for OpenSSL's ECDH key exchange.

2017-07-26 Thread Alexander Rojas


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

(Updated July 26, 2017, 11:25 a.m.)


Review request for mesos, Jie Yu, James Peach, and Till Toenshoff.


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


Repository: mesos


Description
---

This patch adds the configuration necesary so that the Elliptic Curve
Diffie Hellman algorithm can be used for TLS key exchange if the
OpenSSL version used supports it.

It also adds the ssl flag `LIBPROCESS_SSL_ECDH_CURVES` which allows
for the specification of a specific elliptic curve (or set of curves).

Users will need to specify the TLS cipher suite that uses ECDH in order
to enable the new key exchange. By default Mesos does not use any ECDH
cipher suites.

Support for ephemeral ECDH public keys is the default, so that new
public keys are generated for each exchange.

Note that in order to enable ECDSA ciphers an ECDSA is still necesary
instead of the more traditionl RSA one.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
  3rdparty/libprocess/src/openssl.hpp 7ded2c74b2f92aacfa0f366bd27d5e0df2b8f25c 
  3rdparty/libprocess/src/openssl.cpp e6f17e4591f573186e1dc9697e1e7b60a841fe4f 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
8a14dcb865dfab34fb4d0d51f42a28a913fb7ace 


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

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


Testing
---

```shell
make check
```

Launched Mesos with only ECDHE handshake ciphers enabled

```shell
LIBPROCESS_SSL_ENABLED=1 \
LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
LIBPROCESS_SSL_CIPHERS="ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA"
 \
./bin/mesos-master.sh \
--work_dir=/tmp/mesos/master \
--log_dir=/tmp/mesos/master/log
```

Then in another shell:

```shell
http -v --verify=no https://${MESOS_MASTER_IP}:5050/state

# Launches a browser.
open https://${MESOS_MASTER_IP}:5050/state

# List the set of supported ciphers.
# Expected output:
# >  Starting Nmap 7.50 ( https://nmap.org ) at 2017-07-18 11:41 CEST
# >  Nmap scan report for ${MESOS_MASTER_HOSTNAME} (${MESOS_MASTER_IP})
# >  Host is up (0.13s latency).
# >  rDNS record for ${MESOS_MASTER_IP}: ${MESOS_MASTER_HOSTNAME}
# >  
# >  PORT STATE SERVICE
# >  5050/tcp open  mmcc
# >  | ssl-enum-ciphers:
# >  |   TLSv1.2:
# >  | ciphers:
# >  |   TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (secp256r1) - A
# >  |   TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A
# >  | compressors:
# >  |   NULL
# >  | cipher preference: server
# >  |_  least strength: A
# >  
# >  Nmap done: 1 IP address (1 host up) scanned in 1.87 seconds
wget https://svn.nmap.org/nmap/scripts/ssl-enum-ciphers.nse
nmap --script ssl-enum-ciphers.nse -p 5050 ${MESOS_MASTER_IP}
```


Thanks,

Alexander Rojas

Re: Review Request 60913: Adds support for OpenSSL's ECDH key exchange.

2017-07-26 Thread Till Toenshoff


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




3rdparty/libprocess/src/openssl.cpp
Lines 672 (patched)


This would break with OpenSSL < 0.9.8 as the function 
`initialize_ecdh_curve` would not exist.


- Till Toenshoff


On July 25, 2017, 10:11 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60913/
> ---
> 
> (Updated July 25, 2017, 10:11 a.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the configuration necesary so that the Elliptic Curve
> Diffie Hellman algorithm can be used for TLS key exchange if the
> OpenSSL version used supports it.
> 
> It also adds the ssl flag `LIBPROCESS_SSL_ECDH_CURVES` which allows
> for the specification of a specific elliptic curve (or set of curves).
> 
> Users will need to specify the TLS cipher suite that uses ECDH in order
> to enable the new key exchange. By default Mesos does not use any ECDH
> cipher suites.
> 
> Support for ephemeral ECDH public keys is the default, so that new
> public keys are generated for each exchange.
> 
> Note that in order to enable ECDSA ciphers an ECDSA is still necesary
> instead of the more traditionl RSA one.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
>   3rdparty/libprocess/src/openssl.hpp 
> 7ded2c74b2f92aacfa0f366bd27d5e0df2b8f25c 
>   3rdparty/libprocess/src/openssl.cpp 
> e6f17e4591f573186e1dc9697e1e7b60a841fe4f 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 8a14dcb865dfab34fb4d0d51f42a28a913fb7ace 
> 
> 
> Diff: https://reviews.apache.org/r/60913/diff/6/
> 
> 
> Testing
> ---
> 
> ```shell
> make check
> ```
> 
> Launched Mesos with only ECDHE handshake ciphers enabled
> 
> ```shell
> LIBPROCESS_SSL_ENABLED=1 \
> LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
> LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
> LIBPROCESS_SSL_CIPHERS="ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA"
>  \
> ./bin/mesos-master.sh \
> --work_dir=/tmp/mesos/master \
> --log_dir=/tmp/mesos/master/log
> ```
> 
> Then in another shell:
> 
> ```shell
> http -v --verify=no https://${MESOS_MASTER_IP}:5050/state
> 
> # Launches a browser.
> open https://${MESOS_MASTER_IP}:5050/state
> 
> # List the set of supported ciphers.
> # Expected output:
> # >  Starting Nmap 7.50 ( https://nmap.org ) at 2017-07-18 11:41 CEST
> # >  Nmap scan report for ${MESOS_MASTER_HOSTNAME} (${MESOS_MASTER_IP})
> # >  Host is up (0.13s latency).
> # >  rDNS record for ${MESOS_MASTER_IP}: ${MESOS_MASTER_HOSTNAME}
> # >  
> # >  PORT STATE SERVICE
> # >  5050/tcp open  mmcc
> # >  | ssl-enum-ciphers:
> # >  |   TLSv1.2:
> # >  | ciphers:
> # >  |   TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (secp256r1) - A
> # >  |   TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A
> # >  | compressors:
> # >  |   NULL
> # >  | cipher preference: server
> # >  |_  least strength: A
> # >  
> # >  Nmap done: 1 IP address (1 host up) scanned in 1.87 seconds
> wget https://svn.nmap.org/nmap/scripts/ssl-enum-ciphers.nse
> nmap --script ssl-enum-ciphers.nse -p 5050 ${MESOS_MASTER_IP}
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 61111: Extracted strings into constants in balloon framework.

2017-07-26 Thread Alexander Rojas


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




src/examples/balloon_framework.cpp
Line 302 (original), 306 (patched)


wrap is 4 spaces here.


- Alexander Rojas


On July 25, 2017, 5:47 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6/
> ---
> 
> (Updated July 25, 2017, 5:47 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
> 
> 
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
> 
> 
> Diff: https://reviews.apache.org/r/6/diff/1/
> 
> 
> Testing
> ---
> 
> $ make check
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61120: Fixed the sandbox_path volume source path ownership.

2017-07-26 Thread Gilbert Song


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

(Updated July 25, 2017, 11:41 p.m.)


Review request for mesos, Greg Mann, Ilya Pronin, Jie Yu, James Peach, Vinod 
Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

This bugfix addresses the issue from MESOS-7830. Basically, the
sandbox path volume ownership was not set correctly. This issue
can be exposed if a framework user is non-root while the agent
process runs as root. Then, the non-root user does not have
permissions to write to this volume.

The correct solution should be giving permissions to corresponding
users by leveraging supplementary groups. But we can still
introduce a workaround in this patch by changing the ownership
of the sandbox path volume to its sandbox's ownership.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
6f7304d4aa40eb1b4815ffc1fec61f7e98291cba 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 61122: Fixed the host volume relative host path ownership.

2017-07-26 Thread Gilbert Song


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

(Updated July 25, 2017, 11:41 p.m.)


Review request for mesos, Ilya Pronin, Jie Yu, James Peach, Stephan Erb, Vinod 
Kone, and Jiang Yan Xu.


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


Repository: mesos


Description
---

This bugfix addresses the issue from MESOS-5178. Basically, the
host volume ownership was not set correctly. This issue can be
exposed if a framework user is non-root while the agent
process runs as root. Then, the non-root user does not have
permissions to write to this volume.

The correct solution should be giving permissions to corresponding
users by leveraging supplementary groups. But we can still
introduce a workaround in this patch by changing the ownership
of this host volume to its sandbox's ownership.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
bf35b7f00d6e80672ffc27cfc3f3a2fd8de69a99 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

2017-07-26 Thread Gilbert Song


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

(Updated July 25, 2017, 11:18 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


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


Repository: mesos


Description
---

Only statistics information for blkio in protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
  include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 


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

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


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

2017-07-26 Thread Avinash sridharan


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


Ship it!




Ship It!

- Avinash sridharan


On July 26, 2017, 2:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated July 26, 2017, 2:41 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md a06ac82e7b908601b9ba97011931276e6292102d 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp bf9adf03847a113306c1df2d71e839b90ada7dd3 
>   src/slave/flags.cpp a4c1a0c62af5506bbeb1d26d2fd9ca0ba11d8ad7 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

72 matches

Mail list logo