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/#review148156
---



Patch looks great!

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

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. 8, 2016, 5:52 a.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> 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.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 
> 
> Diff: https://reviews.apache.org/r/51480/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51374: Change registry update order on removal, mark-unreachable.

[Neil Conway]

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

(Updated Sept. 8, 2016, 8:41 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This commit changes the master so that we always update the registry
for an important change (e.g., removing an agent or marking an agent
unreachable), before updating the important parts of the master's
in-memory state (e.g., the in-memory list of registered agents).
Previously, the registry was updated first for registration and
reregistration, but second for removal and marking unreachable.
Updating the registry first is simpler, and also avoids possibly
leaking inaccurate information (e.g., via HTTP endpoints) if the
registry operation fails.


Diffs (updated)
-

  src/master/master.hpp 4992ab0a0bb5babbf6a4fa3e6eff3577590fc879 
  src/master/master.cpp 1dcce6cd66804990af238176c61aca03bb5c9471 
  src/tests/master_tests.cpp 6cde15fcd6ca8ec40438c75aed980e83f8de9b86 
  src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 
  src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Guangya Liu]

> On 九月 6, 2016, 9:59 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2594
> > 
> >
> > Seems we should also `Clock::settle();` here, otherwise, this test may 
> > be failed as r/51027
> 
> Jacob Janco wrote:
> Since there is an `AWAIT_READY(allocation)` beforehand for the previous 2 
> calls to the allocator, I believe we should be OK here.

Sorry, I posted this in the wrong place, it should be at 
https://reviews.apache.org/r/51027/ , after https://reviews.apache.org/r/51027/ 
, the `reviveOffer` will not allocate resource directly but will be in a queue 
if no allocation pending.

BTW: I suggest you chenge the order of this patch and 
https://reviews.apache.org/r/51027/ , it is better let this patch depend on 
https://reviews.apache.org/r/51027/ .

You can run `./bin/mesos-tests.sh 
--gtest_filter="HierarchicalAllocatorTest.SuppressAndReviveOffers" 
--gtest_repeat=20` after applied https://reviews.apache.org/r/51027/ and you 
will got some error.


- Guangya


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


On 八月 23, 2016, 8:47 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51028/
> ---
> 
> (Updated 八月 23, 2016, 8:47 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Per MESOS-3157, if cluster events with the possibility
>   of triggering an allocation occur rapidly and test
>   assertions depend on gleaning information from assumed
>   order, it is likely they will fail due to lack of parity
>   between event and actual allocation. Settle the clock
>   between these events in tests sensitive to this to ensure
>   expected allocations occur.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/51028/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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

[Vinod Kone]

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



No unit tests for this?


src/common/type_utils.cpp (lines 401 - 402)


this shouldn't be serialize either. can you add a TODO for adding equality 
operator for TaskInfo as well?


- Vinod Kone


On Sept. 7, 2016, 9:53 p.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> 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
> -
> 
>   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
> 
>

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

[Jay Guo]

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



Could list test steps to verify functionalities? I tried following steps:
// start master
$ mesos-master --ip= --work_dir=
// start agent
$ sudo mesos-agent --master=: 
--work_dir= --isolation=linux/capabilities 
--allowed_capabilities='{"capabilities":["CHOWN"]}'
// mesos-execute
$ mesos-execute --master=: --name="test" 
--command="touch foo && chown nobody foo"

I'm expecting that I could `chown` with `CAP_CHOWN` however I got `operation 
not permitted`. I could be misunderstanding this completely, please advise


src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (line 62)


We need a check here in case that `flags.allowed_capabilities` is not set, 
otherwise it would fail assertion in `get()` and exit.



src/tests/containerizer/isolator_tests.cpp (line 1802)


I think we need more comprehensive tests for all the cases listed in the 
matrix in design doc. For instance, a test case for forbidden operations.


- Jay Guo


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 51477: Implemented `RunTaskGroupMessage` handler on the agent.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/slave.cpp (line 2102)


#2085 explicitly calls stringify(). lets be consistent.

also this could've been better written as

```
vector> taskIds;
foreach (const TaskGroup& taskGroup, taskGroups) {
  vector taskIds_;
  foreach (const TaskInfo& task, taskGroup.tasks()) {
taskIds_.push_back(task.task_id());
  }
  taskIds.push_back(taskIds_);
}

out << stringify(taskIds);
```



src/slave/slave.cpp (line 2168)


no need for this if statement?



src/slave/slave.cpp (line 2199)


no need for this if statement?


- Vinod Kone


On Sept. 8, 2016, 5:19 a.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> 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 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 8, 2016, 5:24 a.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> 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 51710: Minor fix to avoid creating an unnecessary temporary when looping.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 8, 2016, 5:45 a.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Minor fix to avoid creating an unnecessary temporary when looping.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/51710/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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

[Vinod Kone]

> On Sept. 7, 2016, 5:29 p.m., Vinod Kone wrote:
> >
> 
> Anand Mazumdar 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?

I think we are just doing this for whatever lines we touch. I'll leave it upto 
you.


- Vinod


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


On Sept. 7, 2016, 10:11 p.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> 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()`/`_runTask()` refactoring.
> 
> 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 51710: Minor fix to avoid creating an unnecessary temporary when looping.

[Benjamin Bannier]

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



Reading the summary I am confused what the purpose of this patch is. Is this 
about harmonizing the style of the iteration here? The current summary reads 
like this patch somehow optimizes, while in fact it removes explicit caching 
and replaces it with potentially repeated recomputations (which _might_ be 
optimized out for certain compiler settings).

- Benjamin Bannier


On Sept. 8, 2016, 7:45 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51710/
> ---
> 
> (Updated Sept. 8, 2016, 7:45 a.m.)
> 
> 
> 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 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/51710/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51683]

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. 8, 2016, 6:31 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51683/
> ---
> 
> (Updated Sept. 8, 2016, 6:31 a.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
> ---
> 
> 1) Previously if role or reservation info are invalid,
> the result is an empty Resources object, now it returns
> an error.
> 2) Added a new flatten() method to flatten resources with
> star role and empty reservation.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 87828aa5e4818efd40dd72dc199669281b8a15ae 
>   include/mesos/v1/resources.hpp 27bdd275277edc60d97b720e596f21e8e8f8dc27 
>   src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
>   src/common/resources.cpp 77ee92193de1f34131f7a86b7d16731c9c3e17e2 
>   src/examples/dynamic_reservation_framework.cpp 
> 850bb2a5b243dd5d5a8b6476570b4f943fde6185 
>   src/examples/test_framework.cpp a83766a92617d50eadd92ec55113e049a7290d03 
>   src/examples/test_http_framework.cpp 
> 441e86c88b035d9a268b8b51b95da3e1eb842a62 
>   src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 
>   src/tests/hierarchical_allocator_tests.cpp 
> b2179215f3a4c2f4018670e8e54f02e06c39c3b1 
>   src/tests/mesos.hpp 9bcdaf19833d08ccfbfe4781179f8626e141a7e1 
>   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
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="ResourcesOperationTest.FlattenResources"
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from ResourcesOperationTest
> [ RUN  ] ResourcesOperationTest.FlattenResources
> [   OK ] ResourcesOperationTest.FlattenResources (3 ms)
> [--] 1 test from ResourcesOperationTest (3 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (24 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 51720: Updated tests to properly set 'flags.launcher' with correct value.

[Kevin Klues]

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

Review request for mesos, Benjamin Hindman and Jie Yu.


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


Repository: mesos


Description
---

In some of our tests we manually create a 'PosixLauncher' rather than
relying on the value of 'flags.launcher' to decide which type of
launcher to create. Since calls to 'CreateSlaveFlags()' set
'flags.launcher' to 'linux' by default, there was a discrepency in
what the flags said, and what actual launcher type we were creating.

This commit fixes this to explicitly set 'flags.launcher' to the
appropriate type.


Diffs
-

  src/tests/container_logger_tests.cpp e8f934106510fe02b8b92be19c918a1e5c0b78fd 
  src/tests/containerizer/isolator_tests.cpp 
f8056ca08029feed5f164d4f94e24d521183bdfc 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
72346c748598e4c0787dba09d9ccb59f244b0df5 

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


Testing
---

make check


Thanks,

Kevin Klues

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

[Vinod Kone]

> On Sept. 8, 2016, 9:14 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 2112
> > 
> >
> > #2085 explicitly calls stringify(). lets be consistent.
> > 
> > also this could've been better written as
> > 
> > ```
> > vector> taskIds;
> > foreach (const TaskGroup& taskGroup, taskGroups) {
> >   vector taskIds_;
> >   foreach (const TaskInfo& task, taskGroup.tasks()) {
> > taskIds_.push_back(task.task_id());
> >   }
> >   taskIds.push_back(taskIds_);
> > }
> > 
> > out << stringify(taskIds);
> > ```

also realized that master prints task group as a set whereas this code is 
printing it as a vector! we need to be consistent.


- Vinod


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


On Sept. 8, 2016, 5:19 a.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> 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 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51720: Updated tests to properly set 'flags.launcher' with correct value.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [51720]

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

Error:
2016-09-08 10:55:44 URL:https://reviews.apache.org/r/51720/diff/raw/ 
[4309/4309] -> "51720.patch" [1]
error: patch failed: src/tests/containerizer/isolator_tests.cpp:211
error: src/tests/containerizer/isolator_tests.cpp: patch does not apply
error: patch failed: src/tests/containerizer/mesos_containerizer_tests.cpp:139
error: src/tests/containerizer/mesos_containerizer_tests.cpp: patch does not 
apply

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

- Mesos ReviewBot


On Sept. 8, 2016, 10:09 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51720/
> ---
> 
> (Updated Sept. 8, 2016, 10:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Bugs: MESOS-6141
> https://issues.apache.org/jira/browse/MESOS-6141
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In some of our tests we manually create a 'PosixLauncher' rather than
> relying on the value of 'flags.launcher' to decide which type of
> launcher to create. Since calls to 'CreateSlaveFlags()' set
> 'flags.launcher' to 'linux' by default, there was a discrepency in
> what the flags said, and what actual launcher type we were creating.
> 
> This commit fixes this to explicitly set 'flags.launcher' to the
> appropriate type.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> e8f934106510fe02b8b92be19c918a1e5c0b78fd 
>   src/tests/containerizer/isolator_tests.cpp 
> f8056ca08029feed5f164d4f94e24d521183bdfc 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 72346c748598e4c0787dba09d9ccb59f244b0df5 
> 
> Diff: https://reviews.apache.org/r/51720/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 51715: Added a parallel gtest runner.

[Benjamin Bannier]

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

Review request for mesos, Kevin Klues, Till Toenshoff, and Vinod Kone.


Repository: mesos


Description
---

This adds `b814987b28cfb65a7c9635c83399545e423e690a` of
https://github.com/bbannier/gtest-shrdr.


Diffs
-

  support/mesos-gtest-runner.py PRE-CREATION 

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


Testing
---

Tested with e.g., `stout-tests`

$ ./support/mesos-gtest-runner.py build/3rdparty/stout/stout-tests

[PASS]


Thanks,

Benjamin Bannier

Re: Review Request 51715: Added a parallel gtest runner.

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 1:15 p.m.)


Review request for mesos, Kevin Klues, Till Toenshoff, and Vinod Kone.


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


Repository: mesos


Description
---

This adds `b814987b28cfb65a7c9635c83399545e423e690a` of
https://github.com/bbannier/gtest-shrdr.


Diffs
-

  support/mesos-gtest-runner.py PRE-CREATION 

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


Testing
---

Tested with e.g., `stout-tests`

$ ./support/mesos-gtest-runner.py build/3rdparty/stout/stout-tests

[PASS]


Thanks,

Benjamin Bannier

Re: Review Request 51717: Enable Mesos test runner.

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 1:15 p.m.)


Review request for mesos and Till Toenshoff.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 

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


Testing
---

`make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)


Thanks,

Benjamin Bannier

Re: Review Request 51719: Enable Mesos test runner [stout].

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 1:15 p.m.)


Review request for mesos and Till Toenshoff.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/stout/Makefile.am fc2d2b20ec48b54b63f971c9887a492e61bb4a26 

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


Testing
---

`make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)


Thanks,

Benjamin Bannier

Re: Review Request 51716: Added configure option for Mesos test runner.

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 1:15 p.m.)


Review request for mesos and Till Toenshoff.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  configure.ac 57482d39db1f83e92e75fca959cd6df329a1c24f 

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


Testing
---

`make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)


Thanks,

Benjamin Bannier

Re: Review Request 51718: Enable Mesos test runner [libprocess].

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 1:15 p.m.)


Review request for mesos and Till Toenshoff.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/Makefile.am 020b0e1e4c49805a3c7a61b308d897fc82f5e0ce 

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


Testing
---

`make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)


Thanks,

Benjamin Bannier

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

[Vinod Kone]

> On Sept. 8, 2016, 9:45 a.m., Benjamin Bannier wrote:
> > Reading the summary I am confused what the purpose of this patch is. Is 
> > this about harmonizing the style of the iteration here? The current summary 
> > reads like this patch somehow optimizes, while in fact it removes explicit 
> > caching and replaces it with potentially repeated recomputations (which 
> > _might_ be optimized out for certain compiler settings).

just for harmonizing style.


- Vinod


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


On Sept. 8, 2016, 5:45 a.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Minor fix to avoid creating an unnecessary temporary when looping.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/51710/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51501: Exposed metrics in scheduler library.

[Vinod Kone]

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




src/tests/scheduler_tests.cpp (lines 370 - 377)


oh i didn't mean to add a comment about what these metrics "mean". i meant 
which event is this exactly (e.g., is it the heartbeat event).


- Vinod Kone


On Sept. 6, 2016, 8:53 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51501/
> ---
> 
> (Updated Sept. 6, 2016, 8:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6080
> https://issues.apache.org/jira/browse/MESOS-6080
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed metrics scheduler/event_queue_messages(size of
> message queue) and scheduler/event_queue_dispatches
> (size of dispatch queue) in scheduler library.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp 276ed10bd439c4a7830447bec5053292fb2ca4e7 
>   src/tests/scheduler_tests.cpp 931c1857f1ab454c67eece2f34c4649a426178de 
> 
> Diff: https://reviews.apache.org/r/51501/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04
> sudo GTEST_FILTER="*SchedulerTest.MetricsEndpoint*" make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51650: Removed dependency on external Docker image for mesos-tidy.

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 1:57 p.m.)


Review request for mesos and Michael Park.


Changes
---

Harmonize quote usage wrt. `TAG`.


Repository: mesos


Description
---

In order minimize external dependency modify the implementation of
`support/mesos-tidy.sh` to build the mesos-tidy Docker container on
the fly just before executing tests.

We follow the same basic idea already employed in
`support/docker_build.sh` where the image is build, executed, and
removed. Note that this approach likely does not take full advantage
of Docker's caching capabilities.


Diffs (updated)
-

  support/mesos-tidy.sh 36e7f03ba2d9aa4ed982a82d8bbac3416dd52ad7 

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


Testing
---

Confirmed that executing

$ ./support/mesos-tidy.sh

give same results with external, or ad hoc container image.


Thanks,

Benjamin Bannier

Re: Review Request 51716: Added configure option for Mesos test runner.

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 2:41 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Removed unneeded conditional.


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


Repository: mesos


Description (updated)
---

Added configure option for Mesos test runner.


Diffs (updated)
-

  configure.ac 57482d39db1f83e92e75fca959cd6df329a1c24f 

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


Testing
---

`make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)


Thanks,

Benjamin Bannier

Re: Review Request 51719: Enable Mesos test runner [stout].

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 2:42 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Enable Mesos test runner [stout].


Diffs (updated)
-

  3rdparty/stout/Makefile.am fc2d2b20ec48b54b63f971c9887a492e61bb4a26 

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


Testing
---

`make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)


Thanks,

Benjamin Bannier

Re: Review Request 51717: Enable Mesos test runner.

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 2:42 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Enable Mesos test runner.


Diffs (updated)
-

  src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 

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


Testing
---

`make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)


Thanks,

Benjamin Bannier

Re: Review Request 51719: Enable Mesos test runner [stout].

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 3:07 p.m.)


Review request for mesos and Till Toenshoff.


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


Repository: mesos


Description
---

Enable Mesos test runner [stout].


Diffs (updated)
-

  3rdparty/stout/Makefile.am fc2d2b20ec48b54b63f971c9887a492e61bb4a26 

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


Testing (updated)
---

* `make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)
* `make check MESOS_TEST_RUNNER=''` temporarily disables parallel test execution


Thanks,

Benjamin Bannier

Re: Review Request 51717: Enable Mesos test runner.

[Benjamin Bannier]

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

(Updated Sept. 8, 2016, 3:07 p.m.)


Review request for mesos and Till Toenshoff.


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


Repository: mesos


Description
---

Enable Mesos test runner.


Diffs (updated)
-

  src/Makefile.am 15b9a63822eca8d0b428191940026756fba7821e 

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


Testing (updated)
---

* `make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)
* `make check MESOS_TEST_RUNNER=''` temporarily disables parallel test execution


Thanks,

Benjamin Bannier

Re: Review Request 51501: Exposed metrics in scheduler library.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51501]

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. 6, 2016, 8:53 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51501/
> ---
> 
> (Updated Sept. 6, 2016, 8:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6080
> https://issues.apache.org/jira/browse/MESOS-6080
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed metrics scheduler/event_queue_messages(size of
> message queue) and scheduler/event_queue_dispatches
> (size of dispatch queue) in scheduler library.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp 276ed10bd439c4a7830447bec5053292fb2ca4e7 
>   src/tests/scheduler_tests.cpp 931c1857f1ab454c67eece2f34c4649a426178de 
> 
> Diff: https://reviews.apache.org/r/51501/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04
> sudo GTEST_FILTER="*SchedulerTest.MetricsEndpoint*" make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

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

[Vinod Kone]

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




src/slave/slave.cpp (line 2260)


just realized that you need to set proper command info for `DEFAULT` 
executor here? master is not going to fill it!

consequently, you need to ensure that in `doReliableregistration()` command 
info is unset when re-registering `DEFAULT` executor.


- Vinod Kone


On Sept. 8, 2016, 5:19 a.m., Anand Mazumdar wrote:
> 
> ---
> 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.
> 
> 
> 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 2da5a2986c427250664b2bf3456039f86e5c6079 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 51720: Updated tests to properly set 'flags.launcher' with correct value.

[Kevin Klues]

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

(Updated Sept. 8, 2016, 2:24 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


Changes
---

Updated with proper patch dependency in chain.


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


Repository: mesos


Description
---

In some of our tests we manually create a 'PosixLauncher' rather than
relying on the value of 'flags.launcher' to decide which type of
launcher to create. Since calls to 'CreateSlaveFlags()' set
'flags.launcher' to 'linux' by default, there was a discrepency in
what the flags said, and what actual launcher type we were creating.

This commit fixes this to explicitly set 'flags.launcher' to the
appropriate type.


Diffs (updated)
-

  src/tests/container_logger_tests.cpp e8f934106510fe02b8b92be19c918a1e5c0b78fd 
  src/tests/containerizer/isolator_tests.cpp 
f8056ca08029feed5f164d4f94e24d521183bdfc 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
72346c748598e4c0787dba09d9ccb59f244b0df5 

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


Testing
---

make check


Thanks,

Kevin Klues

Re: Review Request 51565: Added a way to set logrotate settings per executor.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/container_loggers/lib_logrotate.cpp (line 104)


"settings" is a bit weird.

how about "overridenFlags" or "localFlags"?


- Vinod Kone


On Sept. 6, 2016, 8:13 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51565/
> ---
> 
> (Updated Sept. 6, 2016, 8:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Cody Maloney, Artem Harutyunyan, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The provided `LogrotateContainerLogger` did not have enough granularity
> when setting log rotation settings.  This patch adds a way for each
> executor to set its own log rotation settings, using the global values
> as defaults.
> 
> The executor settings are provided via environment variables in the
> `ExecutorInfo`.
> 
> 
> Diffs
> -
> 
>   docs/logging.md 3b36870c22336440b0d7bf1359e1d0a97986a0f6 
>   src/slave/container_loggers/lib_logrotate.hpp 
> f216548ef37f5c2245ef64d21e84e06100e8e5ae 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 01552752a56ee7377a631a783f2168ba0eea2799 
>   src/tests/container_logger_tests.cpp 
> e8f934106510fe02b8b92be19c918a1e5c0b78fd 
> 
> Diff: https://reviews.apache.org/r/51565/diff/
> 
> 
> Testing
> ---
> 
> Previewed documentation change via the website previewer.
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Vinod Kone]

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


Fix it, then Ship it!





src/cli/execute.cpp (lines 117 - 121)


We want to add support for running multiple containers with different 
images, not just multiple commands! For example we want to be able to run 
docker images 
"redis" and "nginx" as a group.

In other words each task could potentially have different values for name, 
shell, command, environment, resources, appc_image, docker_image, volumes.

I propose as follows:

```
--taskgroup="{..JSON string}"
```

Parse this into `TaskGroupInfo`



src/cli/execute.cpp (line 550)


use minimal resources here

0.1 cpus, 32 MB RAM and 32 MB disk.


- Vinod Kone


On Sept. 3, 2016, 8:36 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 3, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> sThis patch updates mesos-execute to use LAUNCH_GROUP
> for launching task groups. I made some assumption
> in this patch, like newly introduced `--commands`
> option in mesos-execute takes semicolon dimilited values
> which will certainly not be the case in final solution. I
> am open to suggestion what can we use as delimitter for
> `--commands`. In this patch, I kept old `--command` option
> as well and it is working as expected. Though new
> `--commands` should be able to launch single as well
> as multiple tasks and I hope in future we will stick
> to one `commands` option.
> 
> Suggestions are very welcome for further improvement.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran these commands:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --command="echo hello"
> 
> **Stuck after submitting task to agent as agent code for LAUNCH_GROUP is not 
> yet completed**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --commands="echo hello"
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Vinod Kone]

> On Sept. 8, 2016, 2:59 p.m., Vinod Kone wrote:
> >

oops didnt mean to give a ship it :)


- Vinod


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


On Sept. 3, 2016, 8:36 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 3, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> sThis patch updates mesos-execute to use LAUNCH_GROUP
> for launching task groups. I made some assumption
> in this patch, like newly introduced `--commands`
> option in mesos-execute takes semicolon dimilited values
> which will certainly not be the case in final solution. I
> am open to suggestion what can we use as delimitter for
> `--commands`. In this patch, I kept old `--command` option
> as well and it is working as expected. Though new
> `--commands` should be able to launch single as well
> as multiple tasks and I hope in future we will stick
> to one `commands` option.
> 
> Suggestions are very welcome for further improvement.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran these commands:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --command="echo hello"
> 
> **Stuck after submitting task to agent as agent code for LAUNCH_GROUP is not 
> yet completed**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --commands="echo hello"
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Abhishek Dasgupta]

> On Sept. 8, 2016, 2:59 p.m., Vinod Kone wrote:
> > src/cli/execute.cpp, lines 117-121
> > 
> >
> > We want to add support for running multiple containers with different 
> > images, not just multiple commands! For example we want to be able to run 
> > docker images 
> > "redis" and "nginx" as a group.
> > 
> > In other words each task could potentially have different values for 
> > name, shell, command, environment, resources, appc_image, docker_image, 
> > volumes.
> > 
> > I propose as follows:
> > 
> > ```
> > --taskgroup="{..JSON string}"
> > ```
> > 
> > Parse this into `TaskGroupInfo`

Ok, got it. But in this case do we need to keep old name, shell, command, 
environment, resources, appc_image, docker_image, volumes options as well?? And 
if we keep, then our logic should be if --taskgroup is present, ignore all 
other name, shell, command, environment, resources, appc_image, docker_image, 
volumes options. Just need to confirm this approach.


- Abhishek


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


On Sept. 3, 2016, 8:36 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 3, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> sThis patch updates mesos-execute to use LAUNCH_GROUP
> for launching task groups. I made some assumption
> in this patch, like newly introduced `--commands`
> option in mesos-execute takes semicolon dimilited values
> which will certainly not be the case in final solution. I
> am open to suggestion what can we use as delimitter for
> `--commands`. In this patch, I kept old `--command` option
> as well and it is working as expected. Though new
> `--commands` should be able to launch single as well
> as multiple tasks and I hope in future we will stick
> to one `commands` option.
> 
> Suggestions are very welcome for further improvement.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran these commands:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --command="echo hello"
> 
> **Stuck after submitting task to agent as agent code for LAUNCH_GROUP is not 
> yet completed**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --commands="echo hello"
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51021: Added GC of unreachable agent metadata from the registry.

[Neil Conway]

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

(Updated Sept. 8, 2016, 3:46 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Use linkedhashmap, add test for mass removal behavior.


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


Repository: mesos


Description
---

Information about agents that have been marked unreachable is stored
in the registry. Since the number of unreachable agents can grow without
bound, this commit implements a garbage collection scheme. The current
leading master will periodically examine the registry and discard
information about unreachable agents according to two criteria:
`registry_max_agent_age` and `registry_max_agent_count`. The frequency
with which the master examines the registry is controlled by a third
parameter, `registry_gc_interval`.


Diffs (updated)
-

  src/master/constants.hpp cd80dace80968a1f67a8de5b2c112fb1396e26aa 
  src/master/flags.hpp c6e85303f60387f42b5e187eaedb6a01000f948f 
  src/master/flags.cpp 19ae6c1c30a1054b64a9585f325bd0bf943af218 
  src/master/master.hpp 4992ab0a0bb5babbf6a4fa3e6eff3577590fc879 
  src/master/master.cpp 1dcce6cd66804990af238176c61aca03bb5c9471 
  src/master/registry.proto 03c896c39458b54b3966ce32c45eaa33c890ed28 
  src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 
  src/tests/registrar_tests.cpp b04fc92c66449f1094212a534899e16d5f6f01bd 

Diff: https://reviews.apache.org/r/51021/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. 8, 2016, 3:49 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Simplify test.


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 1dcce6cd66804990af238176c61aca03bb5c9471 
  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 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/
---

(Updated Sept. 8, 2016, 3:49 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Simplify tests, tweak comments.


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

  src/master/master.cpp 1dcce6cd66804990af238176c61aca03bb5c9471 
  src/tests/slave_tests.cpp 701fab20091f9f7df020290b9f0a9db1dec92647 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Vinod Kone]

> On Sept. 8, 2016, 2:59 p.m., Vinod Kone wrote:
> > src/cli/execute.cpp, lines 117-121
> > 
> >
> > We want to add support for running multiple containers with different 
> > images, not just multiple commands! For example we want to be able to run 
> > docker images 
> > "redis" and "nginx" as a group.
> > 
> > In other words each task could potentially have different values for 
> > name, shell, command, environment, resources, appc_image, docker_image, 
> > volumes.
> > 
> > I propose as follows:
> > 
> > ```
> > --taskgroup="{..JSON string}"
> > ```
> > 
> > Parse this into `TaskGroupInfo`
> 
> Abhishek Dasgupta wrote:
> Ok, got it. But in this case do we need to keep old name, shell, command, 
> environment, resources, appc_image, docker_image, volumes options as well?? 
> And if we keep, then our logic should be if --taskgroup is present, ignore 
> all other name, shell, command, environment, resources, appc_image, 
> docker_image, volumes options. Just need to confirm this approach.

yes, lets keep the other flags for backwards compatibility. and correct if 
"taskgroup" is specified others shouldn't be.


- Vinod


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


On Sept. 3, 2016, 8:36 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 3, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> sThis patch updates mesos-execute to use LAUNCH_GROUP
> for launching task groups. I made some assumption
> in this patch, like newly introduced `--commands`
> option in mesos-execute takes semicolon dimilited values
> which will certainly not be the case in final solution. I
> am open to suggestion what can we use as delimitter for
> `--commands`. In this patch, I kept old `--command` option
> as well and it is working as expected. Though new
> `--commands` should be able to launch single as well
> as multiple tasks and I hope in future we will stick
> to one `commands` option.
> 
> Suggestions are very welcome for further improvement.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran these commands:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --command="echo hello"
> 
> **Stuck after submitting task to agent as agent code for LAUNCH_GROUP is not 
> yet completed**
> mesos-execute --master=127.0.0.1:5050 --name="LetsCitar" 
> --containerizer=docker --docker_image="ubuntu" --commands="echo hello"
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51650: Removed dependency on external Docker image for mesos-tidy.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51650]

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. 8, 2016, 11:57 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51650/
> ---
> 
> (Updated Sept. 8, 2016, 11:57 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order minimize external dependency modify the implementation of
> `support/mesos-tidy.sh` to build the mesos-tidy Docker container on
> the fly just before executing tests.
> 
> We follow the same basic idea already employed in
> `support/docker_build.sh` where the image is build, executed, and
> removed. Note that this approach likely does not take full advantage
> of Docker's caching capabilities.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy.sh 36e7f03ba2d9aa4ed982a82d8bbac3416dd52ad7 
> 
> Diff: https://reviews.apache.org/r/51650/diff/
> 
> 
> Testing
> ---
> 
> Confirmed that executing
> 
> $ ./support/mesos-tidy.sh
> 
> give same results with external, or ad hoc container image.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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. 8, 2016, 6:04 p.m.)


Review request for mesos, Jay Guo and Jie Yu.


Changes
---

Addressed some of Jay's review comments.


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 (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 50271: Created an isolator for Linux capabilities.

[Benjamin Bannier]

> On Sept. 8, 2016, 11:12 a.m., Jay Guo wrote:
> > Could list test steps to verify functionalities? I tried following steps:
> > // start master
> > $ mesos-master --ip= --work_dir=
> > // start agent
> > $ sudo mesos-agent --master=: 
> > --work_dir= --isolation=linux/capabilities 
> > --allowed_capabilities='{"capabilities":["CHOWN"]}'
> > // mesos-execute
> > $ mesos-execute --master=: --name="test" 
> > --command="touch foo && chown nobody foo"
> > 
> > I'm expecting that I could `chown` with `CAP_CHOWN` however I got 
> > `operation not permitted`. I could be misunderstanding this completely, 
> > please advise

Your confusion comes from the fact that you assume this patch would allow to 
assign any capabilities to a container. The command you gave would also not 
work under plain Linux had you given an unpreviledged user `CAP_CHOWN` since 
`chown` neither makes direct use of capabilities by itself (it does not link 
against `libcap*`), nor has file capabilities. There is nothing in the current 
patch which would allow it to suddenly gain capability features. To achieve the 
effect you expected you ccould e.g., add `cap_chown` to `chown`'s file-based 
capabilities,

$ sudo /sbin/setcap cap_chown+ep `which chown`

With this addition and above agent invocation the task would succeed, while if 
you had specified an emtpy capability set (`--allowed_capabilities='{}'`) it 
would have failed, just like one would expect.


> On Sept. 8, 2016, 11:12 a.m., Jay Guo wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 1802
> > 
> >
> > I think we need more comprehensive tests for all the cases listed in 
> > the matrix in design doc. For instance, a test case for forbidden 
> > operations.

Agreed, I'll add a parameterized test capturing that matrix.


- Benjamin


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


On Sept. 8, 2016, 6:04 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Sept. 8, 2016, 6:04 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 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.
> 
> Jacob Janco wrote:
> Good catch, I'll run the benchmarks and update 
> https://reviews.apache.org/r/51028/
> 
> Jacob Janco wrote:
> I updated that review if you could take a look. Thanks!
> 
> Guangya Liu wrote:
> I did not see the update, seems you did not publish your update? :-)

Spoke too soon, I need to fix one of the tests before posting another review.


- 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 49617: Add benchmark for failover of many frameworks.

[Jiang Yan Xu]

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




src/tests/hierarchical_allocator_tests.cpp (lines 3977 - 3982)


Let's use the `Allocation` defined 
[here](https://github.com/apache/mesos/blob/9c6097f063405279efc07eec22457c2059653f07/src/tests/hierarchical_allocator_tests.cpp#L79-L83)
 for better consistency in the code. We haven't been doing this consistently 
but we can start making it more consistent. (More context in /r/50868/)



src/tests/hierarchical_allocator_tests.cpp (line 3984)


s/allocations/frameworkAllocations/

Sorry I know you were using the examples from SuppressOffer but we are just 
starting to clean up.

The name `allocations` also refers to a class member so here 
`vector allocations;` is basically shadowing it. To avoid confusion 
I think it's best to avoid variable shadowing.



src/tests/hierarchical_allocator_tests.cpp (line 4051)


IIRC your code used to suppress first but we don't *suppress* anymore so 
let's update the comment.

Plus I think it'll be more clear to have some high level comments. We are 
simulating a "failover" so let's describe how what we are doing here constitute 
steps in the failover.

i.e., here: 

```
// 1. Disconnect all frameworks to simulate a failover.
```

Below (right above `size_t allocationsCount = 5;`)

```
// 2. Frameworks reconnect in the failover. Master processes them in 
batches.
```



src/tests/hierarchical_allocator_tests.cpp (line 4065)


Move settle to above the loop.

When allocations triggered by `deactivateFramework` are running, the 
`foreach` on `allocations` doesn't work.

`recoverResources` doesn't trigger allocations (yet) so we don't need to 
settle after the loop. This may change but let's not worry about it for now.



src/tests/hierarchical_allocator_tests.cpp (line 4083)


Why suppress offers? Not all frameworks do this. I suggested some comments 
in an earlier revision:

```
This is to simulate the behavior of non-greedy frameworks: after the 
failover they immediately decline and suppress offers until further work is 
requested.
```



src/tests/hierarchical_allocator_tests.cpp (line 4085)


s/thus a separate loop/thus a separate loop to simulate it/



src/tests/hierarchical_allocator_tests.cpp (line 4101)


Indeed. Per Guangya's comment, we actually need to move this up to above 
the `recoverResources()` loop.

When allocations triggered by `activateFramework` and `suppressOffers` are 
running, the `foreach` on `allocations` doesn't work.

`recoverResources` doesn't trigger allocations (yet) so we don't need to 
settle after the loop. This may change but let's not worry about it for now.



src/tests/hierarchical_allocator_tests.cpp (line 4109)


As discussed, here let's print out the number of actual allocation runs.

Also

s/allocator/Allocator/

As Guangya suggested, we can add something like "with 5000 frameworks 
failed over and suppressed offers"


- Jiang Yan Xu


On Aug. 16, 2016, 7:26 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated Aug. 16, 2016, 7:26 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> MESOS_BENCHMARK=1 GTEST_FILTER="*BENCHMARK_Test.FrameworkFailover*" make check
> 
> Sample Output:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
> Using 1 agents and 6000 frameworks
> Added 6000 frameworks in 113410us
> Added 1 agents in 6.8398066333mins
> allocator settled after  3.286837mins
> [  

Re: Review Request 49617: Add benchmark for failover of many frameworks.

[Jiang Yan Xu]

> On Aug. 24, 2016, 2:12 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4030
> > 
> >
> > How about adding `ports` resources to the allocation here.
> > 
> > ```
> > // Each agent has a portion of it's resources allocated to a single
> > // framework. We round-robin through the frameworks when allocating.
> > Resources allocation = 
> > Resources::parse("cpus:16;mem:1024;disk:1024").get();
> > 
> > Try<::mesos::Value::Ranges> ranges = fragment(createRange(31000, 
> > 32000), 16);
> > ASSERT_SOME(ranges);
> > ASSERT_EQ(16, ranges->range_size());
> > 
> > allocation += createPorts(ranges.get());
> > ```
> 
> Jacob Janco wrote:
> I took this out to simplify the test.
> 
> Guangya Liu wrote:
> I think that we should simulate a full allocate scernario in the 
> benchmark test to include `ports` as well just like other benchmark test, as 
> we actually have some special logic to handle non-scalar resources in sorter 
> when allocate resources, comments?

What's the special logic that you were referring to? I am not aganist adding to 
ports here but once this test is committed let's do some refactor to pull 
common elements into helpers or member variables so new benchmark tests don't 
need to copy a lot of code.


- Jiang Yan


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


On Aug. 16, 2016, 7:26 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated Aug. 16, 2016, 7:26 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> MESOS_BENCHMARK=1 GTEST_FILTER="*BENCHMARK_Test.FrameworkFailover*" make check
> 
> Sample Output:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
> Using 1 agents and 6000 frameworks
> Added 6000 frameworks in 113410us
> Added 1 agents in 6.8398066333mins
> allocator settled after  3.286837mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
>  (609255 ms)[ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
> Using 2 agents and 1 frameworks
> Added 1 frameworks in 190us
> Added 2 agents in 4.752954secs
> allocator settled after  7us
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
>  (6332 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51719: Enable Mesos test runner [stout].

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51715, 51716, 51717, 51718, 51719]

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. 8, 2016, 1:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51719/
> ---
> 
> (Updated Sept. 8, 2016, 1:07 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-6140
> https://issues.apache.org/jira/browse/MESOS-6140
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enable Mesos test runner [stout].
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am fc2d2b20ec48b54b63f971c9887a492e61bb4a26 
> 
> Diff: https://reviews.apache.org/r/51719/diff/
> 
> 
> Testing
> ---
> 
> * `make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)
> * `make check MESOS_TEST_RUNNER=''` temporarily disables parallel test 
> execution
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 51736: Added fields `chain` and `excludeDevices` to `PortMapper`.

[Avinash sridharan]

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Added fields `chain` and `excludeDevices` to `PortMapper`.


Diffs
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 85547533b0b13011615b512ec8c71b7545f33324 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 51501: Exposed metrics in scheduler library.

[Abhishek Dasgupta]

> On Sept. 8, 2016, 11:48 a.m., Vinod Kone wrote:
> > src/tests/scheduler_tests.cpp, lines 373-380
> > 
> >
> > oh i didn't mean to add a comment about what these metrics "mean". i 
> > meant which event is this exactly (e.g., is it the heartbeat event).

Vinod, in this test there is no message event. The assertion is to validate if 
`scheduler/event_queue_messages` and `scheduler/event_queue_dispatch` is 
present in metrics JSON. However, in this particular test the value for 
`scheduler/event_queue_dispatch` is 1.0 and that is due to `subscribe` call. 
So, for `scheduler/event_queue_dispatch`, I can write `// Dispatch event due to 
Subscribe call.` But for `EXPECT_EQ(1u, 
metrics.values.count("scheduler/event_queue_messages"));`, there is no event in 
the current test that is generating any MessageEvent. Any suggestion what 
should we write as a comment for this? As I can find, heartbeat event is not a 
MessageEvent. Am I missing something??


- Abhishek


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


On Sept. 6, 2016, 8:53 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51501/
> ---
> 
> (Updated Sept. 6, 2016, 8:53 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6080
> https://issues.apache.org/jira/browse/MESOS-6080
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed metrics scheduler/event_queue_messages(size of
> message queue) and scheduler/event_queue_dispatches
> (size of dispatch queue) in scheduler library.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp 276ed10bd439c4a7830447bec5053292fb2ca4e7 
>   src/tests/scheduler_tests.cpp 931c1857f1ab454c67eece2f34c4649a426178de 
> 
> Diff: https://reviews.apache.org/r/51501/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04
> sudo GTEST_FILTER="*SchedulerTest.MetricsEndpoint*" make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51720: Updated tests to properly set 'flags.launcher' with correct value.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51272, 51273, 51274, 51275, 51276, 51277, 51278, 51442, 
51405, 51720]

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. 8, 2016, 2:24 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51720/
> ---
> 
> (Updated Sept. 8, 2016, 2:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Bugs: MESOS-6141
> https://issues.apache.org/jira/browse/MESOS-6141
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In some of our tests we manually create a 'PosixLauncher' rather than
> relying on the value of 'flags.launcher' to decide which type of
> launcher to create. Since calls to 'CreateSlaveFlags()' set
> 'flags.launcher' to 'linux' by default, there was a discrepency in
> what the flags said, and what actual launcher type we were creating.
> 
> This commit fixes this to explicitly set 'flags.launcher' to the
> appropriate type.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> e8f934106510fe02b8b92be19c918a1e5c0b78fd 
>   src/tests/containerizer/isolator_tests.cpp 
> f8056ca08029feed5f164d4f94e24d521183bdfc 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 72346c748598e4c0787dba09d9ccb59f244b0df5 
> 
> Diff: https://reviews.apache.org/r/51720/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51565: Added a way to set logrotate settings per executor.

[Joseph Wu]

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

(Updated Sept. 8, 2016, 1:57 p.m.)


Review request for mesos, Benjamin Mahler, Cody Maloney, Artem Harutyunyan, and 
Vinod Kone.


Changes
---

Rename a local variable.


Repository: mesos


Description
---

The provided `LogrotateContainerLogger` did not have enough granularity
when setting log rotation settings.  This patch adds a way for each
executor to set its own log rotation settings, using the global values
as defaults.

The executor settings are provided via environment variables in the
`ExecutorInfo`.


Diffs (updated)
-

  docs/logging.md 3b36870c22336440b0d7bf1359e1d0a97986a0f6 
  src/slave/container_loggers/lib_logrotate.hpp 
f216548ef37f5c2245ef64d21e84e06100e8e5ae 
  src/slave/container_loggers/lib_logrotate.cpp 
01552752a56ee7377a631a783f2168ba0eea2799 
  src/tests/container_logger_tests.cpp e8f934106510fe02b8b92be19c918a1e5c0b78fd 

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


Testing
---

Previewed documentation change via the website previewer.

make check


Thanks,

Joseph Wu

Re: Review Request 51565: Added a way to set logrotate settings per executor.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51565]

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. 8, 2016, 8:57 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51565/
> ---
> 
> (Updated Sept. 8, 2016, 8:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Cody Maloney, Artem Harutyunyan, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The provided `LogrotateContainerLogger` did not have enough granularity
> when setting log rotation settings.  This patch adds a way for each
> executor to set its own log rotation settings, using the global values
> as defaults.
> 
> The executor settings are provided via environment variables in the
> `ExecutorInfo`.
> 
> 
> Diffs
> -
> 
>   docs/logging.md 3b36870c22336440b0d7bf1359e1d0a97986a0f6 
>   src/slave/container_loggers/lib_logrotate.hpp 
> f216548ef37f5c2245ef64d21e84e06100e8e5ae 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 01552752a56ee7377a631a783f2168ba0eea2799 
>   src/tests/container_logger_tests.cpp 
> e8f934106510fe02b8b92be19c918a1e5c0b78fd 
> 
> Diff: https://reviews.apache.org/r/51565/diff/
> 
> 
> Testing
> ---
> 
> Previewed documentation change via the website previewer.
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jacob Janco]

> On Sept. 6, 2016, 9:59 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2594
> > 
> >
> > Seems we should also `Clock::settle();` here, otherwise, this test may 
> > be failed as r/51027
> 
> Jacob Janco wrote:
> Since there is an `AWAIT_READY(allocation)` beforehand for the previous 2 
> calls to the allocator, I believe we should be OK here.
> 
> Guangya Liu wrote:
> Sorry, I posted this in the wrong place, it should be at 
> https://reviews.apache.org/r/51027/ , after 
> https://reviews.apache.org/r/51027/ , the `reviveOffer` will not allocate 
> resource directly but will be in a queue if no allocation pending.
> 
> BTW: I suggest you chenge the order of this patch and 
> https://reviews.apache.org/r/51027/ , it is better let this patch depend on 
> https://reviews.apache.org/r/51027/ .
> 
> You can run `./bin/mesos-tests.sh 
> --gtest_filter="HierarchicalAllocatorTest.SuppressAndReviveOffers" 
> --gtest_repeat=20` after applied https://reviews.apache.org/r/51027/ and you 
> will got some error.

Missed this, yes this does cause flakiness in this test, caused by that 
dispatched allocate.


- Jacob


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


On Aug. 23, 2016, 8:47 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51028/
> ---
> 
> (Updated Aug. 23, 2016, 8:47 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Per MESOS-3157, if cluster events with the possibility
>   of triggering an allocation occur rapidly and test
>   assertions depend on gleaning information from assumed
>   order, it is likely they will fail due to lack of parity
>   between event and actual allocation. Settle the clock
>   between these events in tests sensitive to this to ensure
>   expected allocations occur.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/51028/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jacob Janco]

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

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


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description (updated)
---

- Per MESOS-3157, if cluster events with the possibility
  of triggering an allocation occur rapidly and test
  assertions depend on gleaning information from assumed
  order, it is likely they will fail due to lack of parity
  between event and actual allocation. Ensure that expected
  allocations occur.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
b2179215f3a4c2f4018670e8e54f02e06c39c3b1 

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


Testing
---

make check


Thanks,

Jacob Janco

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jacob Janco]

> On Sept. 6, 2016, 9:59 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2594
> > 
> >
> > Seems we should also `Clock::settle();` here, otherwise, this test may 
> > be failed as r/51027
> 
> Jacob Janco wrote:
> Since there is an `AWAIT_READY(allocation)` beforehand for the previous 2 
> calls to the allocator, I believe we should be OK here.
> 
> Guangya Liu wrote:
> Sorry, I posted this in the wrong place, it should be at 
> https://reviews.apache.org/r/51027/ , after 
> https://reviews.apache.org/r/51027/ , the `reviveOffer` will not allocate 
> resource directly but will be in a queue if no allocation pending.
> 
> BTW: I suggest you chenge the order of this patch and 
> https://reviews.apache.org/r/51027/ , it is better let this patch depend on 
> https://reviews.apache.org/r/51027/ .
> 
> You can run `./bin/mesos-tests.sh 
> --gtest_filter="HierarchicalAllocatorTest.SuppressAndReviveOffers" 
> --gtest_repeat=20` after applied https://reviews.apache.org/r/51027/ and you 
> will got some error.
> 
> Jacob Janco wrote:
> Missed this, yes this does cause flakiness in this test, caused by that 
> dispatched allocate.

There, review posted =)


- Jacob


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


On Sept. 8, 2016, 10:12 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51028/
> ---
> 
> (Updated Sept. 8, 2016, 10:12 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Per MESOS-3157, if cluster events with the possibility
>   of triggering an allocation occur rapidly and test
>   assertions depend on gleaning information from assumed
>   order, it is likely they will fail due to lack of parity
>   between event and actual allocation. Ensure that expected
>   allocations occur.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> b2179215f3a4c2f4018670e8e54f02e06c39c3b1 
> 
> Diff: https://reviews.apache.org/r/51028/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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.
> 
> Jacob Janco wrote:
> Good catch, I'll run the benchmarks and update 
> https://reviews.apache.org/r/51028/
> 
> Jacob Janco wrote:
> I updated that review if you could take a look. Thanks!
> 
> Guangya Liu wrote:
> I did not see the update, seems you did not publish your update? :-)
> 
> Jacob Janco wrote:
> Spoke too soon, I need to fix one of the tests before posting another 
> review.

Review posted!


- 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 51517: Added MOUNT or PATH disk type in logging resources.

[Jiang Yan Xu]

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




src/common/resources.cpp (line 1694)


Use a `switch()` as is done in `ostream& operator<<(ostream& stream, const 
Volume& volume)`?



src/common/resources.cpp (lines 1695 - 1697)


I don't think we need to do the validation here. It should be already done 
somewhere else and similar output operators in this file don't do this. Here 
because it's protobuf, if the object didn't go through proper validation, the 
worst case it's going to print the default (empty) values.



src/common/resources.cpp (lines 1701 - 1703)


Ditto.



src/common/resources.cpp (lines 1742 - 1744)






src/tests/resources_tests.cpp (line 834)


Not yours, but starting from here this is already incorrect: persistent 
volumes don't have host paths. This is currently not captured by validation 
IIUC, just ignored by the agent.

Anyways, it'll involve some refactoring and reordering to fix this test so 
let's do it later.



src/tests/resources_tests.cpp (line 842)


This output becomes pretty unreadable IMO with this many `:` and many can 
be optional.

To make it a little bit better for this patch, how about we use a `, ` to 
separate source and the rest?

`disk(alice)[MOUNT:/mnt1, hadoop:/hdfs:/data:rw]:1`



src/tests/resources_tests.cpp (line 844)


s/TYPE/type/



src/tests/resources_tests.cpp (line 846)


Move this line to be the last line of the prevous stanza as its cleanup?



src/tests/resources_tests.cpp (line 854)


Don't need the comment if we move `disk.mutable_disk()->clear_source();` up.



src/tests/resources_tests.cpp (line 856)


Move this line to be the last line of the prevous stanza as its cleanup?


- Jiang Yan Xu


On Aug. 30, 2016, 3:22 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51517/
> ---
> 
> (Updated Aug. 30, 2016, 3:22 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6060
> https://issues.apache.org/jira/browse/MESOS-6060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We also log the disk type (MOUNT or PATH) for each persistent disk as
> well as the root path for these disks while logging resources. Note
> that this is logged only when source is present, otherwise there is
> no additional content logged.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp a5f5902d8f7f2757e3aee35619bff5cc3a52f29b 
>   src/tests/resources_tests.cpp 4932d95f4e78cae764b89472373e13527b4354a2 
>   src/v1/resources.cpp 172217505d80d66cb7e10b3635dc273229313601 
> 
> Diff: https://reviews.apache.org/r/51517/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

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

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 7, 2016, 6:49 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51392/
> ---
> 
> (Updated Sept. 7, 2016, 6:49 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
> ---
> 
> 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 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. 8, 2016, 11:51 p.m.)


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


Changes
---

Fix comment.


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

Review Request 51752: Add event for `FRAMEWORK_REMOVED`.

[Zhitao Li]

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

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


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


Repository: mesos


Description
---

Add event for `FRAMEWORK_REMOVED`.


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/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 

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


Testing
---


Thanks,

Zhitao Li

Review Request 51753: Fixed build error for mesos agent provisioner.

[Guangya Liu]

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

Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

Fixed build error for mesos agent provisioner.


Diffs
-

  src/slave/containerizer/mesos/provisioner/paths.cpp 
68bc1b4e8252e2b1653dbab5ad58ac0b7a770bac 

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


Testing
---

make
make check


Thanks,

Guangya Liu

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/#review148289
---



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. 8, 2016, 4:04 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Sept. 8, 2016, 4:04 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 51753: Fixed build error for mesos agent provisioner.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Sept. 8, 2016, 5:04 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51753/
> ---
> 
> (Updated Sept. 8, 2016, 5:04 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed build error for mesos agent provisioner.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 68bc1b4e8252e2b1653dbab5ad58ac0b7a770bac 
> 
> Diff: https://reviews.apache.org/r/51753/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 51736: Added fields `chain` and `excludeDevices` to `PortMapper`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51736]

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. 8, 2016, 6:08 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51736/
> ---
> 
> (Updated Sept. 8, 2016, 6:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added fields `chain` and `excludeDevices` to `PortMapper`.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  85547533b0b13011615b512ec8c71b7545f33324 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
> 
> Diff: https://reviews.apache.org/r/51736/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

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

[Qian Zhang]

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




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


I think you need to put the code below this line into a helper method, and 
call the helper method in `await(subContainers).then(helper)`. Otherwise, we 
will destroy parent container and its nested containers in parallel.


- Qian Zhang


On Sept. 7, 2016, 5:53 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51674/
> ---
> 
> (Updated Sept. 7, 2016, 5:53 a.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 51752: Add event for `FRAMEWORK_REMOVED`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51700, 51752]

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. 8, 2016, 11:52 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51752/
> ---
> 
> (Updated Sept. 8, 2016, 11:52 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6101
> https://issues.apache.org/jira/browse/MESOS-6101
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add event for `FRAMEWORK_REMOVED`.
> 
> 
> 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/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
>   src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 
> 
> Diff: https://reviews.apache.org/r/51752/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 194 - 197)


I realized that it's not sufficient to just pass in top level orphan 
containers to provisioners/isolators. We also want to know about known child 
containers for both checkpointed containers and orphan containers so that 
provisioners/isolators can cleanup unknown child containers.

Consider the following case:
1) containerizer launched a child container A/B under top level container A
2) isolator prepare finishes for container A/B
3) agent crashes before launcher fork is called
4) agent recovers
5) container A is checkpointed, thus considered alive
6) however, provisioners/isolators need to cleanup for container A/B as 
it's unknown to the launcher

Therefore, I suggest we introduce a protobuf 'ContainerRecoverInfo' in 
`include/mesos/slave/containerizer.proto`:

```
message ContainerRecoverInfo {
  repeated ContainerState checkpointed_containers;
  repeated ContainerID orphan_container_ids; // Deprecated. Top level 
orphans.
  repeated COntainerID known_container_ids; // All known containers, 
including child containers.
}
```

And both Provisioner and Isolator recover interface will take this protobuf.


- Jie Yu


On Sept. 7, 2016, 6:49 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Sept. 7, 2016, 6:49 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 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 50695: Renamed agent used resources to `allocation` for benchmark test.

[Jiang Yan Xu]

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


Ship it!




Committing with minor edits.


src/tests/hierarchical_allocator_tests.cpp (line 3800)


I moved this down and changed it to 

```
Resources _allocation = allocation + reserved1 + reserved2;
```

so there's no confusing "why make a copy here" when it's defined far apart 
from the next usage.


- Jiang Yan Xu


On Sept. 7, 2016, 7:04 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50695/
> ---
> 
> (Updated Sept. 7, 2016, 7:04 p.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 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. 9, 2016, 6:46 a.m.)


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


Changes
---

Clean up unnecessary Slave in the test so future slave related test does not 
break this.


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 51752: Add event for `FRAMEWORK_REMOVED`.

[Zhitao Li]

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

(Updated Sept. 9, 2016, 6:49 a.m.)


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


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


Repository: mesos


Description
---

Add event for `FRAMEWORK_REMOVED`.


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/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 

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


Testing
---


Thanks,

Zhitao Li

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. 9, 2016, 6:54 a.m.)


Review request for mesos, Anand Mazumdar, 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 (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 51752: Add event for `FRAMEWORK_REMOVED`.

[Zhitao Li]

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

(Updated Sept. 9, 2016, 6:55 a.m.)


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


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


Repository: mesos


Description
---

Add event for `FRAMEWORK_REMOVED`.


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/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 49617: Add benchmark for failover of many frameworks.

[Guangya Liu]

> On 八月 24, 2016, 9:12 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4030
> > 
> >
> > How about adding `ports` resources to the allocation here.
> > 
> > ```
> > // Each agent has a portion of it's resources allocated to a single
> > // framework. We round-robin through the frameworks when allocating.
> > Resources allocation = 
> > Resources::parse("cpus:16;mem:1024;disk:1024").get();
> > 
> > Try<::mesos::Value::Ranges> ranges = fragment(createRange(31000, 
> > 32000), 16);
> > ASSERT_SOME(ranges);
> > ASSERT_EQ(16, ranges->range_size());
> > 
> > allocation += createPorts(ranges.get());
> > ```
> 
> Jacob Janco wrote:
> I took this out to simplify the test.
> 
> Guangya Liu wrote:
> I think that we should simulate a full allocate scernario in the 
> benchmark test to include `ports` as well just like other benchmark test, as 
> we actually have some special logic to handle non-scalar resources in sorter 
> when allocate resources, comments?
> 
> Jiang Yan Xu wrote:
> What's the special logic that you were referring to? I am not aganist 
> adding to ports here but once this test is committed let's do some refactor 
> to pull common elements into helpers or member variables so new benchmark 
> tests don't need to copy a lot of code.

+1 to add some common helpers, I was proposing adding `ports` here is mainly 
because now all benchmark test include `ports` resources and also the `sorter` 
have some logic for non-scalar resources, such as 
`createStrippedScalarQuantity` etc which will filter out the non-scalar 
resources, so adding `ports` here would simulate a real use cases here.


- Guangya


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


On 八月 17, 2016, 2:26 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated 八月 17, 2016, 2:26 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> MESOS_BENCHMARK=1 GTEST_FILTER="*BENCHMARK_Test.FrameworkFailover*" make check
> 
> Sample Output:
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
> Using 1 agents and 6000 frameworks
> Added 6000 frameworks in 113410us
> Added 1 agents in 6.8398066333mins
> allocator settled after  3.286837mins
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/23
>  (609255 ms)[ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
> Using 2 agents and 1 frameworks
> Added 1 frameworks in 190us
> Added 2 agents in 4.752954secs
> allocator settled after  7us
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/24
>  (6332 ms)
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>