Re: Review Request 50271: Created an isolator for Linux capabilities.

[Benjamin Bannier]

> On Sept. 6, 2016, 9 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/capabilities.hpp, line 41
> > 
> >
> > are we consistent on 'override' keyword? I'd suggest we be consistent 
> > with other isolators for now.

We consistently do not use `override`. I changed this code to explicitly be 
`virtual` as documentation.


> On Sept. 6, 2016, 9 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/capabilities.cpp, lines 45-56
> > 
> >
> > This looks unreadable to me. Can you use if/else instead?

Done.


> On Sept. 6, 2016, 9 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/capabilities.cpp, line 46
> > 
> >
> > I think we should use containerConfig.container_info now as we plan to 
> > supported nested container.

Done.


> On Sept. 6, 2016, 9 p.m., Jie Yu wrote:
> > include/mesos/slave/containerizer.proto, line 144
> > 
> >
> > the two sentenses here look a little duplicate?

I meant them to be orthogonal. Could you suggest a better alternative?


- Benjamin


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


On Sept. 7, 2016, 6:46 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Sept. 7, 2016, 6:46 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This isolator evaluates agent allowed capabilities and passes net
> capabilities on to `mesos-containerizer` which enforces the
> capabilities.
> 
> Capability information is passed via a new field in
> `ContainerLaunchInfo`.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 16dd3a19145b9764273cdb9a8899e353c98730e5 
>   src/CMakeLists.txt 01ef494f7120156de3b826d7def76fb30bcc61b5 
>   src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
>   src/tests/containerizer/isolator_tests.cpp 
> f8056ca08029feed5f164d4f94e24d521183bdfc 
> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50270: Introduced linux capabilities support for mesos containerizer.

[Benjamin Bannier]

> On Sept. 8, 2016, 5:01 a.m., Jie Yu wrote:
> > Any comment on unresolved issues?

Sorry, published comments now.


- Benjamin


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


On Sept. 7, 2016, 6:43 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50270/
> ---
> 
> (Updated Sept. 7, 2016, 6:43 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5303
> https://issues.apache.org/jira/browse/MESOS-5303
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces linux capability based security for unified
> containerizer. A new agent flag \`allowed_capabilities\` has been
> introduced to override the default capabilities of the user or the
> capabilities requested by the user.
> 
> This feature is only available on linux.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
>   src/slave/containerizer/mesos/launch.cpp 
> 13b65d82e029650e150eb2bc3647d95af167bd72 
>   src/slave/flags.hpp 1a006663e7cc58ee548b3dda686cfbac0c240baa 
>   src/slave/flags.cpp 0f2be1700f41b74da4ea1ce699a81ec33cf92a9a 
> 
> Diff: https://reviews.apache.org/r/50270/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50270: Introduced linux capabilities support for mesos containerizer.

[Benjamin Bannier]

> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 378
> > 
> >
> > I am wondering if we should rename this to:
> > ```
> > capabilities->setKeepCaps()
> > capabilities->clearKeepCaps()
> > ```

Done, also submitted https://reviews.apache.org/r/51697/. Could you commit that 
as part of this chain? I did not add a `Capabilities::clearKeepCaps` as we 
currently have no use for it. I am also not sure how useful clearing keep caps 
again might be midterm as we'd likely never execute `setKeepCaps` in anything 
not already executing in a separate process we could just let die (like we do 
here).


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, lines 382-383
> > 
> >
> > `uid` might be None, right?

I reworded the log output slightly to not require some `uid` anymore.


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 417
> > 
> >
> > Why this is not guarded by ifdef linux?

I was trying to minimize code behind `ifdef` guards. While it should not be 
possible to set the capabilities flag on non-Linux systems the `Flag` member is 
defined for all platforms as it has no dependencies on Linux-specific features.


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/flags.cpp, line 466
> > 
> >
> > What's this? I thought we use PR_SET_KEEPCAPS and agent is running 
> > under root.

I think there's actually no need to make a hard requirement on the agent to be 
run as root (at least to make use of capabilities) -- we do only require 
`SETPCAP` and that the capabilities requested for a container are contained in 
the agent's capabilities. I believe it makes sense to anticipate that users of 
this capabilities feature are aware of fine-grained permissions.

I added an additional note to the help text to make clear that `SETPCAP` is 
required for the agent.


> On Sept. 6, 2016, 8:35 nachm., Jie Yu wrote:
> > src/slave/flags.cpp, line 94
> > 
> >
> > This list is not complete. I'd suggest we don't make this change in 
> > this patch. It should point to  a document online with a complete list. 
> > Let's create a ticket to track.

I filed https://issues.apache.org/jira/browse/MESOS-6133.


- Benjamin


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


On Sept. 7, 2016, 6:43 nachm., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50270/
> ---
> 
> (Updated Sept. 7, 2016, 6:43 nachm.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5303
> https://issues.apache.org/jira/browse/MESOS-5303
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces linux capability based security for unified
> containerizer. A new agent flag \`allowed_capabilities\` has been
> introduced to override the default capabilities of the user or the
> capabilities requested by the user.
> 
> This feature is only available on linux.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
>   src/slave/containerizer/mesos/launch.cpp 
> 13b65d82e029650e150eb2bc3647d95af167bd72 
>   src/slave/flags.hpp 1a006663e7cc58ee548b3dda686cfbac0c240baa 
>   src/slave/flags.cpp 0f2be1700f41b74da4ea1ce699a81ec33cf92a9a 
> 
> Diff: https://reviews.apache.org/r/50270/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51480: Added tests for launching task groups on the agent.

[Anand Mazumdar]

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

(Updated Sept. 8, 2016, 5:52 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Trying to make Reviewbot happy again.


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


Repository: mesos


Description (updated)
---

This change adds tests for testing the `runTaskGroup()` handler
on the agent.


Diffs (updated)
-

  src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 

Diff: https://reviews.apache.org/r/51480/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 51710: Minor fix to avoid creating an unnecessary temporary when looping.

[Anand Mazumdar]

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

(Updated Sept. 8, 2016, 5:45 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased


Repository: mesos


Description
---

Minor fix to avoid creating an unnecessary temporary when looping.


Diffs (updated)
-

  src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 

Diff: https://reviews.apache.org/r/51710/diff/


Testing
---


Thanks,

Anand Mazumdar

Re: Review Request 51476: Made the `TaskInfo` argument in `launchExecutor()` optional.

[Anand Mazumdar]

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

(Updated Sept. 8, 2016, 5:28 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased


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


Repository: mesos


Description (updated)
---

Launching a task group requires just the `ExecutorInfo`. We had
already refactored the containerizer `launch()` interface to make
`TaskInfo` as an optional. This change funnels it through.


Diffs (updated)
-

  src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
  src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 

Diff: https://reviews.apache.org/r/51476/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 51709: Removed a no longer needed comment after the `runTasks()` refactoring.

[Anand Mazumdar]

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

(Updated Sept. 8, 2016, 5:24 a.m.)


Review request for mesos, Guangya Liu and Vinod Kone.


Changes
---

Rebased.


Repository: mesos


Description
---

Removed a no longer needed comment after the `runTasks()` refactoring.


Diffs (updated)
-

  src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 

Diff: https://reviews.apache.org/r/51709/diff/


Testing
---


Thanks,

Anand Mazumdar

Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Anand Mazumdar]

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

(Updated Sept. 8, 2016, 5:19 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased due to `b2101157fd61bbe42c9536935ee9fda44a929ee9`


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


Repository: mesos


Description
---

This changes implements the `runTaskGroup()` handler on the
agent ensuring that task group is sent atomically to the executor
via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()`
to go through a common handler function. Also, it ensures that all
tasks in `framework->pending`/`queuedTasks` that are killed before
running the task group result in all the tasks being killed.

Review: https://reviews.apache.org/r/51477/


Diffs (updated)
-

  src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
  src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 

Diff: https://reviews.apache.org/r/51477/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 3, 2016, 9:21 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 484-485
> > 
> >
> > I did some test with benchmark test of 
> > `SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0`
> >  and it failed as following:
> > 
> > ```
> > ../../src/tests/hierarchical_allocator_tests.cpp:3459: Failure
> > Value of: offerCallbacks.load()
> >   Actual: 5
> > Expected: slaveCount
> > Which is: 1000
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1) (497 ms)
> > [--] 1 test from 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (498 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (512 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] 
> > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0,
> >  where GetParam() = (1000, 1)
> > 
> >  1 FAILED TEST
> > ```
> > 
> > The reason is that we cannot make sure all of the `_allocate()` 
> > finished after `addSlave` finished.
> > 
> > Shall we do a `while` loop in the benchmark to wait till all 
> > allocations are got?
> 
> Guangya Liu wrote:
> Perhaps we can simply remove the `ASSERT_EQ` for offer and agent count in 
> `HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave` as this patch will 
> not trigger `allocate()` synchronously, so we cannot make sure the agent 
> count is always same as offer count.

Good catch, I'll run the benchmarks and update 
https://reviews.apache.org/r/51028/


- Jacob


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


On Sept. 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Sept. 3, 2016, 6:38 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1226-1229
> > 
> >
> > Just nit, I think that we should separate the `_allocate()` and 
> > `_deallocate()` to use different time elapse. But as the original logic is 
> > also putting the `_deallocate()` logic in `_allocate()`, so I think that it 
> > is OK to keep such logic but may need an update if we want to make the time 
> > more accurate for diffrent actions: `_allocate()` and `_deallocate()`.
> > 
> > ```
> > stopwatch.start();
> > 
> > metrics.allocation_run.start();
> > 
> > _allocate();
> > 
> > metrics.allocation_run.stop();
> > 
> > stopwatch.stop();
> > 
> > VLOG(1) << "Performed allocation for " << allocationCandidates.size()
> > << " agents in " << stopwatch.elapsed();
> > 
> > stopwatch.start();
> > 
> > _deallocate();
> > 
> > stopwatch.stop();
> > 
> > VLOG(1) << "Performed deallocation for " << allocationCandidates.size()
> > << " agents in " << stopwatch.elapsed();
> > ```

Yea, I think preserving the original behavior was the idea here so the metrics 
reflect that.


- Jacob


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


On Sept. 3, 2016, 6:05 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 3, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> - Batched allocations are handled synchronously.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> 
> With new benchmark https://reviews.apache.org/r/49617: 
> Sample output without 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 57251us
> Added 1 agents in 3.2134535333mins
> allocator settled after  1.6123603833mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (290578 ms)
> 
> Sample output with 51027:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 39817us
> Added 1 agents in 3.2286054167mins
> allocator settled after  25.525654secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22
>  (220137 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 45966: Offer shared resources to frameworks only if opted in.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45966]

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 Sept. 7, 2016, 11:55 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45966/
> ---
> 
> (Updated Sept. 7, 2016, 11:55 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new capability SHARED_RESOURCES that frameworks need to opt
> in if they are interested in receiving shared resources in their
> offers.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
>   include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1f5f3801eff6b15914ad4a740479a008530befc5 
>   src/master/allocator/mesos/hierarchical.cpp 
> 992031a6fbc89727725071841bd3ab827737d8bd 
>   src/tests/hierarchical_allocator_tests.cpp 
> b2179215f3a4c2f4018670e8e54f02e06c39c3b1 
> 
> Diff: https://reviews.apache.org/r/45966/diff/
> 
> 
> Testing
> ---
> 
> Tests updated with new capability.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 50270: Introduced linux capabilities support for mesos containerizer.

[Jie Yu]

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



Any comment on unresolved issues?

- Jie Yu


On Sept. 7, 2016, 4:43 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50270/
> ---
> 
> (Updated Sept. 7, 2016, 4:43 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5303
> https://issues.apache.org/jira/browse/MESOS-5303
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces linux capability based security for unified
> containerizer. A new agent flag \`allowed_capabilities\` has been
> introduced to override the default capabilities of the user or the
> capabilities requested by the user.
> 
> This feature is only available on linux.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
>   src/slave/containerizer/mesos/launch.cpp 
> 13b65d82e029650e150eb2bc3647d95af167bd72 
>   src/slave/flags.hpp 1a006663e7cc58ee548b3dda686cfbac0c240baa 
>   src/slave/flags.cpp 0f2be1700f41b74da4ea1ce699a81ec33cf92a9a 
> 
> Diff: https://reviews.apache.org/r/50270/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Guangya Liu]

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




src/slave/slave.cpp (lines 1839 - 1840)


I think that we always prefer adding the blank space at the beginning as 
#1787, you can fix this in other patches.

```
"The checkpointed resources being used by the task or task group are"
" unknown to the agent",
```



src/slave/slave.cpp (lines 2079 - 2080)


new line here



src/slave/slave.cpp (line 2085)


How about also adding `[]` here?

```
out << "tasks [" << stringify(taskIds) << "]";
```


- Guangya Liu


On 九月 7, 2016, 10:13 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51477/
> ---
> 
> (Updated 九月 7, 2016, 10:13 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes implements the `runTaskGroup()` handler on the
> agent ensuring that task group is sent atomically to the executor
> via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()`
> to go through a common handler function. Also, it ensures that all
> tasks in `framework->pending`/`queuedTasks` that are killed before
> running the task group result in all the tasks being killed.
> 
> Review: https://reviews.apache.org/r/51477/
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
>   src/slave/slave.cpp 11664779ed78c0a5913598bb7dd1bb0e793d6b93 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Guangya Liu]

> On 九月 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.hpp, line 1027
> > 
> >
> > How about const?
> 
> Anand Mazumdar wrote:
> Can you elaborate? Non member functions can't be marked `const`. Also, 
> did you mean `const` on the return type?

Sorry for confuse, yes, what about the return type as const?


> On 九月 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, line 1711
> > 
> >
> > s/for/to
> 
> Anand Mazumdar wrote:
> Why? This looks fine to me.

I do not have strong intention for this, just depend on you, but seems we use 
`send  to ...` more frequently ;-)


> On 九月 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, line 1748
> > 
> >
> > What about enhance the reason as `Task killed before it was launched 
> > due to one task killed in the task group`?
> 
> Anand Mazumdar wrote:
> We haven't been doing such fine grained error messaging on the Master 
> too. We might consider doing it in the future. I would file a JIRA for 
> posterity.

Sounds good, thanks Anand.


> On 九月 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, lines 1988-1989
> > 
> >
> > What about adding `executor state` to the log message?
> 
> Anand Mazumdar wrote:
> Why? Also, we weren't logging the state before either.

We actually have the executor state in log message but not here, please refer 
to https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1824

Another point is that this is a new message that you introduced, and the log 
message for `Executor::REGISTERING` and `Executor::RUNNING` are same, it is 
better distinguish those log messages by adding the executor state here for 
debugability.


- Guangya


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


On 九月 7, 2016, 10:13 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51477/
> ---
> 
> (Updated 九月 7, 2016, 10:13 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes implements the `runTaskGroup()` handler on the
> agent ensuring that task group is sent atomically to the executor
> via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()`
> to go through a common handler function. Also, it ensures that all
> tasks in `framework->pending`/`queuedTasks` that are killed before
> running the task group result in all the tasks being killed.
> 
> Review: https://reviews.apache.org/r/51477/
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
>   src/slave/slave.cpp 11664779ed78c0a5913598bb7dd1bb0e793d6b93 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 50695: Renamed agent used resources to `allocation` for benchmark test.

[Guangya Liu]

> On 九月 7, 2016, 3:32 p.m., Jiang Yan Xu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 3427-3428
> > 
> >
> > This looks alright. Why is it removed?

I added this back, thanks Jiang Yan.


- Guangya


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


On 九月 8, 2016, 2:04 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50695/
> ---
> 
> (Updated 九月 8, 2016, 2:04 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed agent used resources to `allocation` for benchmark test.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> b2179215f3a4c2f4018670e8e54f02e06c39c3b1 
> 
> Diff: https://reviews.apache.org/r/50695/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0
> Using 1000 agents and 1 frameworks
> Added 1 frameworks in 911us
> Added 1000 agents in 1.014938secs
> Updated 1000 agents in 603347us
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0
>  (1683 ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (1683 ms total)
> ```
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/0
> Using 1000 agents and 1 frameworks
> Added 1 frameworks in 861us
> Added 1000 agents in 1.487206secs
> round 0 allocate() took 808744us to make 0 offers after filtering  1000 offers
> round 1 allocate() took 825291us to make 0 offers after filtering  1000 offers
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/0 
> (6126 ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (6126 ms total)
> ```
> 
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/0
> Using 1000 agents and 1 frameworks
> Added 1 frameworks in 998us
> Added 1000 agents in 1.934967secs
> round 0 allocate() took 810760us to make 0 offers after filtering 1000 offers
> round 1 allocate() took 814886us to make 0 offers after filtering 1000 offers
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/0 
> (6747 ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (6747 ms total)
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50695: Renamed agent used resources to `allocation` for benchmark test.

[Guangya Liu]

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

(Updated 九月 8, 2016, 2:04 a.m.)


Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.


Repository: mesos


Description
---

Renamed agent used resources to `allocation` for benchmark test.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
b2179215f3a4c2f4018670e8e54f02e06c39c3b1 

Diff: https://reviews.apache.org/r/50695/diff/


Testing
---

make
make check

```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0
Using 1000 agents and 1 frameworks
Added 1 frameworks in 911us
Added 1000 agents in 1.014938secs
Updated 1000 agents in 603347us
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0 
(1683 ms)
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (1683 ms total)
```

```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/0
Using 1000 agents and 1 frameworks
Added 1 frameworks in 861us
Added 1000 agents in 1.487206secs
round 0 allocate() took 808744us to make 0 offers after filtering  1000 offers
round 1 allocate() took 825291us to make 0 offers after filtering  1000 offers
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/0 
(6126 ms)
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (6126 ms total)
```

```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/0
Using 1000 agents and 1 frameworks
Added 1 frameworks in 998us
Added 1000 agents in 1.934967secs
round 0 allocate() took 810760us to make 0 offers after filtering 1000 offers
round 1 allocate() took 814886us to make 0 offers after filtering 1000 offers
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/0 
(6747 ms)
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (6747 ms total)
```


Thanks,

Guangya Liu

Re: Review Request 51480: Added tests for launching task groups on the agent.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [51480, 51479, 51478, 51710, 51709, 51477, 51476, 51475]

Failed command: ./support/apply-review.sh -n -r 51477

Error:
2016-09-08 01:33:57 URL:https://reviews.apache.org/r/51477/diff/raw/ 
[49111/49111] -> "51477.patch" [1]
error: patch failed: src/slave/slave.cpp:2132
error: src/slave/slave.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/15153/console

- Mesos ReviewBot


On Sept. 7, 2016, 10:12 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51480/
> ---
> 
> (Updated Sept. 7, 2016, 10:12 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds tests for testing the `runTaskGroup()` handler
> on the agent.
> 
> Review: https://reviews.apache.org/r/51480/
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 
> 
> Diff: https://reviews.apache.org/r/51480/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51707: Fixed races between "unreachable" and "unregister" slave transitions.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [51707, 51653, 51706, 51021, 51377, 51376, 51375, 51374, 
51371, 51020, 50846, 50845, 50844, 50707, 50706, 50705, 50704, 50703, 50702, 
50701, 50700, 50699, 50422, 50418, 50417, 50416, 50235]

Failed command: ./support/apply-review.sh -n -r 51374

Error:
2016-09-08 00:37:00 URL:https://reviews.apache.org/r/51374/diff/raw/ 
[21014/21014] -> "51374.patch" [1]
error: patch failed: src/master/master.cpp:1752
error: src/master/master.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/15152/console

- Mesos ReviewBot


On Sept. 7, 2016, 9:36 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51707/
> ---
> 
> (Updated Sept. 7, 2016, 9:36 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5965
> https://issues.apache.org/jira/browse/MESOS-5965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that we update the registry before updating the master's state when
> performing these transitions, it is possible for the master to already
> be marking a slave unreachable when an `UnregisterSlaveMessage` is
> received, or vice versa. Detect these situations and ignore whichever
> transition is triggered second.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
>   src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 
> 
> Diff: https://reviews.apache.org/r/51707/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51683: Avoided resource validation when flatten resources.

[Jiang Yan Xu]

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



Summarize the behavioral change in the summary? i.e., previously if role or 
reservation info are invalid, the result is an empty `Resources` object, now it 
returns a error.


include/mesos/resources.hpp (line 351)


Comment on when the result is an Error?



include/mesos/resources.hpp (lines 351 - 353)


To make it less cumbersome to use for a pretty common case, can we use an 
overload?

```
Resources flatten() const
```

As a result we don't need the default value for the argument `role` here.



src/common/resources.cpp (line 1088)


What if `role == "*"` but `reservation.isSome()`?


- Jiang Yan Xu


On Sept. 6, 2016, 6:37 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51683/
> ---
> 
> (Updated Sept. 6, 2016, 6:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6131
> https://issues.apache.org/jira/browse/MESOS-6131
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided resource validation when flatten resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 87828aa5e4818efd40dd72dc199669281b8a15ae 
>   include/mesos/v1/resources.hpp 27bdd275277edc60d97b720e596f21e8e8f8dc27 
>   src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
>   src/common/resources.cpp 77ee92193de1f34131f7a86b7d16731c9c3e17e2 
>   src/examples/balloon_framework.cpp 5613ff0c61e2d2f84684a206debc97dcb8b2c0d3 
>   src/examples/disk_full_framework.cpp 
> 1221f83d495f7c1ee1bcbcfe067e303db0a921eb 
>   src/examples/dynamic_reservation_framework.cpp 
> 850bb2a5b243dd5d5a8b6476570b4f943fde6185 
>   src/examples/long_lived_framework.cpp 
> 7c57eb5e4a34208504475013690ae8e3cae74155 
>   src/examples/test_framework.cpp a83766a92617d50eadd92ec55113e049a7290d03 
>   src/examples/test_http_framework.cpp 
> 441e86c88b035d9a268b8b51b95da3e1eb842a62 
>   src/master/http.cpp 525ef6cd7545d25f3ac89a6325ace6e6d768262a 
>   src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 
>   src/tests/hierarchical_allocator_tests.cpp 
> 24698ffc9f72bc1e144e5107412f5e7d9c4f7105 
>   src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 266c2a0ff4a99baa96a7c4980f076755603256a9 
>   src/tests/reservation_endpoints_tests.cpp 
> bee5ea6b3a3ee7fafb7312a6d89de79d62c57ec1 
>   src/tests/reservation_tests.cpp 000957826011bf28f7550a83db3e60a796162fb3 
>   src/tests/resources_tests.cpp f627862870279e5a778aa83e1ddcaa88c27ba1cb 
>   src/tests/role_tests.cpp 162c9414a70723a212cfd39ac5c0b8325c3f3b5d 
>   src/v1/resources.cpp 4ba5bf87c9b9fef67eeb447a3f3b520f1e81ad77 
> 
> Diff: https://reviews.apache.org/r/51683/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 11:55 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs
-

  src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
  src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/45962/diff/


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 11:52 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Added capability for the example framework which was earlier in 
https://reviews.apache.org/r/45966.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs (updated)
-

  src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
  src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/45962/diff/


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45966: Offer shared resources to frameworks only if opted in.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 11:52 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Moved the changes in example framework to https://reviews.apache.org/r/45962.


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


Repository: mesos


Description
---

Added a new capability SHARED_RESOURCES that frameworks need to opt
in if they are interested in receiving shared resources in their
offers.


Diffs (updated)
-

  include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
  include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
  src/master/allocator/mesos/hierarchical.hpp 
1f5f3801eff6b15914ad4a740479a008530befc5 
  src/master/allocator/mesos/hierarchical.cpp 
992031a6fbc89727725071841bd3ab827737d8bd 
  src/tests/hierarchical_allocator_tests.cpp 
b2179215f3a4c2f4018670e8e54f02e06c39c3b1 

Diff: https://reviews.apache.org/r/45966/diff/


Testing
---

Tests updated with new capability.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45966: Offer shared resources to frameworks only if opted in.

[Anindya Sinha]

> On Sept. 7, 2016, 10:09 p.m., Jiang Yan Xu wrote:
> > Can we have a simple allocator test with two frameworks, one framework 
> > without the capability doesn't get allocated the shared resource and the 
> > other with the capability gets it?
> > 
> > Could be in a follow up review.

Will add this test in a separate review.


> On Sept. 7, 2016, 10:09 p.m., Jiang Yan Xu wrote:
> > src/examples/persistent_shared_volume_framework.cpp, lines 579-580
> > 
> >
> > Take this out for now and add it later together?

Took it out from here, and moved it to https://reviews.apache.org/r/45962/.


- Anindya


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


On Sept. 7, 2016, 5:57 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45966/
> ---
> 
> (Updated Sept. 7, 2016, 5:57 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new capability SHARED_RESOURCES that frameworks need to opt
> in if they are interested in receiving shared resources in their
> offers.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
>   include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
>   src/tests/hierarchical_allocator_tests.cpp 
> 24698ffc9f72bc1e144e5107412f5e7d9c4f7105 
> 
> Diff: https://reviews.apache.org/r/45966/diff/
> 
> 
> Testing
> ---
> 
> Tests updated with new capability.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51700: Add `FrameworkAdded` event to master event stream.

[Zhitao Li]

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

(Updated Sept. 7, 2016, 11:03 p.m.)


Review request for mesos, Xiaojian Huang, haosdent huang, and Vinod Kone.


Changes
---

Fix test on Linux (g++)


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


Repository: mesos


Description
---

Changes included:
- Moving `model()` helper function to common/protobuf_utils;
- Definition of event and response;
- Sending event from master;
- new test.


Diffs (updated)
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
  src/common/protobuf_utils.cpp 8c4a7264080385789157703b05b42716fb0563b3 
  src/master/http.cpp 52dd80b856cf2317c0b73ba54bf501696786088d 
  src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 

Diff: https://reviews.apache.org/r/51700/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Anand Mazumdar]

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



- Anand Mazumdar


On Sept. 6, 2016, 9:25 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51477/
> ---
> 
> (Updated Sept. 6, 2016, 9:25 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes implements the `runTaskGroup()` handler on the
> agent ensuring that task group is sent atomically to the executor
> via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()`
> to go through a common handler function. Also, it ensures that all
> tasks in `framework->pending`/`queuedTasks` that are killed before
> running the task group result in all the tasks being killed.
> 
> Review: https://reviews.apache.org/r/51477/
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
>   src/slave/slave.cpp 11664779ed78c0a5913598bb7dd1bb0e793d6b93 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Anand Mazumdar]

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

(Updated Sept. 7, 2016, 10:13 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments from Vinod and Guangya.


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


Repository: mesos


Description
---

This changes implements the `runTaskGroup()` handler on the
agent ensuring that task group is sent atomically to the executor
via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()`
to go through a common handler function. Also, it ensures that all
tasks in `framework->pending`/`queuedTasks` that are killed before
running the task group result in all the tasks being killed.

Review: https://reviews.apache.org/r/51477/


Diffs (updated)
-

  src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
  src/slave/slave.cpp 11664779ed78c0a5913598bb7dd1bb0e793d6b93 

Diff: https://reviews.apache.org/r/51477/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 49924: Added libprocess as a shared library.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49688, 49862, 49863, 49870, 49874, 49921, 49924]

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 Sept. 7, 2016, 6:33 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49924/
> ---
> 
> (Updated Sept. 7, 2016, 6:33 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch allow to build libprocess as shared library on OSX and Linux
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> d1547ef6a8762385f653d3824307727e4d0a7e71 
> 
> Diff: https://reviews.apache.org/r/49924/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make
> 
> With this patch and https://reviews.apache.org/r/49862,  Converted libmesos, 
> http_parser and libprocess to shared libraries and we are using libevent 
> shared library, zookeeper does not have a shared library in the 3rdparty (I 
> guess the code is compiled as relocatable) and did not have issues linking.
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 51478: Modified tests to account for `runTask()`/`_runTask()` refactoring.

[Anand Mazumdar]

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

(Updated Sept. 7, 2016, 10:11 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Change deps


Summary (updated)
-

Modified tests to account for `runTask()`/`_runTask()` refactoring.


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


Repository: mesos


Description (updated)
---

This change modified the agent mocks/tests to account for the
`runTask()`/`_runTask()` refactoring.

Review: https://reviews.apache.org/r/51478/


Diffs (updated)
-

  src/tests/master_slave_reconciliation_tests.cpp 
7dda6f67d9d02d14759563ec64269060dc7e643b 
  src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f 
  src/tests/mesos.cpp f5034f9f8de7040182e10f51be125a87b29fdd24 
  src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 

Diff: https://reviews.apache.org/r/51478/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 51480: Added tests for launching task groups on the agent.

[Anand Mazumdar]

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

(Updated Sept. 7, 2016, 10:12 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description
---

This change adds tests for testing the `runTaskGroup()` handler
on the agent.

Review: https://reviews.apache.org/r/51480/


Diffs (updated)
-

  src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 

Diff: https://reviews.apache.org/r/51480/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 51480: Added tests for launching task groups on the agent.

[Anand Mazumdar]

> On Sept. 7, 2016, 5:37 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, lines 4621-4622
> > 
> >
> > why did you need to explicitly do this here but not in the above test?

In the previous test we did not use the `MockSlave` directly but just used the 
`StartSlave()` helper. Hence, it was not needed.


- Anand


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


On Sept. 6, 2016, 9:36 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51480/
> ---
> 
> (Updated Sept. 6, 2016, 9:36 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds tests for testing the `runTaskGroup()` handler
> on the agent.
> 
> Review: https://reviews.apache.org/r/51480/
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 
> 
> Diff: https://reviews.apache.org/r/51480/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51478: Modified tests to account for `_runTask()` rename.

[Anand Mazumdar]

> On Sept. 7, 2016, 5:29 p.m., Vinod Kone wrote:
> >

I intentionally omitted putting the backticks since I was just renaming 
function calls/moving code around due to the refactoring. We might consider 
doing a sweep later. What do you think?


- Anand


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


On Sept. 6, 2016, 9:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51478/
> ---
> 
> (Updated Sept. 6, 2016, 9:27 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modified the agent mocks/tests to account for the
> `runTask()` rename.
> 
> Review: https://reviews.apache.org/r/51478/
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 7dda6f67d9d02d14759563ec64269060dc7e643b 
>   src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f 
>   src/tests/mesos.cpp f5034f9f8de7040182e10f51be125a87b29fdd24 
>   src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 
> 
> Diff: https://reviews.apache.org/r/51478/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 51710: Minor fix to avoid creating an unnecessary temporary when looping.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Minor fix to avoid creating an unnecessary temporary when looping.


Diffs
-

  src/slave/slave.cpp 11664779ed78c0a5913598bb7dd1bb0e793d6b93 

Diff: https://reviews.apache.org/r/51710/diff/


Testing
---


Thanks,

Anand Mazumdar

Review Request 51709: Removed a no longer needed comment after the `runTasks()` refactoring.

[Anand Mazumdar]

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

Review request for mesos, Guangya Liu and Vinod Kone.


Repository: mesos


Description
---

Removed a no longer needed comment after the `runTasks()` refactoring.


Diffs
-

  src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 

Diff: https://reviews.apache.org/r/51709/diff/


Testing
---


Thanks,

Anand Mazumdar

Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Anand Mazumdar]

> On Sept. 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, lines 3120-3135
> > 
> >
> > Can you please show more detail and update the comments here for which 
> > case will cause the `executor->queuedTasks` and 
> > `executor->queuedTaskGroups` have same taskId?

We currently store the tasks in queued task groups also in queued tasks.


> On Sept. 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, line 2391
> > 
> >
> > Just a question here, we are killing task in task group, and we can 
> > even say here is killing a task group, but here the status is still 
> > `TASK_KILLED`, do we need to introduce a new `TASKGROUP_KILLED` for this?
> > 
> > Ditto for other places.

We might consider doing it later but for now this behavior is consistent with 
the behavior on the Master.


> On Sept. 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, lines 1988-1989
> > 
> >
> > What about adding `executor state` to the log message?

Why? Also, we weren't logging the state before either.


> On Sept. 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, line 1711
> > 
> >
> > s/for/to

Why? This looks fine to me.


> On Sept. 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.hpp, line 1027
> > 
> >
> > How about const?

Can you elaborate? Non member functions can't be marked `const`. Also, did you 
mean `const` on the return type?


> On Sept. 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, line 1748
> > 
> >
> > What about enhance the reason as `Task killed before it was launched 
> > due to one task killed in the task group`?

We haven't been doing such fine grained error messaging on the Master too. We 
might consider doing it in the future. I would file a JIRA for posterity.


> On Sept. 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, lines 1825-1847
> > 
> >
> > I think the reason is not correct for other tasks in the task group.
> > 
> > What about sending out TASK_LOST reason separately as following logic:
> > 
> > if checkpoint failure:
> >   task lost with reason as checkpoint failure
> >   
> > if kill:
> >   foreach task but not the checkpoint failure task:
> >  task lost with reason as one task fail cause whole task groupp fail

See above comment.


> On Sept. 7, 2016, 10:17 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, lines 1866-1887
> > 
> >
> > ditto as above for updating `reasons` separately

See above comment.


- Anand


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


On Sept. 6, 2016, 9:25 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51477/
> ---
> 
> (Updated Sept. 6, 2016, 9:25 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes implements the `runTaskGroup()` handler on the
> agent ensuring that task group is sent atomically to the executor
> via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()`
> to go through a common handler function. Also, it ensures that all
> tasks in `framework->pending`/`queuedTasks` that are killed before
> running the task group result in all the tasks being killed.
> 
> Review: https://reviews.apache.org/r/51477/
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
>   src/slave/slave.cpp 11664779ed78c0a5913598bb7dd1bb0e793d6b93 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Anand Mazumdar]

> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1616
> > 
> >
> > couldn't this have been?
> > 
> > ```
> > for (auto it = tasks.begin(); it != tasks.end(); ++it)
> > ```

Was trying to be consistent with the loop a couple of lines before. Would file 
another CR to clean it up too! 
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1562


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1711
> > 
> >
> > you say "other" tasks but you send TASK_KILLED for "all" tasks in the 
> > group. can you remind me why we can't send TASK_KILLED for the killed task 
> > in `killTask()` right away and send TASK_KILLED for the rest of the tasks 
> > in the group here, similar to what we do in `Master::_accept()`?

I don't know _yet_ the reasoning for us doing this in `Master::_accept()`. It 
is pretty non-intuitive and leads to two code paths where we send the status 
update rather then just doing them all in one place as the current code in this 
CR does. 

Also, there is a relevant `TODO` in the code that wants to do the opposite in 
the Master!

```// TODO(bmahler): Do this killing when processing
// the `Kill` call, rather than doing it here.
```

I don't think we even have enough information in `Master::killTask()` to do 
this unless we start storing task group information on the Master. I would 
change the `TODO` in the master to reflect this i.e. send all the status 
updates in `_accept()` similar to what we are doing on the agent.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1752
> > 
> >
> > back ticks instead of quotes.

Other places in this function use quotes for this comment. We might want to do 
a sweep later.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1903
> > 
> >
> > back ticks.

It seems weird that we have a quote and a back tick in the same comment! We 
might want to do a clean sweep later.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.hpp, lines 327-329
> > 
> >
> > kill this comment.

Would do in a separate CR.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1712
> > 
> >
> > if you follow `Master::_accept()` this should be a hashset of TaskIDs.

See earlier comment.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 2182
> > 
> >
> > but `killTask` is not sending a TASK_KILLED update anymore?

It does so still for queued tasks.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 2332-2334
> > 
> >
> > s/bigger//
> > 
> > can you remind me why we don't send TASK_KILLED here? AFAICT `_run()` 
> > has information about the task group based on the argument passed to it, 
> > even if `framework->pending` doesn't itself have that info. if there is no 
> > good reason, lets make this consistent with how we do it in the master.

See earlier comment.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 3137-3159
> > 
> >
> > it's unfortunate that you have to make 2 duplicate 
> > containerizer->update calls here. is there any reason why `__run` work with 
> > both tasks and taskGroups arguments set?

Good Suggestion. The only reason I made `__run()` to not work with both tasks 
and task groups was to maintain consistency with `run()/_run()`. Looks like not 
doing that would allow us to get rid of this redundant code.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 2115
> > 
> >
> > i think you need to also insert a space after each task group? it's 
> > probably best to store taskgroups as a vector of vectors and call stringify.

I modified it a bit to have a `,` after every task group.


> On Sept. 7, 2016, 10:26 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 3123-3126
> > 
> >
> > you couldn't use the default assignment operator for LinkedHashMap?

hmm, seems like I ran into some issues with using the default assignment 
operator. It seems that we store the 

Re: Review Request 45966: Offer shared resources to frameworks only if opted in.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Can we have a simple allocator test with two frameworks, one framework without 
the capability doesn't get allocated the shared resource and the other with the 
capability gets it?

Could be in a follow up review.


src/examples/persistent_shared_volume_framework.cpp (lines 579 - 580)


Take this out for now and add it later together?


- Jiang Yan Xu


On Sept. 7, 2016, 10:57 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45966/
> ---
> 
> (Updated Sept. 7, 2016, 10:57 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new capability SHARED_RESOURCES that frameworks need to opt
> in if they are interested in receiving shared resources in their
> offers.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
>   include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
>   src/tests/hierarchical_allocator_tests.cpp 
> 24698ffc9f72bc1e144e5107412f5e7d9c4f7105 
> 
> Diff: https://reviews.apache.org/r/45966/diff/
> 
> 
> Testing
> ---
> 
> Tests updated with new capability.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51475: Added an equality operator for `TaskGroupInfo`.

[Anand Mazumdar]

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

(Updated Sept. 7, 2016, 9:53 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description
---

This is needed for storing `TaskGroupInfo` in a list and then
invoking `std::find()`/`std::remove()` on it.

Review: https://reviews.apache.org/r/51475/


Diffs (updated)
-

  include/mesos/type_utils.hpp d10b66345e1a70801050d2cf2db63d76fbf9bc8c 
  src/common/type_utils.cpp c7d50334b4e56bc0eb5473b9f9277465b607334f 

Diff: https://reviews.apache.org/r/51475/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 51707: Fixed races between "unreachable" and "unregister" slave transitions.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Now that we update the registry before updating the master's state when
performing these transitions, it is possible for the master to already
be marking a slave unreachable when an `UnregisterSlaveMessage` is
received, or vice versa. Detect these situations and ignore whichever
transition is triggered second.


Diffs
-

  src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
  src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 

Diff: https://reviews.apache.org/r/51707/diff/


Testing
---

make check


Thanks,

Neil Conway

Review Request 51706: Tweaked logging for UnregisterSlaveMessage handler.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Remove a redundant log line: rather than logging when we receive
an UnregisterSlaveMessage, it is sufficient to log how we handle
that message. Also ensure that we log when ignoring an agent
unregister message for an unknown agent, and do some minor code
cleanup.


Diffs
-

  src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 

Diff: https://reviews.apache.org/r/51706/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 51653: Handled agents failing health checks multiple times.

[Neil Conway]

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

(Updated Sept. 7, 2016, 9:35 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak comment.


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


Repository: mesos


Description
---

Now that we wait for the agent to be removed from the registry before
stopping the SlaveObserver, it is possible for an agent to fail health
checks multiple times if the registry operation takes longer than
`agent_ping_timeout`.

This commit updates the master logic to handle this by ignoring health
check failures while the registry operation to mark the agent
unreachable is still in progress.


Diffs (updated)
-

  src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
  src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 

Diff: https://reviews.apache.org/r/51653/diff/


Testing
---

make check on OSX and Linux.

`./src/mesos-tests 
--gtest_filter="Strict/PartitionTest.FailHealthChecksTwice/0" 
--gtest_repeat=1000 --gtest_break_on_failure`


Thanks,

Neil Conway

Re: Review Request 50845: Added `unreachable_time` to TaskStatus.

[Neil Conway]

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

(Updated Sept. 7, 2016, 9:30 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

This field contains the time at which the master marked an agent as
unreachable. It will only be set for status updates (including
reconciliation updates) that describe tasks running on unreachable
(e.g., partitioned) agents.

The intent is to help frameworks decide how to handle tasks running on
partitioned agents: since the framework might have missed the initial
TASK_LOST/TASK_UNREACHABLE status update (e.g., due to framework
downtime/failover), it might otherwise be difficult for frameworks to
determine how long an agent has been partitioned.

This unreachable timestamp is stored in the registry and loaded as part
of master recovery.


Diffs (updated)
-

  include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
  include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
  src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
  src/common/protobuf_utils.cpp ed3ac7fcdd19bd517c30ee64d4746ea7c01628f1 
  src/master/master.hpp c32c7e9d859ef73216354e2c03ecc07d0009b12f 
  src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
  src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 
  src/tests/reconciliation_tests.cpp 8e438bfbce508a074f0d54513cd752344238e3f2 
  src/tests/registrar_tests.cpp b04fc92c66449f1094212a534899e16d5f6f01bd 

Diff: https://reviews.apache.org/r/50845/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 50845: Added `unreachable_time` to TaskStatus.

[Neil Conway]

> On Sept. 7, 2016, 8:57 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1763
> > 
> >
> > indent by 4?

I think this is indented correctly in the revised RR; let me know if that's not 
the case.


- Neil


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


On Sept. 6, 2016, 3:08 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50845/
> ---
> 
> (Updated Sept. 6, 2016, 3:08 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5949
> https://issues.apache.org/jira/browse/MESOS-5949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This field contains the time at which the master marked an agent as
> unreachable. It will only be set for status updates (including
> reconciliation updates) that describe tasks running on unreachable
> (e.g., partitioned) agents.
> 
> The intent is to help frameworks decide how to handle tasks running on
> partitioned agents: since the framework might have missed the initial
> TASK_LOST/TASK_UNREACHABLE status update (e.g., due to framework
> downtime/failover), it might otherwise be difficult for frameworks to
> determine how long an agent has been partitioned.
> 
> This unreachable timestamp is stored in the registry and loaded as part
> of master recovery.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
>   include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
>   src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
>   src/common/protobuf_utils.cpp ed3ac7fcdd19bd517c30ee64d4746ea7c01628f1 
>   src/master/master.hpp c32c7e9d859ef73216354e2c03ecc07d0009b12f 
>   src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
>   src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 
>   src/tests/reconciliation_tests.cpp 8e438bfbce508a074f0d54513cd752344238e3f2 
>   src/tests/registrar_tests.cpp b04fc92c66449f1094212a534899e16d5f6f01bd 
> 
> Diff: https://reviews.apache.org/r/50845/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50707: Added test for race between health checks and agent disconnect.

[Neil Conway]

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

(Updated Sept. 7, 2016, 9:29 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak comment.


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


Repository: mesos


Description
---

This test tries to simulate a race between marking an agent unhealthy
and receiving an `UnregisterSlaveMessage` for that agent.

Unfortunately, this test is a little fragile (we need to manually
dispatch an event to the master process to simulate the action that
would be taken by the slave observer).


Diffs (updated)
-

  src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 

Diff: https://reviews.apache.org/r/50707/diff/


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 46220: Added documentation for Nvidia GPU support.

[Kevin Klues]

> On Sept. 7, 2016, 5:52 p.m., Vinod Kone wrote:
> > docs/gpu-support.md, lines 366-367
> > 
> >
> > you can combine these 2 into one
> > 
> > ```
> > $ make -j check GTEST_FILTER="*NVIDIA_GPU*"
> > ```

I keep them separate so that the build is run as non-root, but the tests are 
run as root (i.e. sudo).


- Kevin


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


On Sept. 7, 2016, 9:12 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46220/
> ---
> 
> (Updated Sept. 7, 2016, 9:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5221
> https://issues.apache.org/jira/browse/MESOS-5221
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for Nvidia GPU support.
> 
> 
> Diffs
> -
> 
>   docs/gpu-support.md PRE-CREATION 
>   docs/home.md c8aeaef1d7e2bc6fc51e7bafaa9ff66aa376c0bc 
> 
> Diff: https://reviews.apache.org/r/46220/diff/
> 
> 
> Testing
> ---
> 
> Ran a local copy of the documentation site to make sure everything looks OK.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 46220: Added documentation for Nvidia GPU support.

[Kevin Klues]

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

(Updated Sept. 7, 2016, 9:13 p.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Added documentation for Nvidia GPU support.


Diffs
-

  docs/gpu-support.md PRE-CREATION 
  docs/home.md c8aeaef1d7e2bc6fc51e7bafaa9ff66aa376c0bc 

Diff: https://reviews.apache.org/r/46220/diff/


Testing
---

Ran a local copy of the documentation site to make sure everything looks OK.


Thanks,

Kevin Klues

Re: Review Request 46220: Added documentation for Nvidia GPU support.

[Kevin Klues]

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

(Updated Sept. 7, 2016, 9:12 p.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


Changes
---

Updated to address Vinod's comments.


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


Repository: mesos


Description
---

Added documentation for Nvidia GPU support.


Diffs (updated)
-

  docs/gpu-support.md PRE-CREATION 
  docs/home.md c8aeaef1d7e2bc6fc51e7bafaa9ff66aa376c0bc 

Diff: https://reviews.apache.org/r/46220/diff/


Testing
---

Ran a local copy of the documentation site to make sure everything looks OK.


Thanks,

Kevin Klues

Re: Review Request 51020: Clarified a log message.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Aug. 24, 2016, 3:46 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51020/
> ---
> 
> (Updated Aug. 24, 2016, 3:46 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4049
> https://issues.apache.org/jira/browse/MESOS-4049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clarified a log message.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 910293ad16a4eab5ac1e0815406f42dda90c4896 
> 
> Diff: https://reviews.apache.org/r/51020/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50846: Added more assertions to the master.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Aug. 29, 2016, 9:50 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50846/
> ---
> 
> (Updated Aug. 29, 2016, 9:50 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5949
> https://issues.apache.org/jira/browse/MESOS-5949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added more assertions to the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b118293823d37bba9b751aac12e0403318ce5736 
>   src/master/master.cpp 2b4aff80385d02c71dc3eeff4ddda326c9824ede 
> 
> Diff: https://reviews.apache.org/r/50846/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50845: Added `unreachable_time` to TaskStatus.

[Vinod Kone]

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


Fix it, then Ship it!





src/master/master.cpp (line 1763)


indent by 4?



src/master/master.cpp (line 1779)


indent by 4?



src/tests/reconciliation_tests.cpp (line 896)


`SlaveObserver`


- Vinod Kone


On Sept. 6, 2016, 3:08 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50845/
> ---
> 
> (Updated Sept. 6, 2016, 3:08 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5949
> https://issues.apache.org/jira/browse/MESOS-5949
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This field contains the time at which the master marked an agent as
> unreachable. It will only be set for status updates (including
> reconciliation updates) that describe tasks running on unreachable
> (e.g., partitioned) agents.
> 
> The intent is to help frameworks decide how to handle tasks running on
> partitioned agents: since the framework might have missed the initial
> TASK_LOST/TASK_UNREACHABLE status update (e.g., due to framework
> downtime/failover), it might otherwise be difficult for frameworks to
> determine how long an agent has been partitioned.
> 
> This unreachable timestamp is stored in the registry and loaded as part
> of master recovery.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
>   include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
>   src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
>   src/common/protobuf_utils.cpp ed3ac7fcdd19bd517c30ee64d4746ea7c01628f1 
>   src/master/master.hpp c32c7e9d859ef73216354e2c03ecc07d0009b12f 
>   src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
>   src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 
>   src/tests/reconciliation_tests.cpp 8e438bfbce508a074f0d54513cd752344238e3f2 
>   src/tests/registrar_tests.cpp b04fc92c66449f1094212a534899e16d5f6f01bd 
> 
> Diff: https://reviews.apache.org/r/50845/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [49571, 45967, 45966, 45963, 45962, 45964, 45961, 51553]

Failed command: ./support/apply-review.sh -n -r 45966

Error:
2016-09-07 20:20:46 URL:https://reviews.apache.org/r/45966/diff/raw/ 
[5783/5783] -> "45966.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.cpp:1416
error: src/master/allocator/mesos/hierarchical.cpp: patch does not apply
error: patch failed: src/tests/hierarchical_allocator_tests.cpp:1286
error: src/tests/hierarchical_allocator_tests.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/15149/console

- Mesos ReviewBot


On Sept. 7, 2016, 5:57 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49571/
> ---
> 
> (Updated Sept. 7, 2016, 5:57 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5771
> https://issues.apache.org/jira/browse/MESOS-5771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocations test has the following resource configurations:
> (1) REGULAR: Offers from every slave have regular resources.
> (2) SHARED: Offers from every slave include a shared resource.
> (3) REGULAR: Offers from every alternate slave contain only regular
> resources; and offers from every other alternate slave contains
> a shared resource.
> 
> This test is parameterized based on number of agents, number of
> frameworks and resource configuration.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 24698ffc9f72bc1e144e5107412f5e7d9c4f7105 
> 
> Diff: https://reviews.apache.org/r/49571/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> Allocations benchmark test results
> ==
> Support of shared resources has a small impact (roughly 10%) on runtime 
> performance in allocations as compared to HEAD (without shared resources). 
> Also, there is no visible impact in performance when shared resources are 
> added in the tests.
> 
> Following is a snapshot with 1000 agents and 200 frameworks.
> 
> With the patch (and no shared resources)
> 
> [ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
> Using 1000 agents and 200 frameworks with resource type 0
> Added 200 frameworks in 6907us
> Added 1000 agents in 2.057098secs
> round 0 allocate() took 1.689164secs to make 1000 offers
> round 50 allocate() took 1.672373secs to make 1000 offers
> round 100 allocate() took 1.680571secs to make 1000 offers
> round 150 allocate() took 1.674683secs to make 1000 offers
> round 199 allocate() took 1.671525secs to make 1000 offers
> 
> With the patch (and shared resources on all agents)
> ---
> [ RUN  ] 
> AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
> Using 1000 agents and 200 frameworks with resource type 1
> Added 200 frameworks in 6888us
> Added 1000 agents in 2.096218secs
> round 0 allocate() took 1.704491secs to make 1000 offers
> round 50 allocate() took 1.718623secs to make 1000 offers
> round 100 allocate() took 1.716224secs to make 1000 offers
> round 150 allocate() took 1.707343secs to make 1000 offers
> round 199 allocate() took 1.727467secs to make 1000 offers
> 
> With the patch (and shared resources on alternate agents)
> -
> [ RUN  ] 
> AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
> Using 1000 agents and 200 frameworks with resource type 2
> Added 200 frameworks in 7304us
> Added 1000 agents in 2.071009secs
> round 0 allocate() took 1.689045secs to make 1000 offers
> round 50 allocate() took 1.691524secs to make 1000 offers
> round 100 allocate() took 1.688873secs to make 1000 offers
> round 150 allocate() took 1.688713secs to make 1000 offers
> round 199 allocate() took 1.691223secs to make 1000 offers
> 
> Based on HEAD, with all regular resources (no shared resources in HEAD 
> supported)
> -
> [ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
> Using 1000 agents and 200 frameworks with resource type 0
> Added 200 frameworks in 6801us
> Added 1000 agents in 1.721447secs
> round 0 allocate() took 1.502953secs to make 1000 offers
> round 50 allocate() took 1.520157secs to make 1000 offers
> round 100 allocate() took 1.517221secs to make 1000 offers
> round 150 allocate() took 1.526446secs to make 1000 offers
> round 199 allocate() took 1.538005secs to make 1000 offers

Re: Review Request 51257: Add external process container logger.

[Joseph Wu]

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




src/slave/container_loggers/lib_externallogger.hpp (lines 20 - 38)


You don't use the following includes:

* #include 
* #include 
* #include 
* #include 



src/slave/container_loggers/lib_externallogger.hpp (lines 52 - 61)


This flag should be required.  You can use `flags.add` like:
```
add(_logger_cmd,
"external_logger_cmd",
None(),
"Path to the external command which will read STDIN for logs",
[](const std::string& executablePath) -> Option {
  if (!os::exists(executablePath)) {
  return Error("Cannot find: " + executablePath);
  }
  return None();
});
```

The `None()` as the third argument is an alias.  We need this to 
disambiguate the various overloads of `flags.add`.

---

Also, add a newline after this block.



src/slave/container_loggers/lib_externallogger.hpp (line 86)


Remove this copied comment.



src/slave/container_loggers/lib_externallogger.hpp (lines 89 - 90)


The comment `piped to the files "stdout" and "stderr"...` is not accurate.  
This will pipe to whatever the operator has configured the 
ExternalContainerLogger to do :)



src/slave/container_loggers/lib_externallogger.hpp (line 98)


Oops, more `journald` :)



src/slave/container_loggers/lib_externallogger.hpp (line 107)


`journald` here too.



src/slave/container_loggers/lib_externallogger.cpp (lines 87 - 89)


You can move this underscore into the flag.  i.e. change the default from 
`MESOS_LOG` to `MESOS_LOG_`.

Maybe someone doesn't want an underscore after the prefix.



src/slave/container_loggers/lib_externallogger.cpp (line 130)


You don't need these flags.  In fact, if your external command parses 
commandline arguments, it will see these as unknown flags and bail out.

The Logrotate module had a dedicated binary, with its own set of defined 
flags.  For this module, everything is passed in via environment variables.



src/slave/container_loggers/lib_externallogger.cpp (line 152)


You should `SETSID` here.  Otherwise, you will lose logging (the external 
command will die) when the agent dies.



src/slave/container_loggers/lib_externallogger.cpp (line 188)


Ditto with these flags.



src/slave/container_loggers/lib_externallogger.cpp (line 199)


Ditto with `SETSID`.


- Joseph Wu


On Sept. 2, 2016, 4:26 a.m., Will Rouesnel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51257/
> ---
> 
> (Updated Sept. 2, 2016, 4:26 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6003
> https://issues.apache.org/jira/browse/MESOS-6003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the external process container logger. This functions like the
> logrotate container logger, but instead calls a custom host binary
> (or script) and passes the executorInfo as JSON via environment
> variables. This makes it very easy for users to configure custom
> logging solutions, without needing to write and maintain logging
> modules.
> 
> Tests passing and complete.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 
>   src/slave/container_loggers/lib_externallogger.hpp PRE-CREATION 
>   src/slave/container_loggers/lib_externallogger.cpp PRE-CREATION 
>   src/tests/container_logger_external.sh PRE-CREATION 
>   src/tests/container_logger_tests.cpp 
> e8f934106510fe02b8b92be19c918a1e5c0b78fd 
>   src/tests/module.hpp e661d95fa44fc1aedfe83c564c826d5b7d32c85b 
>   src/tests/module.cpp 5b83fd6358ddea4c9d849b8992e1a6040ef74505 
> 
> Diff: https://reviews.apache.org/r/51257/diff/
> 
> 
> Testing
> ---
> 
> Adds ContainerLoggerTest.EXTERNAL_RecieveEnvironment which tests all major 
> parameters of the change.
> 
> A synthetic external container logger is provided by the script 
> tests/container_logger_external.sh which is setup to fail if any important 
> output is unavailable to 

Review Request 51700: Add `FrameworkAdded` event to master event stream.

[Zhitao Li]

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

Review request for mesos, Xiaojian Huang, haosdent huang, and Vinod Kone.


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


Repository: mesos


Description
---

Changes included:
- Moving `model()` helper function to common/protobuf_utils;
- Definition of event and response;
- Sending event from master;
- new test.


Diffs
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
  src/common/protobuf_utils.cpp 8c4a7264080385789157703b05b42716fb0563b3 
  src/master/http.cpp 52dd80b856cf2317c0b73ba54bf501696786088d 
  src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 

Diff: https://reviews.apache.org/r/51700/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51697, 50270, 50271]

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 Sept. 7, 2016, 4:46 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Sept. 7, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This isolator evaluates agent allowed capabilities and passes net
> capabilities on to `mesos-containerizer` which enforces the
> capabilities.
> 
> Capability information is passed via a new field in
> `ContainerLaunchInfo`.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 16dd3a19145b9764273cdb9a8899e353c98730e5 
>   src/CMakeLists.txt 01ef494f7120156de3b826d7def76fb30bcc61b5 
>   src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
>   src/tests/containerizer/isolator_tests.cpp 
> f8056ca08029feed5f164d4f94e24d521183bdfc 
> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 45961: Support sharing of resources through reference counting of resources.

[Jiang Yan Xu]

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



Given the way this review has evolved how about the following summary for the 
commit?

```
Support sharing of persistent volumes via shared resources.

- One copy of a shared resource is made allocatable in each allocation 
  cycle. 
- All frameworks that are allocated the same shared resource are
  charged the scalar quantity of the resource in wDRF.
- Resource accounting for *individual* copies of shared resources is
  handled mostly in the same way as non-shared resources in the master 
  although they are grouped within the `Resources` abstraction for
  performance.
- DESTROY for shared persistent volumes is allowed only if there are
  no running or pending tasks using the volume. However, if the volume
  is in a pending offer to one or more frameworks, the master rescinds
  that offer before processing the DESTROY.
- Multiple tasks are allowed to be launched in the same ACCEPT call
  using the same shared resource. For this we update the allocator and
  sorter to allocate additional copies of these shared resources.
```

- Jiang Yan Xu


On Sept. 7, 2016, 10:56 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45961/
> ---
> 
> (Updated Sept. 7, 2016, 10:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael 
> Park, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> o Each shared resource is accouted via its share count. This count is
>   updated based on the resource operations (such as add and subtract)
>   in various scenarios such as task launch and terminate at multiple
>   modules such as master, allocator, sorter, etc.
> o Only allow DESTROY if there are no running or pending tasks using
>   the volume. However, if the volume is in a pending offer to one or
>   more frameworks, rescind that offer before processing the DESTROY.
> o To allow multiple tasks to be launched in the same ACCEPT call
>   using the same shared resource, we update the allocator and
>   sorter with additional copies of shared resources to reflect the
>   true shared count of allocated shared resources.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 09aa685f2bd7197385959d7d70d5411d0fd72f06 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 1f2954c564eea5a3c649a5cc7181cb69329f9e35 
>   src/master/http.cpp 525ef6cd7545d25f3ac89a6325ace6e6d768262a 
>   src/master/master.hpp c32c7e9d859ef73216354e2c03ecc07d0009b12f 
>   src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
>   src/master/validation.hpp 3205ee1e699d0fb7ca16ced96a07be3a07cf8956 
>   src/master/validation.cpp 15e2ecc2de99d6bed522f45f855ba686bf19c008 
> 
> Diff: https://reviews.apache.org/r/45961/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 42861: Ensure two Mount Disk resources do not have the same root path.

[Joris Van Remoortere]

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



As Greg said some tests would be great. Then we can ship this.


src/slave/slave.cpp (lines 552 - 554)


We should improve the error information here. Why can't we identify which 
original paths that were provided are overlapping?
If we need to improve a library function we can add a TODO here and follow 
up with a patch.


- Joris Van Remoortere


On April 1, 2016, 4:43 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated April 1, 2016, 4:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp fc77f594d16a9fb2ca001e089d74e2c0ffeb5baa 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51643: Modified network file setup in `network/cni` isolator.

[Avinash sridharan]

> On Sept. 7, 2016, 5:02 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1537-1538
> > 
> >
> > I would mention that this step is necessary because we change the 
> > hostname to be containerID and libprocess (executor uses) needs to resolve 
> > that hostname.

We actually talk about this in the comments above:
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L1525

Have updated those comments to be a bit more explicit about the hostname change 
to 'continer ID' and the reasoning for this bind mount.


- Avinash


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


On Sept. 7, 2016, 7:02 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51643/
> ---
> 
> (Updated Sept. 7, 2016, 7:02 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6052
> https://issues.apache.org/jira/browse/MESOS-6052
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In case /etc/hosts and /etc/hostname files are not present in the host
> filesystem, we were ignoring these files and assuming that they would
> not be required by the executor when it is launched in a new network
> namespace. This assumption is incorrect, since the executor needs
> /etc/hosts in the new network namespace to resolve its hostname.
> Hence, we are explicitly creating these files in the host file system
> in case they are not present, so that containers /etc/hosts and
> /etc/hostname can be mounted on these mount points. This solves the
> problem in distributions such as CoreOS that don't have /etc/hosts in
> their host filesystem.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> d17a45fe17bb74cbf9ae421dc8a492e5dc5f1a00 
> 
> Diff: https://reviews.apache.org/r/51643/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also, build an open DC/OS image with this patch to run on CoreOS and tried 
> launch a unified containerizer through Marathon using dcos-cli to test this 
> patch on CoreOS.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51643: Modified network file setup in `network/cni` isolator.

[Avinash sridharan]

> On Sept. 7, 2016, 1 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1540-1542
> > 
> >
> > Given that this case is very specific for systems without a dependency 
> > on `/etc/hosts` or `/etc/hostname`, would it make sense to create the files 
> > in the agent's work directory?  i.e. to avoid polluting the host?
> 
> Avinash sridharan wrote:
> We create these files in the agents work directory as well (a directory 
> specific to the `network/cni` isolator). The reason we create these files 
> specifically in the host fs is for the `executor` and containers that don't 
> have their own images. Reason being that both these entities see the host 
> file system, albiet with a different mount name space. Also, since the 
> hostname and IP address is different from the hosts IP and hostname, we need 
> to give them a different view of the /etc/hosts and /etc/hostname files. We 
> do this by bind mounting the files from the containers work dir on to these 
> target mount points on the host file system. 
> 
> We do the same operation below for the containers rootfs as well. 
> However, in case of the containers rootfs we are guaranteed that these mount 
> points exist (if not we can create them).

Modified the comments to clarify this point.


- Avinash


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


On Sept. 7, 2016, 7:02 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51643/
> ---
> 
> (Updated Sept. 7, 2016, 7:02 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6052
> https://issues.apache.org/jira/browse/MESOS-6052
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In case /etc/hosts and /etc/hostname files are not present in the host
> filesystem, we were ignoring these files and assuming that they would
> not be required by the executor when it is launched in a new network
> namespace. This assumption is incorrect, since the executor needs
> /etc/hosts in the new network namespace to resolve its hostname.
> Hence, we are explicitly creating these files in the host file system
> in case they are not present, so that containers /etc/hosts and
> /etc/hostname can be mounted on these mount points. This solves the
> problem in distributions such as CoreOS that don't have /etc/hosts in
> their host filesystem.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> d17a45fe17bb74cbf9ae421dc8a492e5dc5f1a00 
> 
> Diff: https://reviews.apache.org/r/51643/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also, build an open DC/OS image with this patch to run on CoreOS and tried 
> launch a unified containerizer through Marathon using dcos-cli to test this 
> patch on CoreOS.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51643: Modified network file setup in `network/cni` isolator.

[Avinash sridharan]

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

(Updated Sept. 7, 2016, 7:02 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.


Changes
---

Addressed comments from Jie and Joseph.


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


Repository: mesos


Description
---

In case /etc/hosts and /etc/hostname files are not present in the host
filesystem, we were ignoring these files and assuming that they would
not be required by the executor when it is launched in a new network
namespace. This assumption is incorrect, since the executor needs
/etc/hosts in the new network namespace to resolve its hostname.
Hence, we are explicitly creating these files in the host file system
in case they are not present, so that containers /etc/hosts and
/etc/hostname can be mounted on these mount points. This solves the
problem in distributions such as CoreOS that don't have /etc/hosts in
their host filesystem.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
d17a45fe17bb74cbf9ae421dc8a492e5dc5f1a00 

Diff: https://reviews.apache.org/r/51643/diff/


Testing
---

make check

Also, build an open DC/OS image with this patch to run on CoreOS and tried 
launch a unified containerizer through Marathon using dcos-cli to test this 
patch on CoreOS.


Thanks,

Avinash sridharan

Re: Review Request 51358: Implemented recursive helper method findContainerDir for provisioner.

[Gilbert Song]

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

(Updated Sept. 7, 2016, 12:01 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

Implemented recursive helper method findContainerDir for provisioner.


Diffs
-

  src/slave/containerizer/mesos/provisioner/paths.hpp 
9829d6b52c8547ae22297a5bc47852ce5a219e4c 
  src/slave/containerizer/mesos/provisioner/paths.cpp 
86a45f30c22dc1e41f4779c0ce8c11d02dcc46bb 

Diff: https://reviews.apache.org/r/51358/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51359: Added unit test for provisioner helper findContainerDir.

[Gilbert Song]

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

(Updated Sept. 7, 2016, 12:01 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

Added unit test for provisioner helper findContainerDir.


Diffs
-

  src/Makefile.am 8dc4175c60e4a9776ddb8ad21774fa4b30c28d00 
  src/tests/containerizer/provisioner_paths_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/51359/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51343: Refactored the redundant logic in provisioner recover().

[Gilbert Song]

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

(Updated Sept. 7, 2016, noon)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

This patch removed the unnecessary loop to construct an identical
hashmap for info->rootfses.


Diffs
-

  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
8e35ff49ec99a242e764095dcfbb541c5e41ec71 

Diff: https://reviews.apache.org/r/51343/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

> On Sept. 5, 2016, 2:51 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp, line 434
> > 
> >
> > s/sub-container/sub-containers/

Lets us nested containers consistantly.


- Gilbert


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


On Sept. 7, 2016, 11:49 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 7, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51420: Added provisioner appc unit test for provisioning nested container.

[Gilbert Song]

> On Sept. 4, 2016, 7:13 a.m., Guangya Liu wrote:
> > src/tests/containerizer/provisioner_appc_tests.cpp, lines 372-373
> > 
> >
> > Just nit, I saw that the recover case is using 
> > `UUID::random().toString()` to set the id while some using hard code ids, 
> > it is better make them consistent in code.

Fixed. thx.


- Gilbert


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


On Sept. 7, 2016, 11:49 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51420/
> ---
> 
> (Updated Sept. 7, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added provisioner appc unit test for provisioning nested container.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> b3ba176e506a6d1528290c07a8a0555b12c8cf70 
> 
> Diff: https://reviews.apache.org/r/51420/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

> On Sept. 5, 2016, 7:38 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp, lines 432-434
> > 
> >
> > Just a question, how do we ensure when a container is being destroyed, 
> > all its sub-containers are already destroyed? Will we need to add some 
> > logic in the caller of provisioner destroy to ensure this?

This is ensured in containerizer::destroy().


- Gilbert


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


On Sept. 7, 2016, 11:49 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 7, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51392: Supported provisioner listContainers() to be recursive.

[Gilbert Song]

> On Aug. 30, 2016, 3:56 p.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/provisioner/paths.cpp, lines 194-196
> > 
> >
> > You don't really need to check if the subcontainers are empty here.

Fixed.


- Gilbert


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


On Sept. 7, 2016, 11:49 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51392/
> ---
> 
> (Updated Sept. 7, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch supports collecting all containerIds (all containers in the
> nested hierarchy) recursively.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 86a45f30c22dc1e41f4779c0ce8c11d02dcc46bb 
> 
> Diff: https://reviews.apache.org/r/51392/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51392: Supported provisioner listContainers() to be recursive.

[Gilbert Song]

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

(Updated Sept. 7, 2016, 11:49 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

This patch supports collecting all containerIds (all containers in the
nested hierarchy) recursively.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/paths.cpp 
86a45f30c22dc1e41f4779c0ce8c11d02dcc46bb 

Diff: https://reviews.apache.org/r/51392/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51420: Added provisioner appc unit test for provisioning nested container.

[Gilbert Song]

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

(Updated Sept. 7, 2016, 11:49 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

Added provisioner appc unit test for provisioning nested container.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
b3ba176e506a6d1528290c07a8a0555b12c8cf70 

Diff: https://reviews.apache.org/r/51420/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51393: Added unit test for provisioner recursive listContainers().

[Gilbert Song]

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

(Updated Sept. 7, 2016, 11:49 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

Added unit test for provisioner recursive listContainers().


Diffs (updated)
-

  src/Makefile.am a3074826ec346e11cd4cdf807980f63c2e50f1f6 
  src/tests/containerizer/provisioner_paths_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/51393/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Gilbert Song]

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

(Updated Sept. 7, 2016, 11:49 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


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


Repository: mesos


Description
---

Added nested container check in provisioner destroy.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
8e35ff49ec99a242e764095dcfbb541c5e41ec71 

Diff: https://reviews.apache.org/r/51402/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51503: Fixed appc provisioner tests to use absolute work directory.

[Gilbert Song]

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

(Updated Sept. 7, 2016, 11:49 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


Repository: mesos


Description
---

Fixed appc provisioner tests to use absolute work directory.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
b3ba176e506a6d1528290c07a8a0555b12c8cf70 

Diff: https://reviews.apache.org/r/51503/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49862: Changed libraies to shared on OSX and UNIX.

[Srinivas Brahmaroutu]

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

(Updated Sept. 7, 2016, 6:33 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added fles MESOS_DEFAULT_LIBRARY_LINKAGE which is set to STATIC on
WIN32 and SHARED on OSX or Unix. This allows all libraries built
static or shared. Also minor changes made to eliminate leveldb
dependency when build slave or master. Also by setting the flag
CMAKE_POSITION_INDEPENDENT_CODE to true cmake auto sets the
flag -fPIC.


Diffs (updated)
-

  3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
eeb27860f6f95d297ccfe273ed76de5355b50ff8 
  3rdparty/http-parser/CMakeLists.txt.template 
9a671973b754095e1de917f135a7deb978fb6eb6 
  cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
  src/CMakeLists.txt 01ef494f7120156de3b826d7def76fb30bcc61b5 
  src/master/cmake/MasterConfigure.cmake 
6bbd7e87273976f40527d719cc9450ff9a1d2ac7 
  src/slave/cmake/SlaveConfigure.cmake b339239761a5de321d65b92376dae69c339bee5c 
  src/slave/qos_controllers/CMakeLists.txt 
87c92af21c012655c201c01cd4ba5ff912555119 
  src/slave/resource_estimators/CMakeLists.txt 
17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 

Diff: https://reviews.apache.org/r/49862/diff/


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49921: Fixed mesos tests to run most of the tests on Unix and OSX.

[Srinivas Brahmaroutu]

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

(Updated Sept. 7, 2016, 6:33 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

This patch builds /src/mesos-tests
This patch adds all test sources, categorize them so that some are run
in environment with Java, on Unix, etc. Patch allows to run all the
tests that are enabled on a platform by simply running the mesos-tests
executable.


Diffs (updated)
-

  src/CMakeLists.txt 01ef494f7120156de3b826d7def76fb30bcc61b5 
  src/tests/CMakeLists.txt f5d66dc63143455506d8660674fbd9eb227625ff 
  src/tests/cmake/MesosTestsConfigure.cmake 
3f543c010ae87ff04e6b45745bc49ef65b6590ff 

Diff: https://reviews.apache.org/r/49921/diff/


Testing
---

cmake .. && make
src/tests/mesos-tests  (runs 723 tests with no failures)
I did not enable any module that has even a single failue. There are many more 
tests that are passing.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49874: Added logrotate_container_logger for running mesos tests.

[Srinivas Brahmaroutu]

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

(Updated Sept. 7, 2016, 6:33 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Builds library liblogrotate_container_logger useful to operate
logrotate loggers.


Diffs (updated)
-

  src/slave/CMakeLists.txt 33120ace79bce449329a7cc4b7ef264d2867fc13 
  src/slave/cmake/SlaveConfigure.cmake b339239761a5de321d65b92376dae69c339bee5c 
  src/slave/container_loggers/CMakeLists.txt PRE-CREATION 

Diff: https://reviews.apache.org/r/49874/diff/


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49924: Added libprocess as a shared library.

[Srinivas Brahmaroutu]

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

(Updated Sept. 7, 2016, 6:33 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

This patch allow to build libprocess as shared library on OSX and Linux


Diffs (updated)
-

  3rdparty/libprocess/src/CMakeLists.txt 
d1547ef6a8762385f653d3824307727e4d0a7e71 

Diff: https://reviews.apache.org/r/49924/diff/


Testing
---

cmake .. && make

With this patch and https://reviews.apache.org/r/49862,  Converted libmesos, 
http_parser and libprocess to shared libraries and we are using libevent shared 
library, zookeeper does not have a shared library in the 3rdparty (I guess the 
code is compiled as relocatable) and did not have issues linking.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49870: Added test executables required to run tests.

[Srinivas Brahmaroutu]

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

(Updated Sept. 7, 2016, 6:33 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Builds executables dynamic-reservation-framework, test-http-framework,
test-framework, test-executor, test-http-executor, long-lived-framework,
long-lived-executor, no-executor-framework,
docker-no-executor-framework, balloon-framework, balloon-executor,
load-generator-framework, persistent-volume-framework


Diffs (updated)
-

  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

Diff: https://reviews.apache.org/r/49870/diff/


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49863: Added Test Modules that are loaded by mesos tests.

[Srinivas Brahmaroutu]

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

(Updated Sept. 7, 2016, 6:33 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Builds shared libraries for dynamically loading them by various tests:
libexamplemodule, libtestauthorizer, libtestisolator,
libtestresource_estimator, libtestallocator, libtestcontainer_logger,
libtestmastercontender, libtestanonymous, libtesthook,
libtestmasterdetector, libtestauthentication, libtesthttpauthenticator,
libtestqos_controller.


Diffs (updated)
-

  CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
  cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
  src/CMakeLists.txt 01ef494f7120156de3b826d7def76fb30bcc61b5 
  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

Diff: https://reviews.apache.org/r/49863/diff/


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49688: Added cmake build variables for mesos tests.

[Srinivas Brahmaroutu]

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

(Updated Sept. 7, 2016, 6:33 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

The patch simply sets up the mesos-tests target, also defines and sets
up the required dependencies, includ and lib directories. Target and
sources are added after building all test dependencies in a patch.


Diffs (updated)
-

  src/tests/cmake/MesosTestsConfigure.cmake 
3f543c010ae87ff04e6b45745bc49ef65b6590ff 
  src/tests/containerizer/CMakeLists.txt PRE-CREATION 

Diff: https://reviews.apache.org/r/49688/diff/


Testing
---

cmake ..
cmake check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Jie Yu]

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



This patch does not look correct.

I think we need a top level destroy which is nesting aware. We might want to 
rename the existing destory to `_destroy` which is not nesting aware. THe top 
level destroy will recursively call destroy for its children and calling `wait` 
for them to finish before calling `_destory` for itself.


src/slave/containerizer/mesos/containerizer.cpp (line 1634)


why this change there? The original sentense sounds fine to me



src/slave/containerizer/mesos/containerizer.cpp (line 1645)


Again, why this change?



src/slave/containerizer/mesos/containerizer.cpp (lines 1652 - 1657)


This is not correct. 'destroy' simply initiate the destroy. You need to 
call 'wait' to wait for it to finish.


- Jie Yu


On Sept. 6, 2016, 9:53 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51674/
> ---
> 
> (Updated Sept. 6, 2016, 9:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported mesos containerizer destroy to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
> 
> Diff: https://reviews.apache.org/r/51674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 5:57 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
24698ffc9f72bc1e144e5107412f5e7d9c4f7105 

Diff: https://reviews.apache.org/r/49571/diff/


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 45967: Added documentation for shareable resources.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 5:57 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

  docs/home.md c8aeaef1d7e2bc6fc51e7bafaa9ff66aa376c0bc 
  docs/shared-resources.md PRE-CREATION 

Diff: https://reviews.apache.org/r/45967/diff/


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45966: Offer shared resources to frameworks only if opted in.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 5:57 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added a new capability SHARED_RESOURCES that frameworks need to opt
in if they are interested in receiving shared resources in their
offers.


Diffs (updated)
-

  include/mesos/mesos.proto 7fbcdf005718ba4d7ec3b02cf70a3fc53d4f4818 
  include/mesos/v1/mesos.proto 60ec0cce35e5e2d2f4907efadc7ab92680566ef0 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/allocator/mesos/hierarchical.hpp 
dd07ed221d2c1755d2478369641ffdc46ecc4471 
  src/master/allocator/mesos/hierarchical.cpp 
9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
  src/tests/hierarchical_allocator_tests.cpp 
24698ffc9f72bc1e144e5107412f5e7d9c4f7105 

Diff: https://reviews.apache.org/r/45966/diff/


Testing
---

Tests updated with new capability.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 5:57 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, allow the task to be executed only if the ownership of the
persistent volume matches that of the task.
Added an option to run the test in mixed (default) mode or shared-only
mode. In mixed mode, multiple shards alternate between shared and
unshared persistent volumes for the tasks. In shared-only mode, all
shards use shared persistent volumes for the tasks.


Diffs (updated)
-

  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/validation.hpp 3205ee1e699d0fb7ca16ced96a07be3a07cf8956 
  src/master/validation.cpp 15e2ecc2de99d6bed522f45f855ba686bf19c008 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
0a85935550e36c9142d845465cfa70a1634a647a 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
ea418252956c8089acc5a491888ed7f6df6cafcd 
  src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
794b6e5990db5f8eb21a6535872f284ca02e0553 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
af427c6e5691f1770ab3ebef79502eb2c2176c4a 

Diff: https://reviews.apache.org/r/45963/diff/


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45961: Support sharing of resources through reference counting of resources.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 5:56 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael Park, 
and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

o Each shared resource is accouted via its share count. This count is
  updated based on the resource operations (such as add and subtract)
  in various scenarios such as task launch and terminate at multiple
  modules such as master, allocator, sorter, etc.
o Only allow DESTROY if there are no running or pending tasks using
  the volume. However, if the volume is in a pending offer to one or
  more frameworks, rescind that offer before processing the DESTROY.
o To allow multiple tasks to be launched in the same ACCEPT call
  using the same shared resource, we update the allocator and
  sorter with additional copies of shared resources to reflect the
  true shared count of allocated shared resources.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
dd07ed221d2c1755d2478369641ffdc46ecc4471 
  src/master/allocator/mesos/hierarchical.cpp 
9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
  src/master/allocator/sorter/drf/sorter.hpp 
09aa685f2bd7197385959d7d70d5411d0fd72f06 
  src/master/allocator/sorter/drf/sorter.cpp 
1f2954c564eea5a3c649a5cc7181cb69329f9e35 
  src/master/http.cpp 525ef6cd7545d25f3ac89a6325ace6e6d768262a 
  src/master/master.hpp c32c7e9d859ef73216354e2c03ecc07d0009b12f 
  src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
  src/master/validation.hpp 3205ee1e699d0fb7ca16ced96a07be3a07cf8956 
  src/master/validation.cpp 15e2ecc2de99d6bed522f45f855ba686bf19c008 

Diff: https://reviews.apache.org/r/45961/diff/


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 5:56 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs (updated)
-

  src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
  src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/45962/diff/


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45964: Add unit tests for sharing of resources.

[Anindya Sinha]

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

(Updated Sept. 7, 2016, 5:56 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add unit tests for sharing of resources.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
24698ffc9f72bc1e144e5107412f5e7d9c4f7105 
  src/tests/master_validation_tests.cpp 
8bb4c37a54c7f336e4b71310e12078400e648802 
  src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f 
  src/tests/persistent_volume_tests.cpp 
a6f97c4bb5fb29d610c01255036095e2b30c44c5 
  src/tests/sorter_tests.cpp 821e30d6574b045d25d4de4f7c3b8ac5346d3002 

Diff: https://reviews.apache.org/r/45964/diff/


Testing
---

Tests for shared resources added for allocator, resources and sorter.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 51643: Modified network file setup in `network/cni` isolator.

[Avinash sridharan]

> On Sept. 7, 2016, 1 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1540-1542
> > 
> >
> > Given that this case is very specific for systems without a dependency 
> > on `/etc/hosts` or `/etc/hostname`, would it make sense to create the files 
> > in the agent's work directory?  i.e. to avoid polluting the host?

We create these files in the agents work directory as well (a directory 
specific to the `network/cni` isolator). The reason we create these files 
specifically in the host fs is for the `executor` and containers that don't 
have their own images. Reason being that both these entities see the host file 
system, albiet with a different mount name space. Also, since the hostname and 
IP address is different from the hosts IP and hostname, we need to give them a 
different view of the /etc/hosts and /etc/hostname files. We do this by bind 
mounting the files from the containers work dir on to these target mount points 
on the host file system. 

We do the same operation below for the containers rootfs as well. However, in 
case of the containers rootfs we are guaranteed that these mount points exist 
(if not we can create them).


- Avinash


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


On Sept. 6, 2016, 9:09 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51643/
> ---
> 
> (Updated Sept. 6, 2016, 9:09 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6052
> https://issues.apache.org/jira/browse/MESOS-6052
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In case /etc/hosts and /etc/hostname files are not present in the host
> filesystem, we were ignoring these files and assuming that they would
> not be required by the executor when it is launched in a new network
> namespace. This assumption is incorrect, since the executor needs
> /etc/hosts in the new network namespace to resolve its hostname.
> Hence, we are explicitly creating these files in the host file system
> in case they are not present, so that containers /etc/hosts and
> /etc/hostname can be mounted on these mount points. This solves the
> problem in distributions such as CoreOS that don't have /etc/hosts in
> their host filesystem.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> d17a45fe17bb74cbf9ae421dc8a492e5dc5f1a00 
> 
> Diff: https://reviews.apache.org/r/51643/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also, build an open DC/OS image with this patch to run on CoreOS and tried 
> launch a unified containerizer through Marathon using dcos-cli to test this 
> patch on CoreOS.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51553: Changed the way `HAP::updateAllocation()` calls `Resources.apply()`.

[Anindya Sinha]

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


Ship it!




Ship It!

- Anindya Sinha


On Sept. 7, 2016, 5:16 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51553/
> ---
> 
> (Updated Sept. 7, 2016, 5:16 p.m.)
> 
> 
> Review request for mesos and Anindya Sinha.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is needed for supporting LAUNCH operations with shared resources
> (More details in https://reviews.apache.org/r/45961/) but this patch
> itself has no functional change.
> 
> Also, this patch together with https://reviews.apache.org/r/51412/
> results in no change in the granularity in which Offer operations are
> applied. Each single operation in the ACCEPT call is still individually
> applied to Resources objects managed by the master, the allocator and
> the sorters, same as before.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
> 
> Diff: https://reviews.apache.org/r/51553/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 46220: Added documentation for Nvidia GPU support.

[Vinod Kone]

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


Fix it, then Ship it!




This is great! Thanks for being so thorough.


docs/gpu-support.md (line 125)


traditional



docs/gpu-support.md (line 286)


"set LD_LIBRRARY_PATH to `lib`" or "add `lib` to LD_LIBRRARY_PATH"?



docs/gpu-support.md (line 333)


s/unit/Mesos unit/



docs/gpu-support.md (line 360)


binary



docs/gpu-support.md (lines 366 - 367)


you can combine these 2 into one

```
$ make -j check GTEST_FILTER="*NVIDIA_GPU*"
```


- Vinod Kone


On Sept. 7, 2016, 11:19 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46220/
> ---
> 
> (Updated Sept. 7, 2016, 11:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5221
> https://issues.apache.org/jira/browse/MESOS-5221
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for Nvidia GPU support.
> 
> 
> Diffs
> -
> 
>   docs/gpu-support.md PRE-CREATION 
>   docs/home.md c8aeaef1d7e2bc6fc51e7bafaa9ff66aa376c0bc 
> 
> Diff: https://reviews.apache.org/r/46220/diff/
> 
> 
> Testing
> ---
> 
> Ran a local copy of the documentation site to make sure everything looks OK.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 46187: Terminate when receiving the ACK of terminal status update.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46187]

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 Sept. 7, 2016, 2:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated Sept. 7, 2016, 2:03 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5276
> https://issues.apache.org/jira/browse/MESOS-5276
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Terminate when receiving the ACK of terminal status update.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 
> 
> Diff: https://reviews.apache.org/r/46187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 51480: Added tests for launching task groups on the agent.

[Vinod Kone]

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


Ship it!





src/tests/slave_tests.cpp (lines 4621 - 4622)


why did you need to explicitly do this here but not in the above test?


- Vinod Kone


On Sept. 6, 2016, 9:36 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51480/
> ---
> 
> (Updated Sept. 6, 2016, 9:36 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds tests for testing the `runTaskGroup()` handler
> on the agent.
> 
> Review: https://reviews.apache.org/r/51480/
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 
> 
> Diff: https://reviews.apache.org/r/51480/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51479: Added mock overloads for `runTaskGroup()`/`launchGroup()`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 6, 2016, 9:28 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51479/
> ---
> 
> (Updated Sept. 6, 2016, 9:28 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/51479/
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f 
>   src/tests/mesos.cpp f5034f9f8de7040182e10f51be125a87b29fdd24 
> 
> Diff: https://reviews.apache.org/r/51479/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51478: Modified tests to account for `_runTask()` rename.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/slave_tests.cpp (line 2810)


`_run()`



src/tests/slave_tests.cpp (line 2869)


`Slave::_run()`



src/tests/slave_tests.cpp (line 2875)


`Slave::_run()`



src/tests/slave_tests.cpp (line 2901)


`Slave::_run()`



src/tests/slave_tests.cpp (line 2901)


`Slave::_run()`



src/tests/slave_tests.cpp (line 2902)


`Shutdown()`


- Vinod Kone


On Sept. 6, 2016, 9:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51478/
> ---
> 
> (Updated Sept. 6, 2016, 9:27 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
> https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modified the agent mocks/tests to account for the
> `runTask()` rename.
> 
> Review: https://reviews.apache.org/r/51478/
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 7dda6f67d9d02d14759563ec64269060dc7e643b 
>   src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f 
>   src/tests/mesos.cpp f5034f9f8de7040182e10f51be125a87b29fdd24 
>   src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 
> 
> Diff: https://reviews.apache.org/r/51478/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 45961: Support sharing of resources through reference counting of resources.

[Jiang Yan Xu]

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


Ship it!




Minor things that I'll take care of when committing.


src/master/allocator/mesos/hierarchical.cpp (lines 675 - 679)


```
s/additional instances of shared resources/additional instances of the same 
shared resources/
```



src/master/allocator/mesos/hierarchical.cpp (line 1361)


s/cycle/stage/



src/master/allocator/sorter/drf/sorter.cpp (line 163)


s/total/allocated/



src/master/allocator/sorter/drf/sorter.cpp (lines 165 - 168)


Changed the indentation style to

```
  const Resources newShared = resources.shared()
.filter([this, name, slaveId](const Resource& resource) {
  return !allocations[name].resources[slaveId].contains(resource);
});

```

which I found more readable. Similarly in add/remove/unallocated.


- Jiang Yan Xu


On Sept. 6, 2016, 3:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45961/
> ---
> 
> (Updated Sept. 6, 2016, 3:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael 
> Park, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> o Each shared resource is accouted via its share count. This count is
>   updated based on the resource operations (such as add and subtract)
>   in various scenarios such as task launch and terminate at multiple
>   modules such as master, allocator, sorter, etc.
> o Only allow DESTROY if there are no running or pending tasks using
>   the volume. However, if the volume is in a pending offer to one or
>   more frameworks, rescind that offer before processing the DESTROY.
> o To allow multiple tasks to be launched in the same ACCEPT call
>   using the same shared resource, we update the allocator and
>   sorter with additional copies of shared resources to reflect the
>   true shared count of allocated shared resources.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> dd07ed221d2c1755d2478369641ffdc46ecc4471 
>   src/master/allocator/mesos/hierarchical.cpp 
> 9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 09aa685f2bd7197385959d7d70d5411d0fd72f06 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 1f2954c564eea5a3c649a5cc7181cb69329f9e35 
>   src/master/http.cpp 525ef6cd7545d25f3ac89a6325ace6e6d768262a 
>   src/master/master.hpp c32c7e9d859ef73216354e2c03ecc07d0009b12f 
>   src/master/master.cpp b2a19a645528e8fc1fd48f5ac9929d38c9a76b49 
>   src/master/validation.hpp 3205ee1e699d0fb7ca16ced96a07be3a07cf8956 
>   src/master/validation.cpp 15e2ecc2de99d6bed522f45f855ba686bf19c008 
> 
> Diff: https://reviews.apache.org/r/45961/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51553: Changed the way `HAP::updateAllocation()` calls `Resources.apply()`.

[Jiang Yan Xu]

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

(Updated Sept. 7, 2016, 10:16 a.m.)


Review request for mesos and Anindya Sinha.


Changes
---

Comments.


Repository: mesos


Description
---

This is needed for supporting LAUNCH operations with shared resources
(More details in https://reviews.apache.org/r/45961/) but this patch
itself has no functional change.

Also, this patch together with https://reviews.apache.org/r/51412/
results in no change in the granularity in which Offer operations are
applied. Each single operation in the ACCEPT call is still individually
applied to Resources objects managed by the master, the allocator and
the sorters, same as before.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
9e5db2196c6a541dc1208ba8b9f13ef9a518bcc4 

Diff: https://reviews.apache.org/r/51553/diff/


Testing
---

make check


Thanks,

Jiang Yan Xu

Re: Review Request 51697: Renamed a capabilities function.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 7, 2016, 4:43 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51697/
> ---
> 
> (Updated Sept. 7, 2016, 4:43 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed a capabilities function.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.hpp a89f639cba626df6ffe5ea8cfccf0b7f52492060 
>   src/linux/capabilities.cpp e090fc53863605fd9094ecbe39637edb977ab3c4 
>   src/tests/containerizer/capabilities_test_helper.cpp 
> f982dbd92a08e3fd2af48e90d98dbbd7e28ca583 
> 
> Diff: https://reviews.apache.org/r/51697/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51052: Make mesos-docker-execute understand cgroups_enable_cfs.

[Zhitao Li]

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

(Updated Sept. 7, 2016, 4:51 p.m.)


Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.


Changes
---

Update summary and correct related Bug.


Summary (updated)
-

Make mesos-docker-execute understand cgroups_enable_cfs.


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


Repository: mesos


Description
---

This fixes cpu quota for command executor (which runs outside
of the docker container) by ensuing --cpu-quota flag to docker
run.

Note: we have to add the boolean variable to `Docker` class
because `Docker::run()` has reached the maximum argument length
GMOCK can support.


Diffs
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp e07c4aa723f57db7f8bc31d4af5d32a30ebf98f2 
  src/docker/executor.hpp 7b63d784d6b8685912598b77fb38cf6e70646ae3 
  src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
  src/slave/containerizer/docker.cpp e447c58bd46ba080529e8f349347eccf5a54110a 
  src/tests/mesos.cpp f5034f9f8de7040182e10f51be125a87b29fdd24 

Diff: https://reviews.apache.org/r/51052/diff/


Testing
---

I am now able to make docker containers launched through mesos-execute have a 
cpu quota.

Also making sure `make check` still works on mac os for the linux only flag.


Thanks,

Zhitao Li

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Benjamin Bannier]

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

(Updated Sept. 7, 2016, 6:46 p.m.)


Review request for mesos, Jay Guo and Jie Yu.


Changes
---

Addressed jieyu's review comments.


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


Repository: mesos


Description (updated)
---

This isolator evaluates agent allowed capabilities and passes net
capabilities on to `mesos-containerizer` which enforces the
capabilities.

Capability information is passed via a new field in
`ContainerLaunchInfo`.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
16dd3a19145b9764273cdb9a8899e353c98730e5 
  src/CMakeLists.txt 01ef494f7120156de3b826d7def76fb30bcc61b5 
  src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 
  src/slave/containerizer/mesos/containerizer.cpp 
89b7e8db38916d69d9b2d4fe305d4397b0859a10 
  src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
  src/tests/containerizer/isolator_tests.cpp 
f8056ca08029feed5f164d4f94e24d521183bdfc 

Diff: https://reviews.apache.org/r/50271/diff/


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 50270: Introduced linux capabilities support for mesos containerizer.

[Benjamin Bannier]

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

(Updated Sept. 7, 2016, 6:43 p.m.)


Review request for mesos, Jay Guo and Jie Yu.


Changes
---

Address jieyu's review comments.


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


Repository: mesos


Description (updated)
---

This change introduces linux capability based security for unified
containerizer. A new agent flag \`allowed_capabilities\` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.hpp 
0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
  src/slave/containerizer/mesos/launch.cpp 
13b65d82e029650e150eb2bc3647d95af167bd72 
  src/slave/flags.hpp 1a006663e7cc58ee548b3dda686cfbac0c240baa 
  src/slave/flags.cpp 0f2be1700f41b74da4ea1ce699a81ec33cf92a9a 

Diff: https://reviews.apache.org/r/50270/diff/


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Review Request 51697: Renamed a capabilities function.

[Benjamin Bannier]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Renamed a capabilities function.


Diffs
-

  src/linux/capabilities.hpp a89f639cba626df6ffe5ea8cfccf0b7f52492060 
  src/linux/capabilities.cpp e090fc53863605fd9094ecbe39637edb977ab3c4 
  src/tests/containerizer/capabilities_test_helper.cpp 
f982dbd92a08e3fd2af48e90d98dbbd7e28ca583 

Diff: https://reviews.apache.org/r/51697/diff/


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 51654: Fixed help display for mesos-containerizer subcommands.

[Benjamin Bannier]

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

(Updated Sept. 7, 2016, 6:28 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Fixed help display for mesos-containerizer subcommands.


Repository: mesos


Description
---

In this patch we add help displays for all current containerizer
subcommands. We also trim existing display of the usage; here we use
that `Flags::usage` already takes care of providing a header.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
d17a45fe17bb74cbf9ae421dc8a492e5dc5f1a00 
  src/slave/containerizer/mesos/launch.cpp 
13b65d82e029650e150eb2bc3647d95af167bd72 
  src/slave/containerizer/mesos/mount.cpp 
dbd7853a43ee1402f2f91d933a657010efdd76b0 

Diff: https://reviews.apache.org/r/51654/diff/


Testing (updated)
---

make check

Confirmed by explicitly invoking

$ mesos-containerizer launch --help
$ mesos-containerizer mount --help
$ mesos-containerizer network-cni-setup --help


Thanks,

Benjamin Bannier