Re: Review Request 58186: Stop logging the HTTP user agent.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58185, 58223, 58186]

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

- Mesos Reviewbot


On April 5, 2017, 10 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58186/
> ---
> 
> (Updated April 5, 2017, 10 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-7340
> https://issues.apache.org/jira/browse/MESOS-7340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The HTTP user agent can be quite noisy in log files, but is not
> particularly helpful.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 89133e0175ffb792ae95705315844a32550470a1 
> 
> 
> Diff: https://reviews.apache.org/r/58186/diff/2/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Manual inspection of log output.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Jie Yu]

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


Fix it, then Ship it!




I would actually split this patch into two patches (one for code one for test) 
so that we can easily backport the code if we want to.


src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Line 1520 (original), 1520 (patched)


2 lines apart.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Lines 1523 (patched)


Instead of putting this test under LinuxFilesystemIsolatorTest, I would put 
it under CniIsolatorTest. The code change is in CNI isolator, so putting it in 
CniIsolatorTest is more appropriate?

I would write a similar test like this test:
TEST_F(CniIsolatorTest, ROOT_OverrideHostname)


- Jie Yu


On April 5, 2017, 7:53 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated April 5, 2017, 7:53 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 6e95315b70a5d9d3b4b21c4cf235b0a483760190 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 58065: Added initial code for the python mesoshttp package.

[Kevin Klues]

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



One quick comment from glancing through the patch. Why not add the package as 
mesos/http instead of mesoshttp?
I think we should rename lib/mesos in cli_new to lib/cli. That should be one 
nice isolated patch.

Also small things like adding packages to the virtualenv and the gitignore file 
should be separate patches.
The rule of thumb is to group things together that function together, so you 
can write clear concise commit messages about the change. Honestly, I'd even 
split the virtualenv changes into two patches. One for the pytest additions 
explaining how they will be used in a future patch to run unit tests for the 
cli, and one for the mock explaining its intended usage. It may seem like a lot 
of "unnecessary" work up front, but it makes it much easier as a reviewer to 
come in and make a decision right away as to whether a patch is ready to be 
committed or not.
  
It also makes it easier to track changes and revert them in small isolated 
chunks if necessary.

- Kevin Klues


On April 6, 2017, 12:13 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58065/
> ---
> 
> (Updated April 6, 2017, 12:13 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial code for the python mesoshttp package.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bootstrap 6d62e9adf1d543ed00a3a2cf2484edf1c33ee443 
>   src/cli_new/pip-requirements.txt 9cc8d096ac353f4ed2d4318f2d0e439a6edf 
>   src/python/.gitignore PRE-CREATION 
>   src/python/lib/mesoshttp/__init__.py PRE-CREATION 
>   src/python/lib/mesoshttp/exceptions.py PRE-CREATION 
>   src/python/lib/mesoshttp/http.py PRE-CREATION 
>   src/python/lib/tests/mesoshttp/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/mesoshttp/test_http.py PRE-CREATION 
>   support/mesos-style.py 6489c2de8cc8e14f28ef89c7604c94d3edff 
> 
> 
> Diff: https://reviews.apache.org/r/58065/diff/1/
> 
> 
> Testing
> ---
> 
> unit tests:
> $ source src/cli_new/activate
> $ pytest --cov src/python/lib/mesoshttp --cov-report term-missing 
> src/python/lib
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 57952: Added 'config' plugin to the new CLI.

[Kevin Klues]

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




src/cli_new/lib/mesos/plugins/config/main.py
Lines 96-102 (patched)


I would actually leverage the Table abstraction to print th eplugins here. 
I think this might be a discarded RR from me that should probably be revived 
for this.


- Kevin Klues


On March 27, 2017, noon, Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57952/
> ---
> 
> (Updated March 27, 2017, noon)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7269
> https://issues.apache.org/jira/browse/MESOS-7269
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used to load and show the plugins available for the user.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/main.py bbfb52c894540158c70e0f50ebb8a277b692d54d 
>   src/cli_new/bin/settings.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/config/__init__.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/config/main.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57952/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 57896: Renamed config.py to settings.py in the new CLI.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On March 27, 2017, 11:56 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57896/
> ---
> 
> (Updated March 27, 2017, 11:56 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7269
> https://issues.apache.org/jira/browse/MESOS-7269
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Change made due to the upcoming introduction of a `config` plugin.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/config.py 274f8c63b0c642637f17aa2e3c8c4a8a5a059e37 
>   src/cli_new/bin/main.py bbfb52c894540158c70e0f50ebb8a277b692d54d 
> 
> 
> Diff: https://reviews.apache.org/r/57896/diff/3/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58220: Used move semantics when returning gzip compressed responses.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58220]

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

- Mesos Reviewbot


On April 5, 2017, 1:53 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58220/
> ---
> 
> (Updated April 5, 2017, 1:53 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We used to copy the compressed body into the eventual response.
> For large responses especially from `/state` endpoint made by the
> Web UI, these can be pretty expensive. This change replaces the
> copy by a move operation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/encoder.hpp 
> ea629d72a68e093343562db1ef7e5d00c723f03b 
> 
> 
> Diff: https://reviews.apache.org/r/58220/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> For a ~10mb compressed response, the original took 3ms vs 2ns (newer).
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 55065: Updated ldcache flags to 32bit width.

[Kevin Klues]

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


Ship it!




Assuming all of the unit tests pass -- I'll defer to @tillt

- Kevin Klues


On April 3, 2017, 7:32 p.m., Ayanampudi Varsha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55065/
> ---
> 
> (Updated April 3, 2017, 7:32 p.m.)
> 
> 
> Review request for mesos, Kevin Klues and Till Toenshoff.
> 
> 
> Bugs: MESOS-6742
> https://issues.apache.org/jira/browse/MESOS-6742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/linux/ldcache.cpp 6260496672cd0226a24ea1b34003a48af9678a63 
> 
> 
> Diff: https://reviews.apache.org/r/55065/diff/2/
> 
> 
> Testing
> ---
> 
> make check 
> make check GTEST_FILTER="LdcacheTest.Parse"
> 
> 
> Thanks,
> 
> Ayanampudi Varsha
> 
>

Re: Review Request 58200: Fix mesos runs with docker(pid namespace mismatch).

[Deshi Xiao]

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

(Updated 四月 5, 2017, 11:35 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


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


Repository: mesos


Description
---

Becuase MESOS HTTP checks doesn't work when mesos runs with
--docker_mesos_image ( pid namespace mismatch ).So let docker
executor run with container add host pid mapping(--pid=host)


Diffs
-

  src/slave/containerizer/docker.cpp ad9ab847cb3093724ef374d036c896b4e7f18b5e 


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


Testing (updated)
---

first testing : https://gist.github.com/xiaods/c5a11e3ab51e89a9609edc2c477f7ea8


Thanks,

Deshi Xiao

Re: Review Request 57925: Added further tests for executor secret generation.

[Greg Mann]

> On April 4, 2017, 7:37 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp
> > Lines 5904-5905 (patched)
> > 
> >
> > I'm assuming the tasks also fail even if the secret generation succeeds 
> > because of the shutdown sent by the framework? If yes, should we test that 
> > as well or instead?

Good point, the same code path is executed regardless of the state of the 
`Future` returned by the secret generator in this case. I think it 
makes more sense to only test the successful secret generation case, since that 
is most common/relevant, and no need to test the failure case as well.


- Greg


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


On April 5, 2017, 10:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57925/
> ---
> 
> (Updated April 5, 2017, 10:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6999
> https://issues.apache.org/jira/browse/MESOS-6999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds two further tests for executor secret
> generation, `SlaveTest.RunTaskGroupReferenceTypeSecret`
> and `SlaveTest.RunTaskGroupFailedSecretAfterShutdown`.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp cd769687e0f161512c0114ef4651508c31f51639 
> 
> 
> Diff: https://reviews.apache.org/r/57925/diff/8/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> `bin/mesos-tests.sh --gtest_filter="SlaveTest.*Secret*" --gtest_repeat=-1 
> --gtest_break_on_failure` was used to check the new tests.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57667: Added executor authentication to the docs.

[Greg Mann]

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

(Updated April 5, 2017, 10:37 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Added executor authentication to the docs.


Diffs (updated)
-

  docs/authentication.md 279e00a513d386eb6215a4e6d66652a0af3ec56d 
  docs/executor-http-api.md c49c234d73ece925297051b8d07fbe48e47868e0 


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

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


Testing
---


Thanks,

Greg Mann

Re: Review Request 57925: Added further tests for executor secret generation.

[Greg Mann]

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

(Updated April 5, 2017, 10:34 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

This patch adds two further tests for executor secret
generation, `SlaveTest.RunTaskGroupReferenceTypeSecret`
and `SlaveTest.RunTaskGroupFailedSecretAfterShutdown`.


Diffs (updated)
-

  src/tests/slave_tests.cpp cd769687e0f161512c0114ef4651508c31f51639 


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

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


Testing
---

`make check`

`bin/mesos-tests.sh --gtest_filter="SlaveTest.*Secret*" --gtest_repeat=-1 
--gtest_break_on_failure` was used to check the new tests.


Thanks,

Greg Mann

Review Request 58224: RFC: Add some consistency checks for libprocess UPIDs.

[James Peach]

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

Review request for mesos and Mesos Reviewbot.


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer is a legitimate
owner of the UPID it claims in a libprocess message. This change adds
2 checks that make impersonation somewhat harder.

First, we bind the first UPID to the socket context. This prevents a
peer attempting to switch UPIDs during a session.

Second, we enforce that the IP address in the UPID matches the peer
address. This makes spoofing the UPID harder (eg. to send authenticated
messages), but also breaks some legitimate configurations, particularly
on multihomed hosts.


Diffs
-

  3rdparty/libprocess/src/process.cpp d0cba0c2299bddfedeb8bfde5b93aae733a9cd5b 


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


Testing
---

Minimal manual testing.


Thanks,

James Peach

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57884]

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

- Mesos Reviewbot


On April 5, 2017, 7:53 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated April 5, 2017, 7:53 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 6e95315b70a5d9d3b4b21c4cf235b0a483760190 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 58186: Stop logging the HTTP user agent.

[James Peach]

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

(Updated April 5, 2017, 10 p.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Rebased.


Summary (updated)
-

Stop logging the HTTP user agent.


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


Repository: mesos


Description
---

The HTTP user agent can be quite noisy in log files, but is not
particularly helpful.


Diffs (updated)
-

  src/common/http.cpp 89133e0175ffb792ae95705315844a32550470a1 


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

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


Testing
---

Make check (Fedora 25). Manual inspection of log output.


Thanks,

James Peach

Review Request 58223: Add HTTP request logging to the /files endpoint.

[James Peach]

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

Review request for mesos and Anand Mazumdar.


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


Repository: mesos


Description
---

Add HTTP request logging to the /files endpoint.


Diffs
-

  src/files/files.cpp f066146b7cbff35c452717d179b79039bc603cc8 


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


Testing
---

Make check (Fedora 25). Manual inspection of log output.


Thanks,

James Peach

Re: Review Request 58185: Add a common HTTP request log helper function.

[James Peach]

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

(Updated April 5, 2017, 9:57 p.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Addressed review feedback.


Summary (updated)
-

Add a common HTTP request log helper function.


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


Repository: mesos


Description (updated)
---

Consolidate the master and agent HTTP request log helper
functions into common code.


Diffs (updated)
-

  src/common/http.hpp b6e61f7f7f8ebcf5b25a37684cae06cb96188478 
  src/common/http.cpp 89133e0175ffb792ae95705315844a32550470a1 
  src/master/http.cpp 6cf9d350446d1b2d4a6e67d552217daff32657ff 
  src/master/master.hpp 1b077424373d6e195e4ab29e150dedbc3f3f95ab 
  src/master/master.cpp 6a6a570e52d21bfb2443f981e3d7faf8c36f74bc 
  src/slave/http.cpp e253ce9749fc8a03c21dac1ba0e6efe09311414b 
  src/slave/slave.hpp e4f46d42b3c0d0f09cff2d896abf6b84aed6c396 
  src/slave/slave.cpp 65e4a67888fe908e5b2f6ca2ecc9e3a5b9958b2e 


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

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


Testing
---

Make check (Fedora 25). Manual inspection of log output.


Thanks,

James Peach

Review Request 58220: Used move semantics when returning gzip compressed responses.

[Anand Mazumdar]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

We used to copy the compressed body into the eventual response.
For large responses especially from `/state` endpoint made by the
Web UI, these can be pretty expensive. This change replaces the
copy by a move operation.


Diffs
-

  3rdparty/libprocess/src/encoder.hpp ea629d72a68e093343562db1ef7e5d00c723f03b 


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


Testing
---

make check

For a ~10mb compressed response, the original took 3ms vs 2ns (newer).


Thanks,

Anand Mazumdar

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

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

(Updated April 5, 2017, 7:53 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
6e95315b70a5d9d3b4b21c4cf235b0a483760190 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
5e489ef6a522000c55b0fb9a27bce2567f82bb73 


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

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


Testing
---


Thanks,

Silas Snider

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

- Silas


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


On April 5, 2017, 7:53 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated April 5, 2017, 7:53 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 6e95315b70a5d9d3b4b21c4cf235b0a483760190 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

> On March 27, 2017, 5:19 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Line 1910 (original), 1910 (patched)
> > 
> >
> > Are you sure this works? I remembered that for read only bind mount, 
> > you need to do a bind mount and a remount with read only flag.
> > https://lwn.net/Articles/281157/
> > 
> > That probably means we should add a unit test for this. Take a look at 
> > CniIsolatorTest.ROOT_OverrideHostname which will give you some idea how to 
> > adding a unit test for this.

Sorry it took so long -- I was pulled onto another project briefly. I've 
updated to match the recommendation, confirmed that it works by adding a test 
which I confirmed passes now, but does not without my change.


- Silas


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


On April 5, 2017, 7:53 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated April 5, 2017, 7:53 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 6e95315b70a5d9d3b4b21c4cf235b0a483760190 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 57972: Added base stout Environment class to mesos-tests Environment class.

[John Kordich via Review Board]

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

(Updated April 5, 2017, 7:08 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


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


Repository: mesos


Description
---

Added base stout Environment class to mesos-tests Environment class.


Diffs (updated)
-

  src/tests/environment.hpp 06b6d54fc76b327e3e26914eb61c16619a36de42 
  src/tests/environment.cpp e3cff1847c44bb06bbe898211bfca35cf851217a 
  src/tests/main.cpp 5d062c3451bdfb5d5fc459ac7c071ab18e6d8043 


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

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


Testing
---

Ran make check on Linux and the tests on Windows.


Thanks,

John Kordich

Re: Review Request 57971: Added test filtering framework to libprocess-tests.

[John Kordich via Review Board]

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

(Updated April 5, 2017, 7:08 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


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


Repository: mesos


Description
---

Added test filtering framework to libprocess-tests.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/main.cpp 
cda7d61e1f10fde8a457c6077d4eb7aa78b390fc 


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

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


Testing
---

Ran make check on Linux and the tests on Windows.


Thanks,

John Kordich

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[John Kordich via Review Board]

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

(Updated April 5, 2017, 7:08 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Changes
---

I'm not sure if I should leave it to the reviewer to check "Fixed" or "Drop", 
or if I should do it! In any case, I believe I've addressed all of the comments 
to the previous iteration.


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


Repository: mesos


Description
---

Filtered stout tests with symlinks when unable to create symlinks.


Diffs (updated)
-

  3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
  3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
  3rdparty/stout/tests/os/rmdir_tests.cpp 
ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
  3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 


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

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


Testing
---

Ran make check on Linux and the tests on Windows.


Thanks,

John Kordich

Re: Review Request 58214: Fixed a regression hiding previously exposed master and agent flags.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [58214]

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

Error:
2017-04-05 18:45:18 URL:https://reviews.apache.org/r/58214/diff/raw/ 
[11865/11865] -> "58214.patch" [1]
error: patch failed: src/master/flags.hpp:17
error: src/master/flags.hpp: patch does not apply
error: patch failed: src/master/flags.cpp:592
error: src/master/flags.cpp: patch does not apply
error: patch failed: src/master/main.cpp:128
error: src/master/main.cpp: patch does not apply
error: patch failed: src/slave/flags.hpp:17
error: src/slave/flags.hpp: patch does not apply
error: patch failed: src/slave/flags.cpp:953
error: src/slave/flags.cpp: patch does not apply
error: patch failed: src/slave/main.cpp:91
error: src/slave/main.cpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/17672/console

- Mesos Reviewbot


On April 5, 2017, 5:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58214/
> ---
> 
> (Updated April 5, 2017, 5:54 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7316
> https://issues.apache.org/jira/browse/MESOS-7316
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In f441eb9 we in a number of places changed  how 'Flag's were added to
> 'Flags' by moving from ad-hoc invocations of 'FlagsBase::add' on
> particular instances to proper 'Flags' member variables. This was needed
> to ensure 'Flags' instances could always safely be copied. For that we
> introduced local derived 'Flags' classes to support localized parsing
> needs. At the same time, this implementation strategy led to these these
> local variables not being accessible through instances of the original
> class anymore (this was inevitable when making 'Flags' classes properly
> copyable), which e.g., causes a regression in the flags displayed in a
> master's '/flags' endpoint.
> 
> This commit moves the flags into the respective base class removing the
> local classes so that all passed flags are exposed to users.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 41a0edf 
>   src/master/flags.cpp d25cfdd 
>   src/master/main.cpp fa7ba13 
>   src/slave/flags.hpp 2c4bd6a 
>   src/slave/flags.cpp 71935de 
>   src/slave/main.cpp a124d2e 
> 
> 
> Diff: https://reviews.apache.org/r/58214/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` (Fedora 25)
> 
> This is a backport to `1.2.x` of the https://reviews.apache.org/r/57994/ 
> which applied against the then `master` branch (1.3 in spe).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 57994: Fixed a regression hiding previously exposed master and agent flags.

[Benjamin Bannier]

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



Backport to `1.2.x` posted as https://reviews.apache.org/r/58214/.

- Benjamin Bannier


On March 28, 2017, 4:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57994/
> ---
> 
> (Updated March 28, 2017, 4:01 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Michael Park.
> 
> 
> Bugs: MESOS-7316
> https://issues.apache.org/jira/browse/MESOS-7316
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In f441eb9 we in a number of places changed  how 'Flag's were added to
> 'Flags' by moving from ad-hoc invocations of 'FlagsBase::add' on
> particular instances to proper 'Flags' member variables. This was needed
> to ensure 'Flags' instances could always safely be copied. For that we
> introduced local derived 'Flags' classes to support localized parsing
> needs. At the same time, this implementation strategy led to these these
> local variables not being accessible through instances of the original
> class anymore (this was inevitable when making 'Flags' classes properly
> copyable), which e.g., causes a regression in the flags displayed in a
> master's '/flags' endpoint.
> 
> This commit moves the flags into the respective base class removing the
> local classes so that all passed flags are exposed to users.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 41a0edfaecf04759f1efa62a9851fbeeb214e84c 
>   src/master/flags.cpp b7a129b27bf752bf238d214534364403853c1b36 
>   src/master/main.cpp fa7ba1310142a3bef71379ba37fded9b8390aae9 
>   src/slave/flags.hpp 224fac1d06d5a3914d4d1408e880458ac5be010e 
>   src/slave/flags.cpp 76881536e0058880f5720fbf3c1cebc684508235 
>   src/slave/main.cpp 81d61b14accca7611d84db92663a63d5777edd83 
> 
> 
> Diff: https://reviews.apache.org/r/57994/diff/1/
> 
> 
> Testing
> ---
> 
> Tested on a number of platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 58214: Fixed a regression hiding previously exposed master and agent flags.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

In f441eb9 we in a number of places changed  how 'Flag's were added to
'Flags' by moving from ad-hoc invocations of 'FlagsBase::add' on
particular instances to proper 'Flags' member variables. This was needed
to ensure 'Flags' instances could always safely be copied. For that we
introduced local derived 'Flags' classes to support localized parsing
needs. At the same time, this implementation strategy led to these these
local variables not being accessible through instances of the original
class anymore (this was inevitable when making 'Flags' classes properly
copyable), which e.g., causes a regression in the flags displayed in a
master's '/flags' endpoint.

This commit moves the flags into the respective base class removing the
local classes so that all passed flags are exposed to users.


Diffs
-

  src/master/flags.hpp 41a0edf 
  src/master/flags.cpp d25cfdd 
  src/master/main.cpp fa7ba13 
  src/slave/flags.hpp 2c4bd6a 
  src/slave/flags.cpp 71935de 
  src/slave/main.cpp a124d2e 


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


Testing
---

`make check` (Fedora 25)

This is a backport to `1.2.x` of the https://reviews.apache.org/r/57994/ which 
applied against the then `master` branch (1.3 in spe).


Thanks,

Benjamin Bannier

Re: Review Request 58209: Fixed network isolator crash when network interface is not found.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58209]

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

- Mesos Reviewbot


On April 5, 2017, 1:35 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58209/
> ---
> 
> (Updated April 5, 2017, 1:35 p.m.)
> 
> 
> Review request for mesos, Chi Zhang and Jie Yu.
> 
> 
> Bugs: MESOS-7348
> https://issues.apache.org/jira/browse/MESOS-7348
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's not an error if network interface cannot be found, and None is returned 
> by lookup functions as a result in this case. This fix ensures that None is 
> properly handled, when composing an error messge in network isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> dfa71fb6fe867c2f34451ca9f3e7f7f62402133c 
> 
> 
> Diff: https://reviews.apache.org/r/58209/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 58185: Log HTTP accesses to the /files endpoint.

[Anand Mazumdar]

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



Looks good! Mostly minor style nits/naming suggestions.

Ccan you split this patch into two:
- The first one would move the `log(...)` function into the common file and 
make the master/agent code use it.
- This patch would be responsible for making the `Files` route handlers use the 
common logging function that we added in the earlier review?


src/common/http.hpp
Lines 227 (patched)


s/logHttpRequest/logRequest to make the function name protocol agnostic?



src/files/files.cpp
Lines 209 (patched)


We already do `using http = ...` in the file above. Can we remove the 
redundant `process::` prefix?



src/files/files.cpp
Lines 226 (patched)


Nit: Can we remove the redundant `this` and just use `download(...)` 
directly?



src/files/files.cpp
Lines 233 (patched)


Ditto as above



src/files/files.cpp
Lines 273 (patched)


Would fit on the line above?

Ditto for the other occurences



src/slave/slave.hpp
Line 508 (original)


Can we still keep this comment in `src/common/http.cpp`?


- Anand Mazumdar


On April 4, 2017, 8:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58185/
> ---
> 
> (Updated April 4, 2017, 8:11 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-7340
> https://issues.apache.org/jira/browse/MESOS-7340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Consolidate the master and agent HTTP request log helper functions
> into common code. Add HTTP access logging to `FilesProcess` so that the
> `/files` endpoint logs consistently.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp b6e61f7f7f8ebcf5b25a37684cae06cb96188478 
>   src/common/http.cpp 89133e0175ffb792ae95705315844a32550470a1 
>   src/files/files.cpp f066146b7cbff35c452717d179b79039bc603cc8 
>   src/master/http.cpp 6cf9d350446d1b2d4a6e67d552217daff32657ff 
>   src/master/master.hpp 1b077424373d6e195e4ab29e150dedbc3f3f95ab 
>   src/master/master.cpp 6a6a570e52d21bfb2443f981e3d7faf8c36f74bc 
>   src/slave/http.cpp e253ce9749fc8a03c21dac1ba0e6efe09311414b 
>   src/slave/slave.hpp e4f46d42b3c0d0f09cff2d896abf6b84aed6c396 
>   src/slave/slave.cpp 65e4a67888fe908e5b2f6ca2ecc9e3a5b9958b2e 
> 
> 
> Diff: https://reviews.apache.org/r/58185/diff/1/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Manual inspection of log output.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58072: Added resource support for new offer operations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57911, 57997, 57998, 57999, 58048, 58047, 58021, 58071, 58072]

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

- Mesos Reviewbot


On April 5, 2017, 9:26 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58072/
> ---
> 
> (Updated April 5, 2017, 9:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7314
> https://issues.apache.org/jira/browse/MESOS-7314
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When applying offer operations to resources, one has to account for
> changing resources depending on the operation. For resource-related
> operations, disk resources will be converted from a type to another.
> So, by applying such an operation, we will substract the resource
> of the source type and add the resource of the target type of that
> operation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
>   src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d 
>   src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 
> 
> 
> Diff: https://reviews.apache.org/r/58072/diff/7/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 57998: Added ResourceProviderID to Resource protos.

[Benjamin Bannier]

> On April 4, 2017, 11:16 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 1537 (patched)
> > 
> >
> > Instead of just one type of resources, i'd try to use multiple types of 
> > resources here (e.g., "disk" and "cpu") so that we exercise the path that 
> > two different types of the resources might have the same resource provider 
> > ID.

I added some operations involving additional `cpus`.


> On April 4, 2017, 11:16 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 1546 (patched)
> > 
> >
> > I would also check r1's size here.

Done.


> On April 4, 2017, 11:16 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 1547 (patched)
> > 
> >
> > I'd move this to the dedicated 'contains' test

I intoduced a dedicated test for `contains`, but would prefer to leave this 
check in this test since I believe it captures a necessary postcondition of the 
kind of addition we are interested here. Suggest to drop this issue.


> On April 4, 2017, 11:16 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 1548 (patched)
> > 
> >
> > looks like this check is for shared resources. Can you remove this 
> > check from this test?

Removed.


> On April 4, 2017, 11:16 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 1559-1560 (patched)
> > 
> >
> > Ditto. `count` sounds very shared resource related.

I used `Resources::count` as a very simple way to examine the behavior of 
`addable`. If resources with different `provider_id`s where addable `count` 
would yield `0` in both cases, and some other disk would appear. The fact that 
both counts are `1` tells us that the resources were not added and that the 
result has both results as separate resources.

Alternatively one could use e.g., `Resources::filter` to separate resources 
with that provider_id and without it; one could then compare these resources 
against `disk1` and `disk2`. I believe that code would be much lengthier, and 
not necessarily much clearer.

Suggest to drop this issue.


> On April 4, 2017, 11:16 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 1572 (patched)
> > 
> >
> > Ditto. I would prefer we include another type of resources with the 
> > same provider ID here in this test.

Added some operations involving `cpus` here.


> On April 4, 2017, 11:16 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 1577-1578 (patched)
> > 
> >
> > This is contains check, let's factor this out into a separate test.

Removed here.


> On April 4, 2017, 11:16 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp
> > Lines 1594 (patched)
> > 
> >
> > Let's also add some test around equality (i.e., checking if `==` or 
> > `!=` works properly or not. For instance, the fact that you don't have a 
> > `==` defined for unversioned API should be captured here in the unit tests.

I added a dedicated check for (n)equality.


- Benjamin


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


On April 5, 2017, 3:56 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57998/
> ---
> 
> (Updated April 5, 2017, 3:56 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7312
> https://issues.apache.org/jira/browse/MESOS-7312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds an optional resource provider id to resources. In
> future changes we will introduce abstract providers of resources.
> While currently agents are implicit resource providers, later on an
> agent might use multiple resource providers. By having a provider id
> in the resource we can unambigously detect which provider contributed
> which resource.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
>   include/mesos/v1/mesos.proto 228623155c7f68c0f24d173aacbc6eb734f1382f 
>   src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
>   src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d 
>   

Re: Review Request 58047: Introduce BLOCK and RAW disk types.

[Benjamin Bannier]

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

(Updated April 5, 2017, 3:56 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Rebased.


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


Repository: mesos


Description
---

BLOCK and RAW disk types are low-level disk resources which will need
to be transformed into e.g., volumes by dedicated, still to implement
offer operations.


Diffs (updated)
-

  include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
  include/mesos/v1/mesos.proto 228623155c7f68c0f24d173aacbc6eb734f1382f 
  src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
  src/slave/paths.cpp ef22ee167f16030f02d28c8e6bab6c2ca4812d8f 
  src/slave/slave.cpp 65e4a67888fe908e5b2f6ca2ecc9e3a5b9958b2e 
  src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 


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

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


Testing
---

make check (OS X, Fedora25)


Thanks,

Benjamin Bannier

Re: Review Request 57999: Made Path and Mount root optional in Resource.DiskInfo.Source.

[Benjamin Bannier]

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

(Updated April 5, 2017, 3:56 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This commit is preparation for future changes where path or mount
roots might be calculated lazily, i.e., not known early enough
anymore.


Diffs (updated)
-

  include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
  include/mesos/v1/mesos.proto 228623155c7f68c0f24d173aacbc6eb734f1382f 
  src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
  src/slave/paths.cpp ef22ee167f16030f02d28c8e6bab6c2ca4812d8f 
  src/slave/slave.cpp 65e4a67888fe908e5b2f6ca2ecc9e3a5b9958b2e 
  src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 


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

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


Testing
---

make check (OS X, Fedora25)


Thanks,

Benjamin Bannier

Re: Review Request 58048: Added id to Resource.DiskInfo.

[Benjamin Bannier]

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

(Updated April 5, 2017, 3:56 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Rewrapped a line, via Jan.


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


Repository: mesos


Description
---

Ids will allow to create distinguishable resources, e.g., of RAW or
BLOCK type.


Diffs (updated)
-

  include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
  include/mesos/v1/mesos.proto 228623155c7f68c0f24d173aacbc6eb734f1382f 
  src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
  src/tests/mesos.hpp fe897c184829910addec95ace174546092a9e2b2 
  src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d 
  src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 


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

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


Testing
---

make check (OS X, Fedora25)


Thanks,

Benjamin Bannier

Re: Review Request 57998: Added ResourceProviderID to Resource protos.

[Benjamin Bannier]

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

(Updated April 5, 2017, 3:56 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Expanded test, via Jie.


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


Repository: mesos


Description
---

This patch adds an optional resource provider id to resources. In
future changes we will introduce abstract providers of resources.
While currently agents are implicit resource providers, later on an
agent might use multiple resource providers. By having a provider id
in the resource we can unambigously detect which provider contributed
which resource.


Diffs (updated)
-

  include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
  include/mesos/v1/mesos.proto 228623155c7f68c0f24d173aacbc6eb734f1382f 
  src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
  src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d 
  src/tests/resources_utils.cpp 2cef55f7312d671307e097c2c4960c8dcf45c1ff 
  src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 


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

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


Testing
---

make check (OS X, Fedora25)


Thanks,

Benjamin Bannier

Re: Review Request 58202: Tweaked an incorrect comment in allocator test.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58202]

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

- Mesos Reviewbot


On April 5, 2017, 8:56 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58202/
> ---
> 
> (Updated April 5, 2017, 8:56 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6940
> https://issues.apache.org/jira/browse/MESOS-6940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tweaked an incorrect comment in allocator test.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
> 
> 
> Diff: https://reviews.apache.org/r/58202/diff/1/
> 
> 
> Testing
> ---
> 
> no functional change
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 58209: Fixed network isolator crash when network interface is not found.

[Dmitry Zhuk]

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

Review request for mesos, Chi Zhang and Jie Yu.


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


Repository: mesos


Description
---

It's not an error if network interface cannot be found, and None is returned by 
lookup functions as a result in this case. This fix ensures that None is 
properly handled, when composing an error messge in network isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
dfa71fb6fe867c2f34451ca9f3e7f7f62402133c 


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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 58201: Added a 'std::hash' specialization for 'ResourceProviderID'.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58201]

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

- Mesos Reviewbot


On April 5, 2017, 8:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58201/
> ---
> 
> (Updated April 5, 2017, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This makes it possible to use 'ResourceProviderID' as key in a
> 'std::hashmap'.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 2cfbe1065daec1e23ae405db1c6b12178aa5ed9f 
> 
> 
> Diff: https://reviews.apache.org/r/58201/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 58200: Fix mesos runs with docker(pid namespace mismatch).

[haosdent huang]

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



Hi, @deshi May you write down how you verify and test this patch? I would like 
to do the verification in my side.

- haosdent huang


On April 5, 2017, 6:59 a.m., Deshi Xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58200/
> ---
> 
> (Updated April 5, 2017, 6:59 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-7210
> https://issues.apache.org/jira/browse/MESOS-7210
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Becuase MESOS HTTP checks doesn't work when mesos runs with
> --docker_mesos_image ( pid namespace mismatch ).So let docker
> executor run with container add host pid mapping(--pid=host)
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp ad9ab847cb3093724ef374d036c896b4e7f18b5e 
> 
> 
> Diff: https://reviews.apache.org/r/58200/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshi Xiao
> 
>

Re: Review Request 58200: Fix mesos runs with docker(pid namespace mismatch).

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58200]

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

- Mesos Reviewbot


On April 5, 2017, 6:59 a.m., Deshi Xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58200/
> ---
> 
> (Updated April 5, 2017, 6:59 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-7210
> https://issues.apache.org/jira/browse/MESOS-7210
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Becuase MESOS HTTP checks doesn't work when mesos runs with
> --docker_mesos_image ( pid namespace mismatch ).So let docker
> executor run with container add host pid mapping(--pid=host)
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp ad9ab847cb3093724ef374d036c896b4e7f18b5e 
> 
> 
> Diff: https://reviews.apache.org/r/58200/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshi Xiao
> 
>

Re: Review Request 58071: Updated test helpers to support changed disk types.

[Jan Schlicht]

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

(Updated April 5, 2017, 11:40 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


Repository: mesos


Description
---

In addition to creating BLOCK and RAW disk sources,
PATH and MOUNT disk sources can now be created without a root path.


Diffs (updated)
-

  src/tests/mesos.hpp fe897c184829910addec95ace174546092a9e2b2 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 58072: Added resource support for new offer operations.

[Jan Schlicht]

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

(Updated April 5, 2017, 11:26 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

When applying offer operations to resources, one has to account for
changing resources depending on the operation. For resource-related
operations, disk resources will be converted from a type to another.
So, by applying such an operation, we will substract the resource
of the source type and add the resource of the target type of that
operation.


Diffs
-

  src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
  src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d 
  src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 58072: Added resource support for new offer operations.

[Jan Schlicht]

> On April 3, 2017, 3:49 p.m., Benjamin Bannier wrote:
> > src/v1/resources.cpp
> > Lines 1480 (patched)
> > 
> >
> > nit: Let's remove this empty line like elsewhere.

IMO obsolete following the refactor, as there no longer is a `for` loop. Do you 
agree?


- Jan


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


On April 5, 2017, 11:24 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58072/
> ---
> 
> (Updated April 5, 2017, 11:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When applying offer operations to resources, one has to account for
> changing resources depending on the operation. For resource-related
> operations, disk resources will be converted from a type to another.
> So, by applying such an operation, we will substract the resource
> of the source type and add the resource of the target type of that
> operation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
>   src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d 
>   src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 
> 
> 
> Diff: https://reviews.apache.org/r/58072/diff/7/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 58072: Added resource support for new offer operations.

[Jan Schlicht]

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

(Updated April 5, 2017, 11:24 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Removed "WIP", addressed issues.


Summary (updated)
-

Added resource support for new offer operations.


Repository: mesos


Description (updated)
---

When applying offer operations to resources, one has to account for
changing resources depending on the operation. For resource-related
operations, disk resources will be converted from a type to another.
So, by applying such an operation, we will substract the resource
of the source type and add the resource of the target type of that
operation.


Diffs (updated)
-

  src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
  src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d 
  src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Review Request 58202: Tweaked an incorrect comment in allocator test.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Tweaked an incorrect comment in allocator test.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
e343dc37bd7136f0f6dd5dbc22a25cabe715038d 


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


Testing
---

no functional change


Thanks,

Jay Guo

Review Request 58201: Added a 'std::hash' specialization for 'ResourceProviderID'.

[Jan Schlicht]

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

Review request for mesos, Benjamin Bannier and Jie Yu.


Repository: mesos


Description
---

This makes it possible to use 'ResourceProviderID' as key in a
'std::hashmap'.


Diffs
-

  include/mesos/type_utils.hpp 2cfbe1065daec1e23ae405db1c6b12178aa5ed9f 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 57998: Added ResourceProviderID to Resource protos.

[Jan Schlicht]

> On April 4, 2017, 4:40 p.m., Jan Schlicht wrote:
> > Sorry for not noticing this earlier: Because I've just run into this, could 
> > you also add a `std::hash` specialization for `ResourceProviderID` in 
> > `type_utils.hpp`? This will allow us to use `ResourceProviderID` as key in 
> > a hashmap. But I could also create a follow-up review for that.
> 
> Jan Schlicht wrote:
> There's also `operator==` in `type_utils.hpp` which needs to be 
> implemented.
> 
> Jan Schlicht wrote:
> And other operators as well :). They all make sense to be implemented for 
> `ResourceProviderID`.

Uhm, I'll create a review for adding a `std::hash` specialization. This isn't 
the right review for that here, because `ResourceProviderID` has already been 
introduced. Sorry for the inconvenience.


- Jan


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


On March 31, 2017, 11:38 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57998/
> ---
> 
> (Updated March 31, 2017, 11:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7312
> https://issues.apache.org/jira/browse/MESOS-7312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds an optional resource provider id to resources. In
> future changes we will introduce abstract providers of resources.
> While currently agents are implicit resource providers, later on an
> agent might use multiple resource providers. By having a provider id
> in the resource we can unambigously detect which provider contributed
> which resource.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
>   include/mesos/v1/mesos.proto 82d020e05b303a8248a90bc482b76b54b335146c 
>   src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a 
>   src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d 
>   src/tests/resources_utils.cpp 2cef55f7312d671307e097c2c4960c8dcf45c1ff 
>   src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 
> 
> 
> Diff: https://reviews.apache.org/r/57998/diff/3/
> 
> 
> Testing
> ---
> 
> make check (OS X, Fedora25)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 58200: Fix mesos runs with docker(pid namespace mismatch).

[Deshi Xiao]

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

Review request for mesos, Alexander Rukletsov and haosdent huang.


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


Repository: mesos


Description
---

Becuase MESOS HTTP checks doesn't work when mesos runs with
--docker_mesos_image ( pid namespace mismatch ).So let docker
executor run with container add host pid mapping(--pid=host)


Diffs
-

  src/slave/containerizer/docker.cpp ad9ab847cb3093724ef374d036c896b4e7f18b5e 


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


Testing
---


Thanks,

Deshi Xiao