Review Request 51411: Added provisioner tests to provision using meta discovery.

[Srinivas Brahmaroutu]

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

Review request for mesos.


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


Repository: mesos


Description
---

Added two tests that will test two of the image meta discovery paths.
Test ROOT_MetaDiscoveryAppcImageTest pulls a appc image from quay.io and
the test ROOT_MetaDiscoveryDockerImageTest pulls a docker image from the
docker hub. Both tests use "useMetaDiscovery" flag along with other Appc
image labels to invoke meta discovery algorithm from "rkt" tool.


Diffs
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
b3ba176e506a6d1528290c07a8a0555b12c8cf70 

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


Testing
---


Thanks,

Srinivas Brahmaroutu

Review Request 51410: Enabled meta discovery using appc labels.

[Srinivas Brahmaroutu]

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

Review request for mesos.


Repository: mesos


Description
---

Added appc_labels to mesos-execute so that we can indicate meta
discovery using useMetaDiscovery="true" along with other appc labels
that determines the os, arch of the image we are discovering. This logic
needs to be documented so that user can switch between simple and meta
discovery methods. This implementation is necessary as there is no other
way to determine from the URI.


Diffs
-

  src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp 
907e761856e8996b72e4231de27fa15e884d52e3 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 51408: Updated test to account for new 'init' process semantics in a container.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51272, 51273, 51274, 51275, 51276, 51277, 51278, 51405, 
51406, 51407, 51408]

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. 25, 2016, 1:24 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51408/
> ---
> 
> (Updated Aug. 25, 2016, 1:24 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6088
> https://issues.apache.org/jira/browse/MESOS-6088
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated test to account for new 'init' process semantics in a container.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> 2725eb0f76f4e2668381c0d6686a99649f4f9f25 
> 
> Diff: https://reviews.apache.org/r/51408/diff/
> 
> 
> Testing
> ---
> 
> Alot of tests are failing. We will update this once the patches this patch 
> set depends on gets updated.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

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

[Jie Yu]

> On Aug. 24, 2016, 11:49 p.m., Joseph Wu wrote:
> > Looks like the same code in the docker containerizer is missing the same 
> > check.

Can you create a ticket to track? :)


- Jie


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


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

Review Request 51417: Removed a NOTE in MesosContainerizer that no longer applies.

[Jie Yu]

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

Review request for mesos, Gilbert Song and Kevin Klues.


Repository: mesos


Description
---

Previously, we need to initialize `launchInfos` because we don't yet
have an explicit state for PROVISIONING (MESOS-4878). After we
introduce the PROVISIONING state, it's guaranteed that `launchInfo`
will be set if the container is in PREPARING state. Therefore, the
NOTE and the corresponding code is no longer needed.


Diffs
-

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

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


Testing
---

make check


Thanks,

Jie Yu

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

[Guangya Liu]

> On 八月 25, 2016, 12:32 a.m., Kevin Klues wrote:
> > 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.

The `deallocateNvidiaGpu` already have some checking for `nvidiaGpuAllocator`, 
is this enough?


> On 八月 25, 2016, 12:32 a.m., Kevin Klues wrote:
> > 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.

With the current logic, we can have more log messages here with different 
conditions, but seems your proposal is more simple.


- Guangya


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


On 八月 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 八月 22, 2016, 10:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added control logic to allocate/deallocate GPUs to GPU-related task
> when the task is started/terminated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
> 
> Diff: https://reviews.apache.org/r/50841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Review Request 51418: Updated a few comments in MesosContainerizer.

[Jie Yu]

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

Review request for mesos, Gilbert Song and Kevin Klues.


Repository: mesos


Description
---

Updated a few comments in MesosContainerizer.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
1f414cfa332d9a3f8b8f04343249e02924e39d89 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 51414: Fixed the using statements in MesosContainerizer.

[Jie Yu]

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

Review request for mesos, Gilbert Song and Kevin Klues.


Repository: mesos


Description
---

Fixed the using statements in MesosContainerizer.


Diffs
-

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

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 51416: Fixed a TODO in the MesosContainerizer.

[Jie Yu]

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

Review request for mesos, Gilbert Song and Kevin Klues.


Repository: mesos


Description
---

This patch fixed a bug in the destory path. It correctly handles a
race where the container is destroyed while it is being launched. The
idea is to make 'container->status' optional and don't set it until
the container is actually forked.


Diffs
-

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

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 51415: A few consistency fix on indentation.

[Jie Yu]

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

Review request for mesos, Gilbert Song and Kevin Klues.


Repository: mesos


Description
---

A few consistency fix on indentation.


Diffs
-

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

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


Testing
---

make check


Thanks,

Jie Yu

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

[Guangya Liu]

> On 八月 25, 2016, 12:32 a.m., Kevin Klues wrote:
> > 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.

OK, then we can split this to two patches, one for adding those header files 
and the other is for gpu related.


- Guangya


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


On 八月 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 八月 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
> 
>

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

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

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

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add unit tests for sharing of resources.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
cbed333f497016fe2811f755028796012b41db77 
  src/tests/master_validation_tests.cpp 
86b4b22350175af592876fd2f6d3fecca7acabce 
  src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f 
  src/tests/persistent_volume_tests.cpp 
a6f97c4bb5fb29d610c01255036095e2b30c44c5 
  src/tests/resources_tests.cpp 4932d95f4e78cae764b89472373e13527b4354a2 

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


Testing
---

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


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

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

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


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
cbed333f497016fe2811f755028796012b41db77 

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


Testing
---

All tests passed.

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

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6572us
Added 1000 agents in 1.820284secs
round 0 allocate() took 1.602476secs to make 1000 offers
round 50 allocate() took 1.586638secs to make 1000 offers
round 100 allocate() took 1.588735secs to make 1000 offers
round 150 allocate() took 1.581553secs to make 1000 offers
round 199 allocate() took 1.595088secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6869us
Added 1000 agents in 1.847318secs
round 0 allocate() took 1.606464secs to make 1000 offers
round 50 allocate() took 1.603727secs to make 1000 offers
round 100 allocate() took 1.630528secs to make 1000 offers
round 150 allocate() took 1.60693secs to make 1000 offers
round 199 allocate() took 1.613754secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 6903us
Added 1000 agents in 1.856714secs
round 0 allocate() took 1.621012secs to make 1000 offers
round 50 allocate() took 1.60385secs to make 1000 offers
round 100 allocate() took 1.64289secs to make 1000 offers
round 150 allocate() took 1.654631secs to make 1000 offers
round 199 allocate() took 1.609998secs to make 1000 offers

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


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  include/mesos/mesos.proto a93db559ff802587bf45e7763444f861ec46e151 
  include/mesos/v1/mesos.proto 4a7e9987c14379d0ad3b4084fc90ff89385156e5 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 
  src/tests/hierarchical_allocator_tests.cpp 
cbed333f497016fe2811f755028796012b41db77 

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


Testing
---

Tests updated with new capability.
Tests successful.


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs (updated)
-

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

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:14 a.m.)


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


Changes
---

Updated to handle Offer operations within allocator to determine additional 
copies of shared resources to be added in the allocator.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/common/resources.cpp a5f5902d8f7f2757e3aee35619bff5cc3a52f29b 
  src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d 
  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 
  src/master/allocator/sorter/drf/sorter.hpp 
bc6bfb2d5d3b32d55be055a0514861b4e7d889bb 
  src/master/allocator/sorter/drf/sorter.cpp 
ac85b327fc33d34246788e6a8c8bf5a486c61434 
  src/master/http.cpp c6bdad64e6058ec63bfbacda736d174fd15f8a05 
  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 
  src/master/validation.hpp 3205ee1e699d0fb7ca16ced96a07be3a07cf8956 
  src/master/validation.cpp 3d2965e4cfaae075f988fd43386bd00c7d807b2e 
  src/tests/master_validation_tests.cpp 
86b4b22350175af592876fd2f6d3fecca7acabce 
  src/tests/sorter_tests.cpp 821e30d6574b045d25d4de4f7c3b8ac5346d3002 
  src/v1/resources.cpp 172217505d80d66cb7e10b3635dc273229313601 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45960: Added interfaces to handle and track shareable resources.

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:13 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added interfaces to handle and track shareable resources.


Diffs (updated)
-

  include/mesos/resources.hpp b132933a499a70a3f032d6910621202840be5576 
  include/mesos/v1/resources.hpp e7f53292f0b8c7cf7ef04d26ef1f7e42b9a0b687 
  src/common/resources.cpp a5f5902d8f7f2757e3aee35619bff5cc3a52f29b 
  src/v1/resources.cpp 172217505d80d66cb7e10b3635dc273229313601 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Review Request 51412: Add offeredResources to Allocator::updateAllocation() API.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

This is being added to allow for handling of ACCEPT call within the
allocator in the future. For now, we expose the offered resources
in this API to account for additional copies of shared resources in
the allocator when there are more tasks being launched in a single
ACCEPT call for a single copy of shared resource being offered.


Diffs
-

  include/mesos/allocator/allocator.hpp 
41a9d457286e30431490ca626e680d85684b48d6 
  src/master/allocator/mesos/allocator.hpp 
69639be9c14b83157df29f767e819db719588053 
  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 
  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 
  src/tests/allocator.hpp 5ca3057265eb2c00c965132ac7922019e1cc77b8 
  src/tests/hierarchical_allocator_tests.cpp 
cbed333f497016fe2811f755028796012b41db77 
  src/tests/reservation_tests.cpp 6181c6f8abbf254b45ea77f33c48dee918106ef0 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51412: Add offeredResources to Allocator::updateAllocation() API.

[Anindya Sinha]

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

(Updated Aug. 25, 2016, 4:13 a.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

This is being added to allow for handling of ACCEPT call within the
allocator in the future. For now, we expose the offered resources
in this API to account for additional copies of shared resources in
the allocator when there are more tasks being launched in a single
ACCEPT call for a single copy of shared resource being offered.


Diffs
-

  include/mesos/allocator/allocator.hpp 
41a9d457286e30431490ca626e680d85684b48d6 
  src/master/allocator/mesos/allocator.hpp 
69639be9c14b83157df29f767e819db719588053 
  src/master/allocator/mesos/hierarchical.hpp 
bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
  src/master/allocator/mesos/hierarchical.cpp 
234ef98529964a0b6d3f132426a6c8ccbb1263ee 
  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 
  src/tests/allocator.hpp 5ca3057265eb2c00c965132ac7922019e1cc77b8 
  src/tests/hierarchical_allocator_tests.cpp 
cbed333f497016fe2811f755028796012b41db77 
  src/tests/reservation_tests.cpp 6181c6f8abbf254b45ea77f33c48dee918106ef0 

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


Testing (updated)
---

All tests passed.


Thanks,

Anindya Sinha

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51323, 51343, 51358, 51359, 51392, 51393, 51402]

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. 25, 2016, 12:52 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Aug. 25, 2016, 12:52 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 nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

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

[Jiang Yan Xu]

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



Comments mainly on tests (which I didn't look at earlier).


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


At this point we don't know if the resources are **scalars** or not, they 
could be of other types.

So `s/scalars/_resources/` ?



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


Ditto about the name `scalars`.



src/tests/master_validation_tests.cpp (line 609)


s/SharedPersistentVolumes/SharedPersistentVolumeInUse/



src/tests/master_validation_tests.cpp (lines 611 - 612)


s/cpus1/cpus/
s/mem1/mem/



src/tests/master_validation_tests.cpp (line 640)


We can just use `{}` here.



src/tests/master_validation_tests.cpp (line 649)


Ditto.



src/tests/master_validation_tests.cpp (lines 664 - 665)


Ditto.



src/tests/master_validation_tests.cpp (lines 670 - 671)


Ditto.



src/tests/sorter_tests.cpp (line 728)


I see that this is modeled after `SorterTest.DRFSorter` but in general 
shorter and more focused tests are preferred, especially the ones like this 
which require no setup.

So here at least let's try to make it shorter by removing bits that are not 
strictly increasing our test coverage, especially given there's already 
`SorterTest.DRFSorter`.



src/tests/sorter_tests.cpp (line 739)


```
// 1000 'disk' in total (shared + non-shared).
```



src/tests/sorter_tests.cpp (lines 747 - 748)


Let's be consistent with the sequence in a block. Same as with client a: 
first `sorter.add("b");` then prepare b's resources and call `allocated()`. 
Here and below for other clients as well.



src/tests/sorter_tests.cpp (line 751)


This would help me understand better. Here and below.

Also capitalize it.

```
// Shares: a = .1 (dominant: disk), b = .06 (dominant: cpus).
```



src/tests/sorter_tests.cpp (lines 759 - 761)






src/tests/sorter_tests.cpp (lines 759 - 761)


We already have a, b and c, d looks to be just another simple client with 
only non-shared allocation and not all that interesting? Can we keep it simple 
and remove `d`?



src/tests/sorter_tests.cpp (line 763)


Some shared resources specific comments?

```
// 'a' and 'c' share the same shared volume which are the the dominant 
resource for both.
```



src/tests/sorter_tests.cpp (lines 773 - 779)


Doesn't look like this block with `e` add much to the test coverage, remove 
it?



src/tests/sorter_tests.cpp (line 781)


The rest of the test doesn't seem to have much to do with sharedness of 
resources directly so let's just make it clear:

```
// Verify other basic allocator methods work when shared resources are in 
the allocations.
```

Correspondingly, the first half of the test can be summarized as:

```
// Verify sort() works when shared resources are in the allocations.
```


- Jiang Yan Xu


On Aug. 12, 2016, 5:57 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45961/
> ---
> 
> (Updated Aug. 12, 2016, 5:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael 
> Park, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> o Each shared resource is accouted via its share count. This count is
>   updated based on the resource operations (such as add and subtract)
>   in various scenarios such as task launch and terminate at multiple
>   modules such as master, allocator, sorter, etc.
> o Only allow DESTROY if there are no running or pending tasks using
>   the volume. However, if the volume is in a pending offer to one or
> 

Re: Review Request 51404: Updated the agent to reject multiple terminal status updates.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/slave.cpp (line 3468)


s/updateTaskState/updated/

like you did in recover?



src/slave/slave.cpp (line 6201)


remove this CHECK because it is not intuitive that it follows from the 
comment.


- Vinod Kone


On Aug. 25, 2016, 1:29 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51404/
> ---
> 
> (Updated Aug. 25, 2016, 1:29 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6026
> https://issues.apache.org/jira/browse/MESOS-6026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently the agent will allow terminal status updates when updating
> the state of a task, and only the status update manager is responsible
> for dropping duplicate terminal updates.
> 
> As a result of the race in MESOS-6026, the agent may send a duplicate
> terminal update out-of-order. Because this patch rejects duplicate
> terminal updates altogether, this case is no longer possible. With this
> patch, the race will only lead to an error being logged (although we
> should ideally be smart enough to not generate a TASK_FAILED when
> the TASK_FINISHED has already arrived for the executor's task).
> 
> In the process of fixing this, I removed the need for the agent to
> call the separate `Executor::terminateTask()` function (which was
> error-prone), and I've handled errors for unexpected task state updates.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 7ca9923f97d731715db0267703d32cffc5badf0b 
>   src/slave/slave.cpp c686a97149d3a279bea3e532109ba2215947fc4c 
>   src/tests/slave_tests.cpp dcf84545354dd2ae0ab5acad3b15eca0467b9982 
>   src/tests/status_update_manager_tests.cpp 
> 7b6fe314ac5bef24bd1d2734a7e7c35a54530c88 
> 
> Diff: https://reviews.apache.org/r/51404/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> I moved the status update manager test to become an agent test,
> since the agent is now the one responsible for dropping duplicate
> terminal updates.
> 
> It would be good to also add a test that covers the case where
> the containerizer update is delayed and another terminal update
> arrives.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 51403: Added 'at' to LinkedHashMap.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Aug. 25, 2016, 1:06 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51403/
> ---
> 
> (Updated Aug. 25, 2016, 1:06 a.m.)
> 
> 
> 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 51409: Printed all the isolator cleanup errors during destory.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

The current code only prints the first error.


Diffs
-

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

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


Testing
---

make check


Thanks,

Jie Yu

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

[Mesos ReviewBot]

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



Patch looks great!

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

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:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51393/
> ---
> 
> (Updated Aug. 24, 2016, 9:32 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
> ---
> 
> 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 51404: Updated the agent to reject multiple terminal status updates.

[Benjamin Mahler]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Currently the agent will allow terminal status updates when updating
the state of a task, and only the status update manager is responsible
for dropping duplicate terminal updates.

As a result of the race in MESOS-6026, the agent may send a duplicate
terminal update out-of-order. Because this patch rejects duplicate
terminal updates altogether, this case is no longer possible. With this
patch, the race will only lead to an error being logged (although we
should ideally be smart enough to not generate a TASK_FAILED when
the TASK_FINISHED has already arrived for the executor's task).

In the process of fixing this, I removed the need for the agent to
call the separate `Executor::terminateTask()` function (which was
error-prone), and I've handled errors for unexpected task state updates.


Diffs
-

  src/slave/slave.hpp 7ca9923f97d731715db0267703d32cffc5badf0b 
  src/slave/slave.cpp c686a97149d3a279bea3e532109ba2215947fc4c 
  src/tests/slave_tests.cpp dcf84545354dd2ae0ab5acad3b15eca0467b9982 
  src/tests/status_update_manager_tests.cpp 
7b6fe314ac5bef24bd1d2734a7e7c35a54530c88 

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


Testing
---

make check

I moved the status update manager test to become an agent test,
since the agent is now the one responsible for dropping duplicate
terminal updates.

It would be good to also add a test that covers the case where
the containerizer update is delayed and another terminal update
arrives.


Thanks,

Benjamin Mahler

Review Request 51408: Updated test to account for new 'init' process semantics in a container.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Updated test to account for new 'init' process semantics in a container.


Diffs
-

  src/tests/containerizer/isolator_tests.cpp 
2725eb0f76f4e2668381c0d6686a99649f4f9f25 

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


Testing
---

Alot of tests are failing. We will update this once the patches this patch set 
depends on gets updated.


Thanks,

Kevin Klues

Review Request 51407: Updated mesos containerizer to checkpoint the container exit status.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Updated mesos containerizer to checkpoint the container exit status.


Diffs
-

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

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


Testing
---


Thanks,

Kevin Klues

Review Request 51406: Updated 'launcher' to optionally spawn an 'init' process.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Previously the 'mesos-containerizer launch' binary would simply exec
into the actual command we wanted to launch after doing some set of
preperatory work. The problem with this approach, however, is that
this gives us no opportunity to checkpoint the exit status of the
command so the agent can recover it in cases where it is offline at
the time the command completes.

To compensate for this, we now allow 'mesos-containerizer launch' to
take an optional '--exit_status_path' parameter, which indicates where
the exit status of our command should be checkpointed. In order to
do this checkpointing, however, we cannot simply exec into the command
anymore. Instead we now fork-exec the command and reap it once it
completes. We then checkpoint its exit status and return it as our
own. We call the original process the 'init' process of the container
and the fork-exec'd process its child.  As a side effect of doing
things this way, we also have to be careful to forward all signals
sent to the init process down to the child.

In order to properly reap the 'init' process, we also introduce a new
'Launcher::wait' command that knows how to reap the 'init' process
directly and then retreive its exit status from the checkpoint (if
necessary).

In a subsequent commit we will update the mesos containerizer to use
this new functionality.


Diffs
-

  src/slave/containerizer/mesos/launch.hpp 
0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
  src/slave/containerizer/mesos/launch.cpp 
2db8db5ac2f4614d406b950a1b4e8098a0d90e7b 
  src/slave/containerizer/mesos/launcher.hpp 
0eae09515d550a2c71ae1414d4a22943f1d09db9 
  src/slave/containerizer/mesos/launcher.cpp 
413a8afdc56127a58c9599c436d17d6c98e62434 
  src/slave/containerizer/mesos/linux_launcher.hpp 
8fbe1e9742df85b0fc6de46ac81a0c064c845a63 
  src/slave/containerizer/mesos/linux_launcher.cpp 
7377316776646e3d660086da3c0d5b494ce8ace4 
  src/tests/containerizer/launcher.hpp 94c62b761a17436841bd19f3bf622cc8f1047876 
  src/tests/containerizer/launcher.cpp a92d9890f0931425d69ef8ce0896d081b8722079 

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


Testing
---


Thanks,

Kevin Klues

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

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

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

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

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

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 

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

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

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

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

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

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 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*), 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 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 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 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
> 
>

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

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

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

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

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

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

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

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

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