Re: Review Request 51320: Implemented the LaunchGroup Offer::Operation in the master.

[Qian Zhang]

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




include/mesos/mesos.proto (lines 1539 - 1542)


I think here we should follow alphabet order, so it should be:
```
REASON_TASK_GROUP_INVALID = 25;
REASON_TASK_GROUP_UNAUTHORIZED = 26;
REASON_TASK_INVALID = 14;
REASON_TASK_UNAUTHORIZED = 15;
```



include/mesos/v1/mesos.proto (lines 1538 - 1541)


Ditto.


- Qian Zhang


On Aug. 23, 2016, 1:11 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51320/
> ---
> 
> (Updated Aug. 23, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6045
> https://issues.apache.org/jira/browse/MESOS-6045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This operation is all-or-nothing, in that all tasks must be
> launched together. If the operation fails, all tasks will
> fail with TASK_ERROR and the appropriate GROUP reason.
> If a task was killed before delivery to the executor, all
> tasks are killed.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 53b6547281b23ce9f47c9f1a418b60667fb4f602 
>   include/mesos/v1/mesos.proto f6b59e156c92a26dd63b11bf403bdba677b9644b 
>   src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
>   src/messages/messages.proto 7b5e24fb1e9baf09ce024daeca90745f380d4c2f 
> 
> Diff: https://reviews.apache.org/r/51320/diff/
> 
> 
> Testing
> ---
> 
> Added a test in the subsequent patch.
> 
> More tests will be added for all-or-nothing validation / authorization paths.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Guangya Liu]

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



@Jacob, can you please add more test cases in `Testing Done` section? Such as 
some comparision as my above append.

Before your patch:
```
Test result and data
```

After your patch:
```
Test result and data
```

So that people can know how much improvment does your patch made for allocator.

- Guangya Liu


On 八月 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 八月 23, 2016, 8:49 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.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> 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.

[Guangya Liu]

> On 八月 18, 2016, 2:31 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 4109
> > 
> >
> > What about update the message as
> > 
> > ```
> > cout << "allocator settled after " << watch.elapsed()
> >  << " with activate/suppress " << addFrameworkCount << endl;
> > ```

Lost one word in above comment:

```
cout << "allocator settled after " << watch.elapsed()
 << " to activate/suppress " << addFrameworkCount
 << " frameworks" << endl;
```


- Guangya


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


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
> 
>

Re: Review Request 50836: Made add/subtract resource object as private method.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50836]

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 Aug. 23, 2016, 10:19 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50836/
> ---
> 
> (Updated Aug. 23, 2016, 10:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not want people call add/subtract resoure object directly,
> so should make those methods as private.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp bdbe8ea03a50cd1361640bde13a2342909723fc5 
>   include/mesos/v1/resources.hpp c05cb634c7a5add78da00cb84fc75d3472a341bc 
>   src/common/resources.cpp 96b4c39507bdb9b88aca5c2178b88057a5fc1881 
>   src/v1/resources.cpp 3cc7580d5567370530c53759713be05c369bf298 
> 
> Diff: https://reviews.apache.org/r/50836/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --gtest_filter="ResourcesTest.*"
> ./bin/mesos-tests.sh --gtest_filter="SharedResourcesTest.*"
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Guangya Liu]

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




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());
```



src/tests/hierarchical_allocator_tests.cpp (line 4035)


Add `ports` resource here for each agent.



src/tests/hierarchical_allocator_tests.cpp (lines 4091 - 4099)


Can we make sure all of the `allocations` are available when 
`recoverResources`? Do we need a `Clock::settle();` after all `suppressOffers` 
finshed in line 4090?


- Guangya Liu


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
> 
>

Re: Review Request 45081: Excluded reserved resources when got nonRevocable resources in stage 1.

[Guangya Liu]

> On 八月 11, 2016, 10:16 a.m., Michael Park wrote:
> > Hi Klaus, could you explain what the motivation is for this patch?
> > Currently, your analysis seems correct that reserved resources are always 
> > non-revocable.
> > However, the current code seems that it'll be more future-proof.
> > That is, even after reserved resources becomes revocable it would remain 
> > correct.
> > 
> > Anyway, I'm curiuos as to why this patch is being suggested. Thanks!
> 
> Klaus Ma wrote:
> Try to improve the performance by avoid unnecessary operation :).
> 
> Michael Park wrote:
> That would've been my guess. Are there any numbers to support the patch?
> 
> Klaus Ma wrote:
> The number dependent on cases; anyway, I'll append some number for it.

I think that this will not impact performance much as we always need two 
resources operations here: `nonRevocable()` and `+` , the time consumed in 
those two calls should be same even with this fix.


- Guangya


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


On 四月 19, 2016, 4:01 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45081/
> ---
> 
> (Updated 四月 19, 2016, 4:01 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-4988
> https://issues.apache.org/jira/browse/MESOS-4988
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocator will only allocate non-revocable resources to satify quota. As the 
> reserved resources can not be revocable, it's not necessary to call 
> `nonRevocable()` for reserved resources.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
> 
> Diff: https://reviews.apache.org/r/45081/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 50705: Changed master to allow partitioned slaves to reregister.

[Neil Conway]

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




src/master/master.cpp (lines 4894 - 4896)






src/master/master.cpp (lines 5467 - 5468)





- Neil Conway


On Aug. 13, 2016, 11:56 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50705/
> ---
> 
> (Updated Aug. 13, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4049
> https://issues.apache.org/jira/browse/MESOS-4049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous behavior was to shutdown partitioned agents that attempt to
> reregister---unless the master has failed over, in which case the
> reregistration is allowed (when running in "non-strict" mode).
> 
> The new behavior is always to allow partitioned agents to reregister.
> This is part of a longer-term project to allow frameworks to define
> their own policies for handling tasks running on partitioned agents.
> 
> In particular, if a framework has the PARTITION_AWARE capability, any
> tasks running on the partitioned agent will continue to run after
> reregistration. If the framework is not PARTITION_AWARE, any tasks that
> were running on such an agent will be killed after the agent reregisters
> (unless the master has failed over). This is for backward compatibility
> with the previous ("non-strict") behavior. Note that regardless of the
> PARTITION_AWARE capability, the agent will not be shutdown, which is a
> change from the previous Mesos behavior.
> 
> This commit also changes the master so that if an agent is removed and
> then the master receives a message from that agent, the master will no
> longer attempt to shutdown the agent. This is consistent with the goal
> of getting the master out of the business of shutting down agents that
> we suspect are unhealthy. Such an agent will eventually realize it is
> not registered with the master (e.g., because it won't receive any pings
> from the master), which will cause it to reregister.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
>   src/master/master.cpp 0bd1a3490a86fede86a3f5f62ce4745b65aae258 
>   src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 
>   src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 
> 
> Diff: https://reviews.apache.org/r/50705/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50705: Changed master to allow partitioned slaves to reregister.

[Neil Conway]

> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1760
> > 
> >
> > do you want to remove the code related to 
> > "flags.registry_strict == true" (in a separate review)?

Yep, will be done as part of https://issues.apache.org/jira/browse/MESOS-5951 , 
but I'm inclined to do that once the main functional changes are landed.


> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5500
> > 
> >
> > s/unreachableCause/message/

I don't think `message` is a great name, because it isn't clear what "message" 
refers to (is it a message from the slave? A message we want to send to 
frameworks? etc.) `unreachableCause` is more verbose but I think it is clearer.


> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5501
> > 
> >
> > s/registrarResult/future/

Similarly to above, I don't think `future` is a great name, because it isn't 
clear what value `future` corresponds to. I chose `registrarResult` because the 
variable holds the result of the registrar operation.


> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/tests/partition_tests.cpp, lines 581-583
> > 
> >
> > what does ping timeouts have to do with slave re-registering?

The slave will try to reregister when it hasn't seen a ping from the master in 
a suitable length of time (`Slave::pingTimeout`).


> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/tests/partition_tests.cpp, line 539
> > 
> >
> > what's the guarantee that the slave will not try to re-register again?

The clock is paused, so AFAIK the agent won't have any reason to spontaneously 
reregister. (We rely on this assumption in most of the other tests that 
partition agents by dropping PONGs, I believe...).


> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/tests/partition_tests.cpp, line 169
> > 
> >
> > This test is huge. Can you split PARTITION_AWARE and 
> > non-PARTITION_AWARE into separate tests?

It's only ~200 lines of code. I think there is some value in keeping the 
PARTITION_AWARE and non-PARTITION_AWARE cases in the same test: in most 
situations, both cases should behave the same, so we avoid redundancy; it also 
makes the situations where different behavior is expected more clear. It would 
be easy for the two cases to get out of sync if they are written separately.


> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 5521-5522
> > 
> >
> > Is this for backwards compatibility? If so, can you add a NOTE.
> > 
> > Will there be new metrics for unreachability?

The current implementation still increments the `slave_removals` metrics when a 
slave is marked unreachable (similarly to how the non-strict behavior will 
increment the `slave_removals` metric when a slave is lost but later the slave 
might be allowed to reregister). We can rename the metrics or add new ones, but 
I'd prefer to do that later, in a separate patch.

I added a `TODO` here for now.


> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 4932-4934
> > 
> >
> > If the slave was removed, `slave->tasks` should be empty?

No. In the current implementation, an unreachable slave appears in 
`slaves.removed` (because it has been "removed" from the list of admitted 
agents). Importantly, we need to check-for and remove tasks here: if an agent 
was marked unreachable but then it reregisters, any non-partition-aware tasks 
running on the agent will be shutdown and should be removed from the master's 
in-memory state.


- Neil


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


On Aug. 13, 2016, 11:56 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50705/
> ---
> 
> (Updated Aug. 13, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4049
> https://issues.apache.org/jira/browse/MESOS-4049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous behavior was to shutdown partitioned 

Re: Review Request 51275: Factored out a cgroups::isolate function.

[Qian Zhang]

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


Fix it, then Ship it!





src/linux/cgroups.hpp (line 323)


s/Some/Nothing/


- Qian Zhang


On Aug. 22, 2016, 12:54 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51275/
> ---
> 
> (Updated Aug. 22, 2016, 12:54 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleans up code in LinuxLauncher. Adds a useful function for others to
> use other places if necessary.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 047d3ea596660e704447c0f0c7b914a43c4a2187 
>   src/linux/cgroups.cpp f3232c009d04bb82701c0407b12abf999ab60a73 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 7377316776646e3d660086da3c0d5b494ce8ace4 
> 
> Diff: https://reviews.apache.org/r/51275/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 50705: Changed master to allow partitioned slaves to reregister.

[Neil Conway]

> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 5466-5467
> > 
> >
> > so you put a slave in `removed` here and `markingunreachable` !? 
> > previously the slave was only in one of the lists (`registered`, `removed` 
> > etc), but not anymore?

`markingUnreachable` is a transient collection; at present we just need it for 
`CHECK`s.

Right now, `removed` includes both unreachable agents and agents that have 
gracefully shutdown. I think we should clean that up (e.g., get rid of 
`removed` entirely if possible and add a separate `unreachable` map), but I'd 
prefer to handle that in a later review, since the GC work will introduce an 
`unreachable` map anyway.


> On Aug. 23, 2016, 12:56 a.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1700
> > 
> >
> > Should this still be called `removed` ?

See discussion elsewhere about `removed` containing both "shutdown" and 
"unreachable" agents.


- Neil


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


On Aug. 13, 2016, 11:56 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50705/
> ---
> 
> (Updated Aug. 13, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4049
> https://issues.apache.org/jira/browse/MESOS-4049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous behavior was to shutdown partitioned agents that attempt to
> reregister---unless the master has failed over, in which case the
> reregistration is allowed (when running in "non-strict" mode).
> 
> The new behavior is always to allow partitioned agents to reregister.
> This is part of a longer-term project to allow frameworks to define
> their own policies for handling tasks running on partitioned agents.
> 
> In particular, if a framework has the PARTITION_AWARE capability, any
> tasks running on the partitioned agent will continue to run after
> reregistration. If the framework is not PARTITION_AWARE, any tasks that
> were running on such an agent will be killed after the agent reregisters
> (unless the master has failed over). This is for backward compatibility
> with the previous ("non-strict") behavior. Note that regardless of the
> PARTITION_AWARE capability, the agent will not be shutdown, which is a
> change from the previous Mesos behavior.
> 
> This commit also changes the master so that if an agent is removed and
> then the master receives a message from that agent, the master will no
> longer attempt to shutdown the agent. This is consistent with the goal
> of getting the master out of the business of shutting down agents that
> we suspect are unhealthy. Such an agent will eventually realize it is
> not registered with the master (e.g., because it won't receive any pings
> from the master), which will cause it to reregister.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
>   src/master/master.cpp 0bd1a3490a86fede86a3f5f62ce4745b65aae258 
>   src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 
>   src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 
> 
> Diff: https://reviews.apache.org/r/50705/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Alexander Rukletsov]

> On Aug. 23, 2016, 9:26 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1194-1195
> > 
> >
> > `delay` already contains `dispatch`, so under "synchronously" you 
> > actually mean double dispatch.
> 
> Jiang Yan Xu wrote:
> I originally suggested putting this comment inside `batch()` and directly 
> above `allocate()` so it's more clear what `synchronously` applies to: 
> calling `allocate()` without dispatching.
> 
> *delay* in this sentence doesn't mean the `delay()` call but in the 
> literal sense. To make it more clear, how about we say:
> 
> ```
> // We run the allocation synchronously here instead of dispatching it 
> **again** so
> // a batched allocation **doesn't lag behind further** if the allocator 
> is backed up.
> ```
> 
> Note the asterisks are just to emphasize the changes.

This reads better!


> On Aug. 23, 2016, 9:26 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 276-279
> > 
> >
> > Probably extract this snippet in a function, e.g. 
> > `conditionalAllocate()`?
> 
> Jiang Yan Xu wrote:
> I thought about it but was struggling to find a short and clear method 
> name. So to describe the function in a full sentense it's "dispatch an 
> allocate() call if the condition `!allocationPending` is met". I think 
> `conditionalAllocate()` is OK but not great, it's not clear what the 
> condition is and not clear about the dispatch. I agree it's worth doing if we 
> can abstract this out without needing to explain what each line of a 4-line 
> method is doing in the comment...

Let's focus on _why_ instead of _how_. You want to ensure that one and only one 
event allocation happens after the event. Dispatch+flag is _how_ you achieve 
this, which may change in future. Here are some suggestions:
- `ensureAllocate`
- `ensureAllocation`
- `allocateOnce`

Side note: "unique dispatch", i.e. a dispatch that succeeds only if there are 
no identical messages in the actor's mailbox, looks like something we may need 
in other places. Maybe we should consider adding such functionality into 
libprocess.


- Alexander


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


On Aug. 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 8:49 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.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> 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

Re: Review Request 51352: Make docker executor unversioned.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51351, 51352]

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 Aug. 23, 2016, 11:32 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51352/
> ---
> 
> (Updated Aug. 23, 2016, 11:32 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Qian Zhang, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5227
> https://issues.apache.org/jira/browse/MESOS-5227
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix updates the docker executor so that it is unversioned
> (instead of v1).
> 
> This fix is part of MESOS-5227.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
> 
> Diff: https://reviews.apache.org/r/51352/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 51321: Added a test to ensure the master handles launching task groups.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51316, 51317, 51318, 51319, 51320, 51321]

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 Aug. 23, 2016, 5:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51321/
> ---
> 
> (Updated Aug. 23, 2016, 5:11 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6045
> https://issues.apache.org/jira/browse/MESOS-6045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For now this test ensures the message is sent to the agent
> in the successful launch path. More tests will be added to
> test the all-or-nothing paths (killed, invalid, unauthorized).
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp ccd0f2f602a7a1a9a44b68fd0f59bdc8e0fa58b1 
> 
> Diff: https://reviews.apache.org/r/51321/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 51266: Unshared the mount namespace when launching mesos-containerizer.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/launcher/posix/executor.cpp (lines 97 - 98)


Let's rephrase a bit. How about:
Ensure that mount namespace of the executor is not affected by changes in 
its task's namespace, e.g. pivot_root called as part of the task setup in 
mesos-containerizer binary.


- Alexander Rukletsov


On Aug. 21, 2016, 6:42 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51266/
> ---
> 
> (Updated Aug. 21, 2016, 6:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón 
> Kleiman, Gilbert Song, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When launching mesos-containerizer in the mesos-executor, we need to
> ensure mesos-executor unshare the mount namespace with
> mesos-containerizer. Otherwise, the mount and pivot_root operations in
> mesos-containerizer would affect the running context of
> mesos-executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/posix/executor.cpp 43573cacee4e681d4327a7ed7c43b4ee263aa175 
> 
> Diff: https://reviews.apache.org/r/51266/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49555: Updated mesos-docker-executor to use health check via library way.

[Alexander Rukletsov]

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


Ship it!




Thanks a lot for doing manual testing, Haosdent!


src/docker/executor.cpp (line 87)


We don't need this here, but I'm fine leaving it, because most probably 
we'll have to pass it to the `HealthChecker` library in order to locate a 
wrapper for TCP health checks.


- Alexander Rukletsov


On Aug. 21, 2016, 6:42 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49555/
> ---
> 
> (Updated Aug. 21, 2016, 6:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón 
> Kleiman, Gilbert Song, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-5954
> https://issues.apache.org/jira/browse/MESOS-5954
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We change the mesos-executor to use health check via library way in
> https://reviews.apache.org/r/49389/. To keep consistent, we replace
> health check in mesos-docker-executor from binary way to library way.
> In this patch, we rename `healthCheckDir` to `launcherDir` as well to
> keep consistent with mesos-executor.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
> 
> Diff: https://reviews.apache.org/r/49555/diff/
> 
> 
> Testing
> ---
> 
> Test with Marathon locally.
> 
> # 1. Success case.
> 
> I start `sleep 200` with the Docker container and use `ls /mnt/mesos/sandbox` 
> as the health check command.
> 
> According the qurey result of `http://localhost:8080/v2/apps/test-sleep`, 
> could see Marathon receive healthy status.
> 
> ```
> # truncate unnecessary messages
> {
>   "app": {
> "id": "/test-health",
> "cmd": "sleep 200",
> "container": {
>   "type": "DOCKER",
>   "volumes": [],
>   "docker": {
> "image": "ubuntu",
> "network": "HOST"
>   }
> },
> "healthChecks": [
>   {
> "protocol": "COMMAND",
> "command": {
>   "value": "ls /mnt/mesos/sandbox"
> },
> "gracePeriodSeconds": 300,
> "intervalSeconds": 60,
> "timeoutSeconds": 20,
> "maxConsecutiveFailures": 3
>   }
> ],
> "tasksStaged": 0,
> "tasksRunning": 1,
> "tasksHealthy": 1,
> "tasksUnhealthy": 0,
> "tasks": [
>   {
> "id": "test-health.8d4ba884-585c-11e6-9fb5-0242e663b1f5",
> "slaveId": "ec5359a2-9c63-4c2e-86da-3ed8ef94800b-S0",
> "host": "127.0.0.1",
> "startedAt": "2016-08-02T02:55:23.433Z",
> "stagedAt": "2016-08-02T02:55:21.217Z",
> "version": "2016-08-02T02:48:31.054Z",
> "ipAddresses": [
>   {
> "ipAddress": "127.0.0.1",
> "protocol": "IPv4"
>   }
> ],
> "appId": "/test-health",
> "healthCheckResults": [
>   {
> "alive": true,
> "consecutiveFailures": 0,
> "firstSuccess": "2016-08-02T02:55:23.652Z",
> "lastFailure": null,
> "lastSuccess": "2016-08-02T02:55:23.652Z",
> "lastFailureCause": null,
> "taskId": "test-health.8d4ba884-585c-11e6-9fb5-0242e663b1f5"
>   }
> ]
>   }
> ]
>   }
> }
> ```
> 
> # 2. Failed case.
> 
> Then I start a similar application which use `false` as health check command, 
> could see Marathon receive unhealthy status.
> 
> ```
> # truncate unnecessary messages
> {
>   "app": {
> "id": "/test-unhealth",
> "cmd": "sleep 200",
> "container": {
>   "type": "DOCKER",
>   "volumes": [],
>   "docker": {
> "image": "ubuntu",
> "network": "HOST"
>   }
> },
> "healthChecks": [
>   {
> "protocol": "COMMAND",
> "command": {
>   "value": "false"
> },
> "gracePeriodSeconds": 5,
> "intervalSeconds": 3,
> "timeoutSeconds": 20,
> "maxConsecutiveFailures": 3
>   }
> ],
> "tasksStaged": 0,
> "tasksRunning": 1,
> "tasksHealthy": 0,
> "tasksUnhealthy": 1,
> "tasks": [
>   {
> "id": "test-unhealth.48a6318a-585e-11e6-9fb5-0242e663b1f5",
> "slaveId": "ec5359a2-9c63-4c2e-86da-3ed8ef94800b-S0",
> "host": "127.0.0.1",
> "startedAt": "2016-08-02T03:07:47.310Z",
> "stagedAt": "2016-08-02T03:07:45.041Z",
> "ipAddresses": [
>   {
> "ipAddress": "127.0.0.1",
> "protocol": "IPv4"
>   }
> ],
> "appId": "/test-unhealth",
> "healthCheckResults": [
>   {
> "alive": false,
> 

Review Request 51367: Added missing return statements.

[Benjamin Bannier]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/linux/capabilities.cpp bbd1bb34ea8af1214d31d9fc8c0f50ab6e4c9564 

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


Testing
---

make check (Debian Jessie, clang trunk w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 51367: Added missing return statements.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On Aug. 24, 2016, 2:12 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51367/
> ---
> 
> (Updated Aug. 24, 2016, 2:12 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.cpp bbd1bb34ea8af1214d31d9fc8c0f50ab6e4c9564 
> 
> Diff: https://reviews.apache.org/r/51367/diff/
> 
> 
> Testing
> ---
> 
> make check (Debian Jessie, clang trunk w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51367: Added missing return statements.

[Till Toenshoff]

> On Aug. 24, 2016, 2:13 p.m., Till Toenshoff wrote:
> > Ship It!

I am unsure if the variable indentation of the case statements matches our 
style but for consitency in this specific implementation I am fine with it and 
in the end, it is well readable for me.


- Till


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


On Aug. 24, 2016, 2:12 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51367/
> ---
> 
> (Updated Aug. 24, 2016, 2:12 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.cpp bbd1bb34ea8af1214d31d9fc8c0f50ab6e4c9564 
> 
> Diff: https://reviews.apache.org/r/51367/diff/
> 
> 
> Testing
> ---
> 
> make check (Debian Jessie, clang trunk w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51323, 51343, 51358, 51359]

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 Aug. 24, 2016, 2:21 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51359/
> ---
> 
> (Updated Aug. 24, 2016, 2:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for provisioner helper findContainerDir.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8dc4175c60e4a9776ddb8ad21774fa4b30c28d00 
>   src/tests/containerizer/provisioner_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51359/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 50701: Added registrar operations for marking agents (un-)reachable.

[Neil Conway]

> On Aug. 22, 2016, 7:39 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1951
> > 
> >
> > since reachable agents can be marked as reachable again, why not allow 
> > unreachable agents to be marked as unreachable again?

I felt like this was a useful sanity check: this shouldn't be possible given 
the current coding (as far as I know), so it seems better to report an error in 
this situation.


> On Aug. 22, 2016, 7:39 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1976
> > 
> >
> > looks like only 'id' is ever used.
> > 
> > s/SlaveInfo info/SlaveID id/
> > 
> > if you want to keep SlaveInfo for consistency with other operations, 
> > that's fine as well.

I'm inclined to keep using `SlaveInfo` for consistency with the other 
operations.


> On Aug. 22, 2016, 7:39 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 2000
> > 
> >
> > s/:/;/ ?

I think a colon is fine here.


- Neil


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


On Aug. 12, 2016, 12:22 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50701/
> ---
> 
> (Updated Aug. 12, 2016, 12:22 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4049
> https://issues.apache.org/jira/browse/MESOS-4049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added registrar operations for marking agents (un-)reachable.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
>   src/tests/registrar_tests.cpp 9a71d8fd0c8d8e662a5e364015d144396a0b1a4c 
> 
> Diff: https://reviews.apache.org/r/50701/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50701: Added registrar operations for marking agents (un-)reachable.

[Neil Conway]

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

(Updated Aug. 24, 2016, 3:34 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

Added registrar operations for marking agents (un-)reachable.


Diffs (updated)
-

  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/tests/registrar_tests.cpp 9a71d8fd0c8d8e662a5e364015d144396a0b1a4c 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 50702: Renamed metrics from "slave_shutdowns" to "slave_unreachable".

[Neil Conway]

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

(Updated Aug. 24, 2016, 3:36 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

The master will shortly be changed to no longer shutdown unhealthy
agents, so the previous metric name is no longer accurate. The old
metric names have been kept for backwards compatibility, but they
are no longer updated (i.e., they will always be set to zero).


Diffs (updated)
-

  CHANGELOG ffeaf100c5caf2cefdd07751551ba06b0c09fed5 
  docs/monitoring.md e19ecd0ad23538da288577da0237d067459aa7ba 
  src/master/master.cpp 910293ad16a4eab5ac1e0815406f42dda90c4896 
  src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 
  src/master/metrics.cpp 88a752dc2b4b73ccb919e99478b9ea2bd83842a0 
  src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 
  src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 50705: Changed master to allow partitioned slaves to reregister.

[Neil Conway]

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

(Updated Aug. 24, 2016, 3:37 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

The previous behavior was to shutdown partitioned agents that attempt to
reregister---unless the master has failed over, in which case the
reregistration is allowed (when running in "non-strict" mode).

The new behavior is always to allow partitioned agents to reregister.
This is part of a longer-term project to allow frameworks to define
their own policies for handling tasks running on partitioned agents.

In particular, if a framework has the PARTITION_AWARE capability, any
tasks running on the partitioned agent will continue to run after
reregistration. If the framework is not PARTITION_AWARE, any tasks that
were running on such an agent will be killed after the agent reregisters
(unless the master has failed over). This is for backward compatibility
with the previous ("non-strict") behavior. Note that regardless of the
PARTITION_AWARE capability, the agent will not be shutdown, which is a
change from the previous Mesos behavior.

This commit also changes the master so that if an agent is removed and
then the master receives a message from that agent, the master will no
longer attempt to shutdown the agent. This is consistent with the goal
of getting the master out of the business of shutting down agents that
we suspect are unhealthy. Such an agent will eventually realize it is
not registered with the master (e.g., because it won't receive any pings
from the master), which will cause it to reregister.


Diffs (updated)
-

  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/master/master.cpp 910293ad16a4eab5ac1e0815406f42dda90c4896 
  src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 
  src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 

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


Testing
---

make check


Thanks,

Neil Conway

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

[Neil Conway]

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

(Updated Aug. 24, 2016, 3:41 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

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

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

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


Diffs (updated)
-

  include/mesos/mesos.proto 53b6547281b23ce9f47c9f1a418b60667fb4f602 
  include/mesos/v1/mesos.proto f6b59e156c92a26dd63b11bf403bdba677b9644b 
  src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
  src/common/protobuf_utils.cpp 8c4a7264080385789157703b05b42716fb0563b3 
  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/master/master.cpp 910293ad16a4eab5ac1e0815406f42dda90c4896 
  src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 
  src/tests/reconciliation_tests.cpp 8e438bfbce508a074f0d54513cd752344238e3f2 
  src/tests/registrar_tests.cpp 9a71d8fd0c8d8e662a5e364015d144396a0b1a4c 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 50702: Renamed metrics from "slave_shutdowns" to "slave_unreachable".

[Neil Conway]

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

(Updated Aug. 24, 2016, 3:42 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak CHANGELOG text.


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


Repository: mesos


Description
---

The master will shortly be changed to no longer shutdown unhealthy
agents, so the previous metric name is no longer accurate. The old
metric names have been kept for backwards compatibility, but they
are no longer updated (i.e., they will always be set to zero).


Diffs (updated)
-

  CHANGELOG ffeaf100c5caf2cefdd07751551ba06b0c09fed5 
  docs/monitoring.md e19ecd0ad23538da288577da0237d067459aa7ba 
  src/master/master.cpp 910293ad16a4eab5ac1e0815406f42dda90c4896 
  src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 
  src/master/metrics.cpp 88a752dc2b4b73ccb919e99478b9ea2bd83842a0 
  src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 
  src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 51020: Clarified a log message.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Clarified a log message.


Diffs
-

  src/master/master.cpp 910293ad16a4eab5ac1e0815406f42dda90c4896 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 51372: Fixed a flaky test case.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

We previously dropped `ReregisterSlaveMessage` to prevent a slave's
reregistration attempt from succeeding. That was problematic, because a
slave first attempts to authenticate before it sends
`ReregisterSlaveMessage`. It is better to prevent reregistration by
blocking authentication entirely.


Diffs
-

  src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 51374: Modified registry update protocol on removal / reregistration.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

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


Diffs
-

  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/master/master.cpp 910293ad16a4eab5ac1e0815406f42dda90c4896 
  src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 
  src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 
  src/tests/slave_tests.cpp dcf84545354dd2ae0ab5acad3b15eca0467b9982 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 51375: Introduced MockRegistrar.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This allows test cases to inspect how the master interacts with
the registry.

This commit changes `tests::cluster::Master` to use `MockRegistrar`
exclusively, even for test cases that aren't interested in mocking
aspects of the master <-> registrar interaction. However, this should be
harmless, since by default `MockRegistrar` behaves identically to the
normal registrar.


Diffs
-

  src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 
  src/master/registrar.hpp c39dd1b5430084e51376143b5441db346e85a153 
  src/tests/cluster.hpp c6fbbf24e04c38cc9a94ce0c13db445c03e804c7 
  src/tests/cluster.cpp b04653af97d17aaa9d0d3ee872169b66cd67126b 
  src/tests/mock_registrar.hpp PRE-CREATION 
  src/tests/mock_registrar.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 51376: Adjusted existing tests to use MockRegistrar.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This means that `Master::_reregisterSlave` can be made private.


Diffs
-

  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 
  src/tests/reconciliation_tests.cpp 8e438bfbce508a074f0d54513cd752344238e3f2 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 51377: Added test for HTTP endpoints during slave removal.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

When a slave is being removed but before the registry operation to
remove it has completed, querying the HTTP endpoints at the master
should not indicate that the slave has been removed yet. This is
important because the registry update might fail; if the master
fails over, the new master will not have removed the slave.


Diffs
-

  src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 51371: Fixed a fragile test case.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

The goal of this test is to check this interleaving:

  task launch: block on authorization
  slave disconnects: removed by master
  auth succeeds: we expect the task launch to fail

However, the test neglected to ensure that slave removal was completed
before allowing authorization to succeed.


Diffs
-

  src/tests/master_authorization_tests.cpp 
92bd2a9463f861e4b479f76927dd01324765a411 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 51217: Added `os::execlp' in `stout` library.

[Daniel Pravat]

> On Aug. 22, 2016, 6:39 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/shell.hpp, lines 114-115
> > 
> >
> > I'm not sure I understand what this comment is saying.  Are you 
> > explaining the reason behind the `exit(...)`?

On Posix: 
After a process X calls execlp to execute the application Y, the application Y 
runs in the process originally created to run the application X. Therefore the 
parent of X can use PID/pipes from the invocation of X to control the 
application Y
On Windows: 
After a process X calls execlp to execute the application Y, the application Y 
start a new process Y. The process X terminates at this time (it will never 
return from execlp https://msdn.microsoft.com/en-us/library/1kxct8h0.aspx). The 
parent of X cannot use X process handle to control the process Y. With this 
change the parent of X can use the X process handle to wait for the termination 
of Y, as in POSIX.


- Daniel


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


On Aug. 19, 2016, 5:38 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51217/
> ---
> 
> (Updated Aug. 19, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `os::execlp' in `stout` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> c6e141aba0abe2c7fe5410e867f7db47d632e765 
> 
> Diff: https://reviews.apache.org/r/51217/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51028, 51027]

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 Aug. 23, 2016, 8:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 8:49 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.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> 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 49360: Supported TCP in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:46 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Supported TCP in health check.


Diffs
-

  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51266: Unshared the mount namespace when launching mesos-containerizer.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:47 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Address alexr's comment.


Repository: mesos


Description
---

When launching mesos-containerizer in the mesos-executor, we need to
ensure mesos-executor unshare the mount namespace with
mesos-containerizer. Otherwise, the mount and pivot_root operations in
mesos-containerizer would affect the running context of
mesos-executor.


Diffs (updated)
-

  src/launcher/posix/executor.cpp 43573cacee4e681d4327a7ed7c43b4ee263aa175 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49555: Updated mesos-docker-executor to use health check via library way.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:48 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

We change the mesos-executor to use health check via library way in
https://reviews.apache.org/r/49389/. To keep consistent, we replace
health check in mesos-docker-executor from binary way to library way.
In this patch, we rename `healthCheckDir` to `launcherDir` as well to
keep consistent with mesos-executor.


Diffs (updated)
-

  src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 

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


Testing
---

Test with Marathon locally.

# 1. Success case.

I start `sleep 200` with the Docker container and use `ls /mnt/mesos/sandbox` 
as the health check command.

According the qurey result of `http://localhost:8080/v2/apps/test-sleep`, could 
see Marathon receive healthy status.

```
# truncate unnecessary messages
{
  "app": {
"id": "/test-health",
"cmd": "sleep 200",
"container": {
  "type": "DOCKER",
  "volumes": [],
  "docker": {
"image": "ubuntu",
"network": "HOST"
  }
},
"healthChecks": [
  {
"protocol": "COMMAND",
"command": {
  "value": "ls /mnt/mesos/sandbox"
},
"gracePeriodSeconds": 300,
"intervalSeconds": 60,
"timeoutSeconds": 20,
"maxConsecutiveFailures": 3
  }
],
"tasksStaged": 0,
"tasksRunning": 1,
"tasksHealthy": 1,
"tasksUnhealthy": 0,
"tasks": [
  {
"id": "test-health.8d4ba884-585c-11e6-9fb5-0242e663b1f5",
"slaveId": "ec5359a2-9c63-4c2e-86da-3ed8ef94800b-S0",
"host": "127.0.0.1",
"startedAt": "2016-08-02T02:55:23.433Z",
"stagedAt": "2016-08-02T02:55:21.217Z",
"version": "2016-08-02T02:48:31.054Z",
"ipAddresses": [
  {
"ipAddress": "127.0.0.1",
"protocol": "IPv4"
  }
],
"appId": "/test-health",
"healthCheckResults": [
  {
"alive": true,
"consecutiveFailures": 0,
"firstSuccess": "2016-08-02T02:55:23.652Z",
"lastFailure": null,
"lastSuccess": "2016-08-02T02:55:23.652Z",
"lastFailureCause": null,
"taskId": "test-health.8d4ba884-585c-11e6-9fb5-0242e663b1f5"
  }
]
  }
]
  }
}
```

# 2. Failed case.

Then I start a similar application which use `false` as health check command, 
could see Marathon receive unhealthy status.

```
# truncate unnecessary messages
{
  "app": {
"id": "/test-unhealth",
"cmd": "sleep 200",
"container": {
  "type": "DOCKER",
  "volumes": [],
  "docker": {
"image": "ubuntu",
"network": "HOST"
  }
},
"healthChecks": [
  {
"protocol": "COMMAND",
"command": {
  "value": "false"
},
"gracePeriodSeconds": 5,
"intervalSeconds": 3,
"timeoutSeconds": 20,
"maxConsecutiveFailures": 3
  }
],
"tasksStaged": 0,
"tasksRunning": 1,
"tasksHealthy": 0,
"tasksUnhealthy": 1,
"tasks": [
  {
"id": "test-unhealth.48a6318a-585e-11e6-9fb5-0242e663b1f5",
"slaveId": "ec5359a2-9c63-4c2e-86da-3ed8ef94800b-S0",
"host": "127.0.0.1",
"startedAt": "2016-08-02T03:07:47.310Z",
"stagedAt": "2016-08-02T03:07:45.041Z",
"ipAddresses": [
  {
"ipAddress": "127.0.0.1",
"protocol": "IPv4"
  }
],
"appId": "/test-unhealth",
"healthCheckResults": [
  {
"alive": false,
"consecutiveFailures": 1,
"firstSuccess": null,
"lastFailure": "2016-08-02T03:07:53.800Z",
"lastSuccess": null,
"lastFailureCause": "",
"taskId": "test-unhealth.48a6318a-585e-11e6-9fb5-0242e663b1f5"
  }
]
  }
]
  }
}
```


Thanks,

haosdent huang

Re: Review Request 50657: Removed the binary way of health check in libprocess.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:48 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

The binary way of health check is unused and not necessary anymore. We
should remove it.


Diffs (updated)
-

  3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
497f6107567ef47c16a0c906238bc7dfdcf84701 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49556: Removed the binary way of HealthCheck in src.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:48 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

The binary way of health check is unused and not necessary anymore. We
should remove it.


Diffs (updated)
-

  src/CMakeLists.txt ccd9892f5327a70f001411faf3199329f12cd472 
  src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 
  src/health-check/CMakeLists.txt 69e9566a0571220e21c82156b814f61e17802352 
  src/health-check/main.cpp 5346e30d35d4c5c2c4d181eee739ac8ce690a8e1 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51069: Refactored `_commandHealthCheck` in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:49 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Repository: mesos


Description
---

* Removed blocking `Future::await` call.
* Read stdout and stderr of the health check command.
* Adjust the level of some logs.
* Adjust some minor styles.
* Change the interfaces of different health check handlers to
  `Future` to make errors handling more easier.


Diffs (updated)
-

  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 36816: Supported HTTP/HTTPS in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:50 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Supported HTTP/HTTPS in health check.


Diffs (updated)
-

  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51323: Supported provisioner provision() and destroy() to be nested aware.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 23, 2016, 8:05 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51323/
> ---
> 
> (Updated Aug. 23, 2016, 8:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the provisioner provision() and destory() methods
> to be multi-level nested aware, by simply change the helper function
> getContainerDir() to be recursive:
> 
> 1. In provisioner provision(), just need to make sure the rootfs
>returned from getContainerRootfsDir() and the backend dir
>returned from getBackendDir() are correctly nested.
> 2. In provisioner destroy(), need to simply make sure the rootfs
>from getContainerRootfsDir() is correctly nested.
> 
> Both getContainerRootfsDir() and getBackendDir() rely on
> the helper method getContainerDir(). So we can simply make them
> nested aware by change getContainerDir() to be recursive.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/paths.hpp 
> 9829d6b52c8547ae22297a5bc47852ce5a219e4c 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 86a45f30c22dc1e41f4779c0ce8c11d02dcc46bb 
> 
> Diff: https://reviews.apache.org/r/51323/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49360: Supported TCP in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:52 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Supported TCP in health check.


Diffs (updated)
-

  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49556: Removed the binary way of HealthCheck in src.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:52 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

The binary way of health check is unused and not necessary anymore. We
should remove it.


Diffs (updated)
-

  src/CMakeLists.txt ccd9892f5327a70f001411faf3199329f12cd472 
  src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 
  src/health-check/CMakeLists.txt 69e9566a0571220e21c82156b814f61e17802352 
  src/health-check/main.cpp 5346e30d35d4c5c2c4d181eee739ac8ce690a8e1 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51069: Refactored `_commandHealthCheck` in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:52 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


Repository: mesos


Description
---

* Removed blocking `Future::await` call.
* Read stdout and stderr of the health check command.
* Adjust the level of some logs.
* Adjust some minor styles.
* Change the interfaces of different health check handlers to
  `Future` to make errors handling more easier.


Diffs (updated)
-

  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 36816: Supported HTTP/HTTPS in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 4:52 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Supported HTTP/HTTPS in health check.


Diffs (updated)
-

  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51083: Passed the pid of the container into `HealthChecker`.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:03 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

To ensure our health check share same namespaces with the container,
we need to pass the pid of the container into `HealthChecker` for
further inspecting and setting.


Diffs (updated)
-

  src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 
  src/launcher/executor.cpp 71ede1ea4f4e97fe94bd2bd136f17f231cedbce6 

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


Testing
---


Thanks,

haosdent huang

Review Request 51378: Exposed `process::internal::defaultClone` to `process` namespace.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Repository: mesos


Description
---

Exposed `process::internal::defaultClone` to `process` namespace.


Diffs
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
a871fe484905165eed093124687c4920f3968ccc 

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


Testing
---


Thanks,

haosdent huang

Review Request 51379: Entered the appropriate namespaces of the task during health check.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Repository: mesos


Description
---

Entered the appropriate namespaces of the task during health check.


Diffs
-

  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51266: Unshared the mount namespace when launching mesos-containerizer.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:22 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


Repository: mesos


Description
---

When launching mesos-containerizer in the mesos-executor, we need to
ensure mesos-executor unshare the mount namespace with
mesos-containerizer. Otherwise, the mount and pivot_root operations in
mesos-containerizer would affect the running context of
mesos-executor.


Diffs (updated)
-

  src/launcher/posix/executor.cpp 43573cacee4e681d4327a7ed7c43b4ee263aa175 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 50657: Removed the binary way of health check in libprocess.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:22 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

The binary way of health check is unused and not necessary anymore. We
should remove it.


Diffs (updated)
-

  3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
497f6107567ef47c16a0c906238bc7dfdcf84701 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49555: Updated mesos-docker-executor to use health check via library way.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:22 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

We change the mesos-executor to use health check via library way in
https://reviews.apache.org/r/49389/. To keep consistent, we replace
health check in mesos-docker-executor from binary way to library way.
In this patch, we rename `healthCheckDir` to `launcherDir` as well to
keep consistent with mesos-executor.


Diffs (updated)
-

  src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 

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


Testing
---

Test with Marathon locally.

# 1. Success case.

I start `sleep 200` with the Docker container and use `ls /mnt/mesos/sandbox` 
as the health check command.

According the qurey result of `http://localhost:8080/v2/apps/test-sleep`, could 
see Marathon receive healthy status.

```
# truncate unnecessary messages
{
  "app": {
"id": "/test-health",
"cmd": "sleep 200",
"container": {
  "type": "DOCKER",
  "volumes": [],
  "docker": {
"image": "ubuntu",
"network": "HOST"
  }
},
"healthChecks": [
  {
"protocol": "COMMAND",
"command": {
  "value": "ls /mnt/mesos/sandbox"
},
"gracePeriodSeconds": 300,
"intervalSeconds": 60,
"timeoutSeconds": 20,
"maxConsecutiveFailures": 3
  }
],
"tasksStaged": 0,
"tasksRunning": 1,
"tasksHealthy": 1,
"tasksUnhealthy": 0,
"tasks": [
  {
"id": "test-health.8d4ba884-585c-11e6-9fb5-0242e663b1f5",
"slaveId": "ec5359a2-9c63-4c2e-86da-3ed8ef94800b-S0",
"host": "127.0.0.1",
"startedAt": "2016-08-02T02:55:23.433Z",
"stagedAt": "2016-08-02T02:55:21.217Z",
"version": "2016-08-02T02:48:31.054Z",
"ipAddresses": [
  {
"ipAddress": "127.0.0.1",
"protocol": "IPv4"
  }
],
"appId": "/test-health",
"healthCheckResults": [
  {
"alive": true,
"consecutiveFailures": 0,
"firstSuccess": "2016-08-02T02:55:23.652Z",
"lastFailure": null,
"lastSuccess": "2016-08-02T02:55:23.652Z",
"lastFailureCause": null,
"taskId": "test-health.8d4ba884-585c-11e6-9fb5-0242e663b1f5"
  }
]
  }
]
  }
}
```

# 2. Failed case.

Then I start a similar application which use `false` as health check command, 
could see Marathon receive unhealthy status.

```
# truncate unnecessary messages
{
  "app": {
"id": "/test-unhealth",
"cmd": "sleep 200",
"container": {
  "type": "DOCKER",
  "volumes": [],
  "docker": {
"image": "ubuntu",
"network": "HOST"
  }
},
"healthChecks": [
  {
"protocol": "COMMAND",
"command": {
  "value": "false"
},
"gracePeriodSeconds": 5,
"intervalSeconds": 3,
"timeoutSeconds": 20,
"maxConsecutiveFailures": 3
  }
],
"tasksStaged": 0,
"tasksRunning": 1,
"tasksHealthy": 0,
"tasksUnhealthy": 1,
"tasks": [
  {
"id": "test-unhealth.48a6318a-585e-11e6-9fb5-0242e663b1f5",
"slaveId": "ec5359a2-9c63-4c2e-86da-3ed8ef94800b-S0",
"host": "127.0.0.1",
"startedAt": "2016-08-02T03:07:47.310Z",
"stagedAt": "2016-08-02T03:07:45.041Z",
"ipAddresses": [
  {
"ipAddress": "127.0.0.1",
"protocol": "IPv4"
  }
],
"appId": "/test-unhealth",
"healthCheckResults": [
  {
"alive": false,
"consecutiveFailures": 1,
"firstSuccess": null,
"lastFailure": "2016-08-02T03:07:53.800Z",
"lastSuccess": null,
"lastFailureCause": "",
"taskId": "test-unhealth.48a6318a-585e-11e6-9fb5-0242e663b1f5"
  }
]
  }
]
  }
}
```


Thanks,

haosdent huang

Re: Review Request 49556: Removed the binary way of HealthCheck in src.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:23 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

The binary way of health check is unused and not necessary anymore. We
should remove it.


Diffs (updated)
-

  src/CMakeLists.txt ccd9892f5327a70f001411faf3199329f12cd472 
  src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 
  src/health-check/CMakeLists.txt 69e9566a0571220e21c82156b814f61e17802352 
  src/health-check/main.cpp 5346e30d35d4c5c2c4d181eee739ac8ce690a8e1 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 36816: Supported HTTP/HTTPS in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:23 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Supported HTTP/HTTPS in health check.


Diffs (updated)
-

  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51069: Refactored `_commandHealthCheck` in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:23 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


Repository: mesos


Description
---

* Removed blocking `Future::await` call.
* Read stdout and stderr of the health check command.
* Adjust the level of some logs.
* Adjust some minor styles.
* Change the interfaces of different health check handlers to
  `Future` to make errors handling more easier.


Diffs (updated)
-

  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49360: Supported TCP in health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:23 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Supported TCP in health check.


Diffs (updated)
-

  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51378: Exposed `process::internal::defaultClone` to `process` namespace.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:23 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


Repository: mesos


Description
---

Exposed `process::internal::defaultClone` to `process` namespace.


Diffs (updated)
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
a871fe484905165eed093124687c4920f3968ccc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51083: Passed the pid of the container into `HealthChecker`.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:23 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

To ensure our health check share same namespaces with the container,
we need to pass the pid of the container into `HealthChecker` for
further inspecting and setting.


Diffs (updated)
-

  src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
  src/health-check/health_checker.hpp b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 
  src/launcher/executor.cpp 71ede1ea4f4e97fe94bd2bd136f17f231cedbce6 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51266: Unshared the mount namespace when launching mesos-containerizer.

[haosdent huang]

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

(Updated Aug. 24, 2016, 5:28 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Updated `Testing Done` field.


Repository: mesos


Description
---

When launching mesos-containerizer in the mesos-executor, we need to
ensure mesos-executor unshare the mount namespace with
mesos-containerizer. Otherwise, the mount and pivot_root operations in
mesos-containerizer would affect the running context of
mesos-executor.


Diffs
-

  src/launcher/posix/executor.cpp 43573cacee4e681d4327a7ed7c43b4ee263aa175 

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


Testing (updated)
---

Test by running

```
sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="HealthCheckTest.*" --verbose
```


Thanks,

haosdent huang

Re: Review Request 51379: Entered the appropriate namespaces of the task during health check.

[haosdent huang]

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

(Updated Aug. 24, 2016, 6:03 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


Repository: mesos


Description
---

Entered the appropriate namespaces of the task during health check.


Diffs (updated)
-

  src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51378: Exposed `process::internal::defaultClone` to `process` namespace.

[haosdent huang]

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

(Updated Aug. 24, 2016, 6:03 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


Repository: mesos


Description
---

Exposed `process::internal::defaultClone` to `process` namespace.


Diffs (updated)
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
a871fe484905165eed093124687c4920f3968ccc 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51085: Added `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaHTTP`.

[haosdent huang]

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

(Updated Aug. 24, 2016, 6:04 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Test if a docker task in bridge network mode could pass HTTP health
check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 50a252b5267cbc3b21ddc75e1da86c4975f6faf1 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51086: Added `HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP`.

[haosdent huang]

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

(Updated Aug. 24, 2016, 6:04 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón Kleiman, 
Gilbert Song, Jie Yu, and Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Test if a docker task in bridge network mode could pass TCP health
check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 50a252b5267cbc3b21ddc75e1da86c4975f6faf1 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51379: Entered the appropriate namespaces of the task during health check.

[Alexander Rukletsov]

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




src/health-check/health_checker.cpp (line 79)


I'd rather not inline it.



src/health-check/health_checker.cpp (lines 85 - 100)


Let's `#ifdef` the whole function. We can later do one more `#ifdef` and 
decide whether to pass a custom clone or `None`.



src/health-check/health_checker.cpp (line 291)


I was originally thinking that we should not enter the network namespace 
here, but after a second thought I think I uderstand why you do both here: a 
command health check may actually try to communicate with the container.

I don't think there will be any problems if we enter the network namespace 
here. But let's document it


- Alexander Rukletsov


On Aug. 24, 2016, 6:03 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51379/
> ---
> 
> (Updated Aug. 24, 2016, 6:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón 
> Kleiman, Gilbert Song, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Entered the appropriate namespaces of the task during health check.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
>   src/health-check/health_checker.cpp 
> 45a5fe00a95a6e88b1990c1396e03082feb202bc 
> 
> Diff: https://reviews.apache.org/r/51379/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[Zhitao Li]

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

(Updated Aug. 24, 2016, 8:32 p.m.)


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


Changes
---

Unchain so r/51009 can get in faster.


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


Repository: mesos


Description
---

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

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


Diffs
-

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

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


Testing
---

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

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


Thanks,

Zhitao Li

Review Request 51388: Replaced raw pointers in MesosContainerizer with Owned pointers.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

Replaced raw pointers in MesosContainerizer with Owned pointers.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
8a8985a6627859365c4f87ea0cbb5f89190cd4b3 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 51389: A few style cleanups in MesosContainerizer.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

A few style cleanups in MesosContainerizer.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
8a8985a6627859365c4f87ea0cbb5f89190cd4b3 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 51390: Added a TODO about a bug in MesosContainerizer.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

Added a TODO about a bug in MesosContainerizer.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
8a8985a6627859365c4f87ea0cbb5f89190cd4b3 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 51391: Fixed a bug related to logger in MesosContainerizer.

[Jie Yu]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

It's possible that 'destroy' is called while logger 'prepare' is being
called. If that happens, when logger 'prepare' finishes, it'll trigger
an assertion failure.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
8a8985a6627859365c4f87ea0cbb5f89190cd4b3 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 51388: Replaced raw pointers in MesosContainerizer with Owned pointers.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Aug. 24, 2016, 2:08 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51388/
> ---
> 
> (Updated Aug. 24, 2016, 2:08 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced raw pointers in MesosContainerizer with Owned pointers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51388/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51389: A few style cleanups in MesosContainerizer.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Aug. 24, 2016, 2:08 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51389/
> ---
> 
> (Updated Aug. 24, 2016, 2:08 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few style cleanups in MesosContainerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51389/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51210: Update mesos-executor name for Windows.

[Joseph Wu]

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


Fix it, then Ship it!





src/slave/slave.cpp (lines 128 - 130)


s/const/constexpr/

I think there's little difference here, but we use `constexpr` for these 
string constants in other locations.


- Joseph Wu


On Aug. 18, 2016, 11:08 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51210/
> ---
> 
> (Updated Aug. 18, 2016, 11:08 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update mesos-executor name for Windows.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 3688f420e71ce9c79b7dda221f3f5c0042a9b3a1 
> 
> Diff: https://reviews.apache.org/r/51210/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51233: Build a clean `Windows` environment for `mesos-executor`.

[Joseph Wu]

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



Mostly naming and comment suggestions.  But there's one major question near the 
bottom of this review.


3rdparty/libprocess/include/process/windows/subprocess.hpp (line 67)


Comment suggestion:
```
// Retrieves system environment in a std::map.
// These do not include environment variables from the current process.
```



3rdparty/libprocess/include/process/windows/subprocess.hpp (line 68)


I checked this:

https://msdn.microsoft.com/en-us/library/windows/desktop/bb762270(v=vs.85).aspx

Which says:
> If th[e second] parameter is NULL, the returned environment block 
contains system variables only.

s/getCUEnvironment/getSystemEnvironment/



3rdparty/libprocess/include/process/windows/subprocess.hpp (line 73)


Let's use descriptive variable names, here and everywhere.

Maybe:
s/cuEnv/userEnvironment/



3rdparty/libprocess/include/process/windows/subprocess.hpp (line 74)


There's a double space here.

s/envPtr/environmentEntry/



3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 76 - 78)


Suggested comment:
```
// Get the system environment.
// The third parameter (bool) tells the function *not* to inherit
// variables from the current process.
```



3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 81 - 82)


No newline after the `if (...)`

Also, the `envRet` seems to be unused elsewhere, so you can do:
```
if (!CreateEnvironmentBlock((LPVOID*)&envPtr, nullptr, FALSE)) { 
  ...
}
```



3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 86 - 87)


Suggestion:
```
// Save the environment block in order to destroy it later.
wchar_t* environmentBlock = environmentEntry;
```



3rdparty/libprocess/include/process/windows/subprocess.hpp (line 89)


Add a comment here, saying something like:
```
The environment block is a list of null-terminated strings, 
where the list is itself null-terminated. i.e.
foo=1\0bar=2\0\0
```



3rdparty/libprocess/include/process/windows/subprocess.hpp (line 128)


Add a comment above this line:

https://github.com/apache/mesos/blob/3674c58ad5268c96c3868dda3d43375f0f7b02a7/src/slave/slave.cpp#L6319-L6323

And then `#ifdef` the `PATH` replacement there on Windows.

s/cuEnv/systemEnvironment/



3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 130 - 132)


Do we really want to return `None` here?  This would make the subprocess 
inherit environment variables.

And it sounds like a problem if the system environment is blank.

What would be more appropriate to?
1) CHECK that the system environment is non-empty.
2) Ignore an empty system environment and stringify the `env`.



3rdparty/libprocess/include/process/windows/subprocess.hpp (line 134)


s/combEnv/combinedEnvironment/


- Joseph Wu


On Aug. 19, 2016, 10:38 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51233/
> ---
> 
> (Updated Aug. 19, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Build a clean `Windows` environment for `mesos-executor`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> 6bb54c878fbdc4b53c9d4f3298530cbcf55d88b8 
> 
> Diff: https://reviews.apache.org/r/51233/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51217: Added `os::execlp' in `stout` library.

[Joseph Wu]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/os/windows/shell.hpp (lines 114 - 115)


Suggestion for the comment:
```
// On Windows, the `_spawnvp` call creates a new process.
// In order to emulate the semantics of `execvp`, we spawn with `_P_WAIT`,
// which force the parent process to block on the child.  When the child 
exits,
// the exit code is propagated back through the parent (via `exit()`).
```

Similar for the `execlp` comment, but s/vp/lp/.


- Joseph Wu


On Aug. 19, 2016, 10:38 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51217/
> ---
> 
> (Updated Aug. 19, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `os::execlp' in `stout` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> c6e141aba0abe2c7fe5410e867f7db47d632e765 
> 
> Diff: https://reviews.apache.org/r/51217/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51390: Added a TODO about a bug in MesosContainerizer.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Aug. 24, 2016, 2:08 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51390/
> ---
> 
> (Updated Aug. 24, 2016, 2:08 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a TODO about a bug in MesosContainerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51390/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51213: Used `os::execlp` instead of `::execlp`.

[Joseph Wu]

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


Ship it!




- Joseph Wu


On Aug. 19, 2016, 10:38 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51213/
> ---
> 
> (Updated Aug. 19, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `os::execlp` instead of `::execlp`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 2db8db5ac2f4614d406b950a1b4e8098a0d90e7b 
> 
> Diff: https://reviews.apache.org/r/51213/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51234: Add `userenv` import library to `Windows` build.

[Joseph Wu]

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




3rdparty/stout/cmake/StoutConfigure.cmake (line 114)


Why is this added to Stout instead of say, `PROCESS_LIBS` in 
`3rdparty/libprocess/cmake/ProcessConfigure.cmake`?


- Joseph Wu


On Aug. 19, 2016, 10:38 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51234/
> ---
> 
> (Updated Aug. 19, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `userenv` import library to `Windows` build.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutConfigure.cmake 
> 7e483aa9c433ac40d108971d012bc76fa61ba860 
> 
> Diff: https://reviews.apache.org/r/51234/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

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

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Added unit test for provisioner recursive listContainers().


Diffs
-

  src/tests/containerizer/provisioner_paths_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Gilbert Song

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

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, 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.hpp 
9829d6b52c8547ae22297a5bc47852ce5a219e4c 
  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 51320: Implemented the LaunchGroup Offer::Operation in the master.

[Benjamin Mahler]

> On Aug. 24, 2016, 12:38 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3257
> > 
> >
> > do you want to add a "messages_launch_task_groups" metric?

My understanding is that `messages_launch_tasks` remains for backwards 
compatibility and going forward we'll want call-based metrics. For example:

```
calls: 100
calls/decline: 90,
calls/accept: 10,
calls/accept/operations/create: 1,
calls/accept/operations/destroy: 0,
calls/accept/operations/launch: 4,
calls/accept/operations/launch_group: 2,
calls/accept/operations/reserve: 1,
calls/accept/operations/unreserve: 0,
```

I filed [MESOS-6082](https://issues.apache.org/jira/browse/MESOS-6082) and 
[MESOS-6083](https://issues.apache.org/jira/browse/MESOS-6083) to reflect this.


- Benjamin


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


On Aug. 23, 2016, 5:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51320/
> ---
> 
> (Updated Aug. 23, 2016, 5:11 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6045
> https://issues.apache.org/jira/browse/MESOS-6045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This operation is all-or-nothing, in that all tasks must be
> launched together. If the operation fails, all tasks will
> fail with TASK_ERROR and the appropriate GROUP reason.
> If a task was killed before delivery to the executor, all
> tasks are killed.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 53b6547281b23ce9f47c9f1a418b60667fb4f602 
>   include/mesos/v1/mesos.proto f6b59e156c92a26dd63b11bf403bdba677b9644b 
>   src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
>   src/messages/messages.proto 7b5e24fb1e9baf09ce024daeca90745f380d4c2f 
> 
> Diff: https://reviews.apache.org/r/51320/diff/
> 
> 
> Testing
> ---
> 
> Added a test in the subsequent patch.
> 
> More tests will be added for all-or-nothing validation / authorization paths.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 51320: Implemented the LaunchGroup Offer::Operation in the master.

[Benjamin Mahler]

> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 4056
> > 
> >
> > how about s/int/size_t?

Yes for most containers or loop iterators, but protobuf uses `int` for 
`.size()` so I use the same type as them.


> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, lines 4031-4032
> > 
> >
> > What about 
> > 
> > ```
> > "A task within the task group was killed before "
> > "delivery to the executor");
> > ```

I encourage putting the space onto the next line as it is a bit more readable 
and leads to fewer mistakes where we miss the space:

```
LOG(INFO) << "Launching task group " << stringify(taskIds)
  << " of framework " << *framework
  << " with resources " << totalResources
  << " on agent " << *slave;
```

here it's easy to see that each line continues from the previous one, compare 
this with:

```
LOG(INFO) << "Launching task group " << stringify(taskIds) << " "
  << "of framework " << *framework << " "
  << "with resources " << totalResources << " "
  << "on agent " << *slave;
```


> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 3971
> > 
> >
> > kill this

Once I add a newline between the error and reason assignments, the break needs 
a newline as well or it looks strange.


> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, lines 3967-3969
> > 
> >
> > How about:
> > 
> > ```
> > error = Error("Task '" + stringify(task.task_id()) + "' "
> >   "is not authorized to launch as user '" + 
> >   user + "'");
> > ```

See my other comment about spaces on the next line. Also, we should avoid 
opening and closing the quotes on separate lines (as you did with user).


> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 3957
> > 
> >
> > kill this

Once I add a newline between the error and reason assignments, the break needs 
a newline as well or it looks strange.


> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, lines 3953-3955
> > 
> >
> > What about following? 
> > 
> > ```
> > error = Error("Failed to authorize task '" +
> >   stringify(task.task_id()) + "': " +
> >   authorization.failure());
> > ```

See my other comments about quotes across lines and spaces on the next line.


> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 3326
> > 
> >
> > Just a question here: Do we need the `CHECK` here? I think that from 
> > here we are pretty sure that the operation type here is 
> > `Offer::Operation::LAUNCH_GROUP` as we already filtered the operations in 
> > line 3317 and 3318.

Well, I would like the code to fail if someone breaks this, I could use 
UNREACHABLE instead:

```
const RepeatedPtrField& tasks = [&]() {
  if (operation.type() == Offer::Operation::LAUNCH) {
return operation.launch().task_infos();
  } else if (operation.type() == Offer::Operation::LAUNCH_GROUP) {
return operation.launch_group().task_group().tasks();
  }
  UNREACHABLE();
}();
```


> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 93
> > 
> >
> > I'd prefer that we move this under `using std::xxx`.
> > 
> > ```
> > using std::list;
> > using std::set;
> > using std::shared_ptr;
> > using std::string;
> > using std::vector;
> > 
> > using google::protobuf::RepeatedPtrField;
> > 
> > sing process::await;
> > using process::wait; // Necessary on some OS's to disambiguate.
> > ```

I think we follow alphabetical sections, no?


- Benjamin


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


On Aug. 23, 2016, 5:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51320/
> ---

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51052]

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 Aug. 24, 2016, 8:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51052/
> ---
> 
> (Updated Aug. 24, 2016, 8:32 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-2154
> https://issues.apache.org/jira/browse/MESOS-2154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes cpu quota for command executor (which runs outside
> of the docker container) by ensuing --cpu-quota flag to docker
> run.
> 
> Note: we have to add the boolean variable to `Docker` class
> because `Docker::run()` has reached the maximum argument length
> GMOCK can support.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp e07c4aa723f57db7f8bc31d4af5d32a30ebf98f2 
>   src/docker/executor.hpp 7b63d784d6b8685912598b77fb38cf6e70646ae3 
>   src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
>   src/slave/containerizer/docker.cpp e447c58bd46ba080529e8f349347eccf5a54110a 
>   src/tests/mesos.cpp f5034f9f8de7040182e10f51be125a87b29fdd24 
> 
> Diff: https://reviews.apache.org/r/51052/diff/
> 
> 
> Testing
> ---
> 
> I am now able to make docker containers launched through mesos-execute have a 
> cpu quota.
> 
> Also making sure `make check` still works on mac os for the linux only flag.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 50853: Resolved C++11-related TODO in master/master.cpp.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Aug. 5, 2016, 3:30 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50853/
> ---
> 
> (Updated Aug. 5, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resolved C++11-related TODO in master/master.cpp.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 92595097c8f26675aee122c8a11366262534db64 
> 
> Diff: https://reviews.apache.org/r/50853/diff/
> 
> 
> Testing
> ---
> 
> `make check` in OS X, Centos 7, Centos 6, Debian 8, Fedora 23, Ubuntu 14, 
> Ubuntu 12, Ubuntu 15, Ubuntu 16
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 50853: Resolved C++11-related TODO in master/master.cpp.

[Benjamin Mahler]

> On Aug. 24, 2016, 10:04 p.m., Benjamin Mahler wrote:
> > Ship It!

Note that the change isn't C++11 related (my TODO was suggesting to use a 
lambda which requires C++11, but your change just uses defer. Since this didn't 
use to work, I updated the commit summary to be the following:

```
commit 0fb5faf667afebf236e6f6b264ffd15bdcff2726
Author: Gastón Kleiman 
Date:   Wed Aug 24 15:04:54 2016 -0700

Removed a no longer needed function disambiguation.

It appears that with the compiler versions we now support, this
disambiguation is no longer necessary.

Review: https://reviews.apache.org/r/50853/
```


- Benjamin


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


On Aug. 5, 2016, 3:30 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50853/
> ---
> 
> (Updated Aug. 5, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resolved C++11-related TODO in master/master.cpp.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 92595097c8f26675aee122c8a11366262534db64 
> 
> Diff: https://reviews.apache.org/r/50853/diff/
> 
> 
> Testing
> ---
> 
> `make check` in OS X, Centos 7, Centos 6, Debian 8, Fedora 23, Ubuntu 14, 
> Ubuntu 12, Ubuntu 15, Ubuntu 16
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

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

[Gilbert Song]

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

(Updated Aug. 24, 2016, 3:14 p.m.)


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


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


Repository: mesos


Description
---

Implemented recursive helper method findContainerDir for provisioner.


Diffs (updated)
-

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

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


Testing
---

make check


Thanks,

Gilbert Song

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

[Jiang Yan Xu]

> On Aug. 23, 2016, 2:26 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 276-279
> > 
> >
> > Probably extract this snippet in a function, e.g. 
> > `conditionalAllocate()`?
> 
> Jiang Yan Xu wrote:
> I thought about it but was struggling to find a short and clear method 
> name. So to describe the function in a full sentense it's "dispatch an 
> allocate() call if the condition `!allocationPending` is met". I think 
> `conditionalAllocate()` is OK but not great, it's not clear what the 
> condition is and not clear about the dispatch. I agree it's worth doing if we 
> can abstract this out without needing to explain what each line of a 4-line 
> method is doing in the comment...
> 
> Alexander Rukletsov wrote:
> Let's focus on _why_ instead of _how_. You want to ensure that one and 
> only one event allocation happens after the event. Dispatch+flag is _how_ you 
> achieve this, which may change in future. Here are some suggestions:
> - `ensureAllocate`
> - `ensureAllocation`
> - `allocateOnce`
> 
> Side note: "unique dispatch", i.e. a dispatch that succeeds only if there 
> are no identical messages in the actor's mailbox, looks like something we may 
> need in other places. Maybe we should consider adding such functionality into 
> libprocess.

Good point. I like `ensureAllocation()` better than the rest.

```
// Ensure an allocation run is pending in the event queue (i.e,. one is 
dispatched otherwise).
// TODO: Consider generalizing this into a "unique dispatch" functionality in 
libprocess.
void ensureAllocation();
```

Yeah I like the idea of "unique dispatch", see the TODO.


- Jiang Yan


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


On Aug. 23, 2016, 1:49 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Aug. 23, 2016, 1:49 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.
> 
> - Carrying over work from https://reviews.apache.org/r/41658/ and added the 
> previous reviewers
> - Specifically, this patch introduces the boolean flag pendingAllocation, 
> which when set on event 
>   triggered allocations, will prevent additional no-op allocations: the flag 
> is cleared when 
>   the enqueued allocation is processed, subsequent event triggered 
> allocations will update a set
>   of allocation candidates rather than dispatching an additional allocate().
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
> 
> 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 51391: Fixed a bug related to logger in MesosContainerizer.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51388, 51389, 51390, 51391]

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 Aug. 24, 2016, 9:09 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51391/
> ---
> 
> (Updated Aug. 24, 2016, 9:09 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's possible that 'destroy' is called while logger 'prepare' is being
> called. If that happens, when logger 'prepare' finishes, it'll trigger
> an assertion failure.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51391/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51391: Fixed a bug related to logger in MesosContainerizer.

[Joseph Wu]

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


Ship it!




Looks like the same code in the docker containerizer is missing the same check.

- Joseph Wu


On Aug. 24, 2016, 2:09 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51391/
> ---
> 
> (Updated Aug. 24, 2016, 2:09 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's possible that 'destroy' is called while logger 'prepare' is being
> called. If that happens, when logger 'prepare' finishes, it'll trigger
> an assertion failure.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51391/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51320: Implemented the LaunchGroup Offer::Operation in the master.

[Guangya Liu]

> On 八月 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 93
> > 
> >
> > I'd prefer that we move this under `using std::xxx`.
> > 
> > ```
> > using std::list;
> > using std::set;
> > using std::shared_ptr;
> > using std::string;
> > using std::vector;
> > 
> > using google::protobuf::RepeatedPtrField;
> > 
> > sing process::await;
> > using process::wait; // Necessary on some OS's to disambiguate.
> > ```
> 
> Benjamin Mahler wrote:
> I think we follow alphabetical sections, no?

If keeping alphabetical order, then the `using process::await;` should goes 
before `using std::list;`. I saw that in the code, we are always putting 
`std::xxx` first, then others. One example here: 
https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L50-L57


> On 八月 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 3326
> > 
> >
> > Just a question here: Do we need the `CHECK` here? I think that from 
> > here we are pretty sure that the operation type here is 
> > `Offer::Operation::LAUNCH_GROUP` as we already filtered the operations in 
> > line 3317 and 3318.
> 
> Benjamin Mahler wrote:
> Well, I would like the code to fail if someone breaks this, I could use 
> UNREACHABLE instead:
> 
> ```
> const RepeatedPtrField& tasks = [&]() {
>   if (operation.type() == Offer::Operation::LAUNCH) {
> return operation.launch().task_infos();
>   } else if (operation.type() == Offer::Operation::LAUNCH_GROUP) {
> return operation.launch_group().task_group().tasks();
>   }
>   UNREACHABLE();
> }();
> ```

Good to know, thanks Ben.


> On 八月 23, 2016, 11:48 a.m., Guangya Liu wrote:
> > src/master/master.cpp, lines 4031-4032
> > 
> >
> > What about 
> > 
> > ```
> > "A task within the task group was killed before "
> > "delivery to the executor");
> > ```
> 
> Benjamin Mahler wrote:
> I encourage putting the space onto the next line as it is a bit more 
> readable and leads to fewer mistakes where we miss the space:
> 
> ```
> LOG(INFO) << "Launching task group " << stringify(taskIds)
>   << " of framework " << *framework
>   << " with resources " << totalResources
>   << " on agent " << *slave;
> ```
> 
> here it's easy to see that each line continues from the previous one, 
> compare this with:
> 
> ```
> LOG(INFO) << "Launching task group " << stringify(taskIds) << " "
>   << "of framework " << *framework << " "
>   << "with resources " << totalResources << " "
>   << "on agent " << *slave;
> ```

Good to know this.


- Guangya


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


On 八月 23, 2016, 5:11 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51320/
> ---
> 
> (Updated 八月 23, 2016, 5:11 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6045
> https://issues.apache.org/jira/browse/MESOS-6045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This operation is all-or-nothing, in that all tasks must be
> launched together. If the operation fails, all tasks will
> fail with TASK_ERROR and the appropriate GROUP reason.
> If a task was killed before delivery to the executor, all
> tasks are killed.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 53b6547281b23ce9f47c9f1a418b60667fb4f602 
>   include/mesos/v1/mesos.proto f6b59e156c92a26dd63b11bf403bdba677b9644b 
>   src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
>   src/messages/messages.proto 7b5e24fb1e9baf09ce024daeca90745f380d4c2f 
> 
> Diff: https://reviews.apache.org/r/51320/diff/
> 
> 
> Testing
> ---
> 
> Added a test in the subsequent patch.
> 
> More tests will be added for all-or-nothing validation / authorization paths.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 51124: Support more layers through symlink for overlay backend.

[Gilbert Song]

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




src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 155)


s/temp dir/temporary directory/g



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 158)


Just style nits. newline above.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 159)


kill this line.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 161)


newline above.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 165)


ditto.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 167)


Newline above.

s/tempLink/symlink/g



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 172)


Could you add comments for the index? so people know that the symlinks are 
named as 0, 1, ..., N.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 174)


we can use stringify, but both look good to me.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 262)


Could you address it here? We should `getBackendDir` in provisioner destroy 
and pass the backendDir to backend::destroy().



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 267)


no space, please fix the indentation.

s/migration/deprecation/g



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 273)


sounds a little weird. Maybe:

```
"Invalid symlink '" + tempLink + "'"
```



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 275)


s/resolved/realpath/g



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 285 - 286)


You should remove the symlink as well, right?


- Gilbert Song


On Aug. 15, 2016, 6:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51124/
> ---
> 
> (Updated Aug. 15, 2016, 6:09 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6000
> https://issues.apache.org/jira/browse/MESOS-6000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support more layers through symlink for overlay backend.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 387f28a331813c75a509b4a31dbbdc764080b8c1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
> 
> Diff: https://reviews.apache.org/r/51124/diff/
> 
> 
> Testing
> ---
> 
> 1. Make sure `OverlayBackendTest.*` passes;
> 2. Use mesos-execute to provision a container using overlay backend. Observed 
> following log lines:
> 
> ```
> I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
>  to '/tmp/NcmRZt'
> I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with 
> overlayfs: 
> 'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/2
 5d9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
> ...
> (after executor exited)

Re: Review Request 51391: Fixed a bug related to logger in MesosContainerizer.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Aug. 24, 2016, 2:09 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51391/
> ---
> 
> (Updated Aug. 24, 2016, 2:09 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's possible that 'destroy' is called while logger 'prepare' is being
> called. If that happens, when logger 'prepare' finishes, it'll trigger
> an assertion failure.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51391/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 50841: Added GPU scheduling logic to docker containerizer process.

[Kevin Klues]

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




src/slave/containerizer/docker.hpp (lines 505 - 507)


I would just call this variable `gpus`
Also the comment should read:
```
// The number of GPUs allocated to the container.
```



src/slave/containerizer/docker.cpp (line 653)


I would probably call this variable `count`. When I saw the name 
`requestedNvidiaGpu` I thought it was a specific GPU id being passed in, not a 
count.

I would also call the function `allocateNvidiaGpus()` since you can 
allocate more than one with this function.



src/slave/containerizer/docker.cpp (lines 662 - 664)


With type `size_t` you can never have a negative value., so just checking 
`== 0` should be enough here.



src/slave/containerizer/docker.cpp (lines 666 - 668)


I would do this as the first check in this function.  If we don't have an 
allocator set, then we really shouldn't even be calling this function 
regardless of anything else that is going on.

Also, the string should read:
```
"The `allocateNvidiaGpu` function was called without an 
`NvidiaGpuAllocator` set".
```



src/slave/containerizer/docker.cpp (line 681)


I would rename  this function `deallocateNvidiaGpus()` (i.e. with an `s` on 
the end).



src/slave/containerizer/docker.cpp (line 684)


I would move this down just before its first use.



src/slave/containerizer/docker.cpp (lines 690 - 692)


Again, I would move this up to the top of the function, with a similar 
Failure string as before.



src/slave/containerizer/docker.cpp (lines 694 - 696)


Why do you need this level of indirection? Why not just pass 
`containers_[containerId]->gpuAllocated` directly to 
`nvidiaGpuAllocator->deallocate()`?



src/slave/containerizer/docker.cpp (lines 698 - 710)


Why don't you just return from the `deallocate()` call with a `.then()`? 
I.e.

```
  return nvidiaGpuAllocator->deallocate(deallocated)
.then(defer(self(), [=](const Nothing& nothing) {
  containers_[containerId]->gpuAllocated.clear();
  return Nothing();
}));
```

If any failures happen in the deallocation, they should get propagated 
through.



src/slave/containerizer/docker.cpp (lines 1376 - 1379)


I would probably remove the intermediate variable above called `requested` 
and instead save a temporary `Future` to the `allocateNvidiaGpu()` call. I 
would also move all of the GPU logic together instead of separating it out.

Something like:

```

Future allocateGpus = Nothing();

const Option& taskInfo = container->task;
if (taskInfo.isNone()) {
  return Failure("No task information found");
}

if (taskInfo->resources.gpus().isSome()) {
  // Make sure that the `gpus` resource is not fractional.
  // We rely on scalar resources to only have 3 digits of precision.
  if (static_cast(gpus.get() * 1000.0) % 1000 != 0) {
return Failure("The 'gpus' resource must be an unsigned integer");
  }

  allocateGpus = allocateNvidiaGpu(gpus.get(), containerId);
}

...
...
...

return allocateGpus
  .then(...)
  .then(...);
```



src/slave/containerizer/docker.cpp (line 1555)


I wouldn't just blindly call this function here. It should be wrapped in 
some logic that makes sure it's OK to call it (i.e. checks to make sure that we 
have the nvidia->allocator component passed in).

Again, you could have some logic above which saves a temporary `Future` 
that is set to `Nothing()` by default and is the result of calling 
`deallocateNvidiaGpu()` otherwise.



src/slave/containerizer/docker.cpp (line 2035)


Same here, use the temporary mentioned above to do the deallocation or not.


- Kevin Klues


On Aug. 22, 2016, 10:11 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50841/
> ---
> 
> (Updated Aug. 22, 2016, 10:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahle

Re: Review Request 50128: Added helper functions to 'Docker::Device'.

[Kevin Klues]

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




src/docker/docker.hpp (lines 73 - 84)


In general, we don't typically have constructors for `structs` like this. 
Instead, we just set its fields directly at the call site.

If you see the comment below about making the `Device::parse()` function 
static, then you could (in theory) always build the device from that instead of 
calling the constructor directly, i.e.:

```
Try device = Device::parse(deviceString);
```

Device device;
device.hostPath = host;
device.containerPath = container;
device.access = ;
```



src/docker/docker.hpp (lines 86 - 95)


I woudn't implement a `serialize()` function directly, but rather overload 
the `<<` operator. That way you can just run the global `stringify()` function 
to turn it into a string.



src/docker/docker.hpp (line 97)


This should probably be a static function, not a member function. It should 
also return a `Try`, not a `bool`.

See how things are done in for device entries in `src/linux/cgroups.*pp`



src/docker/docker.cpp (lines 389 - 392)


Here is where you could use the new `parse()` function.



src/docker/docker.cpp (line 763)


Here you would just use `stringify(device)` if you overload `<<` as 
suggested above.


- Kevin Klues


On Aug. 22, 2016, 10:11 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50128/
> ---
> 
> (Updated Aug. 22, 2016, 10:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Wrapped helper functions 'serialize()' and 'parse()' to 'Docker::Device'
> to handle data tranformation between 'Docker::Device' structure and
> string.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp e07c4aa723f57db7f8bc31d4af5d32a30ebf98f2 
> 
> Diff: https://reviews.apache.org/r/50128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50123: Added GPU scheduler for docker containerizer process.

[Kevin Klues]

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




src/slave/containerizer/docker.hpp (lines 20 - 25)


What does adding all of these headers have to do with this patch? They may 
be needed, but I don't see why they would be part of this patch, specifically.



src/slave/containerizer/docker.cpp (line 19)


Again, it's not clear why this is added as part of this commit. I.e. I 
don't see `set` being used anwywhere in the code that's being added.



src/slave/containerizer/docker.cpp (lines 171 - 179)


It feels a little strange to me to do things this way (i.e. duplicating the 
`new DockerContainerizer()` call). I would like to see what @bmahler thinks, 
but I'd prefer something like:
```
Option allocator = None();

if (nvidia.isSome()) {
  allocator = nvidia->allocator;
}

return new DockerContainerizer(
  flags,
  fetcher,
  Owned(logger.get()),
  docker,
  allocator);
```



src/slave/containerizer/docker.cpp (line 1284)


I would remove this temporary variable and just call the following:
```
Option gpus = taskInfo->resources().gpus();
```


- Kevin Klues


On Aug. 22, 2016, 10:11 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50123/
> ---
> 
> (Updated Aug. 22, 2016, 10:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This added 'NvidiaGpuAllocator' to docker containerizer process so that
> docker containerizer process is ready to use it to allocate GPUs to task
> with 'gpus' resource.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
>   src/tests/mesos.hpp ad31276aeb2cb7ed5ba3e091a9085f35addf17c4 
>   src/tests/mesos.cpp 62e8fcc6fa7bd856aab6148ca6e6cad66b436f04 
> 
> Diff: https://reviews.apache.org/r/50123/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Review Request 51400: Stopped passing messages in MesosContainerizer destroy methods.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

The extra message is introduced in:
https://reviews.apache.org/r/28141.

I don't see a strong reason why we need such messages from that patch.
Furthermore, the message is not quite useful because it is not the
root cause of the destroy. Finally, the `Executor terminated` message
no longer applies when we destroy a nested container. The same message
is already generated properly in the agent.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
1f414cfa332d9a3f8b8f04343249e02924e39d89 
  src/slave/containerizer/mesos/containerizer.cpp 
8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
c0dda11c18a770f594b9dcf2e506ec624706dbbd 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 51401: Removed 'status' in the destroy chain in MesosContainerizer.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

'status' is already part of the 'Container' struct. No need to pass it
in the destroy chain. This patch removed it.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
1f414cfa332d9a3f8b8f04343249e02924e39d89 
  src/slave/containerizer/mesos/containerizer.cpp 
8a8985a6627859365c4f87ea0cbb5f89190cd4b3 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 51278: Refactored LinuxLauncher to be nested container aware.

[Kevin Klues]

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




src/slave/containerizer/mesos/linux_launcher.cpp (line 132)


There's something wrong with this logic. The top level directory of the 
cgroup begins with `mesos`, not the first container ID. We either need to strip 
this here or at a level above.


- Kevin Klues


On Aug. 22, 2016, 4:54 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51278/
> ---
> 
> (Updated Aug. 22, 2016, 4:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note, this doesn't yet include the use of 'ns::enter' for creating
> nested containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 8fbe1e9742df85b0fc6de46ac81a0c064c845a63 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 7377316776646e3d660086da3c0d5b494ce8ace4 
> 
> Diff: https://reviews.apache.org/r/51278/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

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

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, 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

Review Request 51403: Added 'at' to LinkedHashMap.

[Benjamin Mahler]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/stout/include/stout/linkedhashmap.hpp 
38acdc8775f0e4aa7a6f202d337e052dbedaa389 

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


Testing
---

make check

Also used this in my subsequent patch.


Thanks,

Benjamin Mahler

Review Request 51405: Updated a few more tests to create a temporary 'runtime_dir'.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Updated a few more tests to create a temporary 'runtime_dir'.


Diffs
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
c0dda11c18a770f594b9dcf2e506ec624706dbbd 

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


Testing
---


Thanks,

Kevin Klues