Re: Review Request 41613: Added `DEFAULT_ROLE` constant to persistent volume tests.

[Greg Mann]

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

(Updated Sept. 21, 2016, 5:55 a.m.)


Review request for mesos, Alexander Rukletsov, Jie Yu, and Michael Park.


Changes
---

Rebase!


Repository: mesos


Description
---

Added `DEFAULT_ROLE` constant to persistent volume tests.


Diffs (updated)
-

  src/tests/mesos.hpp 7095101cc4b19d7ccbfcfe1c9681d4a591983a95 
  src/tests/persistent_volume_tests.cpp 
c38d8484fbd62b36bf124a96d17c8cae5e51e594 

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


Testing
---

`GTEST_FILTER="*PersistentVolumeTest*" bin/mesos-tests.sh`


Thanks,

Greg Mann

Re: Review Request 52092: Avoided to concat cgroup internally in subsystems.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52090, 52091, 52092]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 20, 2016, 7:21 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52092/
> ---
> 
> (Updated Sept. 20, 2016, 7:21 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To avoid using `path::join(flags.cgroups_root, containerId.value())` to
> concat cgroup internally in subsystems, pass cgroup to the subsystem
> interfaces directly. And since containerId is not necessary for the
> subsystems anymore, remove it in the subsystem interfaces as well.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> c2fcc0b21c5b1e51cb38b61245d4bbd3856a9512 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 6594110bd45a71c6d41a24b3f5b73c4a4e3a7335 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 05cb8b8efadd2355789d08ce5c9da9d3de0fc72a 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
> c1f1004d7578f59dfa9e28323e7a55df41c4cb6b 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 5912c6504a375178b892bf8099d5f75fa9e91c05 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
> 10fbbc9cb39a3b41d64cc090b0e890a2416b2306 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
> a35ecb073acefb903d7e8c4737d6cd048a2b494d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
> b5d5ed233c0c8ddc339d2550c0f57a02dd3f14c3 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
> 0afc24867eb0b1949e95b3f5a8914be013dbf531 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> a1c87ce2ace33f1a779e843578f55d7502562c8d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> bab10ae5d488585603db2637ae15ee02b1982354 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
> 16208655e16b81f1e4bbf97bf5e32e75f4c3f3a9 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
> 93539f1fc8265f66b62294c22f4eaba704b8b876 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
> 5a0adfdb8705ae6b20c61a827380142e418c0ae4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp 
> cc39f287a64a4125260597e74784dc0847953376 
> 
> Diff: https://reviews.apache.org/r/52092/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52071: Updated docs to reflect handling of disk resource with size of 0.

[Anindya Sinha]

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

(Updated Sept. 21, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When a disk resource with size of 0 is specified in `--resources`
startup flag of mesos agent, the disk size is auto detected by the
agent. This is done for both root disks as well as MOUNT disks, but
is not allowed for PATH disks since PATH disks can be carved into
smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Determine disk size when not specified in static resources.

[Anindya Sinha]

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

(Updated Sept. 21, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Refactored based on revised APIs in resources.


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


Repository: mesos


Description
---

When static resources indicate disks with a positive size, we use that
for the disk resources on the agent. However, --resources can include
disks with size of 0, which indicates that mesos agent determine the
size of those disks from the host and uses that information instead.
Note that this is not allowed for PATH disks since PATH disks can be
carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

[Anindya Sinha]

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

(Updated Sept. 21, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated based on review comments.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 478fb5ae01dbdf1a735680edf7c5f30867726e07 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

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

(Updated Sept. 21, 2016, 4:14 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 187-188
> > 
> >
> > Why update here, I prefer
> > 
> > ```
> > parses text in the form "name(role):value;name:value;...".
> > ```

I do not think it matters. The change was done since the previous line did not 
fit this within 80 chars.
Anyways with the refactor, this is not an issue anymore.


> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 201
> > 
> >
> > I think that we should find a better name for this, the above API 
> > `parse` is actually also parsing a text.
> > 
> > What about `parseResourceVector` or else some other meaningful name in 
> > your mind?

Resolved with the refactor.


> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > src/slave/containerizer/containerizer.cpp, line 64
> > 
> >
> > I suggest that we move this to 
> > https://reviews.apache.org/r/51879/diff/3#index_header

Resolved with the refactor.


- Anindya


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


On Sept. 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

[Anindya Sinha]

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

(Updated Sept. 21, 2016, 4:14 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated based on review comments.


Summary (updated)
-

Refactor parsing of resources.


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


Repository: mesos


Description (updated)
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing (updated)
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

> On Sept. 20, 2016, 5 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 60-64
> > 
> >
> > We can get rid of this forward declaration if we can get rid of the 
> > internal convertJSON.

Based on the refactor, we are keeping `convertJSON()`.


> On Sept. 20, 2016, 5 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 201
> > 
> >
> > This is too similar to 
> > 
> > ```
> > static Try parse(
> >   const std::string& text,
> >   const std::string& defaultRole = "*");
> > ```
> > 
> > I was originally suggesting moving over convertJSON but I guess it's 
> > more suitable here in the following form:
> > 
> > ```
> > Try fromJSONString(
> > const std::string& text,
> > const std::string& defaultRole)
> > ```
> > 
> > Similarly for simple flat strings:
> > 
> > ```
> > Try fromSimpleString(
> > const std::string& text,
> > const std::string& defaultRole)
> > ```
> > 
> > When we have both, then 
> > 
> > ```
> > static Try parse(
> > const std::string& text,
> > const std::string& defaultRole = "*");
> > ```
> > 
> > becomes a mere convenience method that calls the above two methods in 
> > sequence. The convenience method can be simply documented as:
> > 
> > ```
> >   /** 
> >* Returns Resources from an input string.
> >*
> >* Parses Resources from text in the form of a JSON array (see
> >* `fromJSONString()`). If that fails, parses text in the simple 
> >* string form (see `fromSimpleString()`).
> >*/
> > ```
> > 
> > How does it sound?

Well, we can do this in a couple of ways. The current approach was to use 
`Resources::parseText()` [or we could call it `Resources::parse()` with an 
additional parameter to include or not include empty resources], which 
`Resources::parse()` would call to implicitly convert to `Resources` from 
`vecor`, whereas in `Containerizer::resources()`, we would call 
`Resources::parseText()` or `Resources::parse()` with the additional parameter.
Another option we discussed is to expose another `Resources::parse()` that 
returns `vecor` in a separate namespace which probably is cleaner but 
would require refactor in other call sites.
For now, we would follow the approach specified above.


- Anindya


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


On Sept. 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52109: Missing `endl` in `cout` for default executor.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 21, 2016, 3:32 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52109/
> ---
> 
> (Updated Sept. 21, 2016, 3:32 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Missing `endl` in `cout` for default executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 369db3e15be75f6cbf0a0b3f4374fe3ddedbf2f9 
> 
> Diff: https://reviews.apache.org/r/52109/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 52095: Add the new flag for executor launching with different user.

[Sivaram Kannan]

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

(Updated Sept. 21, 2016, 3:54 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Add the new flag for executor launching with different user.


Diffs (updated)
-

  src/slave/container_loggers/lib_logrotate.hpp 
a8653d716a898f03cce83f7b88b666dc46c0ea90 
  src/slave/container_loggers/logrotate.hpp 
f906a167f8897385af5f54e1e77cb790121a 

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


Testing
---


Thanks,

Sivaram Kannan

Re: Review Request 52096: Read user details passed in executorInfo and assign it to user flag.

[Sivaram Kannan]

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

(Updated Sept. 21, 2016, 3:54 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Read  user details passed in executorInfo and assign it to user flag.


Diffs (updated)
-

  src/slave/container_loggers/lib_logrotate.cpp 
1c170e5d11ef31d468b200c2c4cbd27abeeb418a 

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


Testing
---


Thanks,

Sivaram Kannan

Re: Review Request 52097: Functionality to switch user when executor launches as non-root user.

[Sivaram Kannan]

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

(Updated Sept. 21, 2016, 3:54 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Functionality to switch user when executor launches as non-root user.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

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


Testing
---


Thanks,

Sivaram Kannan

Re: Review Request 52108: Updated default executor to send TASK_KILLED updates.

[Guangya Liu]

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




src/launcher/default_executor.cpp (line 230)


You may want `endl` to the end.

```
cout << "Received kill for task '" << taskId << "'" << endl;
```

Fixed here https://reviews.apache.org/r/52109/diff/1#index_header



src/tests/default_executor_tests.cpp (line 151)


I was a bit confused: For which case we need to add quota for enum values 
and which cannot?

The update in `src/launcher/default_executor.cpp` including quota for enum 
values but here for `TASK_KILLED` does not use quota.


- Guangya Liu


On 九月 21, 2016, 1:14 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52108/
> ---
> 
> (Updated 九月 21, 2016, 1:14 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Just like LAUNCH_GROPU implementation this is a dummy implementation
> that sends the TASK_KILLED upates without doing any actual kills.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp f0feb95f7f07710bd0cea6f2ac87ef5b875f31ec 
>   src/tests/default_executor_tests.cpp 
> 055a3d76e0224265f51b8393406070c6032541fc 
> 
> Diff: https://reviews.apache.org/r/52108/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 52109: Missing `endl` in `cout` for default executor.

[Guangya Liu]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Missing `endl` in `cout` for default executor.


Diffs
-

  src/launcher/default_executor.cpp 369db3e15be75f6cbf0a0b3f4374fe3ddedbf2f9 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 52100: Added validation of NESTED_CONTAINER_* calls in the agent API.

[Guangya Liu]

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




src/Makefile.am (line 2121)


Just a question here: for the new added test files for agent, do we want to 
continue name it as slave_xxx_tests.cpp or agent_xxx_tests.cpp?



src/slave/validation.cpp (line 112)


s/ContainerID/`ContainerID`

Ditto here and elsewhere.



src/slave/validation.cpp (line 129)


s/ContainerID/`ContainerID`


- Guangya Liu


On 九月 20, 2016, 9:28 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52100/
> ---
> 
> (Updated 九月 20, 2016, 9:28 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2449
> https://issues.apache.org/jira/browse/MESOS-2449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b6b64bc93980cb81fc50380835865af1f6e4e59f 
>   src/slave/validation.cpp a9f318214182a87783a985cd8495a0ec00c95378 
>   src/tests/slave_validation_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52100/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 52103: Implement quota update through `PUT` method.

[Zhitao Li]

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

Review request for mesos, Alexander Rukletsov and Xiaojian Huang.


Repository: mesos


Description
---

Implement quota update through `PUT` method.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
2d56bd011f2c87c67a02d0ae467a4a537d36867e 
  src/master/http.cpp 9005e7c308d5f57c6f5c573951d468a3ba730740 
  src/master/master.hpp 35db198748b8652eb53e17f592f6b40d1e6a3ed9 
  src/master/quota_handler.cpp bf6a613a7bb3c62fd77d1ffde3170749d6c21fa2 
  src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51985: Waited for agent finish registering in test case.

[haosdent huang]

> On Sept. 19, 2016, 7:27 p.m., Joseph Wu wrote:
> > src/tests/slave_recovery_tests.cpp, lines 4073-4081
> > 
> >
> > This change doesn't seem necessary, as waiting for an offer includes 
> > waiting for the agent to (re)register.
> > 
> > Ditto for the below `AWAIT`s.

Thx @kaysoky! I discard this.


- haosdent


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


On Sept. 17, 2016, 3:41 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51985/
> ---
> 
> (Updated Sept. 17, 2016, 3:41 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, Joseph Wu, and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6180
> https://issues.apache.org/jira/browse/MESOS-6180
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Waited for agent finish registering in test case.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 23cf9d8cd76d568fa841817d6f4279c4aa2fdc75 
> 
> Diff: https://reviews.apache.org/r/51985/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52108: Updated default executor to send TASK_KILLED updates.

[Anand Mazumdar]

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


Fix it, then Ship it!




LGTM!


src/launcher/default_executor.cpp (line 233)


Newline before `TODO`

Also missing a quote after `KILL_NESTED_CONTAINER`


- Anand Mazumdar


On Sept. 21, 2016, 1:14 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52108/
> ---
> 
> (Updated Sept. 21, 2016, 1:14 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Just like LAUNCH_GROPU implementation this is a dummy implementation
> that sends the TASK_KILLED upates without doing any actual kills.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp f0feb95f7f07710bd0cea6f2ac87ef5b875f31ec 
>   src/tests/default_executor_tests.cpp 
> 055a3d76e0224265f51b8393406070c6032541fc 
> 
> Diff: https://reviews.apache.org/r/52108/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 46264: Fixed a typo in docker_containerizer_tests.cpp.

[Timothy Chen]

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




src/tests/containerizer/docker_containerizer_tests.cpp (line 2395)


Thanks for fixing this!


- Timothy Chen


On April 25, 2016, 8:23 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46264/
> ---
> 
> (Updated April 25, 2016, 8:23 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a typo in docker_containerizer_tests.cpp.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> f43165388f29513ab8be6594ab6647e8f9eb5a93 
> 
> Diff: https://reviews.apache.org/r/46264/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 50969: Made use of SSL flags to determine scheduler/executor scheme.

[Joseph Wu]

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


Fix it, then Ship it!





src/executor/executor.cpp (lines 36 - 38)


You don't need to #ifdef the header.  But you do need to #ifdef the _usage_ 
of the header :)

Ditto in the other file.


- Joseph Wu


On Sept. 13, 2016, 11:10 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50969/
> ---
> 
> (Updated Sept. 13, 2016, 11:10 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-3753
> https://issues.apache.org/jira/browse/MESOS-3753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP scheduler and executor
> libraries to use `process::network::openssl::flags()`
> to determine whether or not SSL is enabled.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp d1ec817c53d0224a483fec545031016cf90620da 
>   src/scheduler/scheduler.cpp 2ed6ce2a2b61a70a55546e3aeb48b32d90e74ac6 
> 
> Diff: https://reviews.apache.org/r/50969/diff/
> 
> 
> Testing
> ---
> 
> Tested after the following patch in this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 52107: Updated default executor to set `healthy` status in status update.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Sept. 21, 2016, 1:13 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52107/
> ---
> 
> (Updated Sept. 21, 2016, 1:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Right now the executor always sets the health status as true. In the
> future this will be based on the health checker.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp f0feb95f7f07710bd0cea6f2ac87ef5b875f31ec 
>   src/tests/default_executor_tests.cpp 
> 055a3d76e0224265f51b8393406070c6032541fc 
> 
> Diff: https://reviews.apache.org/r/52107/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 52106: Updated the default executor to not automatically send TASK_FINISHED.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Sept. 21, 2016, 1:13 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52106/
> ---
> 
> (Updated Sept. 21, 2016, 1:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also updated the test accordingly.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp f0feb95f7f07710bd0cea6f2ac87ef5b875f31ec 
>   src/tests/default_executor_tests.cpp 
> 055a3d76e0224265f51b8393406070c6032541fc 
> 
> Diff: https://reviews.apache.org/r/52106/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 52108: Updated default executor to send TASK_KILLED updates.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Repository: mesos


Description
---

Just like LAUNCH_GROPU implementation this is a dummy implementation
that sends the TASK_KILLED upates without doing any actual kills.


Diffs
-

  src/launcher/default_executor.cpp f0feb95f7f07710bd0cea6f2ac87ef5b875f31ec 
  src/tests/default_executor_tests.cpp 055a3d76e0224265f51b8393406070c6032541fc 

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


Testing
---

make check


Thanks,

Vinod Kone

Review Request 52107: Updated default executor to set `healthy` status in status update.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Repository: mesos


Description
---

Right now the executor always sets the health status as true. In the
future this will be based on the health checker.


Diffs
-

  src/launcher/default_executor.cpp f0feb95f7f07710bd0cea6f2ac87ef5b875f31ec 
  src/tests/default_executor_tests.cpp 055a3d76e0224265f51b8393406070c6032541fc 

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


Testing
---

make check


Thanks,

Vinod Kone

Review Request 52106: Updated the default executor to not automatically send TASK_FINISHED.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Benjamin Mahler.


Repository: mesos


Description
---

Also updated the test accordingly.


Diffs
-

  src/launcher/default_executor.cpp f0feb95f7f07710bd0cea6f2ac87ef5b875f31ec 
  src/tests/default_executor_tests.cpp 055a3d76e0224265f51b8393406070c6032541fc 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 51065: Changed hostname used for SSL cert creation in tests.

[Joseph Wu]

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


Ship it!




LGTM.

- Joseph Wu


On Sept. 19, 2016, 11:47 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51065/
> ---
> 
> (Updated Sept. 19, 2016, 11:47 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Vinod Kone.
> 
> 
> Bugs: MESOS-3753
> https://issues.apache.org/jira/browse/MESOS-3753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes the SSL helpers in libprocess to generate
> certs using a hostname determined using the same address
> that is advertised by libprocess. This assures that
> validation of the certificates will succeed. The test
> `SSLTest.BasicSameProcess` is also updated to accommodate
> this change.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 2ad623a5ea3b3286983e895010af756f14f51b64 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> a5ac03969694c4345c2c5004051b08b6d1da8555 
> 
> Diff: https://reviews.apache.org/r/51065/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Ubuntu 14.04
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 52097: Functionality to switch user when executor launches as non-root user.

[Sivaram Kannan]

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

(Updated Sept. 21, 2016, 1:02 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Functionality to switch user when executor launches as non-root user.


Diffs
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

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


Testing
---


Thanks,

Sivaram Kannan

Re: Review Request 52096: Read user details passed in executorInfo and assign it to user flag.

[Sivaram Kannan]

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

(Updated Sept. 21, 2016, 1:02 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Read  user details passed in executorInfo and assign it to user flag.


Diffs
-

  src/slave/container_loggers/lib_logrotate.cpp 
1c170e5d11ef31d468b200c2c4cbd27abeeb418a 

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


Testing
---


Thanks,

Sivaram Kannan

Re: Review Request 52095: Add the new flag for executor launching with different user.

[Sivaram Kannan]

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

(Updated Sept. 21, 2016, 1:02 a.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Add the new flag for executor launching with different user.


Diffs
-

  src/slave/container_loggers/lib_logrotate.hpp 
a8653d716a898f03cce83f7b88b666dc46c0ea90 
  src/slave/container_loggers/logrotate.hpp 
f906a167f8897385af5f54e1e77cb790121a 

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


Testing
---


Thanks,

Sivaram Kannan

Re: Review Request 51857: Modified the `network/cni` isolator to be nesting aware.

[Jie Yu]

> On Sept. 21, 2016, 12:54 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1647-1652
> > 
> >
> > What if the top level container join non-host network but its child 
> > container join host network without a rootfs? Looks like we do need to 
> > setup etc files in that case because the host etc files won't work?
> > 
> > Maybe, it's time to address this TODO?

Sorry, i typed to fast. I mean the child container does not have a rootfs.


- Jie


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


On Sept. 21, 2016, 12:06 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 21, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network file setup in the `network/cni` isolator is now nesting
> aware. Since the children share the network and UTS namespace with the
> parent, the network files need to be created only for the parent
> container. For the child containers, the network files will be simply
> a bind mount of the parents network files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 949da8f70fb1cd13d6359780b032cb170693ea3e 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51857: Modified the `network/cni` isolator to be nesting aware.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 378)


Should we skip nested container recover here? It's better to be explicit.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 612)


We need to return failure if parent does not exist.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 684 - 685)


I would also mentioned that we need a mount namespace because we need to 
bind mount /etc/ files



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 706)


For the contaienr (both top level or nested)



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1249)


Let's add a nested container check similar to what we have in cgroups 
isolator:
```
if (containerId.has_parent()) {
  return Failure("Not supported for nested containers");
}
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1253)


Also, you should mention that we don't maintain info for nested container. 
IP address can be obtained from its parent container.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1312)


s/child/nested/



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 1644 - 1649)


What if the top level container join non-host network but its child 
container join host network without a rootfs? Looks like we do need to setup 
etc files in that case because the host etc files won't work?

Maybe, it's time to address this TODO?


- Jie Yu


On Sept. 21, 2016, 12:06 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 21, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network file setup in the `network/cni` isolator is now nesting
> aware. Since the children share the network and UTS namespace with the
> parent, the network files need to be created only for the parent
> container. For the child containers, the network files will be simply
> a bind mount of the parents network files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 949da8f70fb1cd13d6359780b032cb170693ea3e 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

> On Sept. 20, 2016, 9:34 p.m., Jiang Yan Xu wrote:
> > Given these comments, do you want to punt on improving the simple string 
> > parsing and get the JSON format working first?
> > 
> > Also we should have independent unit tests here in resources_tests.cpp, 
> > this shouldn't to be coupled with the containerizer.

Ok. I will close this one for now.


- Anindya


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


On Sept. 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

> On Sept. 20, 2016, 5:49 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 613
> > 
> >
> > Not yours, but I prefer that we quota token as 
> > 
> > ```
> > "Bad value for resources, missing or extra ':' in '" + token + "'");
> > ```

Both patterns are in use. I suggest if we want to make that consistent, we do 
it separately.


> On Sept. 20, 2016, 5:49 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 633-634
> > 
> >
> > I prefer that we quota `key` and `token`
> > 
> > ```
> > return Error(
> > "Bad value for resources, mismatched brackets in '" +
> >  key + "' for '" + token + "'");
> > ```
> > 
> > Ditto for others

Same comment as before.


> On Sept. 20, 2016, 5:49 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 700
> > 
> >
> > What about adding `UNREACHABLE()` here?

`Resource::DiskInfo::Source::Type_Parse()` returns false if it does not result 
in a valid `Resource::DiskInfo::Source::Type` and in that case, we return an 
error. So, at this point, we type should contain valid values.


- Anindya


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


On Sept. 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51124]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 20, 2016, 5:28 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51124/
> ---
> 
> (Updated Sept. 20, 2016, 5:28 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/backend.hpp 
> 93819c887f2f801f06aae7020084b56cc81ce818 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> a5ac13bc17b8f291472b622ebfdd0cd1dd072273 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> 4c5cdb660dea205aea29d217ba80d845d1108fdf 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> 19d44c6a9a2cd5f8de79f33c76e9d79900ab003f 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> e57bb3d2fb4e67e9caae416824a6a748db1460a1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 387f28a331813c75a509b4a31dbbdc764080b8c1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> f37c45ccfa572876dfbba6a0797c223896db5a7f 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 7c9a7cd5c733f74e10316fc1234115e6820cc2cb 
> 
> Diff: https://reviews.apache.org/r/51124/diff/
> 
> 
> Testing
> ---
> 
> 1. Make sure `OverlayBackendTest.*` passes;
> 2. Use mesos-execute to provision a container using overlay backend. Observed 
> following log lines:
> 
> ```
> I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
>  to '/tmp/NcmRZt'
> I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with 
> overlayfs: 
> 'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/2
 5d9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
> ...
> (after executor exited)
> I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory 
> '/tmp/NcmRZt' pointed by 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 46264: Fixed a typo in docker_containerizer_tests.cpp.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 25, 2016, 8:23 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46264/
> ---
> 
> (Updated April 25, 2016, 8:23 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a typo in docker_containerizer_tests.cpp.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> f43165388f29513ab8be6594ab6647e8f9eb5a93 
> 
> Diff: https://reviews.apache.org/r/46264/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 51857: Modified the `network/cni` isolator to be nesting aware.

[Jie Yu]

> On Sept. 20, 2016, 9:10 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 725-733
> > 
> >
> > This is problematic.
> > 
> > If both rootDir and pluginDir is not set, meaning that containers will 
> > only join host network. Say the parent container joins the host network. 
> > The child container will join host network as well, but it has a rootfs so 
> > we need to setup network files for it.
> > 
> > First of all, the CHECK_SOME above will fail. Then the 
> > `CHECK(!infos.contains(...))` will fail as well.
> > 
> > That's the reason I highly suggested previously that you combine the 
> > logic here with the if block in line 701.
> > 
> > For child containers, do not add 'containerNetworks' to its info 
> > because itself does not create/join a new CNI network. You'll need to get 
> > the Info for its parent container anyway, so that's redundant information.
> > 
> > In that way, the 'if' condition at line 701 still applies to child 
> > containers:
> > ```
> > if (infos[containerId]->containerNetworks.empty() &&
> > infos[containerId]->rootfs.isSome()) {
> > ```
> > 
> > However, instead of using the host file directly, you may want to check 
> > if the parent container has network files or not. It's likely that the 
> > parent container joins host network without rootfs. In that case, it does 
> > not have network files.
> 
> Avinash sridharan wrote:
> Why is this is an issue for a child container with a rootfs ? 
> The child container with a rootfs is no different than the parent 
> container. For these containers, the code would never reach line `727`. The 
> execution would be that of code block `707` to `725`. This shouldn't any 
> different than the way it is working today.
> 
> Avinash sridharan wrote:
> By the way my above comment is valid only for child containers with 
> rootfs joining the host network.

oops. my bad. I forgot that the if block starting from line 701 will short 
circuit.

So info.containerNetworks for nested container is the networks of its parent. 
Let's document this in hpp.


- Jie


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


On Sept. 21, 2016, 12:06 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 21, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network file setup in the `network/cni` isolator is now nesting
> aware. Since the children share the network and UTS namespace with the
> parent, the network files need to be created only for the parent
> container. For the child containers, the network files will be simply
> a bind mount of the parents network files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 949da8f70fb1cd13d6359780b032cb170693ea3e 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51857: Modified the `network/cni` isolator to be nesting aware.

[Avinash sridharan]

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

(Updated Sept. 21, 2016, 12:06 a.m.)


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


Changes
---

Merged patches for `cleanup` and `supportsNesting` into this patch.


Summary (updated)
-

Modified the `network/cni` isolator to be nesting aware.


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


Repository: mesos


Description
---

The network file setup in the `network/cni` isolator is now nesting
aware. Since the children share the network and UTS namespace with the
parent, the network files need to be created only for the parent
container. For the child containers, the network files will be simply
a bind mount of the parents network files.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
949da8f70fb1cd13d6359780b032cb170693ea3e 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
359479083894e887647a694a1a133dce44817073 

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


Testing
---

make 
make check
and
sudo ./bin/mesos-tests.sh

The only tests that failed were the SUDO make check tests:
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
[  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
[  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume


Thanks,

Avinash sridharan

Re: Review Request 52100: Added validation of NESTED_CONTAINER_* calls in the agent API.

[Vinod Kone]

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


Fix it, then Ship it!




LGTM modulo the names as mentioned in the earlier review.


src/slave/validation.cpp (lines 117 - 127)


so `UUID::fromBytes()` (used for status update uuids) doesn't impose a 
restriction on the version but we want to impose version restriction here?



src/slave/validation.cpp (lines 117 - 127)


I would flip the order of these two.



src/tests/slave_validation_tests.cpp (line 20)


new line?



src/tests/slave_validation_tests.cpp (line 42)


shouldn't these be backticks? here and everywhere else?


- Vinod Kone


On Sept. 20, 2016, 9:28 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52100/
> ---
> 
> (Updated Sept. 20, 2016, 9:28 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2449
> https://issues.apache.org/jira/browse/MESOS-2449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b6b64bc93980cb81fc50380835865af1f6e4e59f 
>   src/slave/validation.cpp a9f318214182a87783a985cd8495a0ec00c95378 
>   src/tests/slave_validation_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52100/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 52105: Add smarter master redirects.

[Charles Allen]

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

Review request for mesos.


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


Repository: mesos


Description
---

Add smarter master redirects.


Diffs
-

  docs/endpoints/master/redirect.md 3387b64c32fe394949863cbcb3e2b58be1b00bd0 
  src/master/http.cpp 9005e7c308d5f57c6f5c573951d468a3ba730740 

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


Testing
---

Single Node started with `./src/mesos-master --work_dir=/tmp/mesos`


```
Charless-MacBook-Pro:~ charlesallen$ curl -v http://127.0.0.1:5050/redirect
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 5050 (#0)
> GET /redirect HTTP/1.1
> Host: 127.0.0.1:5050
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 307 Temporary Redirect
< Date: Tue, 20 Sep 2016 23:12:24 GMT
< Location: //10.17.97.185:5050
< Content-Length: 0
<
* Connection #0 to host 127.0.0.1 left intact
Charless-MacBook-Pro:~ charlesallen$ curl -v http://127.0.0.1:5050/redirect/foo
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 5050 (#0)
> GET /redirect/foo HTTP/1.1
> Host: 127.0.0.1:5050
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 307 Temporary Redirect
< Date: Tue, 20 Sep 2016 23:12:35 GMT
< Location: //10.17.97.185:5050/foo
< Content-Length: 0
<
* Connection #0 to host 127.0.0.1 left intact
Charless-MacBook-Pro:~ charlesallen$ curl -v 
http://127.0.0.1:5050/master/redirect/test
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 5050 (#0)
> GET /master/redirect/test HTTP/1.1
> Host: 127.0.0.1:5050
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 307 Temporary Redirect
< Date: Tue, 20 Sep 2016 23:12:49 GMT
< Location: //10.17.97.185:5050/test
< Content-Length: 0
<
* Connection #0 to host 127.0.0.1 left intact
Charless-MacBook-Pro:~ charlesallen$ curl -v 
http://127.0.0.1:5050/master/redirectNOTFOUND/test
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 5050 (#0)
> GET /master/redirectNOTFOUND/test HTTP/1.1
> Host: 127.0.0.1:5050
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 404 Not Found
< Date: Tue, 20 Sep 2016 23:12:57 GMT
< Content-Length: 0
<
* Connection #0 to host 127.0.0.1 left intact
Charless-MacBook-Pro:~ charlesallen$ curl -v 
http://127.0.0.1:5050/master/redirect/
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 5050 (#0)
> GET /master/redirect/ HTTP/1.1
> Host: 127.0.0.1:5050
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 404 Not Found
< Date: Tue, 20 Sep 2016 23:13:32 GMT
< Content-Length: 0
<
* Connection #0 to host 127.0.0.1 left intact
Charless-MacBook-Pro:~ charlesallen$ curl -v 
http://127.0.0.1:5050/master/redirect/test
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 5050 (#0)
> GET /master/redirect/test HTTP/1.1
> Host: 127.0.0.1:5050
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 307 Temporary Redirect
< Date: Tue, 20 Sep 2016 23:13:43 GMT
< Location: //10.17.97.185:5050/test
< Content-Length: 0
<
* Connection #0 to host 127.0.0.1 left intact
```

It is worth noting that in this PR /master/redirect/  and /redirect/ return 
404, not 307. This behavior is consistent with master.


Thanks,

Charles Allen

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Sept. 20, 2016, 11:32 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Re: Review Request 52099: Updated scheduler library to handle UUID parsing error.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 20, 2016, 9:24 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52099/
> ---
> 
> (Updated Sept. 20, 2016, 9:24 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously this would have thrown an exception.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp 2ed6ce2a2b61a70a55546e3aeb48b32d90e74ac6 
> 
> Diff: https://reviews.apache.org/r/52099/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52098: Updated UUID::fromString to not throw an exception on error.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 20, 2016, 9:24 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52098/
> ---
> 
> (Updated Sept. 20, 2016, 9:24 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The exception from the string_generator needs to be caught so
> that we can surface a Try to the caller.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/uuid.hpp 
> e638d458c6cb4aaee121a903bb381ccfdeae3fad 
>   3rdparty/stout/tests/uuid_tests.cpp 
> 8f28fcf491e43e87ddd5b54737fd5aef0f6db0a6 
> 
> Diff: https://reviews.apache.org/r/52098/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52056: Exposed unknown container case from Containerizer::destroy.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/containerizer/composing.cpp (line 478)


s/the launch/`launch()`/



src/slave/containerizer/composing.cpp (line 483)


"return a successful destroy" is a bit confusing. How about "indicate a 
successful destroy (by setting `Container.destroyed` to true in `_launch()`)"?



src/slave/containerizer/composing.cpp (line 485)


Maybe add at the end "If we do not defer here and instead associate the 
future right away, the setting of `Container.destroy` in `_launch()` will be a 
no-op; this might result in users waiting on the future incorrectly thinking 
that the destroy failed when in fact the destory is implicitly successful 
because the launch failed."


- Vinod Kone


On Sept. 20, 2016, 1:22 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52056/
> ---
> 
> (Updated Sept. 20, 2016, 1:22 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently the callers of `destroy` cannot determine if the call
> succeeds or fails (without a secondary call to `wait()`).
> 
> This also allows the caller to distinguish between a failure and
> waiting on an unknown container. This is important for the upcoming
> agent child container API, as the end-user would benefit from the
> distinction.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> ef3c2ee22cf0ced35c64025c7f9f7ca75165d54d 
>   src/slave/containerizer/composing.cpp 
> 5ff3e65facd5831e2637ae1cee3ea01b21d0f6b2 
>   src/slave/containerizer/containerizer.hpp 
> f13669d0dfc4ce3287cfe630cabd0470dc765b51 
>   src/slave/containerizer/docker.hpp a378fa9772742b4934578efcec54aeeaf5791a83 
>   src/slave/containerizer/docker.cpp 110a1eb41b1ff7cb94f3630a1843d9f01efbe09c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 078ef4f4e7bf5e1522804a720c51cfa5518d8efd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 
>   src/tests/containerizer.hpp f1fd57945c09fb80b7790f9124843a5d4ea785ee 
>   src/tests/containerizer.cpp bda3e6f84f77a95f0eb9ff3aabdc9513f0f18b3f 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> 51aab33cd190b53328339e39fd12853714882454 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 28cd3fa66886dbdbae3fdeca77707147faafcb7a 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52056/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 51857: Modified the `prepare` and `isolate` methods to be nesting aware.

[Avinash sridharan]

> On Sept. 20, 2016, 9:10 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 725-733
> > 
> >
> > This is problematic.
> > 
> > If both rootDir and pluginDir is not set, meaning that containers will 
> > only join host network. Say the parent container joins the host network. 
> > The child container will join host network as well, but it has a rootfs so 
> > we need to setup network files for it.
> > 
> > First of all, the CHECK_SOME above will fail. Then the 
> > `CHECK(!infos.contains(...))` will fail as well.
> > 
> > That's the reason I highly suggested previously that you combine the 
> > logic here with the if block in line 701.
> > 
> > For child containers, do not add 'containerNetworks' to its info 
> > because itself does not create/join a new CNI network. You'll need to get 
> > the Info for its parent container anyway, so that's redundant information.
> > 
> > In that way, the 'if' condition at line 701 still applies to child 
> > containers:
> > ```
> > if (infos[containerId]->containerNetworks.empty() &&
> > infos[containerId]->rootfs.isSome()) {
> > ```
> > 
> > However, instead of using the host file directly, you may want to check 
> > if the parent container has network files or not. It's likely that the 
> > parent container joins host network without rootfs. In that case, it does 
> > not have network files.
> 
> Avinash sridharan wrote:
> Why is this is an issue for a child container with a rootfs ? 
> The child container with a rootfs is no different than the parent 
> container. For these containers, the code would never reach line `727`. The 
> execution would be that of code block `707` to `725`. This shouldn't any 
> different than the way it is working today.

By the way my above comment is valid only for child containers with rootfs 
joining the host network.


- Avinash


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


On Sept. 20, 2016, 5:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 20, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network file setup in the `network/cni` isolator is now nesting
> aware. Since the children share the network and UTS namespace with the
> parent, the network files need to be created only for the parent
> container. For the child containers, the network files will be simply
> a bind mount of the parents network files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51857: Modified the `prepare` and `isolate` methods to be nesting aware.

[Avinash sridharan]

> On Sept. 20, 2016, 9:10 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 725-733
> > 
> >
> > This is problematic.
> > 
> > If both rootDir and pluginDir is not set, meaning that containers will 
> > only join host network. Say the parent container joins the host network. 
> > The child container will join host network as well, but it has a rootfs so 
> > we need to setup network files for it.
> > 
> > First of all, the CHECK_SOME above will fail. Then the 
> > `CHECK(!infos.contains(...))` will fail as well.
> > 
> > That's the reason I highly suggested previously that you combine the 
> > logic here with the if block in line 701.
> > 
> > For child containers, do not add 'containerNetworks' to its info 
> > because itself does not create/join a new CNI network. You'll need to get 
> > the Info for its parent container anyway, so that's redundant information.
> > 
> > In that way, the 'if' condition at line 701 still applies to child 
> > containers:
> > ```
> > if (infos[containerId]->containerNetworks.empty() &&
> > infos[containerId]->rootfs.isSome()) {
> > ```
> > 
> > However, instead of using the host file directly, you may want to check 
> > if the parent container has network files or not. It's likely that the 
> > parent container joins host network without rootfs. In that case, it does 
> > not have network files.

Why is this is an issue for a child container with a rootfs ? 
The child container with a rootfs is no different than the parent container. 
For these containers, the code would never reach line `727`. The execution 
would be that of code block `707` to `725`. This shouldn't any different than 
the way it is working today.


- Avinash


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


On Sept. 20, 2016, 5:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 20, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network file setup in the `network/cni` isolator is now nesting
> aware. Since the children share the network and UTS namespace with the
> parent, the network files need to be created only for the parent
> container. For the child containers, the network files will be simply
> a bind mount of the parents network files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Jiang Yan Xu]

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




include/mesos/resources.hpp (lines 273 - 274)


Does it need to be a Try? i.e., if it's not a disk, it's not a root disk, 
right? We have `validate()` to make sure resources are valid.



include/mesos/resources.hpp (lines 276 - 278)


Would it be simpler to just have 

```
static bool Resources::isMountDisk(const Resource& resource);
static bool Resources::isPathDisk(const Resource& resource);
```

?

We have already lost enumerability (or swtichability) given that the root 
disk does not have a type. :)



src/common/resources.cpp (line 920)


Not `!resource.has_disk()` but rather `!resource.disk().has_source()` right?

See examples below.



src/common/resources.cpp (lines 924 - 943)


It seems to be cleaner to implement the following instead:

```
static bool Resources::isMountDisk(const Resource& resource)
{
  return resource.has_disk() && 
 resource.disk().has_source() &&
 resource.disk().source().type() == 
Resource::DiskInfo::Source::MOUNT;
}

static bool Resources::isPathDisk(const Resource& resource)
{
  return resource.has_disk() && 
 resource.disk().has_source() &&
 resource.disk().source().type() == 
Resource::DiskInfo::Source::PATH;
}
```


- Jiang Yan Xu


On Sept. 19, 2016, 3:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Sept. 19, 2016, 3:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper methods to determine types of disk resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52002/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52014: Added `supportsNesting` method to `network/cni` isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51857, 51986, 52014]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 20, 2016, 5:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52014/
> ---
> 
> (Updated Sept. 20, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `supportsNesting` method to `network/cni` isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 949da8f70fb1cd13d6359780b032cb170693ea3e 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/52014/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52092: Avoided to concat cgroup internally in subsystems.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 332)


I'd prefer we pass in both containerId and cgroup. Ditto else where. 
ContainerID will be useful anyway (e.g., for logging purpose).


- Jie Yu


On Sept. 20, 2016, 7:21 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52092/
> ---
> 
> (Updated Sept. 20, 2016, 7:21 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To avoid using `path::join(flags.cgroups_root, containerId.value())` to
> concat cgroup internally in subsystems, pass cgroup to the subsystem
> interfaces directly. And since containerId is not necessary for the
> subsystems anymore, remove it in the subsystem interfaces as well.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> c2fcc0b21c5b1e51cb38b61245d4bbd3856a9512 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
> 6594110bd45a71c6d41a24b3f5b73c4a4e3a7335 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 05cb8b8efadd2355789d08ce5c9da9d3de0fc72a 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
> c1f1004d7578f59dfa9e28323e7a55df41c4cb6b 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 5912c6504a375178b892bf8099d5f75fa9e91c05 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
> 10fbbc9cb39a3b41d64cc090b0e890a2416b2306 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
> a35ecb073acefb903d7e8c4737d6cd048a2b494d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
> b5d5ed233c0c8ddc339d2550c0f57a02dd3f14c3 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
> 0afc24867eb0b1949e95b3f5a8914be013dbf531 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> a1c87ce2ace33f1a779e843578f55d7502562c8d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> bab10ae5d488585603db2637ae15ee02b1982354 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
> 16208655e16b81f1e4bbf97bf5e32e75f4c3f3a9 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
> 93539f1fc8265f66b62294c22f4eaba704b8b876 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
> 5a0adfdb8705ae6b20c61a827380142e418c0ae4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp 
> cc39f287a64a4125260597e74784dc0847953376 
> 
> Diff: https://reviews.apache.org/r/52092/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52091: Replaced `set` to `hashset` in perf interfaces.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 20, 2016, 7:22 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52091/
> ---
> 
> (Updated Sept. 20, 2016, 7:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `set` to `hashset` in perf interfaces.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp c7dfe0ac4965a450b25085715502f49bee972b5c 
>   src/linux/perf.cpp 16a30eac9d346b14f2245128003866b9894bb94a 
> 
> Diff: https://reviews.apache.org/r/52091/diff/
> 
> 
> Testing
> ---
> 
> In https://reviews.apache.org/r/52092/, we change from
> 
> ```
> set cgroups;
> perf::sample(events, cgroups, flags.perf_duration)
> ```
> 
> to 
> 
> ```
> perf::sample(events, infos.keys(), flags.perf_duration)
> ```
> 
> `infos.keys()` return a `hashset`. So need to change the perf interfaces to 
> use `hashset` first.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52055: Exposed unknown container case from Containerizer::wait.

[Vinod Kone]

> On Sept. 20, 2016, 7:45 a.m., Guangya Liu wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp, line 3785
> > 
> >
> > Why need `recover` here?

yea, can you add a comment on why you need to do a `recover` here?


- Vinod


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


On Sept. 20, 2016, 12:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52055/
> ---
> 
> (Updated Sept. 20, 2016, 12:40 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the caller to distinguish between a failure and waiting
> on an unknown container. This is important for the upcoming agent
> child container API, as the end-user would benefit from the
> distinction.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> ef3c2ee22cf0ced35c64025c7f9f7ca75165d54d 
>   src/slave/containerizer/composing.cpp 
> 5ff3e65facd5831e2637ae1cee3ea01b21d0f6b2 
>   src/slave/containerizer/containerizer.hpp 
> f13669d0dfc4ce3287cfe630cabd0470dc765b51 
>   src/slave/containerizer/docker.hpp a378fa9772742b4934578efcec54aeeaf5791a83 
>   src/slave/containerizer/docker.cpp 110a1eb41b1ff7cb94f3630a1843d9f01efbe09c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 078ef4f4e7bf5e1522804a720c51cfa5518d8efd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 
>   src/slave/slave.hpp e659c44fcc415f10a6980e7b29cc1ce536dfe657 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
>   src/tests/cluster.cpp b04653af97d17aaa9d0d3ee872169b66cd67126b 
>   src/tests/containerizer.hpp f1fd57945c09fb80b7790f9124843a5d4ea785ee 
>   src/tests/containerizer.cpp bda3e6f84f77a95f0eb9ff3aabdc9513f0f18b3f 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> 51aab33cd190b53328339e39fd12853714882454 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 28cd3fa66886dbdbae3fdeca77707147faafcb7a 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> df4642d2667407b1758ffe2efcfdbf9968cf2c33 
>   src/tests/containerizer/isolator_tests.cpp 
> 9bb1e69209f34b18b5b64c3daf5ea26780f2ab74 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 7fac6ca1550acf54616fb4cef2eab1335f5e9760 
>   src/tests/health_check_tests.cpp 50a252b5267cbc3b21ddc75e1da86c4975f6faf1 
>   src/tests/hook_tests.cpp d864ef3d6153dedab8cc47403b1b9f76d75aeb5c 
>   src/tests/slave_recovery_tests.cpp d58d9bc0dbb0a60e114699485d1306202981ecf5 
> 
> Diff: https://reviews.apache.org/r/52055/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52055: Exposed unknown container case from Containerizer::wait.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 20, 2016, 12:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52055/
> ---
> 
> (Updated Sept. 20, 2016, 12:40 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the caller to distinguish between a failure and waiting
> on an unknown container. This is important for the upcoming agent
> child container API, as the end-user would benefit from the
> distinction.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> ef3c2ee22cf0ced35c64025c7f9f7ca75165d54d 
>   src/slave/containerizer/composing.cpp 
> 5ff3e65facd5831e2637ae1cee3ea01b21d0f6b2 
>   src/slave/containerizer/containerizer.hpp 
> f13669d0dfc4ce3287cfe630cabd0470dc765b51 
>   src/slave/containerizer/docker.hpp a378fa9772742b4934578efcec54aeeaf5791a83 
>   src/slave/containerizer/docker.cpp 110a1eb41b1ff7cb94f3630a1843d9f01efbe09c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 078ef4f4e7bf5e1522804a720c51cfa5518d8efd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 
>   src/slave/slave.hpp e659c44fcc415f10a6980e7b29cc1ce536dfe657 
>   src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
>   src/tests/cluster.cpp b04653af97d17aaa9d0d3ee872169b66cd67126b 
>   src/tests/containerizer.hpp f1fd57945c09fb80b7790f9124843a5d4ea785ee 
>   src/tests/containerizer.cpp bda3e6f84f77a95f0eb9ff3aabdc9513f0f18b3f 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> 51aab33cd190b53328339e39fd12853714882454 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 28cd3fa66886dbdbae3fdeca77707147faafcb7a 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> df4642d2667407b1758ffe2efcfdbf9968cf2c33 
>   src/tests/containerizer/isolator_tests.cpp 
> 9bb1e69209f34b18b5b64c3daf5ea26780f2ab74 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 7fac6ca1550acf54616fb4cef2eab1335f5e9760 
>   src/tests/health_check_tests.cpp 50a252b5267cbc3b21ddc75e1da86c4975f6faf1 
>   src/tests/hook_tests.cpp d864ef3d6153dedab8cc47403b1b9f76d75aeb5c 
>   src/tests/slave_recovery_tests.cpp d58d9bc0dbb0a60e114699485d1306202981ecf5 
> 
> Diff: https://reviews.apache.org/r/52055/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52090: Fixed compile error in ppc64le.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp (lines 
109 - 114)


you can use os::pagesize() here


- Jie Yu


On Sept. 20, 2016, 7:20 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52090/
> ---
> 
> (Updated Sept. 20, 2016, 7:20 p.m.)
> 
> 
> Review request for mesos, Zhiwei Chen, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed compile error in ppc64le.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
> a1c87ce2ace33f1a779e843578f55d7502562c8d 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> bab10ae5d488585603db2637ae15ee02b1982354 
> 
> Diff: https://reviews.apache.org/r/52090/diff/
> 
> 
> Testing
> ---
> 
> Fix the error saw in 
> https://builds.apache.org/job/Mesos-PPC64LE/154/BUILDTOOL=autotools,COMPILER=gcc,CONFIGURATION=--verbose,ENVIRONMENT=GLOG_v=1%20MESOS_VERBOSE=1,label_exp=ppc64le/console
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 52100: Added validation of NESTED_CONTAINER_* calls in the agent API.

[Benjamin Mahler]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/Makefile.am b6b64bc93980cb81fc50380835865af1f6e4e59f 
  src/slave/validation.cpp a9f318214182a87783a985cd8495a0ec00c95378 
  src/tests/slave_validation_tests.cpp PRE-CREATION 

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


Testing
---

Added tests.


Thanks,

Benjamin Mahler

Re: Review Request 50736: Added SSL support to libprocess HTTP request helpers.

[Joseph Wu]

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


Fix it, then Ship it!





3rdparty/libprocess/src/http.cpp (line 1376)


`scheme.getOrElse("http")` is a bit terser.  Ditto below.


- Joseph Wu


On Aug. 12, 2016, 11:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50736/
> ---
> 
> (Updated Aug. 12, 2016, 11:10 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-5966
> https://issues.apache.org/jira/browse/MESOS-5966
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to test libprocess HTTP functionality with SSL
> both enabled and disabled, the HTTP helper functions that
> libprocess uses in tests must be parametrized by SSL
> configuration.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 404196bb198c1ff958b55d72fb29c5fe92dba429 
>   3rdparty/libprocess/src/http.cpp 298bd460afdeccd18e201d8a505b961fd4cf3d3e 
> 
> Diff: https://reviews.apache.org/r/50736/diff/
> 
> 
> Testing
> ---
> 
> Find testing information in the subsequent patch in this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 52098: Updated UUID::fromString to not throw an exception on error.

[Benjamin Mahler]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

The exception from the string_generator needs to be caught so
that we can surface a Try to the caller.


Diffs
-

  3rdparty/stout/include/stout/uuid.hpp 
e638d458c6cb4aaee121a903bb381ccfdeae3fad 
  3rdparty/stout/tests/uuid_tests.cpp 8f28fcf491e43e87ddd5b54737fd5aef0f6db0a6 

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


Testing
---

Added tests.


Thanks,

Benjamin Mahler

Re: Review Request 52014: Added `supportsNesting` method to `network/cni` isolator.

[Jie Yu]

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



Ditto on merging this patch.

- Jie Yu


On Sept. 20, 2016, 5:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52014/
> ---
> 
> (Updated Sept. 20, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `supportsNesting` method to `network/cni` isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 949da8f70fb1cd13d6359780b032cb170693ea3e 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/52014/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51986: Modified `cleanup` to be nested container aware.

[Jie Yu]

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



Let's merge this patch with the prepare isolate patch as well.

- Jie Yu


On Sept. 20, 2016, 5:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51986/
> ---
> 
> (Updated Sept. 20, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified `cleanup` to be nested container aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/51986/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 50857: Modified a scheduler test to run with SSL enabled.

[Greg Mann]

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

(Updated Sept. 20, 2016, 8:47 p.m.)


Review request for mesos, Joseph Wu and Vinod Kone.


Changes
---

Rebase


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


Repository: mesos


Description
---

This patch modifies the test `SchedulerTest.Teardown` to
be parametrized by both `ContentType` and SSL configuration,
and renames it to `SchedulerSSLTest.RunTaskAndTeardown`.
This allows the test to verify the scheduler's behavior with
SSL both enabled and disabled.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp b0ea0bbcce9d847285fda40f778caaf721804457 

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


Testing
---

The test was run in repetition as follows:

`GTEST_REPEAT=-1 GTEST_BREAK_ON_FAILURE=1 
GTEST_FILTER="ContentTypeAndSSLConfig/SchedulerSSLTest.RunTaskAndTeardown*" 
bin/mesos-tests.sh`


Thanks,

Greg Mann

Review Request 52095: Add the new flag for executor launching with different user.

[Sivaram Kannan]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Add the new flag for executor launching with different user.


Diffs
-

  src/slave/container_loggers/lib_logrotate.hpp 
a8653d716a898f03cce83f7b88b666dc46c0ea90 
  src/slave/container_loggers/logrotate.hpp 
f906a167f8897385af5f54e1e77cb790121a 

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


Testing
---


Thanks,

Sivaram Kannan

Review Request 52097: Functionality to switch user when executor launches as non-root user.

[Sivaram Kannan]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Functionality to switch user when executor launches as non-root user.


Diffs
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

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


Testing
---


Thanks,

Sivaram Kannan

Review Request 52096: Read user details passed in executorInfo and assign it to user flag.

[Sivaram Kannan]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Read  user details passed in executorInfo and assign it to user flag.


Diffs
-

  src/slave/container_loggers/lib_logrotate.cpp 
1c170e5d11ef31d468b200c2c4cbd27abeeb418a 

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


Testing
---


Thanks,

Sivaram Kannan

Re: Review Request 52057: Introduced an agent API for managing child containers.

[Benjamin Mahler]

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

(Updated Sept. 20, 2016, 7:34 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Hindman, Jie Yu, and Vinod 
Kone.


Changes
---

Added implementation and validation wiring to avoid breaking the build if this 
is committed on its own.


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


Repository: mesos


Description
---

This allows the executor to manage child containers within its
container. This is useful for the implementation of task groups
("pod"-like containers) as well as for executors that would like
to implement isolation between sub-tasks.

This initial patch provides the ability to launch, wait, and
destroy child containers. This was added as an agent API rather
than an executor API in order to enable operators to inject
subcontainers for future use-cases. For example, an operator
may want to inject a debugging container at run-time.


Diffs (updated)
-

  include/mesos/agent/agent.proto 5b91677487f8e23301e67af14c2115c6b4a533ac 
  include/mesos/v1/agent/agent.proto 8145669073553dc8aa56cfe2c0a0b756d70fee0e 
  src/slave/http.cpp 2cbf3c57e294f0d47d64915371b4a6bbf9cdfc0a 
  src/slave/slave.hpp e659c44fcc415f10a6980e7b29cc1ce536dfe657 
  src/slave/validation.cpp a9f318214182a87783a985cd8495a0ec00c95378 

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


Testing
---

Tests will be added alongside the implementation.


Thanks,

Benjamin Mahler

Re: Review Request 52037: Removed stout's `Set`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52036, 52037]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 20, 2016, 4:05 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52037/
> ---
> 
> (Updated Sept. 20, 2016, 4:05 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6159
> https://issues.apache.org/jira/browse/MESOS-6159
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed stout's `Set`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am fc2d2b20ec48b54b63f971c9887a492e61bb4a26 
>   3rdparty/stout/include/stout/set.hpp 
> bd7a9b9af51fc947cf105328843249e396f0282b 
>   3rdparty/stout/include/stout/stringify.hpp 
> b4d3e619d2a8fb346964771d9c1d8843a8c0f0b6 
>   3rdparty/stout/tests/CMakeLists.txt 
> f85dc998e8ba5f8d727933373c3e14500d0bdfae 
>   3rdparty/stout/tests/set_tests.cpp a271e99181785f6516f1adab465faa33fdc535f0 
> 
> Diff: https://reviews.apache.org/r/52037/diff/
> 
> 
> Testing
> ---
> 
> `make check` in various environments
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 52091: Replaced `set` to `hashset` in perf interfaces.

[haosdent huang]

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

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


Repository: mesos


Description
---

Replaced `set` to `hashset` in perf interfaces.


Diffs
-

  src/linux/perf.hpp c7dfe0ac4965a450b25085715502f49bee972b5c 
  src/linux/perf.cpp 16a30eac9d346b14f2245128003866b9894bb94a 

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


Testing
---

In https://reviews.apache.org/r/52092/, we change from

```
set cgroups;
perf::sample(events, cgroups, flags.perf_duration)
```

to 

```
perf::sample(events, infos.keys(), flags.perf_duration)
```

`infos.keys()` return a `hashset`. So need to change the perf interfaces to use 
`hashset` first.


Thanks,

haosdent huang

Review Request 52092: Avoided to concat cgroup internally in subsystems.

[haosdent huang]

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

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


Repository: mesos


Description
---

To avoid using `path::join(flags.cgroups_root, containerId.value())` to
concat cgroup internally in subsystems, pass cgroup to the subsystem
interfaces directly. And since containerId is not necessary for the
subsystems anymore, remove it in the subsystem interfaces as well.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
c2fcc0b21c5b1e51cb38b61245d4bbd3856a9512 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 
6594110bd45a71c6d41a24b3f5b73c4a4e3a7335 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
05cb8b8efadd2355789d08ce5c9da9d3de0fc72a 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 
c1f1004d7578f59dfa9e28323e7a55df41c4cb6b 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
5912c6504a375178b892bf8099d5f75fa9e91c05 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
10fbbc9cb39a3b41d64cc090b0e890a2416b2306 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
a35ecb073acefb903d7e8c4737d6cd048a2b494d 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 
b5d5ed233c0c8ddc339d2550c0f57a02dd3f14c3 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
0afc24867eb0b1949e95b3f5a8914be013dbf531 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
a1c87ce2ace33f1a779e843578f55d7502562c8d 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
bab10ae5d488585603db2637ae15ee02b1982354 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp 
16208655e16b81f1e4bbf97bf5e32e75f4c3f3a9 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
93539f1fc8265f66b62294c22f4eaba704b8b876 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 
5a0adfdb8705ae6b20c61a827380142e418c0ae4 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp 
cc39f287a64a4125260597e74784dc0847953376 

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


Testing
---


Thanks,

haosdent huang

Review Request 52090: Fixed compile error in ppc64le.

[haosdent huang]

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

Review request for mesos, Zhiwei Chen, Gilbert Song, Jie Yu, and Qian Zhang.


Repository: mesos


Description
---

Fixed compile error in ppc64le.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp 
a1c87ce2ace33f1a779e843578f55d7502562c8d 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
bab10ae5d488585603db2637ae15ee02b1982354 

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


Testing
---

Fix the error saw in 
https://builds.apache.org/job/Mesos-PPC64LE/154/BUILDTOOL=autotools,COMPILER=gcc,CONFIGURATION=--verbose,ENVIRONMENT=GLOG_v=1%20MESOS_VERBOSE=1,label_exp=ppc64le/console


Thanks,

haosdent huang

Re: Review Request 52083: Changed reconciliation for unregistering, reregistering agents.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50235, 50416, 50417, 50418, 50422, 50699, 50700, 50701, 
50702, 50703, 50704, 50705, 50706, 50707, 50844, 50845, 50846, 51020, 51371, 
51374, 51375, 51376, 51377, 51021, 51706, 51653, 51707, 51805, 51845, 51891, 
51909, 51913, 51953, 51954, 51955, 51956, 51957, 51958, 52039, 52083]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 20, 2016, 3:26 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52083/
> ---
> 
> (Updated Sept. 20, 2016, 3:26 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6206
> https://issues.apache.org/jira/browse/MESOS-6206
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, explicit reconciliation for an agent that was in the process
> of reregistering or unregistering returned no results. This degree of
> cleverness seems unwarranted: if the agent hasn't completed the
> reregistration or unregistration process, it seems quite reasonable for
> the master to return the previous state of the agent (this is what the
> framework would observe if their reconcile request lost the race with
> the reregister/unregister, anyway).
> 
> Note that since reregistering agents are no longer considered to be "in
> transition", we need to slightly adjust the rules for how we update the
> `slaves.recovered` collection in the master: an agent remains in the
> "recovered" collection until it has been marked reachable in the
> registry (rather than removing it from "recovered" as soon as the
> reregistration process beings). This is more consistent with how we
> manage the other collections in the master anyway.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 35db198748b8652eb53e17f592f6b40d1e6a3ed9 
>   src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
>   src/tests/master_tests.cpp e6c8362da6a5669e2a2d18f6eb4e454365a84f60 
>   src/tests/reconciliation_tests.cpp 1412090299df388456f04ed58a1d384ce3ff550a 
> 
> Diff: https://reviews.apache.org/r/52083/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX, Linux.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Zhitao Li]

> On Sept. 20, 2016, 5:21 p.m., Gilbert Song wrote:
> > @Zhitao, the patch LGTM. Thanks for working on it. Could you please rebase?

Done.


- Zhitao


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


On Sept. 20, 2016, 5:28 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51124/
> ---
> 
> (Updated Sept. 20, 2016, 5:28 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/backend.hpp 
> 93819c887f2f801f06aae7020084b56cc81ce818 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> a5ac13bc17b8f291472b622ebfdd0cd1dd072273 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> 4c5cdb660dea205aea29d217ba80d845d1108fdf 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> 19d44c6a9a2cd5f8de79f33c76e9d79900ab003f 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> e57bb3d2fb4e67e9caae416824a6a748db1460a1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 387f28a331813c75a509b4a31dbbdc764080b8c1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> f37c45ccfa572876dfbba6a0797c223896db5a7f 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 7c9a7cd5c733f74e10316fc1234115e6820cc2cb 
> 
> Diff: https://reviews.apache.org/r/51124/diff/
> 
> 
> Testing
> ---
> 
> 1. Make sure `OverlayBackendTest.*` passes;
> 2. Use mesos-execute to provision a container using overlay backend. Observed 
> following log lines:
> 
> ```
> I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
>  to '/tmp/NcmRZt'
> I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with 
> overlayfs: 
> 'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/2
 5d9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
> ...
> (after executor exited)
> I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory 
> '/tmp/NcmRZt' pointed by 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Zhitao Li]

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

(Updated Sept. 20, 2016, 5:28 p.m.)


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


Changes
---

Rebase with latest master. No conflict.


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


Repository: mesos


Description
---

Support more layers through symlink for overlay backend.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/backend.hpp 
93819c887f2f801f06aae7020084b56cc81ce818 
  src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
a5ac13bc17b8f291472b622ebfdd0cd1dd072273 
  src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
4c5cdb660dea205aea29d217ba80d845d1108fdf 
  src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
19d44c6a9a2cd5f8de79f33c76e9d79900ab003f 
  src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d 
  src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e 
  src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
e57bb3d2fb4e67e9caae416824a6a748db1460a1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
387f28a331813c75a509b4a31dbbdc764080b8c1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
f37c45ccfa572876dfbba6a0797c223896db5a7f 
  src/tests/containerizer/provisioner_backend_tests.cpp 
7c9a7cd5c733f74e10316fc1234115e6820cc2cb 

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/25d
 9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
...
(after executor exited)
I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory 
'/tmp/NcmRZt' pointed by 
'/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
```


Thanks,

Zhitao Li

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



@Zhitao, the patch LGTM. Thanks for working on it. Could you please rebase?

- Gilbert Song


On Aug. 25, 2016, 9:22 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51124/
> ---
> 
> (Updated Aug. 25, 2016, 9:22 a.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/backend.hpp 
> 93819c887f2f801f06aae7020084b56cc81ce818 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> a5ac13bc17b8f291472b622ebfdd0cd1dd072273 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> 4c5cdb660dea205aea29d217ba80d845d1108fdf 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> 19d44c6a9a2cd5f8de79f33c76e9d79900ab003f 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> e57bb3d2fb4e67e9caae416824a6a748db1460a1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 387f28a331813c75a509b4a31dbbdc764080b8c1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> ad19feccf08112f94d6514a134963c54ca541e88 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 7c9a7cd5c733f74e10316fc1234115e6820cc2cb 
> 
> Diff: https://reviews.apache.org/r/51124/diff/
> 
> 
> Testing
> ---
> 
> 1. Make sure `OverlayBackendTest.*` passes;
> 2. Use mesos-execute to provision a container using overlay backend. Observed 
> following log lines:
> 
> ```
> I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
>  to '/tmp/NcmRZt'
> I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with 
> overlayfs: 
> 'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/2
 5d9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
> ...
> (after executor exited)
> I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory 
> '/tmp/NcmRZt' pointed by 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51986: Modified `cleanup` to be nested container aware.

[Avinash sridharan]

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

(Updated Sept. 20, 2016, 5:10 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Modified `cleanup` to be nested container aware.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
359479083894e887647a694a1a133dce44817073 

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


Testing
---

make
make check


Thanks,

Avinash sridharan

Re: Review Request 52014: Added `supportsNesting` method to `network/cni` isolator.

[Avinash sridharan]

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

(Updated Sept. 20, 2016, 5:10 p.m.)


Review request for mesos, Jie Yu and Joseph Wu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added `supportsNesting` method to `network/cni` isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
949da8f70fb1cd13d6359780b032cb170693ea3e 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
359479083894e887647a694a1a133dce44817073 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 51857: Modified the `prepare` and `isolate` methods to be nesting aware.

[Avinash sridharan]

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

(Updated Sept. 20, 2016, 5:10 p.m.)


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


Changes
---

Merged the `prepare` and `isolate` patches, and added comemnts to `prepare` to 
highlgiht the cases where we need to store the container information in the 
`infos` structure.


Summary (updated)
-

Modified the `prepare` and `isolate` methods to be nesting aware.


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


Repository: mesos


Description (updated)
---

The network file setup in the `network/cni` isolator is now nesting
aware. Since the children share the network and UTS namespace with the
parent, the network files need to be created only for the parent
container. For the child containers, the network files will be simply
a bind mount of the parents network files.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
359479083894e887647a694a1a133dce44817073 

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


Testing
---

make 
make check
and
sudo ./bin/mesos-tests.sh

The only tests that failed were the SUDO make check tests:
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
[  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
[  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume


Thanks,

Avinash sridharan

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Jiang Yan Xu]

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




include/mesos/resources.hpp (lines 60 - 64)


We can get rid of this forward declaration if we can get rid of the 
internal convertJSON.



include/mesos/resources.hpp (line 183)


Use `/**`.



include/mesos/resources.hpp (line 201)


This is too similar to 

```
static Try parse(
  const std::string& text,
  const std::string& defaultRole = "*");
```

I was originally suggesting moving over convertJSON but I guess it's more 
suitable here in the following form:

```
Try fromJSONString(
const std::string& text,
const std::string& defaultRole)
```

Similarly for simple flat strings:

```
Try fromSimpleString(
const std::string& text,
const std::string& defaultRole)
```

When we have both, then 

```
static Try parse(
const std::string& text,
const std::string& defaultRole = "*");
```

becomes a mere convenience method that calls the above two methods in 
sequence. The convenience method can be simply documented as:

```
  /** 
   * Returns Resources from an input string.
   *
   * Parses Resources from text in the form of a JSON array (see
   * `fromJSONString()`). If that fails, parses text in the simple 
   * string form (see `fromSimpleString()`).
   */
```

How does it sound?


- Jiang Yan Xu


On Sept. 19, 2016, 3:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 3:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51631: Tracked recovered and prepared cgroups subsystems for containers.

[Jie Yu]

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



Can you follow up with some tests on this? Should be similar to 
SlaveRecoveryTest. For example:
1) start a slave with cgroups/cpu
2) launch a task
3) stop the slave and start a new one with cgroups/cpu,cgroups/mem
4) make sure the old task is ok
5) launch a new task, make sure it is ok

- Jie Yu


On Sept. 20, 2016, 1:27 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51631/
> ---
> 
> (Updated Sept. 20, 2016, 1:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6063
> https://issues.apache.org/jira/browse/MESOS-6063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Recover newly added cgroups subsystems on existing containers would
> fail, and continue to perform the `update` and other operations of
> the newly added subsystems on them don't make sense. This patch add
> the tracking for the recovered or prepared cgroups subsystems of a
> container and skip performing unnecessary subsystem operations on the
> container if the subsystem is never recovered or prepared.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> cf143eef2411b17b775579b1be07c927b64ea835 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 937356b3ef76408bbb01ab699b7a2bd097cdfe82 
> 
> Diff: https://reviews.apache.org/r/51631/diff/
> 
> 
> Testing
> ---
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*CGROUPS*" --verbose
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

[Guangya Liu]

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




src/tests/containerizer/common_containerizer_tests.cpp (line 96)


move this to #103



src/tests/containerizer/common_containerizer_tests.cpp (lines 110 - 111)


new line



src/tests/containerizer/common_containerizer_tests.cpp (lines 112 - 113)


new line



src/tests/containerizer/common_containerizer_tests.cpp (lines 121 - 122)


new line



src/tests/containerizer/common_containerizer_tests.cpp (lines 123 - 124)


new line



src/tests/containerizer/common_containerizer_tests.cpp (lines 146 - 148)


This maybe not enough, what about the following:

```
EXPECT_SOME_EQ(2, resources->cpus());
EXPECT_SOME_EQ(Megabytes(1024), resources->mem());
EXPECT_SOME_EQ(Megabytes(1024), resources->disk());
```

Ditto for others.



src/tests/containerizer/common_containerizer_tests.cpp (line 235)


two spaces

ditto for others



src/tests/containerizer/common_containerizer_tests.cpp (lines 235 - 236)


new line



src/tests/containerizer/common_containerizer_tests.cpp (lines 307 - 308)


Adjust to make this less jagged.
```
// Resources with cpus, mem and disk (default and
// PATH disks with a specific size).
```



src/tests/containerizer/common_containerizer_tests.cpp (lines 335 - 336)


Adjust to make this less jagged.
```
// Resources with cpus, mem and disk (default and
// PATH disks with a specific size).
```



src/tests/containerizer/common_containerizer_tests.cpp (lines 368 - 369)


Adjust to make this less jagged.
```
// Resources with cpus, mem and disk (default and
// PATH/MOUNT disks with a specific size).
```



src/tests/containerizer/common_containerizer_tests.cpp (lines 418 - 419)


```
// Resources with cpus, mem and disk (default and
// PATH/MOUNT disks with a specific size).
```


- Guangya Liu


On 九月 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51880/
> ---
> 
> (Updated 九月 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests to determine disk size for MOUNT disks.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
>   src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51880/diff/
> 
> 
> Testing
> ---
> 
> All tests including the additional tests in this RR passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51631: Tracked recovered and prepared cgroups subsystems for containers.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51631]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 20, 2016, 1:27 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51631/
> ---
> 
> (Updated Sept. 20, 2016, 1:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6063
> https://issues.apache.org/jira/browse/MESOS-6063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Recover newly added cgroups subsystems on existing containers would
> fail, and continue to perform the `update` and other operations of
> the newly added subsystems on them don't make sense. This patch add
> the tracking for the recovered or prepared cgroups subsystems of a
> container and skip performing unnecessary subsystem operations on the
> container if the subsystem is never recovered or prepared.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> cf143eef2411b17b775579b1be07c927b64ea835 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 937356b3ef76408bbb01ab699b7a2bd097cdfe82 
> 
> Diff: https://reviews.apache.org/r/51631/diff/
> 
> 
> Testing
> ---
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*CGROUPS*" --verbose
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[haosdent huang]

> On Sept. 7, 2016, 7:21 p.m., Joris Van Remoortere wrote:
> > src/slave/slave.cpp, lines 559-561
> > 
> >
> > We should improve the error information here. Why can't we identify 
> > which original paths that were provided are overlapping?
> > If we need to improve a library function we can add a TODO here and 
> > follow up with a patch.
> 
> haosdent huang wrote:
> Hi, @jvanremoortere, do you mean we should check 
> `path::overlapping(source.mount().root())` without call `realpath`?
> 
> Joris Van Remoortere wrote:
> Hey @haosdent,
> No, I think using realpath is correct.
> I mean that a user currently won't have a good idea about *which* paths 
> are overlapping.
> Should `path::overlapping()` return an `Option` or some 
> other way to provide the list of actually overlapping paths so that we can 
> provide a more meaningful error message?

Make sense! Let me update.


- haosdent


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


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

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

[Joris Van Remoortere]

> On Sept. 7, 2016, 7:21 p.m., Joris Van Remoortere wrote:
> > src/slave/slave.cpp, lines 559-561
> > 
> >
> > We should improve the error information here. Why can't we identify 
> > which original paths that were provided are overlapping?
> > If we need to improve a library function we can add a TODO here and 
> > follow up with a patch.
> 
> haosdent huang wrote:
> Hi, @jvanremoortere, do you mean we should check 
> `path::overlapping(source.mount().root())` without call `realpath`?

Hey @haosdent,
No, I think using realpath is correct.
I mean that a user currently won't have a good idea about *which* paths are 
overlapping.
Should `path::overlapping()` return an `Option` or some other 
way to provide the list of actually overlapping paths so that we can provide a 
more meaningful error message?


- Joris


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


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

Re: Review Request 52082: Tweaked initialization order in executor driver.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 20, 2016, 2:26 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52082/
> ---
> 
> (Updated Sept. 20, 2016, 2:26 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We should initialize libprocess before calling into any libprocess
> facilities (in this case, `process::Latch`).
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp c1cda5aca81dd8c73c50dd01ce49ef69805bbf09 
> 
> Diff: https://reviews.apache.org/r/52082/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 20, 2016, 1:23 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 481-482
> > 
> >
> > Is there any information we can provide here about where we are looking 
> > for the defaults to help the user identify the problem?
> 
> Till Toenshoff wrote:
> The defaults are baked into the openssl libraries at compile-time. The 
> user may override those using openssl's `SSL_CERT_FILE` and `SSL_CERT_DIR`. 
> There seems to be no public way to extract those paths back out to get them 
> displayed.
> 
> Quick background: that information is obviously attached to the context, 
> internally that specific certificate stuff is handled by the 
> `X509_STORE`-API. The above call effectively attaches a new cert store to our 
> context and populates it with the content of the given file/dir path. The 
> result is a (bunch of) certificate/s attached. The source path however is 
> unknown later on - at least from the API point of view. So all we could 
> possibly show here are the context attached certificates but not their source 
> locations.
> 
> Till Toenshoff wrote:
> The documentation totally stays silent on `X509_get_default_cert_file` 
> and `X509_get_default_cert_dir`. However after checking their 
> implementations, to me it seems as if they would never return the value/s of 
> user-environment supplied overrides (e.g. `SSL_CERT_FILE`) but only the baked 
> in defaults. So instead of being helpful, in cases where the user used the 
> OpenSSL specific environment variables the output of those functions would be 
> even more confusing. In other words, if the user set `SSL_CERT_FILE` towards 
> `/foo/bar/cert.pem`, calling `X509_get_default_cert_file` would yield the 
> baked in default (e.g. `SSLCERTS:cert.pem`).

At this point I only see the following as an option for displaying helpful 
information:
Emulate the internal openssl bizzlogic by first checking the env var 
`SSL_CERT_FILE` for content - if set, return that one in our debug logging -- 
if not set, return the baked in default. See 
https://github.com/openssl/openssl/blob/master/crypto/x509/by_file.c#L50 for 
the details on their implementation.


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 20, 2016, 1:23 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 481-482
> > 
> >
> > Is there any information we can provide here about where we are looking 
> > for the defaults to help the user identify the problem?
> 
> Till Toenshoff wrote:
> The defaults are baked into the openssl libraries at compile-time. The 
> user may override those using openssl's `SSL_CERT_FILE` and `SSL_CERT_DIR`. 
> There seems to be no public way to extract those paths back out to get them 
> displayed.
> 
> Quick background: that information is obviously attached to the context, 
> internally that specific certificate stuff is handled by the 
> `X509_STORE`-API. The above call effectively attaches a new cert store to our 
> context and populates it with the content of the given file/dir path. The 
> result is a (bunch of) certificate/s attached. The source path however is 
> unknown later on - at least from the API point of view. So all we could 
> possibly show here are the context attached certificates but not their source 
> locations.

The documentation totally stays silent on `X509_get_default_cert_file` and 
`X509_get_default_cert_dir`. However after checking their implementations, to 
me it seems as if they would never return the value/s of user-environment 
supplied overrides (e.g. `SSL_CERT_FILE`) but only the baked in defaults. So 
instead of being helpful, in cases where the user used the OpenSSL specific 
environment variables the output of those functions would be even more 
confusing. In other words, if the user set `SSL_CERT_FILE` towards 
`/foo/bar/cert.pem`, calling `X509_get_default_cert_file` would yield the baked 
in default (e.g. `SSLCERTS:cert.pem`).


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51784, 51930, 51931, 52081, 50271]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Sept. 20, 2016, 11:40 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Sept. 20, 2016, 11:40 a.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This isolator evaluates agent allowed capabilities and passes net
> capabilities on to `mesos-containerizer` which enforces the
> capabilities.
> 
> Capability information is passed via a new field in
> `ContainerLaunchInfo`.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 20db010ea158a813034b41ce9cddac7d8317 
>   src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
>   src/Makefile.am 478fb5ae01dbdf1a735680edf7c5f30867726e07 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.hpp 
> 859990cb85e9e8c06400397256cfc512f0811800 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
>   src/slave/flags.hpp 3952d04f6a00ac1dca1adf2bea7cc6e415620ce5 
>   src/tests/containerizer/isolator_tests.cpp 
> 93ce75180520d382f63ce7323be844fe43c6594e 
> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51930: Introduced Linux capabilities support for Mesos executor.

[Benjamin Bannier]

> On Sept. 20, 2016, 2:36 a.m., Jie Yu wrote:
> > src/launcher/executor.cpp, line 817
> > 
> >
> > It looks wierd that we have ifdef here but not have that for the field 
> > member. I'd suggest we don't do any ifdef for flags as they are optional 
> > anyway. Otherwise, you'll have to change all the constructors as well, 
> > which is a little messy.

Hmm, that would mean that that flag would be there under e.g., macos or 
windows, but have no effect which we'd need to patch by either documentation or 
adding a validation step.

I am not sure I understand your comment re:constructors; the member is there 
and can be default constructed. I think the code should be fine as is.


- Benjamin


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


On Sept. 19, 2016, 4:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51930/
> ---
> 
> (Updated Sept. 19, 2016, 4:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces Linux capability-based security the Mesos
> exector. A new flag `capabilities` is introduced to optionally specify
> the capabilities tasks launched by the Mesos executor are allowed to
> use.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 
>   src/launcher/posix/executor.hpp 9e467263dcc0623fce83b18887896547db9aa16b 
>   src/launcher/posix/executor.cpp 7c40ebebb983b7977526fee123bf07c86fbb5e50 
> 
> Diff: https://reviews.apache.org/r/51930/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/50271/ on ubuntu-14.04 w/ 
> gcc-4.8.4 w/o optimizations.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 52036: Moved user of stout's `Set` to `std::set`.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

After the move to C++11 stout's `Set` offers no benefits over
`std::set`, and will be removed in a subsequent change set.


Diffs
-

  src/linux/capabilities.hpp e3203f80ae1b5448460313e9289d3dd5fff54475 
  src/linux/capabilities.cpp abaf7218a449177369dcc3f55223b678e3112271 
  src/log/log.cpp be511d9a233a663cc29014536284d26737947c15 
  src/slave/containerizer/mesos/launch.cpp 
fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
  src/tests/containerizer/capabilities_test_helper.cpp 
4c617bfc3e2a3212f7b18303b5bf7823161cb4af 
  src/tests/containerizer/capabilities_tests.cpp 
3ba46404c389385e02d2a1789e33dab3e8f15902 

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


Testing
---

`make check` in various environments


Thanks,

Benjamin Bannier

Review Request 52037: Removed stout's `Set`.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Removed stout's `Set`.


Diffs
-

  3rdparty/stout/Makefile.am fc2d2b20ec48b54b63f971c9887a492e61bb4a26 
  3rdparty/stout/include/stout/set.hpp bd7a9b9af51fc947cf105328843249e396f0282b 
  3rdparty/stout/include/stout/stringify.hpp 
b4d3e619d2a8fb346964771d9c1d8843a8c0f0b6 
  3rdparty/stout/tests/CMakeLists.txt f85dc998e8ba5f8d727933373c3e14500d0bdfae 
  3rdparty/stout/tests/set_tests.cpp a271e99181785f6516f1adab465faa33fdc535f0 
  src/linux/capabilities.hpp e3203f80ae1b5448460313e9289d3dd5fff54475 

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


Testing
---

`make check` in various environments


Thanks,

Benjamin Bannier

Re: Review Request 51631: Tracked recovered and prepared cgroups subsystems for containers.

[haosdent huang]

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

(Updated Sept. 20, 2016, 1:27 p.m.)


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


Changes
---

Updated dependencies.


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


Repository: mesos


Description
---

Recover newly added cgroups subsystems on existing containers would
fail, and continue to perform the `update` and other operations of
the newly added subsystems on them don't make sense. This patch add
the tracking for the recovered or prepared cgroups subsystems of a
container and skip performing unnecessary subsystem operations on the
container if the subsystem is never recovered or prepared.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
cf143eef2411b17b775579b1be07c927b64ea835 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
937356b3ef76408bbb01ab699b7a2bd097cdfe82 

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


Testing
---

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


Thanks,

haosdent huang

Re: Review Request 52056: Exposed unknown container case from Containerizer::destroy.

[Guangya Liu]

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




src/slave/containerizer/composing.cpp (line 312)


Do we need a swept for this? This update may make the file not consistent, 
some using `containers_.at(containerId)` whlile some using 
`containers_[containerId]`



src/slave/containerizer/composing.cpp (line 461)


Why kill this message? Ditto for others



src/slave/containerizer/mesos/containerizer.hpp (lines 107 - 108)


one line?



src/slave/containerizer/mesos/containerizer.hpp (lines 176 - 177)


one line?



src/tests/containerizer/composing_containerizer_tests.cpp (line 171)


s/mockContainerizer/mockContainerizer1



src/tests/containerizer/docker_containerizer_tests.cpp (line 3334)


why wait recover here?


- Guangya Liu


On 九月 20, 2016, 1:22 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52056/
> ---
> 
> (Updated 九月 20, 2016, 1:22 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently the callers of `destroy` cannot determine if the call
> succeeds or fails (without a secondary call to `wait()`).
> 
> This also allows the caller to distinguish between a failure and
> waiting on an unknown container. This is important for the upcoming
> agent child container API, as the end-user would benefit from the
> distinction.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> ef3c2ee22cf0ced35c64025c7f9f7ca75165d54d 
>   src/slave/containerizer/composing.cpp 
> 5ff3e65facd5831e2637ae1cee3ea01b21d0f6b2 
>   src/slave/containerizer/containerizer.hpp 
> f13669d0dfc4ce3287cfe630cabd0470dc765b51 
>   src/slave/containerizer/docker.hpp a378fa9772742b4934578efcec54aeeaf5791a83 
>   src/slave/containerizer/docker.cpp 110a1eb41b1ff7cb94f3630a1843d9f01efbe09c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 078ef4f4e7bf5e1522804a720c51cfa5518d8efd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 
>   src/tests/containerizer.hpp f1fd57945c09fb80b7790f9124843a5d4ea785ee 
>   src/tests/containerizer.cpp bda3e6f84f77a95f0eb9ff3aabdc9513f0f18b3f 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> 51aab33cd190b53328339e39fd12853714882454 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 28cd3fa66886dbdbae3fdeca77707147faafcb7a 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52056/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52080: Ensured that `Latch*` in exec, sched drivers is initialized.

[Neil Conway]

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

(Updated Sept. 20, 2016, 1:11 p.m.)


Review request for mesos and Michael Park.


Changes
---

Use a different approach.


Summary (updated)
-

Ensured that `Latch*` in exec, sched drivers is initialized.


Repository: mesos


Description (updated)
---

In error code paths, this value was previously left uninitialized, which
might subsequently result in attempting to delete an uninitialized
pointer value.


Diffs (updated)
-

  src/exec/exec.cpp c1cda5aca81dd8c73c50dd01ce49ef69805bbf09 
  src/sched/sched.cpp 6a1a201202b8241e853a821ffcec49187a5bdf35 

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


Testing
---

`make check` on OSX.


Thanks,

Neil Conway

Review Request 52082: Tweaked initialization order in executor driver.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

We should initialize libprocess before calling into any libprocess
facilities (in this case, `process::Latch`).


Diffs
-

  src/exec/exec.cpp c1cda5aca81dd8c73c50dd01ce49ef69805bbf09 

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


Testing
---

`make check` on OSX.


Thanks,

Neil Conway

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 20, 2016, 1:23 a.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 481-482
> > 
> >
> > Is there any information we can provide here about where we are looking 
> > for the defaults to help the user identify the problem?

The defaults are baked into the openssl libraries at compile-time. The user may 
override those using openssl's `SSL_CERT_FILE` and `SSL_CERT_DIR`. There seems 
to be no public way to extract those paths back out to get them displayed.

Quick background: that information is obviously attached to the context, 
internally that specific certificate stuff is handled by the `X509_STORE`-API. 
The above call effectively attaches a new cert store to our context and 
populates it with the content of the given file/dir path. The result is a 
(bunch of) certificate/s attached. The source path however is unknown later on 
- at least from the API point of view. So all we could possibly show here are 
the context attached certificates but not their source locations.


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 19, 2016, 7:02 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 509-510
> > 
> >
> > I'd prefer the parentheses containing the `stringify(error)` to mention 
> > OpenSSL, since the number of ambiguous/confusing otherwise.  
> > 
> > i.e. `(OpenSSL error #___): ...`
> > 
> > Ditto other error messages.
> 
> Till Toenshoff wrote:
> I agree, those are confusing. On a second thought, shall we simply remove 
> those error-code-numbers? I was trying to follow the initial example but now 
> feel we should complete get rid of those as we can expect the error string to 
> be helpful.
> 
> Joseph Wu wrote:
> I think the error codes might be useful for debugging.  I'm guessing the 
> error strings may change between OpenSSL versions (I wonder if there's 
> localization too), but the error codes will not.
> 
> Joris Van Remoortere wrote:
> The error codes are very helpful. Please don't remove them. As Joseph has 
> suggested, they're the easiest way to search for references on google, 
> openssl mailing list, etc.

Ok, you guys got me convinced :)  thanks!


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52025: Replaced unchecked `dynamic_cast` with `static_cast`.

[Michael Park]

> On Sept. 20, 2016, 10:16 a.m., Michael Park wrote:
> > Just curious whether this is just a drive-by cleanup, or whether it's 
> > caused us problems?
> > 
> > Also, could we add a `static_assert`? something like:
> > ```
> > static_assert(
> > std::is_base_of::value,
> > "`Process` requires the CRTP pattern. Please have `T` inherit from 
> > `Process`.");
> > ```
> 
> Benjamin Bannier wrote:
> This is a drive-by fix triggered by this coverity report 
> https://scan5.coverity.com/reports.htm#v42942/p10429/fileInstanceId=99295949=28546354=1063931.
>  Coverity warns that `dynamic_cast` could in principle return a nullptr here. 
> AFAIK this has not caused any issues (this code has been around since 2013).
> 
> Re:`static_assert`, do we need this? We perform a `static_cast` of `this` 
> to `T*` (this is of course all at compile time) so compilers can and already 
> do verify that inheritance relationship for us, e.g., with
> 
> template  struct Base {
>   virtual ~Base() = default;
>   const T *get() const { return static_cast(this); }
> };
> 
> struct A {
>   virtual ~A() = default;
> };
> 
> struct B : public Base {
>   virtual ~B() = default;
> };
> 
> int main() {
>   B b;
>   b.get();
> }
> 
> I get with clang-3.9
> 
> test.cpp:3:12: error: static_cast from 'const Base *' to 'const A 
> *', which are not related by inheritance, is not allowed
> return static_cast(this);
>^~~~
> test.cpp:13:5: note: in instantiation of member function 
> 'Base::get' requested here
>   b.get();
> ^
> 1 error generated.
> 
> and with g++-6
> 
> test.cpp: In instantiation of 'const T* Base::get() const [with T 
> = A]':
> test.cpp:13:9:   required from here
> test.cpp:3:12: error: invalid static_cast from type 'const Base*' 
> to type 'const A*'
>  return static_cast(this);
> ^~~~
> 
> I am not sure if any of this is required, but also cannot imagine 
> anything useful a compiler could do with code where `T` and `Process` are not 
> part of the same inheritance hierarchy (and that is what `std::is_base_of` 
> checks). Note that `dynamic_cast` can perform cross casts, and don't seem to 
> trigger these warnings here.
> 
> I did add a `static_assert` to `self` and a compile of the code base 
> under OS X brought up no issues.

ah, right. awesome. thanks for the analysis!


- Michael


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


On Sept. 19, 2016, 10:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52025/
> ---
> 
> (Updated Sept. 19, 2016, 10:19 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Process` is a CRTP class where we know that `T` is a valid derived
> class. Also, it is customary to use `static_cast` when implementing
> CRTP methods.
> 
> This change uses a `static_cast` instead of a `dynamic_cast`;
> `dynamic_cast` can result in a `nullptr` which should be checked. Here
> this is not required, so `static_cast` is good enough for this use
> case.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 54c7d2e7ed3923ab15ab86e36552b023f9de5215 
> 
> Diff: https://reviews.apache.org/r/52025/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang/trunk w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52025: Replaced unchecked `dynamic_cast` with `static_cast`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 19, 2016, 10:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52025/
> ---
> 
> (Updated Sept. 19, 2016, 10:19 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Process` is a CRTP class where we know that `T` is a valid derived
> class. Also, it is customary to use `static_cast` when implementing
> CRTP methods.
> 
> This change uses a `static_cast` instead of a `dynamic_cast`;
> `dynamic_cast` can result in a `nullptr` which should be checked. Here
> this is not required, so `static_cast` is good enough for this use
> case.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 54c7d2e7ed3923ab15ab86e36552b023f9de5215 
> 
> Diff: https://reviews.apache.org/r/52025/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang/trunk w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51631: Tracked recovered and prepared cgroups subsystems for containers.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: [51631, 51791, 51790, 51789, 51788, 51794, 51787, 51783, 51835]

Error:
Circular dependency detected for review 51783.Please fix the 'depends_on' field.

- Mesos ReviewBot


On Sept. 20, 2016, 6:20 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51631/
> ---
> 
> (Updated Sept. 20, 2016, 6:20 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6063
> https://issues.apache.org/jira/browse/MESOS-6063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Recover newly added cgroups subsystems on existing containers would
> fail, and continue to perform the `update` and other operations of
> the newly added subsystems on them don't make sense. This patch add
> the tracking for the recovered or prepared cgroups subsystems of a
> container and skip performing unnecessary subsystem operations on the
> container if the subsystem is never recovered or prepared.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> cf143eef2411b17b775579b1be07c927b64ea835 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 937356b3ef76408bbb01ab699b7a2bd097cdfe82 
> 
> Diff: https://reviews.apache.org/r/51631/diff/
> 
> 
> Testing
> ---
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*CGROUPS*" --verbose
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52025: Replaced unchecked `dynamic_cast` with `static_cast`.

[Benjamin Bannier]

> On Sept. 20, 2016, 12:16 p.m., Michael Park wrote:
> > Just curious whether this is just a drive-by cleanup, or whether it's 
> > caused us problems?
> > 
> > Also, could we add a `static_assert`? something like:
> > ```
> > static_assert(
> > std::is_base_of::value,
> > "`Process` requires the CRTP pattern. Please have `T` inherit from 
> > `Process`.");
> > ```

This is a drive-by fix triggered by this coverity report 
https://scan5.coverity.com/reports.htm#v42942/p10429/fileInstanceId=99295949=28546354=1063931.
 Coverity warns that `dynamic_cast` could in principle return a nullptr here. 
AFAIK this has not caused any issues (this code has been around since 2013).

Re:`static_assert`, do we need this? We perform a `static_cast` of `this` to 
`T*` (this is of course all at compile time) so compilers can and already do 
verify that inheritance relationship for us, e.g., with

template  struct Base {
  virtual ~Base() = default;
  const T *get() const { return static_cast(this); }
};

struct A {
  virtual ~A() = default;
};

struct B : public Base {
  virtual ~B() = default;
};

int main() {
  B b;
  b.get();
}

I get with clang-3.9

test.cpp:3:12: error: static_cast from 'const Base *' to 'const A *', 
which are not related by inheritance, is not allowed
return static_cast(this);
   ^~~~
test.cpp:13:5: note: in instantiation of member function 'Base::get' 
requested here
  b.get();
^
1 error generated.

and with g++-6

test.cpp: In instantiation of 'const T* Base::get() const [with T = A]':
test.cpp:13:9:   required from here
test.cpp:3:12: error: invalid static_cast from type 'const Base*' to 
type 'const A*'
 return static_cast(this);
^~~~

I am not sure if any of this is required, but also cannot imagine anything 
useful a compiler could do with code where `T` and `Process` are not part of 
the same inheritance hierarchy (and that is what `std::is_base_of` checks). 
Note that `dynamic_cast` can perform cross casts, and don't seem to trigger 
these warnings here.

I did add a `static_assert` to `self` and a compile of the code base under OS X 
brought up no issues.


- Benjamin


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


On Sept. 19, 2016, 12:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52025/
> ---
> 
> (Updated Sept. 19, 2016, 12:19 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Process` is a CRTP class where we know that `T` is a valid derived
> class. Also, it is customary to use `static_cast` when implementing
> CRTP methods.
> 
> This change uses a `static_cast` instead of a `dynamic_cast`;
> `dynamic_cast` can result in a `nullptr` which should be checked. Here
> this is not required, so `static_cast` is good enough for this use
> case.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 54c7d2e7ed3923ab15ab86e36552b023f9de5215 
> 
> Diff: https://reviews.apache.org/r/52025/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang/trunk w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Benjamin Bannier]

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

(Updated Sept. 20, 2016, 1:40 p.m.)


Review request for mesos, Jay Guo and Jie Yu.


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


Repository: mesos


Description
---

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

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


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
20db010ea158a813034b41ce9cddac7d8317 
  src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
  src/Makefile.am 478fb5ae01dbdf1a735680edf7c5f30867726e07 
  src/slave/containerizer/mesos/containerizer.cpp 
e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
  src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.hpp 
859990cb85e9e8c06400397256cfc512f0811800 
  src/slave/containerizer/mesos/launch.cpp 
fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
  src/slave/flags.hpp 3952d04f6a00ac1dca1adf2bea7cc6e415620ce5 
  src/tests/containerizer/isolator_tests.cpp 
93ce75180520d382f63ce7323be844fe43c6594e 

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


Testing
---

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


Thanks,

Benjamin Bannier

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

[Benjamin Bannier]

> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/Makefile.am, line 834
> > 
> >
> > This should belong linux files below?

This isolator has no hard dependency on Linux so we can build it on all 
platforms. I think it would be great to have as much code as possible not 
behind `ifdef`s. What do you think?


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/Makefile.am, line 959
> > 
> >
> > Ditto.

See above.


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 99
> > 
> >
> > Not yours, but why this is not wrapped with ifdef linux?

This isolator builds fine under e.g., macos, so we can build it everywhere. I 
believe this is A Good Thing.


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 73
> > 
> >
> > Hum, the headers here is crazy (too many ifdef linux here). The order 
> > of this is not correct.
> > 
> > I suggest we group all linux related headers together.

I moved this to l.59 outside of any conditionally included code.

I also added https://reviews.apache.org/r/52081 up the chain to reorganize the 
includes here, could you shepherd that one?


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1242-1251
> > 
> >
> > I understand why you want to do this check, but we usually do not do 
> > such checks in containerizer. We rely on operator to properly configure the 
> > agent.
> > 
> > The correct solution I believe is to advertise agent isolation 
> > capabilities to the master and let master reject those tasks.
> > 
> > I would simply remove this check here for now.

Like you probably know I added this to satisfy a requirement from the design 
doc where we specified for the case when `--isolation=capabilities` is absent:

a) ContainerInfo::CapabilityInfo not provided:
All capabilities for the user. (no isolation)
b) ContainerInfo::CapabilityInfo provided, ContainerInfo::CapabilityInfo is 
empty:
Not supported. Agent will fail the task with appropriate reasoning.
c) ContainerInfo::CapabilityInfo is NOT empty:
Not supported. Agent will fail the task with appropriate reasoning.

Do you mean the design doc is wrong when it says this work should be performed 
in the agent (instead the master should be fail the task)? In that case I 
wonder if we wouldn't still need to check in the agent, e.g., to allow 
restarting agents with changed capabilities.


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp, lines 
> > 103-104
> > 
> >
> > Hum, if i specify `--allowed_capabilities` and the framework does not 
> > specify capabilities for TaskInfo.container. Will this work?
> > 
> > I think you need to consider three cases here:
> > 1) For command task that has rootfs. In that case, we need to make sure 
> > when we launch the executor, we don't specify capabilities (keep it None() 
> > in launchInfo, indicating that we don't want to limit capabilities). This 
> > is because the executor will perform priviledged operations like pivot_root 
> > and setuid, we want to make sure it is not restricted.
> > 2) For command task that has no rootfs. In that case, simply set 
> > launchInfo.capabilities, meaning that when launching the executor, we limit 
> > the capabilities of it. So the task will inherit the capabilities when the 
> > executor fork-exec the task.
> > 3) For custom executor case, simply set launchInfo.capabilities.
> > 
> > To check if it is a command task is by checking 
> > containerConfig.has_task_info().

You are right, there are two issues here (improper handling of the custom 
executor case, and incorrect setup for command task with rootfs of in case only 
agent `--allowed_capabilities` are give.

I simplified and fixed the logic here like you suggested above.


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp, line 94
> > 
> >
> > I don't follow the if/else here. Should we simply calculate "what will 
> > be the capabilities for the task/executor, depending on what is inside 
> > containerConfig.container_info() and flags.allowed_capabilities" first, and 
> > then, depending on if the container is a command task (w or w/ rootfs) or 
> > custom executor, setting launchInfo 

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

[Benjamin Bannier]

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

(Updated Sept. 20, 2016, 1:12 p.m.)


Review request for mesos, Jay Guo and Jie Yu.


Changes
---

Addressed one round of comments from Jie.


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


Repository: mesos


Description (updated)
---

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

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


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
20db010ea158a813034b41ce9cddac7d8317 
  src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
  src/Makefile.am 478fb5ae01dbdf1a735680edf7c5f30867726e07 
  src/slave/containerizer/mesos/containerizer.cpp 
e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
  src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.hpp 
859990cb85e9e8c06400397256cfc512f0811800 
  src/slave/containerizer/mesos/launch.cpp 
fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
  src/slave/flags.hpp 3952d04f6a00ac1dca1adf2bea7cc6e415620ce5 
  src/tests/containerizer/isolator_tests.cpp 
93ce75180520d382f63ce7323be844fe43c6594e 

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


Testing
---

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


Thanks,

Benjamin Bannier