Re: Review Request 59641: Follow symlinks when resolving paths specified in the various master/agent endpoints.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59641]

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 May 31, 2017, 6:57 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59641/
> ---
> 
> (Updated May 31, 2017, 6:57 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, James Peach, Vinod Kone, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-7572
> https://issues.apache.org/jira/browse/MESOS-7572
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main benefit of following symlinks in endpoints such as `/files` is that 
> frameworks will be able to construct a path to the sandbox much easier. This 
> will assist framework developers in making features that need to provide a 
> path when hitting various operator API endpoints. Currently, making use of a 
> path ending in `runs/latest` throws a 404.
> 
> One such application could be a scheduler providing the ability for users to 
> work with their task's sandbox directly without going to the Mesos UI, API 
> endpoints, or the actual system themselves.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 14de72fa4 
>   src/tests/files_tests.cpp c703cae03 
> 
> 
> Diff: https://reviews.apache.org/r/59641/diff/3/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j2 && make check -j2`
> 
> Checked the original behavior: 
> ```
> curl -i 
> localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001/executors/testapp-924b5c1524d84676a6a71665e2054d31/runs/cbec99c9-da2d-4aaa-bab7-4cee9a4df031
> HTTP/1.1 200 OK
> Date: Tue, 30 May 2017 17:43:08 GMT
> Content-Length: 644
> Content-Type: application/json
> 
> [{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stdout","size":1142,"uid":"root"}]
> ```
> 
> Checked the new behavior (this would return a 404 before this patch):
> ```
> curl -i 
> localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4eors/testapp-924b5c1524d84676a6a71665e2054d31/runs/latest
> HTTP/1.1 200 OK
> Date: Tue, 30 May 2017 17:43:13 GMT
> Content-Length: 584
> Content-Type: application/json
> 
> [{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stdout","size":1142,"uid":"root"}]
> ```
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 59696: Added test to verify GPU filtering with '--filter_gpu_resources=false'.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/tests/containerizer/nvidia_gpu_isolator_tests.cpp
Lines 423-425 (patched)


Same comment here, would be nice if we could run this without needing the 
gpu isolator.


- Benjamin Mahler


On May 31, 2017, 9:39 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59696/
> ---
> 
> (Updated May 31, 2017, 9:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7576
> https://issues.apache.org/jira/browse/MESOS-7576
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test to verify GPU filtering with '--filter_gpu_resources=false'.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 9a78ae65c1cd414b5093b54ff51724e31e31c9d3 
> 
> 
> Diff: https://reviews.apache.org/r/59696/diff/1/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ sudo GTEST_FILTER="*GPUFilterDisabled*" src/mesos-tests
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from NvidiaGpuTest
> [ RUN  ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_GPUFilterDisabled
> Executing pre-exec command 
> '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/klueska\/projects\/mesos\/build\/src\/mesos-containerizer"}'
> I0530 19:45:29.213250 33200 exec.cpp:162] Version: 1.4.0
> I0530 19:45:29.228093 33183 exec.cpp:237] Executor registered on agent 
> 5c94c185-8f83-4e70-af3e-2bebcb49a310-S0
> Received SUBSCRIBED event
> Subscribed executor on core-dev
> Received LAUNCH event
> Starting task dabc46f6-2866-474b-a1f5-3419a7b5927d
> Running '/home/klueska/projects/mesos/build/src/mesos-containerizer launch 
> '
> Forked command at 33228
> Command exited with status 0 (pid: 33228)
> I0530 19:45:29.424254 33195 exec.cpp:435] Executor asked to shutdown
> Received SHUTDOWN event
> Shutting down
> [   OK ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyGPUFilterDisabled 
> (9111 ms)
> [--] 1 test from NvidiaGpuTest (9113 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (9146 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 59695: Added test to verify GPU filtering with '--filter_gpu_resources=true'.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/tests/containerizer/nvidia_gpu_isolator_tests.cpp
Lines 367-369 (patched)


Is it possible to not use the isolator, and just specify 1 gpu, which 
should let us run the test without NVML?


- Benjamin Mahler


On May 31, 2017, 9:39 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59695/
> ---
> 
> (Updated May 31, 2017, 9:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7576
> https://issues.apache.org/jira/browse/MESOS-7576
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test to verify GPU filtering with '--filter_gpu_resources=true'.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 9a78ae65c1cd414b5093b54ff51724e31e31c9d3 
> 
> 
> Diff: https://reviews.apache.org/r/59695/diff/1/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ sudo GTEST_FILTER="*GPUFilterEnabled*" src/mesos-tests
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from NvidiaGpuTest
> [ RUN  ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_GPUFilterEnabled
> Executing pre-exec command 
> '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/klueska\/projects\/mesos\/build\/src\/mesos-containerizer"}'
> I0530 19:45:29.213250 33200 exec.cpp:162] Version: 1.4.0
> I0530 19:45:29.228093 33183 exec.cpp:237] Executor registered on agent 
> 5c94c185-8f83-4e70-af3e-2bebcb49a310-S0
> Received SUBSCRIBED event
> Subscribed executor on core-dev
> Received LAUNCH event
> Starting task dabc46f6-2866-474b-a1f5-3419a7b5927d
> Running '/home/klueska/projects/mesos/build/src/mesos-containerizer launch 
> '
> Forked command at 33228
> Command exited with status 0 (pid: 33228)
> I0530 19:45:29.424254 33195 exec.cpp:435] Executor asked to shutdown
> Received SHUTDOWN event
> Shutting down
> [   OK ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyGPUFilterDisabled 
> (9111 ms)
> [--] 1 test from NvidiaGpuTest (9113 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (9146 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

[Jiang Yan Xu]

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



Sorry this patch had to go through this long delay. We couldn't resolve a few 
open issues last time and I still think we need some follow-ups but I think we 
can get this patch into a mergable state and iterate on it. :)


src/common/resources.cpp
Line 617 (original), 617-622 (patched)


This is exatctly the content of `Resources::fromString` (which was added 
after we introduced `Resources::fromSimpleString`).



src/slave/containerizer/containerizer.cpp
Lines 113 (patched)


Sorry I know you had https://reviews.apache.org/r/52002 which was 
originally intended for all disk types and later evolved into a single `bool 
isRootDisk()` but given this is the only use of the helper can we just simply 
do the following here?

```
bool isRootDisk = !resource.has_disk();
```

Now I think a proper follow up would be to actually to give the root disk a 
type which is set by default.

e.g.,

```
message Source {
  enum Type {
UNKNOWN = 0;
PATH = 1;
MOUNT = 2;
ROOT = 3;
  }
  ...
  optional Source source = 3 [default = ROOT];
}
```

Which would have made things much simpler in many places.



src/slave/containerizer/containerizer.cpp
Lines 141 (patched)


The awkward situation here is that the code here is in fact reachable. This 
is the result of the us doing auto-detection before some minimal validation. 
Therefore a json input with a UNKNOWN type could crash the agent here.

To fix this particular case we can return an Error here but the general 
situation that we "detect without for minimal validation" is a bit concerning.

Let's just return an error right now. I think a proper follow-up would be 
to split `Resources::validate()` into composable pieces, similar to how we 
introduced `Resources::from*`.



src/slave/containerizer/containerizer.cpp
Lines 205 (patched)


You can take a copy by removing the `const&` above.



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
Lines 153-154 (original), 153-160 (patched)


This is covered by `Resources::fromString`.



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
Lines 160-163 (original), 166-173 (patched)


s/gpus/_resources/

Otherwise gpus vs. resources looks odd. (They are the same thing of 
different types)



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
Lines 182-190 (original), 192-196 (patched)


So if we are not auot-detecting for "gpus:" (which I think is fine for now 
as an incremental step), are the changes in 
"src/slave/containerizer/mesos/isolators/gpu/allocator.cpp" just a follow-up 
for https://reviews.apache.org/r/55761/?

Then can we separate it out to simplify this patch?



src/tests/persistent_volume_endpoints_tests.cpp
Lines 2180-2195 (patched)


Are these just reordering? Why do it?

Same for other changes in this file?



src/tests/reservation_endpoints_tests.cpp
Lines 1784-1791 (original)


Ditto. Why reordering?



src/v1/resources.cpp
Line 619 (original), 619-624 (patched)


This is exatctly the content of `Resources::fromString` (which was added 
after we introduced `Resources::fromSimpleString`).


- Jiang Yan Xu


On May 24, 2017, 12:56 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated May 24, 2017, 12:56 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When static resources indicate resources with a positive size, we use
> that for the resources on the agent. However, --resources can include
> resources with no size, which indicates that mesos agent determine the
> size of those resources from the agent and uses that information. Note
> that auto-detection of resources is allowed for all known resource
> types represented in JSON format only. Auto-detection is not done when
> the resources 

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

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

(Updated June 1, 2017, 12:38 a.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Fixed merge conflict.


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


Repository: mesos


Description
---

If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
framework (re)registration, the allocator does not offer resources for
those roles to such frameworks. Note that this functionality is added
for `v1::SUBSCRIBE` only (and not for scheduler driver API).

In addition, the master validates the suppressed roles to be a subset
of all the roles being (re)registered by the framework.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
  src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
  src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
  src/master/allocator/mesos/allocator.hpp 
b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
  src/master/allocator/mesos/hierarchical.hpp 
5e7c3068061012c51d4b9220dedf476408016a12 
  src/master/allocator/mesos/hierarchical.cpp 
6679d47ea53bbcd68d375654edf6e85890e5772a 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
  src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
  src/tests/hierarchical_allocator_tests.cpp 
eb2b647d3247a85e8b9b82e5589232c74ad8570f 
  src/tests/master_allocator_tests.cpp 0082045ccaebe015725c5e178aa89dac32b1c50c 
  src/tests/persistent_volume_endpoints_tests.cpp 
9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
  src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
  src/tests/resource_offers_tests.cpp 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
  src/tests/slave_recovery_tests.cpp e140f4d902a43b8fb2390541a357a217896b6228 


Diff: https://reviews.apache.org/r/57817/diff/12/

Changes: https://reviews.apache.org/r/57817/diff/11-12/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Anindya Sinha]

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

(Updated June 1, 2017, 12:38 a.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Fixed merge conflict.


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


Repository: mesos


Description
---

Added unit tests to verify offers are suppressed based on registration.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 


Diff: https://reviews.apache.org/r/57818/diff/12/

Changes: https://reviews.apache.org/r/57818/diff/11-12/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 59082: Added provider ID to offers.

[Jie Yu]

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




include/mesos/mesos.proto
Lines 1453 (patched)


Given we already have `provider_id` in `Resource`. Do we still need that?


- Jie Yu


On May 31, 2017, 10:02 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59082/
> ---
> 
> (Updated May 31, 2017, 10:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7593
> https://issues.apache.org/jira/browse/MESOS-7593
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b063ddaca60398db0ca8ef8a6a41e472b9f4a404 
>   include/mesos/v1/mesos.proto bc7dbe0d99de6e5a23b842751fce43896a6fcce9 
> 
> 
> Diff: https://reviews.apache.org/r/59082/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 59136: Added functions to add/remove resource providers.

[Jie Yu]

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




src/master/master.hpp
Lines 1857 (patched)


s/LocalResourceProviders/ResourceProviders/


- Jie Yu


On May 31, 2017, 12:44 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59136/
> ---
> 
> (Updated May 31, 2017, 12:44 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7592
> https://issues.apache.org/jira/browse/MESOS-7592
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
> 
> 
> Diff: https://reviews.apache.org/r/59136/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 59680: Added the 'LocalResourceProvider' structure.

[Jie Yu]

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




src/master/master.hpp
Lines 291 (patched)


Why both `LocalResourceProvider` and `ResourceProvider`? I'd prefer we 
don't have `LocalResourceProvider` top level, and rely on some field in 
`ResourceProvider` to tell


- Jie Yu


On May 31, 2017, 12:43 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59680/
> ---
> 
> (Updated May 31, 2017, 12:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7592
> https://issues.apache.org/jira/browse/MESOS-7592
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A local resource provider is a resource provider that is associated
> with an agent. Hence its lifetime is also tied to the lifetime of an
> agent. The 'LocalResourceProvider' structure stores the ID of the
> associated agent as well as the UUID that is used for registration.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
> 
> 
> Diff: https://reviews.apache.org/r/59680/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 59135: Refactored resource handling to use resource providers.

[Jie Yu]

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




src/master/master.hpp
Lines 159 (patched)


please use explicit `=delete` here.



src/master/master.hpp
Lines 124 (patched)


I'd kill this line. Let's group logically connected methods in a single 
block



src/master/master.hpp
Lines 126 (patched)


kill this line



src/master/master.hpp
Lines 128 (patched)


kill this line



src/master/master.hpp
Lines 133 (patched)


`info` contain `id` already?



src/master/master.hpp
Lines 212 (patched)


Can you put some comments on this?

Also, I suggest to use `resourceProvider` instead of just `provider`



src/master/master.hpp
Lines 250-254 (original), 270-274 (patched)


I believe this should be in resource provider as well. This is very 
resource provider specific.



src/master/master.hpp
Lines 250-254 (original), 270-274 (patched)


I believe this should be in resource provider as well. This is very 
resource provider specific.



src/master/master.cpp
Lines 9402-9407 (patched)


Let's move this to protobuf utils



src/master/master.cpp
Lines 9540-9553 (original), 9587-9600 (patched)


Any reason you have this function kept?


- Jie Yu


On May 31, 2017, 12:43 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59135/
> ---
> 
> (Updated May 31, 2017, 12:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7592
> https://issues.apache.org/jira/browse/MESOS-7592
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This refactoring add the 'ResourceProvider' abstraction for all
> resource related operations. Every agent instance has a resource
> provider. Only the checkpointed resources aren't abstracted away as this
> is specific to agents. This abstraction can be re-used for local
> resource providers and external resource providers.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
>   src/master/http.cpp eb80830fa003ad8f58243d3dc4cec9e03285e96f 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
>   src/master/quota_handler.cpp 7fe55808d4561ae5a8850ad904d09a7c65e014ce 
>   src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c 
> 
> 
> Diff: https://reviews.apache.org/r/59135/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [57818, 57817, 57815]

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

Error:
2017-05-31 23:39:53 URL:https://reviews.apache.org/r/57817/diff/raw/ 
[78096/78096] -> "57817.patch" [1]
error: patch failed: src/master/master.cpp:6237
error: src/master/master.cpp: patch does not apply
error: patch failed: src/tests/master_allocator_tests.cpp:1427
error: src/tests/master_allocator_tests.cpp: patch does not apply

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

- Mesos Reviewbot


On May 31, 2017, 2:53 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57818/
> ---
> 
> (Updated May 31, 2017, 2:53 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests to verify offers are suppressed based on registration.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 
> 
> 
> Diff: https://reviews.apache.org/r/57818/diff/11/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 59691: Renamed `flags` to `libprocess_flags` to fix build.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On May 31, 2017, 1:34 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59691/
> ---
> 
> (Updated May 31, 2017, 1:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-7597
> https://issues.apache.org/jira/browse/MESOS-7597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, the use of pre-compiled headers causes problems when reusing
> the identifier `flags`. This commit renames it to `libprocess_flags` to
> avoid conflicts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 4e60231802325afe7f9c0f30dc9efa431501a617 
> 
> 
> Diff: https://reviews.apache.org/r/59691/diff/1/
> 
> 
> Testing
> ---
> 
> Built and ran `stout-tests` and `libprocess-tests` on Windows. Also ran `make 
> check` in `build/3rdparty` on Linux. This resolves the build break.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Review Request 59696: Added test to verify GPU filtering with '--filter_gpu_resources=false'.

[Kevin Klues]

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

Review request for mesos, Anand Mazumdar and Benjamin Mahler.


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


Repository: mesos


Description
---

Added test to verify GPU filtering with '--filter_gpu_resources=false'.


Diffs
-

  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
9a78ae65c1cd414b5093b54ff51724e31e31c9d3 


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


Testing
---

$ GTEST_FILTER="" make -j check
$ sudo GTEST_FILTER="*GPUFilterDisabled*" src/mesos-tests
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from NvidiaGpuTest
[ RUN  ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_GPUFilterDisabled
Executing pre-exec command 
'{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/klueska\/projects\/mesos\/build\/src\/mesos-containerizer"}'
I0530 19:45:29.213250 33200 exec.cpp:162] Version: 1.4.0
I0530 19:45:29.228093 33183 exec.cpp:237] Executor registered on agent 
5c94c185-8f83-4e70-af3e-2bebcb49a310-S0
Received SUBSCRIBED event
Subscribed executor on core-dev
Received LAUNCH event
Starting task dabc46f6-2866-474b-a1f5-3419a7b5927d
Running '/home/klueska/projects/mesos/build/src/mesos-containerizer launch 
'
Forked command at 33228
Command exited with status 0 (pid: 33228)
I0530 19:45:29.424254 33195 exec.cpp:435] Executor asked to shutdown
Received SHUTDOWN event
Shutting down
[   OK ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyGPUFilterDisabled 
(9111 ms)
[--] 1 test from NvidiaGpuTest (9113 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (9146 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Kevin Klues

Review Request 59695: Added test to verify GPU filtering with '--filter_gpu_resources=true'.

[Kevin Klues]

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

Review request for mesos, Anand Mazumdar and Benjamin Mahler.


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


Repository: mesos


Description
---

Added test to verify GPU filtering with '--filter_gpu_resources=true'.


Diffs
-

  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
9a78ae65c1cd414b5093b54ff51724e31e31c9d3 


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


Testing
---

$ GTEST_FILTER="" make -j check
$ sudo GTEST_FILTER="*GPUFilterEnabled*" src/mesos-tests
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from NvidiaGpuTest
[ RUN  ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_GPUFilterEnabled
Executing pre-exec command 
'{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/klueska\/projects\/mesos\/build\/src\/mesos-containerizer"}'
I0530 19:45:29.213250 33200 exec.cpp:162] Version: 1.4.0
I0530 19:45:29.228093 33183 exec.cpp:237] Executor registered on agent 
5c94c185-8f83-4e70-af3e-2bebcb49a310-S0
Received SUBSCRIBED event
Subscribed executor on core-dev
Received LAUNCH event
Starting task dabc46f6-2866-474b-a1f5-3419a7b5927d
Running '/home/klueska/projects/mesos/build/src/mesos-containerizer launch 
'
Forked command at 33228
Command exited with status 0 (pid: 33228)
I0530 19:45:29.424254 33195 exec.cpp:435] Executor asked to shutdown
Received SHUTDOWN event
Shutting down
[   OK ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyGPUFilterDisabled 
(9111 ms)
[--] 1 test from NvidiaGpuTest (9113 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (9146 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Kevin Klues

Re: Review Request 59677: Added '--filter_gpu_resources' flag to the mesos master.

[Kevin Klues]

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

(Updated May 31, 2017, 9:34 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Updated based on @bmahler's comments.

Two new patches have been added for the tests:
  https://reviews.apache.org/r/59695/
  https://reviews.apache.org/r/59696/


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


Repository: mesos


Description
---

When set to true, this flag will cause the mesos master to filter
offers containing GPU resources by only sending them to frameworks
that opt into the 'GPU_RESOURCES' framework capability. When set to
false, this flag will cause the master to not filter offers containing
GPU resources, and indiscriminately send them to all frameworks
whether they set the 'GPU_RESOURCES' capability or not.  This flag is
meant as a temporary workaround towards the eventual deprecation of
the 'GPU_RESOURCES' capability.

Please see the following link for more information:
https://www.mail-archive.com/dev@mesos.apache.org/msg37571.html


Diffs (updated)
-

  docs/configuration.md 8c3be230b789d0a56f74c0e015f73492efb65603 
  include/mesos/allocator/allocator.hpp 
dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 
  src/master/allocator/mesos/allocator.hpp 
119b461136123229f85ed3c3cfd41137974a6b9b 
  src/master/allocator/mesos/hierarchical.hpp 
82fea40ad09c95f0096934c4210a39a6f0638ed9 
  src/master/allocator/mesos/hierarchical.cpp 
ca368bb5ef0dcfe83672004d58a4de6f85d6b4bc 
  src/master/flags.hpp 93ca9b9348732a9219440dd9196c73d93c181120 
  src/master/flags.cpp b1c0886977292afe938a4eb36193b224931b0eb2 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
  src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 
  src/tests/api_tests.cpp 97a8cc9714f2ee64c1398a9961d96cd40edc3a75 
  src/tests/master_allocator_tests.cpp b9e04226a1688499847bc25c7c220c0b82ba8db7 
  src/tests/master_quota_tests.cpp 2c7ec5ac9e585c1ab52bc3771e215e5ee30abb3a 
  src/tests/persistent_volume_endpoints_tests.cpp 
21136b29d724959527b889f52611baee0668 
  src/tests/reservation_endpoints_tests.cpp 
8c195eb7d788fbca8722d66d81c77d399702160a 
  src/tests/reservation_tests.cpp 47ccf7f6f6ee48236f6794ce3084ce4f425c93c5 
  src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c 
  src/tests/slave_recovery_tests.cpp df0c5c88786190be06df7ef3602834aa8985cefe 


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

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


Testing
---

$ GTEST_FILTER="" make -j check
$ sudo GTEST_FILTER="*GPUFilterDisabled*" src/mesos-tests
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from NvidiaGpuTest
[ RUN  ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyGPUFilterDisabled
Executing pre-exec command 
'{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/klueska\/projects\/mesos\/build\/src\/mesos-containerizer"}'
I0530 19:45:29.213250 33200 exec.cpp:162] Version: 1.4.0
I0530 19:45:29.228093 33183 exec.cpp:237] Executor registered on agent 
5c94c185-8f83-4e70-af3e-2bebcb49a310-S0
Received SUBSCRIBED event
Subscribed executor on core-dev
Received LAUNCH event
Starting task dabc46f6-2866-474b-a1f5-3419a7b5927d
Running '/home/klueska/projects/mesos/build/src/mesos-containerizer launch 
'
Forked command at 33228
Command exited with status 0 (pid: 33228)
I0530 19:45:29.424254 33195 exec.cpp:435] Executor asked to shutdown
Received SHUTDOWN event
Shutting down
[   OK ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyGPUFilterDisabled 
(9111 ms)
[--] 1 test from NvidiaGpuTest (9113 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (9146 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Kevin Klues

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

[Jiang Yan Xu]

> On Dec. 15, 2016, 11:58 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp
> > Lines 165-169 (original), 288-305 (patched)
> > 
> >
> > Here what I found to be a bit awkward:
> > 
> > We already know here that we want to process "cpus", "mem", "disk" and 
> > "ports". We don't do it directly but we call a helper method which has to 
> > check that the input are precisely what this method provides.
> > 
> > ```
> > if (name != "cpus" && name != "mem" && name != "disk" && name != 
> > "ports") {
> >   return Error(
> > "Auto-detection of resource type '" + name + "' not supported");
> > }
> > ```
> > 
> > and from this method we have the check the output.
> > 
> > This makes the helper not a natrual abstraction but rather a tightly 
> > coupled blocked to code to avoid duplicating logic for all predefined 
> > resources. If that's the objective, then we can just do:
> > 
> > ```
> > const hashset predefined = ({"cpus", "mem", "disk", "ports", 
> > "gpus"};
> > 
> > foreach (const string& name, predefined) {
> > #ifdef __linux__
> >   // GPUS are handled separately.
> >   if (name == "gpus") {
> > Try gpus = NvidiaGpuAllocator::resources(flags);
> > if (gpus.isError()) {
> >   return Error("Failed to obtain GPU resources: " + gpus.error());
> > }
> > 
> > foreach(const Resource& gpu, gpus.get()) {
> >   result.push_back(gpu);
> > }
> >   }
> > #endif
> >   
> >   // Content of `detect()` except we can just add stuff to `result` 
> > without returning.
> > }
> > 
> > // Custom resources.
> > foreach(const Resource& resource, parsed.get()) {
> >   if (!predefined.contains(resource.name())) {
> > result.push_back(resource);
> >   }
> > }
> > ```
> > 
> > The result should be less amount of code without any increase in code 
> > duplication.
> 
> Anindya Sinha wrote:
> I removed the following block from `Try detect()` since 
> if the `name` does not match the resources that cannot be auto-detected, it 
> would be a no-op, ie. returns the resources with no modifications.
> 
> ```
> if (name != "cpus" && name != "mem" && name != "disk" && name != "ports") 
> {
>   return Error(
> "Auto-detection of resource type '" + name + "' not supported");
> }
> ```
> 
> I think I would still want to keep the `detect()` function separate. 
> Seems cleaner to me, otherwise we would have to have bunch of `if` conditions 
> within this `foreach` for each of the predefined resource types. How does 
> this look instead?
> 
> ```
>   const hashset predefined = ({"cpus", "mem", "disk", "ports", 
> "gpus"});
> 
>   foreach (const string& name, predefined) {
> #ifdef __linux__
> // GPU resource is handled separately.
> if (name == "gpus") {
>   Try gpus = NvidiaGpuAllocator::resources(flags);
>   if (gpus.isError()) {
> return Error("Failed to obtain GPU resources: " + gpus.error());
>   }
> 
>   foreach (const Resource& gpu, gpus.get()) {
> result.push_back(gpu);
>   }
> }
> #endif
> 
> if (name != "gpus") {
>   Try _resources = detect(name, flags, 
> parsed.get());
>   if (_resources.isError()) {
> return Error(
> "Failed to obtain " + name + " resources: " + 
> _resources.error());
>   }
> 
>   result.insert(
>   result.end(), _resources.get().begin(), _resources.get().end());
> }
>   }
> 
>   // Custom resources.
>   foreach (const Resource& resource, parsed.get()) {
> if (!predefined.contains(resource.name())) {
>   result.push_back(resource);
> }
>   }
> ```

This is a reiteration of the comment above and I am inclined to get this 
shipped first and then work on a refactor.

But for posterity, to summarize in a short example, I don't think this is a 
good pattern.

```
void genericHelper(condition)
{
  // common code.
  
  switch (something) {
case A:
  customHelperA();
case B:
  customHelperB();
  }
}

void highLevelMethod()
{
  switch (condition) {
case A:
  genericHelper(c1);
case B:
  genericHelper(c2);
  }
}
```

In this case the generic helper is not really generic. In the high level method 
if we already have to enumerate specific cases, we should be able to directly 
call the custom helpers.

To address of the concern of "code duplication" we should be able to just pull 
out the common code into the high-level method. I think we can do something 
similar to: 

Re: Review Request 59662: Changed `Executor` and `Framework` to class.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59662/
> ---
> 
> (Updated May 30, 2017, 11:15 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's wierd that `Executor` and `Framework` are struct, but they have
> `private` fields.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59662/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59551: Change launcher working directory before dropping privilege.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2017, 11:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59551/
> ---
> 
> (Updated May 24, 2017, 11:45 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The launcher needs to change its working directory before dropping
> privilege by switching users and installing capabilities, because
> afterwards it might not have access to traverse to the desired
> working directory.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> f48d294a0a832dfe248c4a83849ee5a63cb76bce 
> 
> 
> Diff: https://reviews.apache.org/r/59551/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 59671: Reordered `Slave` copy constructor and assignment operator.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:50 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59671/
> ---
> 
> (Updated May 30, 2017, 11:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reordered `Slave` copy constructor and assignment operator.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59671/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59671: Reordered `Slave` copy constructor and assignment operator.

[Jie Yu]

> On May 31, 2017, 4:59 p.m., Anand Mazumdar wrote:
> > Can we do these in the same review where we started using `delete`?

Sorry, rebasing will be hard :(


- Jie


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


On May 30, 2017, 11:50 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59671/
> ---
> 
> (Updated May 30, 2017, 11:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reordered `Slave` copy constructor and assignment operator.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59671/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59666: Moved Framework::addTask to the source file.

[Jie Yu]

> On May 31, 2017, 4:45 p.m., Anand Mazumdar wrote:
> > src/slave/slave.hpp
> > Lines 868 (patched)
> > 
> >
> > Update the summary/description to `hasTask()` instead of `addTask()`?

Fixed. Good catch.


- Jie


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


On May 30, 2017, 11:17 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59666/
> ---
> 
> (Updated May 30, 2017, 11:17 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved Framework::addTask to the source file.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 055e4967a4c820d7835f5fecdc46f82eaddd9117 
> 
> 
> Diff: https://reviews.apache.org/r/59666/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59662: Changed `Executor` and `Framework` to class.

[Jie Yu]

> On May 31, 2017, 4:37 p.m., Anand Mazumdar wrote:
> > src/slave/slave.hpp
> > Lines 689 (patched)
> > 
> >
> > hmm, I like it the other way around i.e., since this class has a lot of 
> > public fields, `struct` is more reasonable here. A `class` with default 
> > scoping as `public` seems non-intuitive.
> > 
> > @bmahler what do you think?

Google style suggests to use class over struct if the methods are non-trivial.
https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

I like being more explicit. For the sake of consistency, I'll fix other places.


- Jie


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


On May 30, 2017, 11:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59662/
> ---
> 
> (Updated May 30, 2017, 11:15 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's wierd that `Executor` and `Framework` are struct, but they have
> `private` fields.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59662/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59659: Used `delete` for `Slave` copy constructor and assignment operator.

[Jie Yu]

> On May 31, 2017, 4:32 p.m., Anand Mazumdar wrote:
> > src/slave/slave.hpp
> > Lines 507-508 (original), 507-508 (patched)
> > 
> >
> > Nit: Can we move it after the constructor declaration now that it no 
> > longer needs to be private. This would help readability.

See subsequent patches.


- Jie


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


On May 30, 2017, 11:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59659/
> ---
> 
> (Updated May 30, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `delete` for `Slave` copy constructor and assignment operator.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59659/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59659: Used `delete` for `Slave` copy constructor and assignment operator.

[Jie Yu]

> On May 31, 2017, 4:32 p.m., Anand Mazumdar wrote:
> > - Can we do a sweep for the `Framework`/`Executor` structs too? I haven't 
> > had a look later in the chain yet.
> > - Can we do a similar cleanup for the Master header file too?

Framework and Executor cleanup is in the subsequent patch. I'll follow up with 
Master later.


- Jie


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


On May 30, 2017, 11:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59659/
> ---
> 
> (Updated May 30, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `delete` for `Slave` copy constructor and assignment operator.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59659/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59233: Updated v6 address for containers running on host network.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [59233, 59131, 59130, 59129, 59688, 59128, 59127]

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

Error:
error: patch failed: 3rdparty/CMakeLists.txt:282
error: 3rdparty/CMakeLists.txt: patch does not apply
error: patch failed: 3rdparty/cmake/Versions.cmake:1
error: 3rdparty/cmake/Versions.cmake: patch does not apply
error: patch failed: CHANGELOG:192
error: CHANGELOG: patch does not apply
error: patch failed: cmake/CompilationConfigure.cmake:228
error: cmake/CompilationConfigure.cmake: patch does not apply
error: patch failed: src/slave/main.cpp:450
error: src/slave/main.cpp: patch does not apply
error: patch failed: src/tests/CMakeLists.txt:193
error: src/tests/CMakeLists.txt: patch does not apply
error: patch failed: 
src/tests/containerizer/environment_secret_isolator_tests.cpp:1
error: src/tests/containerizer/environment_secret_isolator_tests.cpp: patch 
does not apply
error: patch failed: src/tests/containerizer/volume_secret_isolator_tests.cpp:1
error: src/tests/containerizer/volume_secret_isolator_tests.cpp: patch does not 
apply
error: patch failed: src/tests/containerizer/xfs_quota_tests.cpp:590
error: src/tests/containerizer/xfs_quota_tests.cpp: patch does not apply
error: patch failed: src/uri/fetchers/docker.cpp:527
error: src/uri/fetchers/docker.cpp: patch does not apply

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

- Mesos Reviewbot Windows


On May 31, 2017, 6:38 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59233/
> ---
> 
> (Updated May 31, 2017, 6:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7488
> https://issues.apache.org/jira/browse/MESOS-7488
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently the agent is populating only the v4 address for containers
> running on the host network. However, clusters running on a dual stack
> network need to report v4 and v6 address for containers running on the
> host network. This change allows v6 address specified by operators to be
> advertised for containers running on the host network.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 209aff180db1a68ae360d7c278937fe645efdb2b 
> 
> 
> Diff: https://reviews.apache.org/r/59233/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 59320: Added test case for agent re-registration.

[Neil Conway]

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

(Updated May 31, 2017, 8:35 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Improve comment, try enabling test on Windows.


Repository: mesos


Description
---

Add an explanatory comment and a test case for a particular case in the
master's logic for handling agent re-registration.


Diffs (updated)
-

  src/master/master.cpp a5376f9fc3c3c6c72b22cfd36ec56a1de0d03a3a 
  src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 


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

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


Testing
---

`make check`

Checked that if you add `CHECK_NOTNULL(framework)` to the master code (circa 
the newly added comment), the incorrect `CHECK` is triggered by this test case 
(but not by any existing test cases).


Thanks,

Neil Conway

Review Request 59691: Renamed `flags` to `libprocess_flags` to fix build.

[Andrew Schwartzmeyer]

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

Review request for mesos, Benjamin Mahler and Joseph Wu.


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


Repository: mesos


Description
---

On Windows, the use of pre-compiled headers causes problems when reusing
the identifier `flags`. This commit renames it to `libprocess_flags` to
avoid conflicts.


Diffs
-

  3rdparty/libprocess/src/process.cpp 4e60231802325afe7f9c0f30dc9efa431501a617 


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


Testing
---

Built and ran `stout-tests` and `libprocess-tests` on Windows. Also ran `make 
check` in `build/3rdparty` on Linux. This resolves the build break.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 59320: Added test case for agent re-registration.

[Neil Conway]

> On May 31, 2017, 12:09 a.m., Benjamin Mahler wrote:
> > src/tests/partition_tests.cpp
> > Lines 2387 (patched)
> > 
> >
> > Hm.. why disabled on windows? Do you know why or is this just copied? 
> > For example, it's not clear to me why the above partition test isn't 
> > disabled but the below one is.

I don't really have a way to run tests on Windows, and I'm nervous just 
depending on the Windows reviewbot/CI. I'll try enabling it and see what 
happens...


- Neil


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


On May 16, 2017, 8:19 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59320/
> ---
> 
> (Updated May 16, 2017, 8:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an explanatory comment and a test case for a particular case in the
> master's logic for handling agent re-registration.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad 
>   src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 
> 
> 
> Diff: https://reviews.apache.org/r/59320/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Checked that if you add `CHECK_NOTNULL(framework)` to the master code (circa 
> the newly added comment), the incorrect `CHECK` is triggered by this test 
> case (but not by any existing test cases).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

[Andrew Schwartzmeyer]

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



Does not build on Windows. Please ensure that the tests include _at least_ a 
compilation on Windows before committing. The build is broken now.

- Andrew Schwartzmeyer


On May 10, 2017, 6:06 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 10, 2017, 6:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> 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 a check 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 
> 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/11/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass 
> except ``ExamplesTest.DiskFullFramework``, however enabling this will 
> definitely break some libprocess APIs (though not in the way that Mesos uses 
> them) and legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 59677: Added '--filter_gpu_resources' flag to the mesos master.

[Benjamin Mahler]

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


Fix it, then Ship it!





docs/configuration.md
Lines 637-643 (patched)


We should clarify that it's (offers (from agents with GPU resources)) 
rather than (offers with GPU resources).

E.g. to filter offers from GPU agents



docs/configuration.md
Lines 644 (patched)


Do we also have the ticket to point to?



src/master/flags.cpp
Lines 465-474 (patched)


Ditto here about this filtering all offers on GPU agents rather than only 
those offers with the GPUs.



src/master/flags.cpp
Lines 474 (patched)


Ditto here w.r.t the ticket.



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp
Lines 365-367 (patched)


Do we have already have a test for the positive case where we do expect 
filtering?

If not, would be great to have one.

Also, if you want easier cherry-picking, you can pull this test into a 
separate patch, and have yet another one for the positive filter test case.



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp
Lines 365-366 (patched)


How about: This test ensures that a scheduler can be allocated offers from 
GPU agents without the GPU_RESOURCES framework capability, when the ...



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp
Lines 368 (patched)


Maybe just s/Verify// since verifying something is implicitly what each 
test is doing?



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp
Lines 390-393 (patched)


You may want to assert that the capability isn't there rather than assuming 
so.


- Benjamin Mahler


On May 31, 2017, 2:47 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59677/
> ---
> 
> (Updated May 31, 2017, 2:47 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7576
> https://issues.apache.org/jira/browse/MESOS-7576
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When set to true, this flag will cause the mesos master to filter
> offers containing GPU resources by only sending them to frameworks
> that opt into the 'GPU_RESOURCES' framework capability. When set to
> false, this flag will cause the master to not filter offers containing
> GPU resources, and indiscriminately send them to all frameworks
> whether they set the 'GPU_RESOURCES' capability or not.  This flag is
> meant as a temporary workaround towards the eventual deprecation of
> the 'GPU_RESOURCES' capability.
> 
> Please see the following link for more information:
> https://www.mail-archive.com/dev@mesos.apache.org/msg37571.html
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 8c3be230b789d0a56f74c0e015f73492efb65603 
>   include/mesos/allocator/allocator.hpp 
> dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 
>   src/master/allocator/mesos/allocator.hpp 
> 119b461136123229f85ed3c3cfd41137974a6b9b 
>   src/master/allocator/mesos/hierarchical.hpp 
> 82fea40ad09c95f0096934c4210a39a6f0638ed9 
>   src/master/allocator/mesos/hierarchical.cpp 
> ca368bb5ef0dcfe83672004d58a4de6f85d6b4bc 
>   src/master/flags.hpp 93ca9b9348732a9219440dd9196c73d93c181120 
>   src/master/flags.cpp b1c0886977292afe938a4eb36193b224931b0eb2 
>   src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
>   src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 
>   src/tests/api_tests.cpp 97a8cc9714f2ee64c1398a9961d96cd40edc3a75 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 9a78ae65c1cd414b5093b54ff51724e31e31c9d3 
>   src/tests/master_allocator_tests.cpp 
> b9e04226a1688499847bc25c7c220c0b82ba8db7 
>   src/tests/master_quota_tests.cpp 2c7ec5ac9e585c1ab52bc3771e215e5ee30abb3a 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 21136b29d724959527b889f52611baee0668 
>   src/tests/reservation_endpoints_tests.cpp 
> 8c195eb7d788fbca8722d66d81c77d399702160a 
>   src/tests/reservation_tests.cpp 47ccf7f6f6ee48236f6794ce3084ce4f425c93c5 
>   src/tests/resource_offers_tests.cpp 
> f0bca1d9e03013ce35215b0ffa6b50b38972dc0c 
>   src/tests/slave_recovery_tests.cpp df0c5c88786190be06df7ef3602834aa8985cefe 
> 
> 
> Diff: https://reviews.apache.org/r/59677/diff/1/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ sudo GTEST_FILTER="*GPUFilterDisabled*" 

Re: Review Request 59641: Follow symlinks when resolving paths specified in the various master/agent endpoints.

[Aaron Wood via Review Board]

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

(Updated May 31, 2017, 6:57 p.m.)


Review request for mesos, Andrew Schwartzmeyer, James Peach, Vinod Kone, and 
Zhitao Li.


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


Repository: mesos


Description
---

The main benefit of following symlinks in endpoints such as `/files` is that 
frameworks will be able to construct a path to the sandbox much easier. This 
will assist framework developers in making features that need to provide a path 
when hitting various operator API endpoints. Currently, making use of a path 
ending in `runs/latest` throws a 404.

One such application could be a scheduler providing the ability for users to 
work with their task's sandbox directly without going to the Mesos UI, API 
endpoints, or the actual system themselves.


Diffs (updated)
-

  src/slave/slave.cpp 14de72fa4 
  src/tests/files_tests.cpp c703cae03 


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

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


Testing
---

`mkdir build && cd build && cmake .. && make -j2 && make check -j2`

Checked the original behavior: 
```
curl -i 
localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001/executors/testapp-924b5c1524d84676a6a71665e2054d31/runs/cbec99c9-da2d-4aaa-bab7-4cee9a4df031
HTTP/1.1 200 OK
Date: Tue, 30 May 2017 17:43:08 GMT
Content-Length: 644
Content-Type: application/json

[{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stdout","size":1142,"uid":"root"}]
```

Checked the new behavior (this would return a 404 before this patch):
```
curl -i 
localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4eors/testapp-924b5c1524d84676a6a71665e2054d31/runs/latest
HTTP/1.1 200 OK
Date: Tue, 30 May 2017 17:43:13 GMT
Content-Length: 584
Content-Type: application/json

[{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stdout","size":1142,"uid":"root"}]
```


Thanks,

Aaron Wood

Re: Review Request 59302: Simplified master logic for agent re-registration.

[Neil Conway]

> On May 30, 2017, 11:44 p.m., Benjamin Mahler wrote:
> > Looking at this patch in isolation, it's hard to see what this 
> > simplifcation has to do with 0.x agents, so the description was a little 
> > confusing for me. Did some previous patches in the chain set us up for 
> > simplifying this? It would be great to have a little more description.

Sorry, I missed this review comment -- but yeah, this change is setup by the 
previous changes to assume that we always have the `FrameworkInfo` for a task 
that the master knows about.


- Neil


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


On May 15, 2017, 11:51 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59302/
> ---
> 
> (Updated May 15, 2017, 11:51 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that 0.x agents are not supported, the master logic can be
> simplified somewhat.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad 
> 
> 
> Diff: https://reviews.apache.org/r/59302/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 59131: Added an IPv6 address storage to UPID.

[Avinash sridharan]

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

(Updated May 31, 2017, 6:40 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

`libprocess` pids can now hold a v6 address along with a v4 address.


Diffs (updated)
-

  3rdparty/libprocess/include/process/pid.hpp 
c634916a37f570194945b9c7d7b786ffeae0408a 
  3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 


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

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


Testing
---

make check.


Thanks,

Avinash sridharan

Re: Review Request 59130: Added storage for IPv6 in a `libprocess` process.

[Avinash sridharan]

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

(Updated May 31, 2017, 6:40 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

The operator can specify a v6 address to be used by a `libprocess`
process by specifying the `LIBPROCESS_IP6` environment variable.
Currently the `lipbrocess` stores the IPv6 address but doesn't open a
socket on the v6 address in order to listen or send traffic on the v6
address.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 


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

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


Testing
---

make check.


Thanks,

Avinash sridharan

Re: Review Request 59129: Introduced `inet6::Address` to handle IPv6 addresses in `libprocess`.

[Avinash sridharan]

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

(Updated May 31, 2017, 6:38 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Introduced `inet6::Address` to handle IPv6 addresses in `libprocess`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/address.hpp 
6b143c3d00c1d7ebd1697c26b6d312a64f30839a 
  3rdparty/libprocess/src/socket.cpp 85920a1abb3a2b2da167647dcf1351b825c72c80 


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

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


Testing
---

make check.


Thanks,

Avinash sridharan

Re: Review Request 59233: Updated v6 address for containers running on host network.

[Avinash sridharan]

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

(Updated May 31, 2017, 6:38 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Currently the agent is populating only the v4 address for containers
running on the host network. However, clusters running on a dual stack
network need to report v4 and v6 address for containers running on the
host network. This change allows v6 address specified by operators to be
advertised for containers running on the host network.


Diffs (updated)
-

  src/slave/slave.cpp 209aff180db1a68ae360d7c278937fe645efdb2b 


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

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


Testing
---

make check


Thanks,

Avinash sridharan

Re: Review Request 59688: Moved `net::IPNetwork` to `net::IP::Network`.

[Avinash sridharan]

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

(Updated May 31, 2017, 6:37 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


Changes
---

Added `make check`.


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


Repository: mesos


Description
---

Moved `net::IPNetwork` to `net::IP::Network`.


Diffs
-

  3rdparty/stout/include/stout/ip.hpp ad2bd922158eecd12b51b272d2a003b8ec8d3550 
  3rdparty/stout/tests/ip_tests.cpp 4c6f81bd53413360182b447ddb149a18e406870e 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
3aecd8c5a2eb1319fe31ebeba815b6766e50904f 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 ce795628f19f0e8138ad9a0b50b7d644b9d734a8 
  src/tests/containerizer/cni_isolator_tests.cpp 
565e58ae75e918453e4386f5e35a5a844a8b15f8 
  src/tests/utils.hpp a2ae43c6a20ab3753a3501d8ce23ffb0f6ac0005 
  src/tests/utils.cpp 053976d3c4cf120ff5e7dff6e96c6862a5b617b1 


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


Testing (updated)
---

make check


Thanks,

Avinash sridharan

Review Request 59688: Moved `net::IPNetwork` to `net::IP::Network`.

[Avinash sridharan]

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

Review request for mesos, Benjamin Hindman and Jie Yu.


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


Repository: mesos


Description
---

Moved `net::IPNetwork` to `net::IP::Network`.


Diffs
-

  3rdparty/stout/include/stout/ip.hpp ad2bd922158eecd12b51b272d2a003b8ec8d3550 
  3rdparty/stout/tests/ip_tests.cpp 4c6f81bd53413360182b447ddb149a18e406870e 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
3aecd8c5a2eb1319fe31ebeba815b6766e50904f 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 ce795628f19f0e8138ad9a0b50b7d644b9d734a8 
  src/tests/containerizer/cni_isolator_tests.cpp 
565e58ae75e918453e4386f5e35a5a844a8b15f8 
  src/tests/utils.hpp a2ae43c6a20ab3753a3501d8ce23ffb0f6ac0005 
  src/tests/utils.cpp 053976d3c4cf120ff5e7dff6e96c6862a5b617b1 


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


Testing
---


Thanks,

Avinash sridharan

Re: Review Request 59219: Removed logic for handling missing FrameworkID in ExecutorInfo.

[Neil Conway]

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

(Updated May 31, 2017, 6:27 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

The master will inject missing `FrameworkID`s into `ExecutorInfo` since
Mesos 0.23 (see MESOS-2290), so it should be safe to assume it is always
set.


Diffs (updated)
-

  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 


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

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 59428: Moved a couple of logging lines to `WARNING` level.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [59504, 59428]

Failed command: ['cmd', '/c', 'support\\windows-build.bat 2>&1 > build_59428']

Error:

C:\Users\mesos\mesos>REM Licensed to the Apache Software Foundation (ASF) under 
one 

C:\Users\mesos\mesos>REM or more contributor license agreements.  See the 
NOTICE file 

C:\Users\mesos\mesos>REM distributed with this work for additional information 

C:\Users\mesos\mesos>REM regarding copyright ownership.  The ASF licenses this 
file 

C:\Users\mesos\mesos>REM to you under the Apache License, Version 2.0 (the 

C:\Users\mesos\mesos>REM "License"); you may not use this file except in 
compliance 

C:\Users\mesos\mesos>REM with the License.  You may obtain a copy of the 
License at 

C:\Users\mesos\mesos>REM

C:\Users\mesos\mesos>REM http://www.apache.org/licenses/LICENSE-2.0 

C:\Users\mesos\mesos>REM

C:\Users\mesos\mesos>REM Unless required by applicable law or agreed to in 
writing, software 

C:\Users\mesos\mesos>REM distributed under the License is distributed on an "AS 
IS" BASIS, 

C:\Users\mesos\mesos>REM WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either 
express or implied. 

C:\Users\mesos\mesos>REM See the License for the specific language governing 
permissions and 

C:\Users\mesos\mesos>REM limitations under the License. 

C:\Users\mesos\mesos>REM NOTE: Before you run this script, you must have the 
Visual Studio 

C:\Users\mesos\mesos>REM environment variables set up. Visual Studio provides a 
script to do this, 

C:\Users\mesos\mesos>REM depending on the version of Visual Studio installed: 

C:\Users\mesos\mesos>REM /path/to/Visual Studio 14/VC/vcvarsall.bat 

C:\Users\mesos\mesos>REM /path/to/Microsoft Visual 
Studio/2017/Community/VC/Auxiliary/Build/vcvars64.bat 

C:\Users\mesos\mesos>REM NOTE: Batch doesn't have any way of exiting upon 
failing a command. 

C:\Users\mesos\mesos>REM The best we can do is add this line after every 
command that matters: 

C:\Users\mesos\mesos>REM if 0 neq 0 exit /b 0 

C:\Users\mesos\mesos>REM NOTE: In order to run the tests, your Windows volume 
must have a 

C:\Users\mesos\mesos>REM folder named `tmp` at the top level. You can create it 
like this: 

C:\Users\mesos\mesos>REM MKDIR C:\tmp 

C:\Users\mesos\mesos>REM Make sure that we are in the right directory. We do 
this by checking that 

C:\Users\mesos\mesos>REM the `support` folder exists in the current directory 
and is not a symlink. 

C:\Users\mesos\mesos>REM This code is awkwardly split across two conditionals 
because batch scripts 

C:\Users\mesos\mesos>REM do not support logical operators like `&&`. 

C:\Users\mesos\mesos>if not exist support (goto not_in_root ) 

C:\Users\mesos\mesos>fsutil reparsepoint query "support"   | find "Symbolic 
Link"   1>nul  && (goto not_in_root ) 

C:\Users\mesos\mesos>REM Create a build directory. 

C:\Users\mesos\mesos>MKDIR build 

C:\Users\mesos\mesos>CD build 

C:\Users\mesos\mesos\build>if 0 NEQ 0 exit /b 0 

C:\Users\mesos\mesos\build>REM Generate the Visual Studio solution. 

C:\Users\mesos\mesos\build>REM You can pass in other flags by setting 
`OTHER_CMAKE_OPTIONS` before 

C:\Users\mesos\mesos\build>REM calling the script. For example, the ASF CI will 
add `-DPATCHEXE_PATH=...` 

C:\Users\mesos\mesos\build>REM because the path to GNU Patch is not the 
default. 

C:\Users\mesos\mesos\build>if not defined CMAKE_GENERATOR (set 
CMAKE_GENERATOR=Visual Studio 15 2017 Win64 ) 

C:\Users\mesos\mesos\build>cmake .. -G "Visual Studio 15 2017 Win64" -T 
"host=x64" -DENABLE_LIBEVENT=1 -DHAS_AUTHENTICATION=0  
-- The C compiler identification is MSVC 19.10.25017.0
-- The CXX compiler identification is MSVC 19.10.25017.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual 
Studio/2017/BuildTools/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual 
Studio/2017/BuildTools/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual 
Studio/2017/BuildTools/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual 
Studio/2017/BuildTools/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- cotire 1.7.9 loaded.
-- 
-- * Beginning Mesos CMake configuration step *
-- 
-- INSTALLATION PREFIX: C:/Program Files/Mesos

Re: Review Request 59428: Moved a couple of logging lines to `WARNING` level.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On May 23, 2017, 9:57 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59428/
> ---
> 
> (Updated May 23, 2017, 9:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This ensures ease of debugging when a HTTP request fails due to
> access permissions (4xx errors) or when the path is not found.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 96ce7dbc486a2f1d55d2238a8a102bf024b12b1c 
> 
> 
> Diff: https://reviews.apache.org/r/59428/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 59685: Fixed flakiness in OneWayPartitionTest.MasterToSlave.

[Neil Conway]

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

(Updated May 31, 2017, 6:04 p.m.)


Review request for mesos.


Changes
---

Add comment.


Repository: mesos


Description (updated)
---

The test did not pause the clock. This allowed the following sequence of
events to occur, with low probability:

  (1) Agent sends register message M1 to master.
  (2) Agent register timer expires, sends register message M2 to master.
  (3) Master sees M1 and adds agent with ID A1.
  (4) Agent gets SlaveRegisteredMessage with ID A1.
  (5) Test case injects `exited` event for agent; master marks agent as
  disconnected
  (6) Master sees M2; since the agent is currently disconnected, the
  master removes A1 and adds the agent with ID A2.
  (7) Agent gets SlaveRegisteredMessage with ID A2. Since this is
  unexpected, it exits ("Registered but got wrong id").

This commit fixes the test case to pause the clock; this prevents the
second registration attempt in step (2) above.

The scenario described above might occur in an actual Mesos deployment,
albeit with very low probability. This would result in a Mesos agent
shutting down immediately after initial registration. MESOS-7596 has
been created to track this issue.


Diffs (updated)
-

  src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 


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

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


Testing
---

`./src/mesos-tests --gtest_filter="OneWayPartitionTest.MasterToSlave" 
--gtest_repeat=1 --gtest_break_on_failure`

Without this change, the test fails once per ~300 iterations.


Thanks,

Neil Conway

Re: Review Request 56721: Implemented pruneImages with a mark and sweep in docker store.

[Zhitao Li]

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

(Updated May 31, 2017, 6:01 p.m.)


Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This includes the following changes:
- add a `pruneImages()` function on the chain of relevant classes;
- implement prune in docker store;
- fix mock interface to keep existing tests pass.


Diffs (updated)
-

  src/slave/containerizer/composing.hpp 
bef6d880ca1d815fc21d9d017f6de2d26ad5218f 
  src/slave/containerizer/composing.cpp 
a003e1b80dc9b4dec5b3fbbadb2daecf855c90c7 
  src/slave/containerizer/containerizer.hpp 
0954ed6175e4c8f963bf371e54e0f9ffe7bc9c1c 
  src/slave/containerizer/docker.hpp b602a5698cae12686f51c4b9370a06042cda6270 
  src/slave/containerizer/docker.cpp 9f84109d7de22a39ace6e44e0c7d8d501bcb24de 
  src/slave/containerizer/mesos/containerizer.hpp 
ea0691945d3450fcc336df03d9cf7b48afbd8b3d 
  src/slave/containerizer/mesos/containerizer.cpp 
f3e6210eccd4a6b445ffd4447e69526d424ea36d 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp 
954da1681778878c8aff6150002e52ecb648d1bb 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
d86afd2a6ff0bf87e624db1c99255c85068bf6ab 
  src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
232c027f8f96da0cb30b957bce4607d3695050d2 
  src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
cd684b33eb308ce1eeb4539a5b2d51985d835db7 
  src/slave/containerizer/mesos/provisioner/docker/store.hpp 
1cf68665d33bd40a7605d26c96fb7b618407fdd0 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
8058dcb31b9de59fa8d3638b553632235542 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
7cba54ce490d1e6e17081cd7e04fd6759ceddb8e 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
3d4da9069c8bc8b71243f5ebd0d6f68b7d9c0cac 
  src/slave/containerizer/mesos/provisioner/store.hpp 
01ab83dca79e51b8c96d18ee65705912b0ac8324 
  src/slave/containerizer/mesos/provisioner/store.cpp 
cc5cc81e05f29bb0e11ffa13cdb8d63d4397114f 
  src/tests/containerizer.hpp 4bd40c32625bc1f7998d523c6ee81bf78ac74538 
  src/tests/containerizer.cpp 1d2b6391cf7a7545fa44206c59d05764f3e8cdfb 
  src/tests/containerizer/mock_containerizer.hpp 
0adcb01e6c12d6cc4abed1f14fa2df833ffc6569 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 55727: Checkpoint and track docker image layer sizes.

[Zhitao Li]

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

(Updated May 31, 2017, 6 p.m.)


Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.


Changes
---

Rebase and merge layers metrics.


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


Repository: mesos


Description (updated)
---

This patch added the following capabilities:
1. add the size and fetch timestamp of each docker image layer in metadata;
2. checkpoint and recover size (timestamp will be added later);
3. reuse the `DiskUsageCollector` class to track size of each downloaded store;
4. for upgrade path, reuse same collector to back fill the size of layers.
5. a counter for number of layers pulled
6. a gauge for total number of layers
7. a timer for pull latency distribution in the last hour

Tests to be added later. Sending out review for earlier feedback.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/message.proto 
a55ac9010d5e5b5691ec34b6c342e57811aee4ce 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.hpp 
954da1681778878c8aff6150002e52ecb648d1bb 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
d86afd2a6ff0bf87e624db1c99255c85068bf6ab 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
8058dcb31b9de59fa8d3638b553632235542 


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

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


Testing
---


Thanks,

Zhitao Li

Review Request 59687: Added tests for recovering ContainerConfig.

[Zhitao Li]

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

Review request for mesos, Gilbert Song and Jason Lai.


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


Repository: mesos


Description
---

Added tests for recovering ContainerConfig.


Diffs
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
e46643434dc85d766bd549a037f36a89a6738678 


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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 53105: Added an hourly timer for `slave/docker_containerizer/pull`.

[Zhitao Li]

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

(Updated May 31, 2017, 5:58 p.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, Jason Lai, and Jie Yu.


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


Repository: mesos


Description
---

Added an hourly timer for `slave/docker_containerizer/pull`.


Diffs
-

  src/slave/containerizer/docker.hpp 44efa44586455df416fc9cdc51c92bd9027f24c3 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 


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


Testing
---

Ran `mesos-execute` with a docker image and docker containerizer, observed the 
following metrics:
```
curl -s localhost:5051/metrics/snapshot | jq . | grep pull
  "containerizer/docker/pull_ms/p999": 26209.36173824,
  "containerizer/docker/pull_ms/p90": 21036.405248,
  "containerizer/docker/pull_ms/p": 26256.388615424,
  "containerizer/docker/pull_ms/p50": 135.570944,
  "containerizer/docker/pull_ms/max": 26261.613824,
  "containerizer/docker/pull_ms/p95": 23649.009536,
  "containerizer/docker/pull_ms/min": 103.215872,
  "containerizer/docker/pull_ms/p99": 25739.0929664,
  "containerizer/docker/pull_ms": 26261.613824,
  "containerizer/docker/pull_ms/count": 3
```


Thanks,

Zhitao Li

Re: Review Request 55335: Backfilled required fields in TaskInfo in MesosContainerizer* tests.

[Zhitao Li]

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

(Updated May 31, 2017, 5:57 p.m.)


Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

These tests were using a `TaskInfo` field which is missing required
protobuf fields. After the previous patch begins to checkpoint
`ContainerConfig`, the incomplete `TaskInfo` cannot be checkpointed,
thus caused these tests to fail.

This patch backfills these fields to make these tests pass.


Diffs (updated)
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
d3a2bd7a9a91fe5c2c21066b1633289f36b6c8c5 


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

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


Testing
---

GTEST_FILTER="MesosContainerizer*" make check


Thanks,

Zhitao Li

Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in Mesos containerizer.

[Zhitao Li]

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

(Updated May 31, 2017, 5:57 p.m.)


Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether 
recovered.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
ea0691945d3450fcc336df03d9cf7b48afbd8b3d 
  src/slave/containerizer/mesos/containerizer.cpp 
f3e6210eccd4a6b445ffd4447e69526d424ea36d 
  src/slave/containerizer/mesos/paths.hpp 
12ae7c7329f9e8d334cb32c30cc38465bddc046c 
  src/slave/containerizer/mesos/paths.cpp 
0c61c20c345a327ec469b382558aaeed0280e754 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 59685: Fixed flakiness in OneWayPartitionTest.MasterToSlave.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/partition_tests.cpp
Lines 3419 (patched)


Can you add a comment here that you pause to ensure only one registration 
attempt is made?


- Vinod Kone


On May 31, 2017, 4:57 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59685/
> ---
> 
> (Updated May 31, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test did not pause the clock. This allowed the following sequence of
> events to occur, with low probability:
> 
>   (1) Agent sends register message M1 to master.
>   (2) Agent register timer expires, sends register message M2 to master.
>   (3) Master sees M1 and adds agent with ID A1.
>   (4) Agent gets SlaveRegisteredMessage with ID A1.
>   (5) Test case injects `exited` event for agent; master marks agent as
>   disconnected
>   (6) Master sees M2; since the agent is currently disconnected, the
>   master removes A1 and adds the agent with ID A2.
>   (7) Agent gets SlaveRegisteredMessage with ID A2. Since this is
>   unexpected, it exits ("Registered but got wrong id").
> 
> This commit fixes the test case to pause the clock; this prevents the
> second registration attempt in step (2) above.
> 
> 
> Diffs
> -
> 
>   src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 
> 
> 
> Diff: https://reviews.apache.org/r/59685/diff/1/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests --gtest_filter="OneWayPartitionTest.MasterToSlave" 
> --gtest_repeat=1 --gtest_break_on_failure`
> 
> Without this change, the test fails once per ~300 iterations.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 59673: Combined an 'undef' workaround for Windows.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:51 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59673/
> ---
> 
> (Updated May 30, 2017, 11:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Combined an 'undef' workaround for Windows.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59673/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59672: Reordered gauge methods in `Slave` class.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:51 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59672/
> ---
> 
> (Updated May 30, 2017, 11:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reordered gauge methods in `Slave` class.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 055e4967a4c820d7835f5fecdc46f82eaddd9117 
> 
> 
> Diff: https://reviews.apache.org/r/59672/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59671: Reordered `Slave` copy constructor and assignment operator.

[Anand Mazumdar]

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



Can we do these in the same review where we started using `delete`?

- Anand Mazumdar


On May 30, 2017, 11:50 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59671/
> ---
> 
> (Updated May 30, 2017, 11:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reordered `Slave` copy constructor and assignment operator.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59671/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59670: Removed an unnecessary declaration in slave.hpp.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:50 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59670/
> ---
> 
> (Updated May 30, 2017, 11:50 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unnecessary declaration in slave.hpp.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 055e4967a4c820d7835f5fecdc46f82eaddd9117 
> 
> 
> Diff: https://reviews.apache.org/r/59670/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59669: Removed some unnecessary std:: prefix in slave.cpp.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 133 (patched)


Move this below `std::set`?


- Anand Mazumdar


On May 30, 2017, 11:18 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59669/
> ---
> 
> (Updated May 30, 2017, 11:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some unnecessary std:: prefix in slave.cpp.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 055e4967a4c820d7835f5fecdc46f82eaddd9117 
> 
> 
> Diff: https://reviews.apache.org/r/59669/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59685: Fixed flakiness in OneWayPartitionTest.MasterToSlave.

[Mesos Reviewbot Windows]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot Windows


On May 31, 2017, 4:57 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59685/
> ---
> 
> (Updated May 31, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test did not pause the clock. This allowed the following sequence of
> events to occur, with low probability:
> 
>   (1) Agent sends register message M1 to master.
>   (2) Agent register timer expires, sends register message M2 to master.
>   (3) Master sees M1 and adds agent with ID A1.
>   (4) Agent gets SlaveRegisteredMessage with ID A1.
>   (5) Test case injects `exited` event for agent; master marks agent as
>   disconnected
>   (6) Master sees M2; since the agent is currently disconnected, the
>   master removes A1 and adds the agent with ID A2.
>   (7) Agent gets SlaveRegisteredMessage with ID A2. Since this is
>   unexpected, it exits ("Registered but got wrong id").
> 
> This commit fixes the test case to pause the clock; this prevents the
> second registration attempt in step (2) above.
> 
> 
> Diffs
> -
> 
>   src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 
> 
> 
> Diff: https://reviews.apache.org/r/59685/diff/1/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests --gtest_filter="OneWayPartitionTest.MasterToSlave" 
> --gtest_repeat=1 --gtest_break_on_failure`
> 
> Without this change, the test fails once per ~300 iterations.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 59685: Fixed flakiness in OneWayPartitionTest.MasterToSlave.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

The test did not pause the clock. This allowed the following sequence of
events to occur, with low probability:

  (1) Agent sends register message M1 to master.
  (2) Agent register timer expires, sends register message M2 to master.
  (3) Master sees M1 and adds agent with ID A1.
  (4) Agent gets SlaveRegisteredMessage with ID A1.
  (5) Test case injects `exited` event for agent; master marks agent as
  disconnected
  (6) Master sees M2; since the agent is currently disconnected, the
  master removes A1 and adds the agent with ID A2.
  (7) Agent gets SlaveRegisteredMessage with ID A2. Since this is
  unexpected, it exits ("Registered but got wrong id").

This commit fixes the test case to pause the clock; this prevents the
second registration attempt in step (2) above.


Diffs
-

  src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 


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


Testing
---

`./src/mesos-tests --gtest_filter="OneWayPartitionTest.MasterToSlave" 
--gtest_repeat=1 --gtest_break_on_failure`

Without this change, the test fails once per ~300 iterations.


Thanks,

Neil Conway

Re: Review Request 59668: Removed an unnecessary function declaration from slave.hpp.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:18 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59668/
> ---
> 
> (Updated May 30, 2017, 11:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unnecessary function declaration from slave.hpp.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 055e4967a4c820d7835f5fecdc46f82eaddd9117 
> 
> 
> Diff: https://reviews.apache.org/r/59668/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59667: Reordered the methods in `Framework` into logic units.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 4885 (patched)


Can you update the summary/description given that it also reorders methods 
in the `Slave` struct?



src/slave/slave.cpp
Line 6971 (original), 6991 (patched)


Move this above `checkpointFramework()`?


- Anand Mazumdar


On May 30, 2017, 11:17 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59667/
> ---
> 
> (Updated May 30, 2017, 11:17 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also, make sure the order in the header is in sync with that in the
> corresponding source file.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 055e4967a4c820d7835f5fecdc46f82eaddd9117 
> 
> 
> Diff: https://reviews.apache.org/r/59667/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59550: Check bounding capabilities at isolator creation time.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2017, 11:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59550/
> ---
> 
> (Updated May 24, 2017, 11:45 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When we create the `linux/capabilities` isolator, enforce the rule that
> the bounding capabilities are a superset of the allowed capabilities
> when both are specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 
> 60d22aa877c1ab62a08222e5efe8800e337684da 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1 
> 
> 
> Diff: https://reviews.apache.org/r/59550/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 59666: Moved Framework::addTask to the source file.

[Anand Mazumdar]

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


Fix it, then Ship it!





src/slave/slave.hpp
Lines 868 (patched)


Update the summary/description to `hasTask()` instead of `addTask()`?


- Anand Mazumdar


On May 30, 2017, 11:17 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59666/
> ---
> 
> (Updated May 30, 2017, 11:17 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved Framework::addTask to the source file.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 055e4967a4c820d7835f5fecdc46f82eaddd9117 
> 
> 
> Diff: https://reviews.apache.org/r/59666/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59665: Reordered the streaming functions in slave.hpp|cpp.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:16 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59665/
> ---
> 
> (Updated May 30, 2017, 11:16 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that the streaming functions are sorted according to alphabet
> order.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
>   src/slave/slave.cpp 055e4967a4c820d7835f5fecdc46f82eaddd9117 
> 
> 
> Diff: https://reviews.apache.org/r/59665/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59664: Used `delete` for deleting copy constructor and assignment operator.

[Anand Mazumdar]

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


Fix it, then Ship it!




ahh, seems like the `Executor`/`Framework` struct are here. Moving them all 
into a single review along with the `Slave` changes might be a good idea.


src/slave/slave.hpp
Lines 838-839 (original), 838-839 (patched)


Move it closer to the constructor now that it does not need to be private 
anymore?


- Anand Mazumdar


On May 30, 2017, 11:16 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59664/
> ---
> 
> (Updated May 30, 2017, 11:16 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is for `Executor` and `Framework` class in slave.hpp.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59664/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59663: Removed unnecessary friend declaration for an stream operator.

[Anand Mazumdar]

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


Ship it!




Good catch!

- Anand Mazumdar


On May 30, 2017, 11:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59663/
> ---
> 
> (Updated May 30, 2017, 11:15 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unnecessary friend declaration for an stream operator.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59663/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59662: Changed `Executor` and `Framework` to class.

[Anand Mazumdar]

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




src/slave/slave.hpp
Lines 689 (patched)


hmm, I like it the other way around i.e., since this class has a lot of 
public fields, `struct` is more reasonable here. A `class` with default scoping 
as `public` seems non-intuitive.

@bmahler what do you think?


- Anand Mazumdar


On May 30, 2017, 11:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59662/
> ---
> 
> (Updated May 30, 2017, 11:15 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's wierd that `Executor` and `Framework` are struct, but they have
> `private` fields.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59662/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59661: Fixed some style issues in slave.hpp.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59661/
> ---
> 
> (Updated May 30, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed some style issues in slave.hpp.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59661/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59660: Reordered some method definitions in `Slave` class.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 30, 2017, 11:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59660/
> ---
> 
> (Updated May 30, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved all the metrics related continuation methods close to each other
> so it's more consistent.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59660/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59659: Used `delete` for `Slave` copy constructor and assignment operator.

[Anand Mazumdar]

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


Fix it, then Ship it!




- Can we do a sweep for the `Framework`/`Executor` structs too? I haven't had a 
look later in the chain yet.
- Can we do a similar cleanup for the Master header file too?


src/slave/slave.hpp
Lines 507-508 (original), 507-508 (patched)


Nit: Can we move it after the constructor declaration now that it no longer 
needs to be private. This would help readability.


- Anand Mazumdar


On May 30, 2017, 11:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59659/
> ---
> 
> (Updated May 30, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `delete` for `Slave` copy constructor and assignment operator.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59659/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59549: Add the agent --bounding_capabilities flag.

[Jie Yu]

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


Fix it, then Ship it!





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


I would do the same as that in `allowed_capabilities`

```
containerizer (currently only supported in MesosContainerizer)
```



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


iff `linux/capabilities` isolator is enabled.



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


ditto. `linux/capabilities` isolator is enabled.


- Jie Yu


On May 24, 2017, 11:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59549/
> ---
> 
> (Updated May 24, 2017, 11:45 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add the agent --bounding_capabilities flag to enable the operator to
> specify a default bounding capabilities set.
> 
> 
> Diffs
> -
> 
>   src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc 
>   src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44 
> 
> 
> Diff: https://reviews.apache.org/r/59549/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 59548: Add a `bounding_capabilities` field to ContainerLaunchInfo.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2017, 11:42 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59548/
> ---
> 
> (Updated May 24, 2017, 11:42 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add a `bounding_capabilities` field to ContainerLaunchInfo and propagate
> bounding capabilities through the command executor.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 0f96334c236027780db1d88807e09685ccda4562 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
> 
> 
> Diff: https://reviews.apache.org/r/59548/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 59547: Rename ContainerLaunchInfo `capabilities` field.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2017, 11:41 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59547/
> ---
> 
> (Updated May 24, 2017, 11:41 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename the ContainerLaunchInfo `capabilities` field to
> `effective_capabilities` since it is intended to be the set of
> capabilities we actually make effective in the launched task.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 0f96334c236027780db1d88807e09685ccda4562 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 403faa3be771be19726617a4e5e968edffb54554 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 
> 60d22aa877c1ab62a08222e5efe8800e337684da 
>   src/slave/containerizer/mesos/launch.cpp 
> f48d294a0a832dfe248c4a83849ee5a63cb76bce 
> 
> 
> Diff: https://reviews.apache.org/r/59547/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [57815, 57817, 57818]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On May 31, 2017, 2:53 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57818/
> ---
> 
> (Updated May 31, 2017, 2:53 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests to verify offers are suppressed based on registration.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 
> 
> 
> Diff: https://reviews.apache.org/r/57818/diff/11/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 59641: Follow symlinks when resolving paths specified in the various master/agent endpoints.

[Aaron Wood via Review Board]

> On May 31, 2017, 1:05 a.m., Vinod Kone wrote:
> > src/files/files.cpp
> > Line 874 (original), 874 (patched)
> > 
> >
> > I don't follow this. Why are we checking random prefixes for whether 
> > they are symlinks on the file system?

I had done this because the path parts that are stored in the map represent the 
resolved symlink. The path that's passed into this method here is the symlink 
pre-resolved. Without that check the `continue` statement gets hit until we're 
out of the loop and then return `None()`.


> On May 31, 2017, 1:05 a.m., Vinod Kone wrote:
> > src/files/files.cpp
> > Lines 892-909 (patched)
> > 
> >
> > Instead of doing all this, can we just "attach" "./run/latest" to 
> > the files actor when we attach the ".../run/"? See 
> > `Framework::addExecutor` and `Framework::recoverExecutor` in slave.cpp. 
> > Look for `files->attach(...)`. That to me seems more straightforward?

Sounds good, I'll have a look.


> On May 31, 2017, 1:05 a.m., Vinod Kone wrote:
> > src/files/files.cpp
> > Line 893 (original), 912 (patched)
> > 
> >
> > I would do the shadow variable fix in a separate dependent review 
> > instead of mixing it here.

I'll revert my naming changes here. Why is it that this kind of change needs a 
separate patch?


- Aaron


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


On May 30, 2017, 8:14 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59641/
> ---
> 
> (Updated May 30, 2017, 8:14 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, James Peach, Vinod Kone, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-7572
> https://issues.apache.org/jira/browse/MESOS-7572
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main benefit of following symlinks in endpoints such as `/files` is that 
> frameworks will be able to construct a path to the sandbox much easier. This 
> will assist framework developers in making features that need to provide a 
> path when hitting various operator API endpoints. Currently, making use of a 
> path ending in `runs/latest` throws a 404.
> 
> One such application could be a scheduler providing the ability for users to 
> work with their task's sandbox directly without going to the Mesos UI, API 
> endpoints, or the actual system themselves.
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp b03279ee0 
>   src/tests/files_tests.cpp c703cae03 
> 
> 
> Diff: https://reviews.apache.org/r/59641/diff/2/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j2 && make check -j2`
> 
> Checked the original behavior: 
> ```
> curl -i 
> localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001/executors/testapp-924b5c1524d84676a6a71665e2054d31/runs/cbec99c9-da2d-4aaa-bab7-4cee9a4df031
> HTTP/1.1 200 OK
> Date: Tue, 30 May 2017 17:43:08 GMT
> Content-Length: 644
> Content-Type: application/json
> 
> [{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stdout","size":1142,"uid":"root"}]
> ```
> 
> Checked the new behavior (this would return a 404 before this patch):
> ```
> curl -i 
> localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4eors/testapp-924b5c1524d84676a6a71665e2054d31/runs/latest
> HTTP/1.1 200 OK
> Date: Tue, 30 May 2017 17:43:13 GMT
> Content-Length: 584
> Content-Type: application/json
> 
> 

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Anindya Sinha]

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

(Updated May 31, 2017, 2:53 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

rebase


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


Repository: mesos


Description
---

Added unit tests to verify offers are suppressed based on registration.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 


Diff: https://reviews.apache.org/r/57818/diff/11/

Changes: https://reviews.apache.org/r/57818/diff/10-11/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

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

(Updated May 31, 2017, 2:52 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Fix compilation for tests.


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


Repository: mesos


Description
---

If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
framework (re)registration, the allocator does not offer resources for
those roles to such frameworks. Note that this functionality is added
for `v1::SUBSCRIBE` only (and not for scheduler driver API).

In addition, the master validates the suppressed roles to be a subset
of all the roles being (re)registered by the framework.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 
  src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
  src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
  src/master/allocator/mesos/allocator.hpp 
119b461136123229f85ed3c3cfd41137974a6b9b 
  src/master/allocator/mesos/hierarchical.hpp 
82fea40ad09c95f0096934c4210a39a6f0638ed9 
  src/master/allocator/mesos/hierarchical.cpp 
ca368bb5ef0dcfe83672004d58a4de6f85d6b4bc 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
  src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 
  src/tests/hierarchical_allocator_tests.cpp 
eb2b647d3247a85e8b9b82e5589232c74ad8570f 
  src/tests/master_allocator_tests.cpp b9e04226a1688499847bc25c7c220c0b82ba8db7 
  src/tests/persistent_volume_endpoints_tests.cpp 
21136b29d724959527b889f52611baee0668 
  src/tests/reservation_tests.cpp 47ccf7f6f6ee48236f6794ce3084ce4f425c93c5 
  src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c 
  src/tests/slave_recovery_tests.cpp df0c5c88786190be06df7ef3602834aa8985cefe 


Diff: https://reviews.apache.org/r/57817/diff/11/

Changes: https://reviews.apache.org/r/57817/diff/10-11/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 59605: Use glog for logging in executors.

[Andrei Budnik]

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




src/docker/executor.cpp
Line 734 (original), 734 (patched)


The justification of the use of cout/cerr in some places described in the 
related ticket [#MESOS-7586](https://issues.apache.org/jira/browse/MESOS-7586).


- Andrei Budnik


On May 29, 2017, 2:59 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59605/
> ---
> 
> (Updated May 29, 2017, 2:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6961
> https://issues.apache.org/jira/browse/MESOS-6961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use glog for logging in executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
> 
> 
> Diff: https://reviews.apache.org/r/59605/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. jenkins: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/
> 
> 3. I've modified NoTaskKillingCapability from command_executor_tests.cpp to 
> launch command "sleep 0.1" (as well as "sleep 0"),
> then to wait for a task completion. The test was launched in an endless loop:
> 
> GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
> --gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
>  --verbose
> 
> I've run this test multiple times for a while both on mac os and linux 
> (gru1.hw.ca1.mesosphere.com).
> Race condition problem described in a jira ticket wasn't reproduced.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59634: Updated 'config.py' in the new Mesos CLI to take settings as parameter.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59634]

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 May 30, 2017, 9:13 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59634/
> ---
> 
> (Updated May 30, 2017, 9:13 p.m.)
> 
> 
> Review request for mesos and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we were importing settings directly from the `bin`
> directory, which is obviously not portable. Instead, we need to be
> passing the settings into the constructor for the config so that it
> can access it from there.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/main.py d0ec2bdb04631fbaa62efb6ae67e138140a1296a 
>   src/cli_new/lib/cli/config.py 353785b8bdc13b745d2dc9469201c16a8816ebc6 
> 
> 
> Diff: https://reviews.apache.org/r/59634/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 59525: Added filtering of `/slaves` endpoint and `GET_AGENTS` API call.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59453, 58955, 58964, 59099, 59100, 59101, 59147, 59525]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On May 31, 2017, 12:41 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59525/
> ---
> 
> (Updated May 31, 2017, 12:41 p.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-7416
> https://issues.apache.org/jira/browse/MESOS-7416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support of the `VIEW_ROLE` ACL to the results generated by
> the `/slaves` as well as the `GET_AGENTS` API v1 call. This means
> that calls to this endpoint (API call) will hide roles that the
> user making the request is not authorized to see.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp eb80830fa003ad8f58243d3dc4cec9e03285e96f 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
> 
> 
> Diff: https://reviews.apache.org/r/59525/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58720: Extended the unit test infrastructure in the new Mesos CLI.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [58720]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On May 31, 2017, 2:10 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 31, 2017, 2:10 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/16/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 59136: Added functions to add/remove resource providers.

[Jan Schlicht]

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

(Updated May 31, 2017, 2:44 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 59680: Added the 'LocalResourceProvider' structure.

[Jan Schlicht]

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

(Updated May 31, 2017, 2:43 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

A local resource provider is a resource provider that is associated
with an agent. Hence its lifetime is also tied to the lifetime of an
agent. The 'LocalResourceProvider' structure stores the ID of the
associated agent as well as the UUID that is used for registration.


Diffs
-

  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 59135: Refactored resource handling to use resource providers.

[Jan Schlicht]

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

(Updated May 31, 2017, 2:43 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

This refactoring add the 'ResourceProvider' abstraction for all
resource related operations. Every agent instance has a resource
provider. Only the checkpointed resources aren't abstracted away as this
is specific to agents. This abstraction can be re-used for local
resource providers and external resource providers.


Diffs
-

  src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
  src/master/http.cpp eb80830fa003ad8f58243d3dc4cec9e03285e96f 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
  src/master/quota_handler.cpp 7fe55808d4561ae5a8850ad904d09a7c65e014ce 
  src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 59525: Added filtering of `/slaves` endpoint and `GET_AGENTS` API call.

[Alexander Rojas]

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

(Updated May 31, 2017, 2:41 p.m.)


Review request for mesos, Adam B and Greg Mann.


Summary (updated)
-

Added filtering of `/slaves` endpoint and `GET_AGENTS` API call.


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


Repository: mesos


Description (updated)
---

Adds support of the `VIEW_ROLE` ACL to the results generated by
the `/slaves` as well as the `GET_AGENTS` API v1 call. This means
that calls to this endpoint (API call) will hide roles that the
user making the request is not authorized to see.


Diffs (updated)
-

  src/master/http.cpp eb80830fa003ad8f58243d3dc4cec9e03285e96f 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 58720: Extended the unit test infrastructure in the new Mesos CLI.

[Armand Grillet]

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

(Updated May 31, 2017, 12:10 p.m.)


Review request for mesos and Kevin Klues.


Changes
---

Last issue fixed and code rebased.


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


Repository: mesos


Description
---

This infrastructure includes the ability to bring up a test cluster to
run the CLI against. Future unit tests will use this infrastructure to
test their commands against a running mesos cluster. The tests require
some binaries created when building Mesos.


Diffs (updated)
-

  src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
  src/cli_new/lib/cli/tests/base.py PRE-CREATION 
  src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
  src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
  src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 


Diff: https://reviews.apache.org/r/58720/diff/16/

Changes: https://reviews.apache.org/r/58720/diff/15-16/


Testing
---

PEP8 and Pylint used to make sure that the code style is correct. Manuel test:

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Armand Grillet

Re: Review Request 59681: Updated offer handling to support local resource providers.

[Jan Schlicht]

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

(Updated May 31, 2017, 1:57 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Repository: mesos


Description
---

The offer callbacks used by the allocator now also check if the
'ResourceProviderID' in the offered resources is of a local resource
provider. In that case the provider ID field in the created offer will
be set. When a framework accepts such an offer, this field is used
to determine if this applies to a resource provider or an agent.


Diffs
-

  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 59525: Added filtering of roles in the endpoint.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59453, 58955, 58964, 59099, 59100, 59101, 59147, 59525]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On May 31, 2017, 12:50 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59525/
> ---
> 
> (Updated May 31, 2017, 12:50 p.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-7416
> https://issues.apache.org/jira/browse/MESOS-7416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added filtering of roles in the  endpoint.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
> 
> 
> Diff: https://reviews.apache.org/r/59525/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Review Request 59525: Added filtering of roles in the endpoint.

[Alexander Rojas]

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

Review request for mesos, Adam B and Greg Mann.


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


Repository: mesos


Description
---

Added filtering of roles in the  endpoint.


Diffs
-

  src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 


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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59677: Added '--filter_gpu_resources' flag to the mesos master.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59677]

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 May 31, 2017, 2:47 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59677/
> ---
> 
> (Updated May 31, 2017, 2:47 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7576
> https://issues.apache.org/jira/browse/MESOS-7576
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When set to true, this flag will cause the mesos master to filter
> offers containing GPU resources by only sending them to frameworks
> that opt into the 'GPU_RESOURCES' framework capability. When set to
> false, this flag will cause the master to not filter offers containing
> GPU resources, and indiscriminately send them to all frameworks
> whether they set the 'GPU_RESOURCES' capability or not.  This flag is
> meant as a temporary workaround towards the eventual deprecation of
> the 'GPU_RESOURCES' capability.
> 
> Please see the following link for more information:
> https://www.mail-archive.com/dev@mesos.apache.org/msg37571.html
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 8c3be230b789d0a56f74c0e015f73492efb65603 
>   include/mesos/allocator/allocator.hpp 
> dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 
>   src/master/allocator/mesos/allocator.hpp 
> 119b461136123229f85ed3c3cfd41137974a6b9b 
>   src/master/allocator/mesos/hierarchical.hpp 
> 82fea40ad09c95f0096934c4210a39a6f0638ed9 
>   src/master/allocator/mesos/hierarchical.cpp 
> ca368bb5ef0dcfe83672004d58a4de6f85d6b4bc 
>   src/master/flags.hpp 93ca9b9348732a9219440dd9196c73d93c181120 
>   src/master/flags.cpp b1c0886977292afe938a4eb36193b224931b0eb2 
>   src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
>   src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 
>   src/tests/api_tests.cpp 97a8cc9714f2ee64c1398a9961d96cd40edc3a75 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 9a78ae65c1cd414b5093b54ff51724e31e31c9d3 
>   src/tests/master_allocator_tests.cpp 
> b9e04226a1688499847bc25c7c220c0b82ba8db7 
>   src/tests/master_quota_tests.cpp 2c7ec5ac9e585c1ab52bc3771e215e5ee30abb3a 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 21136b29d724959527b889f52611baee0668 
>   src/tests/reservation_endpoints_tests.cpp 
> 8c195eb7d788fbca8722d66d81c77d399702160a 
>   src/tests/reservation_tests.cpp 47ccf7f6f6ee48236f6794ce3084ce4f425c93c5 
>   src/tests/resource_offers_tests.cpp 
> f0bca1d9e03013ce35215b0ffa6b50b38972dc0c 
>   src/tests/slave_recovery_tests.cpp df0c5c88786190be06df7ef3602834aa8985cefe 
> 
> 
> Diff: https://reviews.apache.org/r/59677/diff/1/
> 
> 
> Testing
> ---
> 
> $ GTEST_FILTER="" make -j check
> $ sudo GTEST_FILTER="*GPUFilterDisabled*" src/mesos-tests
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from NvidiaGpuTest
> [ RUN  ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyGPUFilterDisabled
> Executing pre-exec command 
> '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/klueska\/projects\/mesos\/build\/src\/mesos-containerizer"}'
> I0530 19:45:29.213250 33200 exec.cpp:162] Version: 1.4.0
> I0530 19:45:29.228093 33183 exec.cpp:237] Executor registered on agent 
> 5c94c185-8f83-4e70-af3e-2bebcb49a310-S0
> Received SUBSCRIBED event
> Subscribed executor on core-dev
> Received LAUNCH event
> Starting task dabc46f6-2866-474b-a1f5-3419a7b5927d
> Running '/home/klueska/projects/mesos/build/src/mesos-containerizer launch 
> '
> Forked command at 33228
> Command exited with status 0 (pid: 33228)
> I0530 19:45:29.424254 33195 exec.cpp:435] Executor asked to shutdown
> Received SHUTDOWN event
> Shutting down
> [   OK ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyGPUFilterDisabled 
> (9111 ms)
> [--] 1 test from NvidiaGpuTest (9113 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (9146 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 59082: Added provider ID to offers.

[Jan Schlicht]

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

(Updated May 31, 2017, 12:02 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  include/mesos/mesos.proto b063ddaca60398db0ca8ef8a6a41e472b9f4a404 
  include/mesos/v1/mesos.proto bc7dbe0d99de6e5a23b842751fce43896a6fcce9 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 59136: Added functions to add/remove resource providers.

[Jan Schlicht]

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

(Updated May 31, 2017, 12:01 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Rebased to use `LocalResourceProvider`.


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Review Request 59680: Added the 'LocalResourceProvider' structure.

[Jan Schlicht]

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

Review request for mesos, Benjamin Bannier and Jie Yu.


Repository: mesos


Description
---

A local resource provider is a resource provider that is associated
with an agent. Hence its lifetime is also tied to the lifetime of an
agent. The 'LocalResourceProvider' structure stores the ID of the
associated agent as well as the UUID that is used for registration.


Diffs
-

  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 


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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 59135: Refactored resource handling to use resource providers.

[Jan Schlicht]

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

(Updated May 31, 2017, 11:58 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Rebased, refactored and slightly changed the scope of this RR: local resource 
providers are introduced in #59680


Summary (updated)
-

Refactored resource handling to use resource providers.


Repository: mesos


Description (updated)
---

This refactoring add the 'ResourceProvider' abstraction for all
resource related operations. Every agent instance has a resource
provider. Only the checkpointed resources aren't abstracted away as this
is specific to agents. This abstraction can be re-used for local
resource providers and external resource providers.


Diffs (updated)
-

  src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
  src/master/http.cpp eb80830fa003ad8f58243d3dc4cec9e03285e96f 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
  src/master/quota_handler.cpp 7fe55808d4561ae5a8850ad904d09a7c65e014ce 
  src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 59081: Updated master to use resource provider IDs in allocator calls.

[Jan Schlicht]

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

(Updated May 31, 2017, 11:56 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Fixed `addSlave` call.


Repository: mesos


Description
---

The allocator interface expects resource provider IDs instead of slave
IDs. The internal slave instance in the master will still use their
slave IDs and create a resource provider ID from them to provide
backwards compatibility.
A later patch will change the callbacks of the allocator, until then
the resource provider ID is copied from the slave ID of the callback.


Diffs (updated)
-

  src/master/http.cpp eb80830fa003ad8f58243d3dc4cec9e03285e96f 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
  src/master/quota_handler.cpp 7fe55808d4561ae5a8850ad904d09a7c65e014ce 
  src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Anindya Sinha]

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

(Updated May 31, 2017, 8 a.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests to verify offers are suppressed based on registration.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 


Diff: https://reviews.apache.org/r/57818/diff/10/

Changes: https://reviews.apache.org/r/57818/diff/9-10/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

> On May 23, 2017, 9:01 p.m., Vinod Kone wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Line 309 (original), 309 (patched)
> > 
> >
> > I don't quite follow why you need to have this variable here? Looks 
> > like the sorter can be the source of truth of whether a role is activated 
> > or not?
> 
> Anindya Sinha wrote:
> `FrameworkInfo::roles` indicates all the roles the framework registers 
> as, and `Call::Subscribe::suppressed_roles` is a subset of roles for which 
> the master would not offer resources. The effective `suppressed_roles` can 
> change (after the framework is (re)registered) with `SUPPRESS` and `REVIVE` 
> calls, which means the roles for which offers are sent to frameworks can 
> change based on roles contained in these messages.
> 
> Now, the existing `activateFramework()` (and `deactivateFramework()`) 
> activates (and deactivates) all roles of the framework. I think that should 
> not be the case any longer. We should activate (and deactivate) roles of the 
> framework which are not contained in `suppressed_roles` at any given point of 
> time. To achieve this, we keep track of 
> `HierarchicalAllocatorProcess::Framework::Framework::suppressed_roles` to 
> ensure we keep track of `suppressed_roles` based on `SUPPRESS` and `REVIVE` 
> calls during the life cycle of the framework. This would help us determine if 
> we activate (or deactivate) for a specific role based on whether the role is 
> a `suppressed_roles`.
> 
> Vinod Kone wrote:
> I see. Makes sense. Can you add a comment to 
> activateFramework/deactivateFramework about the new semantics for posterity?

Added a comment in `activateFramework()` and `deactivateFramework()`.


> On May 23, 2017, 9:01 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 2804-2808 (patched)
> > 
> >
> > why do you need this check? is the below one not sufficient?
> 
> Anindya Sinha wrote:
> Just an optimization to not loop through the roles in `suppressedRoles` 
> incase the sizes of `frameworkRoles` and `suppressedRoles` differ. But I 
> think it should be fine to loop through since we do not anticipate the number 
> of roles to be too many.
> 
> Vinod Kone wrote:
> looks like you fixed it, not sure why you marked it as "dropped".

Sorry, my mistake. Updated it.


- Anindya


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


On May 23, 2017, 10:19 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
> 
> (Updated May 23, 2017, 10:19 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
> 
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 
>   src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
>   src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
>   src/master/allocator/mesos/allocator.hpp 
> 119b461136123229f85ed3c3cfd41137974a6b9b 
>   src/master/allocator/mesos/hierarchical.hpp 
> 123f97cf495bff0f822838e09df0d88818f04da6 
>   src/master/allocator/mesos/hierarchical.cpp 
> b75ed9a20a9a42f958cebbacd91e5e15b0d21394 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp 1c89a1159d8ac01a40f567beb14ed424ac613912 
>   src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 
>   src/tests/hierarchical_allocator_tests.cpp 
> 6dee2296d5a14185dbf7eee17968b20148839bfd 
>   src/tests/master_allocator_tests.cpp 
> d05ee441a5120144aff42d78c095e1ce5051a6ac 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 8c54372b7f6d94f0335561086f2a8cb90373e285 
>   src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad 
>   src/tests/resource_offers_tests.cpp 
> f0bca1d9e03013ce35215b0ffa6b50b38972dc0c 
>   src/tests/slave_recovery_tests.cpp 52e78b6b6280a159233b402ce2849448204d4f11 
> 
> 
> Diff: https://reviews.apache.org/r/57817/diff/9/
> 
> 
> 

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

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

(Updated May 31, 2017, 8 a.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
framework (re)registration, the allocator does not offer resources for
those roles to such frameworks. Note that this functionality is added
for `v1::SUBSCRIBE` only (and not for scheduler driver API).

In addition, the master validates the suppressed roles to be a subset
of all the roles being (re)registered by the framework.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 
  src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
  src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
  src/master/allocator/mesos/allocator.hpp 
119b461136123229f85ed3c3cfd41137974a6b9b 
  src/master/allocator/mesos/hierarchical.hpp 
82fea40ad09c95f0096934c4210a39a6f0638ed9 
  src/master/allocator/mesos/hierarchical.cpp 
ca368bb5ef0dcfe83672004d58a4de6f85d6b4bc 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp 14007e08f509446005423e223d5dd76a70744e27 
  src/tests/allocator.hpp 4ea722491f63f5ceda5a47228aafddc020f643b0 
  src/tests/hierarchical_allocator_tests.cpp 
eb2b647d3247a85e8b9b82e5589232c74ad8570f 
  src/tests/master_allocator_tests.cpp b9e04226a1688499847bc25c7c220c0b82ba8db7 
  src/tests/persistent_volume_endpoints_tests.cpp 
21136b29d724959527b889f52611baee0668 
  src/tests/reservation_tests.cpp 47ccf7f6f6ee48236f6794ce3084ce4f425c93c5 
  src/tests/resource_offers_tests.cpp f0bca1d9e03013ce35215b0ffa6b50b38972dc0c 
  src/tests/slave_recovery_tests.cpp df0c5c88786190be06df7ef3602834aa8985cefe 


Diff: https://reviews.apache.org/r/57817/diff/10/

Changes: https://reviews.apache.org/r/57817/diff/9-10/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 59634: Updated 'config.py' in the new Mesos CLI to take settings as parameter.

[Armand Grillet]

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


Ship it!




The ReviewBot is acting strange, I have tested the patch and the CLI seems to 
work perfectly.

- Armand Grillet


On May 30, 2017, 9:13 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59634/
> ---
> 
> (Updated May 30, 2017, 9:13 p.m.)
> 
> 
> Review request for mesos and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we were importing settings directly from the `bin`
> directory, which is obviously not portable. Instead, we need to be
> passing the settings into the constructor for the config so that it
> can access it from there.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/main.py d0ec2bdb04631fbaa62efb6ae67e138140a1296a 
>   src/cli_new/lib/cli/config.py 353785b8bdc13b745d2dc9469201c16a8816ebc6 
> 
> 
> Diff: https://reviews.apache.org/r/59634/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually, PEP8 and Pylint used to make sure that the code style is 
> correct.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 59673: Combined an 'undef' workaround for Windows.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59651, 59659, 59660, 59661, 59662, 59663, 59664, 59665, 
59666, 59667, 59668, 59669, 59670, 59671, 59672, 59673]

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 May 30, 2017, 11:51 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59673/
> ---
> 
> (Updated May 30, 2017, 11:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Combined an 'undef' workaround for Windows.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 77fb93abc701cd34b69c75b6219c219fdb784a67 
> 
> 
> Diff: https://reviews.apache.org/r/59673/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>