Re: Review Request 56711: Fixed fetcher to not pick up environment variables it should not see.

[Adam B]

> On Feb. 16, 2017, 1:39 a.m., Adam B wrote:
> > src/slave/containerizer/fetcher.cpp, lines 827-828
> > 
> >
> > How confident are you that the fetcher doesn't actually need any of 
> > these env vars? How can we test/prove it?
> 
> Till Toenshoff wrote:
> So far I simply tried to identify such possible conflicts by looking at 
> the code.
> 
> The `mesos-fetcher` itself is offering support for 
> `mesos::internal::logging::Flags` which add the following options:
> ```
>   add(::quiet,
>   add(::logging_level,
>   add(::log_dir,
>   add(::logbufsecs,
>   add(::initialize_driver_logging,
>   add(::external_log_file,
> ```
> All of the above are certainly supported by use if `MESOS_` prefixed 
> environment variable names. Other than that, the `mesos-fetcher` has support 
> for `MESOS_FETCHER_INFO` which we explicitly pass on even after that patch we 
> are discussing here. 
> So the things to look at should be limited towards the candidates covered 
> by those `Flags`. To be entirely clean we might want to consider passing 
> those candidates through explicitly.

Well, we certainly want to support MESOS_FETCHER_INFO and the others, so let's 
consider blacklist/whitelist.


- Adam


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


On Feb. 15, 2017, 5:31 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56711/
> ---
> 
> (Updated Feb. 15, 2017, 5:31 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Bugs: MESOS-6751 and MESOS-7133
> https://issues.apache.org/jira/browse/MESOS-6751
> https://issues.apache.org/jira/browse/MESOS-7133
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 9ec38dc95dddfcd990369d0146986e20b15da1a0 
> 
> Diff: https://reviews.apache.org/r/56711/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 55888: Test to ensure non-authorized users cannot launch tasks on agents.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/tests/slave_tests.cpp (lines 4952 - 4953)


"Authorize run task" is very broad. At least in the comment can we make 
clear that we are specifically testing using the RUN_TASK ACL to whitelist 
users?



src/tests/slave_tests.cpp (line 4958)


s/current/the current/



src/tests/slave_tests.cpp (line 4964)


s/current/the current/



src/tests/slave_tests.cpp (line 4965)


s/users/user/



src/tests/slave_tests.cpp (line 5040)


s/Status Updates/status updates/



src/tests/slave_tests.cpp (lines 5041 - 5054)


It feels that the following is more straightforward (which avoids the issue 
of ordering and looping/if conditions altogether).

```
hashmap statues(
{{status1->task_id(), status1.get()},
{{status2->task_id(), status2.get()}}});

ASSERT_TRUE(statues.contains(task1.task_id());
EXPECT_EQ(TASK_ERROR, statues.at(task1.task_id()).state());
EXPECT_EQ(TaskStatus::SOURCE_SLAVE, statues.at(task1.task_id()).source());
EXPECT_EQ(TaskStatus::REASON_TASK_UNAUTHORIZED, 
  statues.at(task1.task_id()).reason());

ASSERT_TRUE(statues.contains(task2.task_id());
EXPECT_EQ(TASK_RUNNING, statues.at(task2.task_id()).state());
```


- Jiang Yan Xu


On Feb. 7, 2017, 7:54 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55888/
> ---
> 
> (Updated Feb. 7, 2017, 7:54 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6953
> https://issues.apache.org/jira/browse/MESOS-6953
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test to ensure non-authorized users cannot launch tasks on agents.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 7f357a653335b559b5c78d4618926715dc4a63c7 
> 
> Diff: https://reviews.apache.org/r/55888/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51053: Update leveldb to 1.19.

[haosdent huang]

> On Aug. 22, 2016, 2:29 a.m., Vinod Kone wrote:
> > Have you also done compatibility tests mentioned in the ticket?
> 
> Tomasz Janiszewski wrote:
> No. It's minior upgrade from 1.18 to 1.19 but I can got thur 
> https://docs.google.com/document/d/1fv2OMvH6hVm6waacOejSrTJwUuDQeXlqqPDZjBmbcKU/edit#
>  and check if everything is OK.
> 
> haosdent huang wrote:
> I could help to run from 1.14 to 1.19 if it is necessary. :)
> 
> Tomasz Janiszewski wrote:
> I took care of it. Folloed steps presented by Haosdent it works the same. 
> Benchamrks were run as presented in ticket comments `MESOS_BENCHMARK=1 
> GTEST_FILTER="*BENCHMARK*.*/1" make check`. All tests performed on Ubuntu 
> 16.04 4.4.0-34-generic x86_64.
> I've got a problem with running `support/test-upgrade.py` it failed with 
> `mesos 1.0.0 framework failed` framework output ends with `Registered!`

As I test, `support/test-upgrade.py` requires pass all path with absolute 
paths, otherwise would get 

```
sh: ./mesos_1.4/build/src/test-executor: No such file or directory
```

in stderr of tasks.

After use absolute paths as the parameters of `support/test-upgrade.py`, 
everything works fine.


- haosdent


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


On Feb. 20, 2017, 4:41 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51053/
> ---
> 
> (Updated Feb. 20, 2017, 4:41 a.m.)
> 
> 
> Review request for mesos, Bing Li, Benjamin Mahler, Zhiwei Chen, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-970
> https://issues.apache.org/jira/browse/MESOS-970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Leveldb in modern version is required to support s390x and arm64.
> It's also required to replace default byte-wise comparator
> with varint comparator in \`src/log/leveldb.cpp\`.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am bbf9cfe7125b193003115f440a00c91e2e68f404 
>   3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
> eeb27860f6f95d297ccfe273ed76de5355b50ff8 
>   3rdparty/cmake/Versions.cmake ad23f3895b49d623709c0939d63a0529654788ef 
>   3rdparty/leveldb-1.19.patch PRE-CREATION 
>   3rdparty/leveldb-1.19.tar.gz PRE-CREATION 
>   3rdparty/leveldb-1.4.patch b899f0141d633b1ffb2321e573395256fc893b16 
>   3rdparty/leveldb-1.4.tar.gz 2ddbc0c2e02054406ff0ea43ddc10d14979de8d8 
>   3rdparty/versions.am 26f839cd8d454de91f10c2e616e78decc30eff1b 
>   LICENSE 7946d1d46174ff30459a33b3d0b44ab62672af0a 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/python/native_common/ext_modules.py.in 
> 2d4a45efa224b32f80ace4542a00062c5ccb06d5 
> 
> Diff: https://reviews.apache.org/r/51053/diff/
> 
> 
> Testing
> ---
> 
> make -j 8 distcheck
> 
> Test document: 
> https://docs.google.com/document/d/1fv2OMvH6hVm6waacOejSrTJwUuDQeXlqqPDZjBmbcKU/edit?usp=sharing
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 51053: Update leveldb to 1.19.

[haosdent huang]

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


Ship it!




Need to change `support/mesos-tidy/entrypoint.sh` as well, I would change this 
when commit it.

```
diff --git a/support/mesos-tidy/entrypoint.sh b/support/mesos-tidy/entrypoint.sh
index 4f03a6011..5dbaa6086 100755
--- a/support/mesos-tidy/entrypoint.sh
+++ b/support/mesos-tidy/entrypoint.sh
@@ -45,7 +45,7 @@ cmake --build 3rdparty --target http_parser-2.6.2 -- -j 
$(nproc)
 cmake --build 3rdparty --target libev-4.22 -- -j $(nproc) || true
 cmake --build 3rdparty --target libevent-2.1.5-beta -- -j $(nproc) || true

-cmake --build 3rdparty --target leveldb-1.4 -- -j $(nproc)
+cmake --build 3rdparty --target leveldb-1.19 -- -j $(nproc)
```

- haosdent huang


On Feb. 20, 2017, 4:41 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51053/
> ---
> 
> (Updated Feb. 20, 2017, 4:41 a.m.)
> 
> 
> Review request for mesos, Bing Li, Benjamin Mahler, Zhiwei Chen, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-970
> https://issues.apache.org/jira/browse/MESOS-970
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Leveldb in modern version is required to support s390x and arm64.
> It's also required to replace default byte-wise comparator
> with varint comparator in \`src/log/leveldb.cpp\`.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am bbf9cfe7125b193003115f440a00c91e2e68f404 
>   3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
> eeb27860f6f95d297ccfe273ed76de5355b50ff8 
>   3rdparty/cmake/Versions.cmake ad23f3895b49d623709c0939d63a0529654788ef 
>   3rdparty/leveldb-1.19.patch PRE-CREATION 
>   3rdparty/leveldb-1.19.tar.gz PRE-CREATION 
>   3rdparty/leveldb-1.4.patch b899f0141d633b1ffb2321e573395256fc893b16 
>   3rdparty/leveldb-1.4.tar.gz 2ddbc0c2e02054406ff0ea43ddc10d14979de8d8 
>   3rdparty/versions.am 26f839cd8d454de91f10c2e616e78decc30eff1b 
>   LICENSE 7946d1d46174ff30459a33b3d0b44ab62672af0a 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/python/native_common/ext_modules.py.in 
> 2d4a45efa224b32f80ace4542a00062c5ccb06d5 
> 
> Diff: https://reviews.apache.org/r/51053/diff/
> 
> 
> Testing
> ---
> 
> make -j 8 distcheck
> 
> Test document: 
> https://docs.google.com/document/d/1fv2OMvH6hVm6waacOejSrTJwUuDQeXlqqPDZjBmbcKU/edit?usp=sharing
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 56667: Added support for JSON Web Tokens.

[Greg Mann]

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



Regarding the description: I'm curious how exactly the current implementation 
isn't compliant with RFCs 7515/7519? The one thing I noticed was the lack of 
support for the 'crit' header parameter.


3rdparty/libprocess/include/process/jwt.hpp (lines 44 - 48)


Do we want to include support for the 'crit' header parameter for spec 
compliance?



3rdparty/libprocess/include/process/jwt.hpp (line 77)


Do you think it's worth using a `JSON::Object` for the header? This would 
let the module accommodate arbitrary header keys (AKA 'Private Header Parameter 
Names' from RFC-7515), which could be useful for users who want to use the 
module for other purposes?



3rdparty/libprocess/src/jwt.cpp (lines 74 - 77)


Just one newline here?


- Greg Mann


On Feb. 16, 2017, 9:35 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> ---
> 
> (Updated Feb. 16, 2017, 9:35 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JSON Web Tokens can be used to create claim-based access tokens and is
> typically used for HTTP authentication.
> This implementation is intended for internal use, e.g. Mesos is supposed
> to only parse tokens that it also created. It doesn't fully comply with
> RFC 7519. Currently the only supported cryptographic algorithm is HMAC
> with SHA-256.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56667/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56666: Added a HMAC SHA256 generator.

[Greg Mann]

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




3rdparty/libprocess/include/process/ssl/utilities.hpp (line 119)


s/,/./


- Greg Mann


On Feb. 16, 2017, 9:34 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5/
> ---
> 
> (Updated Feb. 16, 2017, 9:34 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> HMAC SHA256 can be used to create or verify message signatures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> c2f64a91a505675d568ddf5aa081125e4e32fe17 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 8aec613312eee3dd11d9df8c3828a5185407e073 
> 
> Diff: https://reviews.apache.org/r/5/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56375: Added a test to ensure offers with different roles cannot be combined.

[Jay Guo]

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

(Updated Feb. 20, 2017, 3:35 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.


Changes
---

rebase.


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


Repository: mesos


Description
---

Multi-role framework cannot combine offers allocated to different
roles of that framework in a single launchTask call.


Diffs (updated)
-

  src/tests/master_tests.cpp 64443dc6e2ca4ab8f37269a0dced49908526649b 

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


Testing
---

Added a test
make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"


Thanks,

Jay Guo

Re: Review Request 56665: Added a URL-safe base64 implementation.

[Greg Mann]

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




3rdparty/stout/include/stout/base64.hpp (lines 174 - 177)


Could you include a comment here describing the `padding` parameter?



3rdparty/stout/include/stout/base64.hpp (line 175)


s/an URL/a URL/



3rdparty/stout/include/stout/base64.hpp (line 185)


s/an URL/a URL/


- Greg Mann


On Feb. 16, 2017, 9:33 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56665/
> ---
> 
> (Updated Feb. 16, 2017, 9:33 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Base64 has many variants that use different alphabets for encoding.
> "Base 64 Encoding with URL and Filename Safe Alphabet" is a variant
> described in RFC 4648.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/base64.hpp 
> 2ac04c4602bc919633a2a480dd2168b7aa301bd7 
>   3rdparty/stout/tests/base64_tests.cpp 
> 32e516861d44c7e3a36e1a29b4d1fe5960684e3b 
> 
> Diff: https://reviews.apache.org/r/56665/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56645: Added agent capabilities to `/state`(v0) endpoint of agent.

[Jay Guo]

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

(Updated Feb. 20, 2017, 3:19 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
Michael Park.


Changes
---

address bbannier's comments.


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


Repository: mesos


Description
---

Added agent capabilities to `/state`(v0) endpoint of agent.


Diffs (updated)
-

  src/slave/http.cpp af70b6f294a04f23b04cd1d8c36c1c3e86d7d5e6 
  src/tests/slave_tests.cpp 16bb14b170d724bd8424778a76de28b0efccc6ed 

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


Testing
---

make check


Thanks,

Jay Guo

Re: Review Request 56644: Added a constant to store agent capabilities.

[Jay Guo]

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

(Updated Feb. 20, 2017, 3:18 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
Michael Park.


Changes
---

address bbannier's comments.


Summary (updated)
-

Added a constant to store agent capabilities.


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


Repository: mesos


Description (updated)
---

Instead of hardcoding in code, agent capabilities are stored in a
constant and could be used by both agent initialization and http
response generation.


Diffs (updated)
-

  src/slave/constants.hpp 725689a51e7682218d34f1b0f6f1630140e2e2d0 
  src/slave/slave.cpp 7564e8d39530794131dbbc928fcbc59fb65ef471 

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


Testing
---

make check


Thanks,

Jay Guo

Re: Review Request 55887: Check task user before allowing a task to be launched on the agent.

[Jiang Yan Xu]

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




src/slave/slave.cpp (line 1662)


Although I can tell what `transaction` means as an analogy here, perhaps 
just saying "because they are either a single task or tasks in a single task 
group" is more direct?



src/slave/slave.cpp (line 1663)


We can also comment that 

```
Note that we only report the first error we encounter for the task group.
```



src/slave/slave.cpp (lines 1669 - 1670)


We have already said "If any of the authorizations fails, we fail all the 
tasks" above.



src/slave/slave.cpp (line 1671)


In this case I think we don't need an `await`, we can use a `collect` 
because of the "failing all tasks when one authorization fails" semantics.



src/slave/slave.cpp (line 1689)


s/taskUser/user/ would be shorter but still sufficient?



src/slave/slave.cpp (line 1706)


s/Status Update/status update/



src/slave/slave.cpp (lines 1710 - 1711)


Two-space indentation.



src/slave/slave.cpp (line 1713)


s/tasks(s) // 
because it's already in `taskOrTaskGroup`?

Also s/WARNING/ERROR/ because this is actually an error, if we do log it.



src/slave/slave.cpp (line 1722)


Why not TASK_ERROR?



src/slave/slave.cpp (lines 1735 - 1739)


The way it is written, `Slave::run()` (and the slave actor) is going to be 
blocked by `authorized.get()` which is not good. We should introduce a 
continuation method `_run` to put the rest of the method and contents in the 
lambda above and rename the current `_run` as `__run`.

Then at the end of run():

```
  collect(authorizations)
.onAny(defer(self(),
 ::_run,
 ...));
```



src/slave/slave.cpp (line 6161)


This method is identitcal to the one on the master and at first glance 
people would probably wonder why this is done again on the agent. We could use 
the space to explain why we do the identitcal thing here for run tasks while 
not for other things.

The reason as I see is that

1. In general we shouldn't need to re-authorize actions that have already 
been authorized by the master. To prevent a client from spoofing as the master 
and exploiting the agent, the agent should authenticate the client as the 
master.

2. We specially re-authorize the `RUN_TASK` action even though the same ACL 
is on the master because a) in cases where hosts have heterogeneous 
user-account configurations, it makes sense to set the ACL on the agent instead 
of on the master; 2) compared to other actions such as killing a task and 
shutting down a framework, it's a greater security risk if malicious tasks are 
launched as a superuser on the agent.


- Jiang Yan Xu


On Feb. 7, 2017, 7:53 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55887/
> ---
> 
> (Updated Feb. 7, 2017, 7:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6953
> https://issues.apache.org/jira/browse/MESOS-6953
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for action `run_tasks` on the agent's flag `acl`. Based on
> the ACL configured for `run_tasks`, a task to be launched on the agent
> will be (dis)allowed to launch on the agent.
> If a task or task group cannot be launched due to failed authorization,
> a `TASK_FAILED` Status Update shall be sent with a reason code of
> `REASON_TASK_UNAUTHORIZED` or `REASON_TASK_GROUP_UNAUTHORIZED` as
> applicable.
> Note that in case of a task group, all tasks fail if any of the tasks
> within the task group encounter the authorization error.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 5049eb783b8ad7b9599f20c3701f7d3d654b4491 
>   src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 
> 
> Diff: https://reviews.apache.org/r/55887/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 56530: Prevent resource of old agents being allocated to MULTI_ROLE frameworks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56837, 56530]

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 Feb. 20, 2017, 4:48 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56530/
> ---
> 
> (Updated Feb. 20, 2017, 4:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6940
> https://issues.apache.org/jira/browse/MESOS-6940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a mixed cluster, when an old agent (not MULTI_ROLE capable)
> registers with new master (MULTI_ROLE capable), it cannot correctly
> handle tasks from a MULTI_ROLE framework. Therefore, we should
> disallow resources from these agents being allocated to MULTI_ROLE
> frameworks.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5441fa9d1fad1ca7819038db49c6d88e40571e4a 
> 
> Diff: https://reviews.apache.org/r/56530/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56537: Added a test to ensure framework can upgrade to support MULTI_ROLE.

[Jay Guo]

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

(Updated Feb. 20, 2017, 1:56 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
Michael Park.


Changes
---

rebase and address bbannier's comments.


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


Repository: mesos


Description
---

A framework can be upgraded to be MULTI_ROLE capable even with task
running, as long as it does not change role while upgrading.


Diffs (updated)
-

  src/tests/master_tests.cpp 64443dc6e2ca4ab8f37269a0dced49908526649b 

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


Testing
---

make check


Thanks,

Jay Guo

Review Request 56835: Made the default executor handle on termination policy for a task.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Made the default executor handle on termination policy for a task.


Diffs
-

  src/launcher/default_executor.cpp d97324b0e5eb3f145ee93aa0821205c5eefbfce0 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 56836: Added test for a task specifying on termination policy.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added test for a task specifying on termination policy.


Diffs
-

  src/tests/default_executor_tests.cpp eaf63946735b28b21bfb7e16aca5924a5a82 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 56834: Send a TASK_ERROR update if a task specifies `OnTerminationPolicy`.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

The docker/command executor currently do not support it yet.


Diffs
-

  src/docker/executor.cpp 6c11320fc40ba1eebdbdf95f0a52ac383646d9f8 
  src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 56831: Minor fix to a comment.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Modified a comment now that the default executor can launch
multiple task groups.


Diffs
-

  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 56833: Added validation for on termination policy to the master.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added validation for on termination policy to the master.


Diffs
-

  src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 56832: Added output operator for `OnTerminationPolicy` enum.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added output operator for `OnTerminationPolicy` enum.


Diffs
-

  include/mesos/type_utils.hpp c7f86ac3a8c166512410d89678a4dd2622771bf0 
  src/common/type_utils.cpp d86d56d4e1d353d6e82ccff89357bf2abec6eded 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 56830: Introduced `TaskInfo::OnTerminationPolicy` protobuf.

[Anand Mazumdar]

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

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


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


Repository: mesos


Description
---

Describes the on termination policy associated with a task and is
applied upon a task termination.


Diffs
-

  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 

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


Testing
---

make check (tests later in the chain)


Thanks,

Anand Mazumdar

Re: Review Request 56530: Prevent resource of old agents being allocated to MULTI_ROLE frameworks.

[Jay Guo]

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

(Updated Feb. 20, 2017, 12:48 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
Michael Park.


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


Repository: mesos


Description (updated)
---

In a mixed cluster, when an old agent (not MULTI_ROLE capable)
registers with new master (MULTI_ROLE capable), it cannot correctly
handle tasks from a MULTI_ROLE framework. Therefore, we should
disallow resources from these agents being allocated to MULTI_ROLE
frameworks.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
  src/tests/hierarchical_allocator_tests.cpp 
5441fa9d1fad1ca7819038db49c6d88e40571e4a 

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


Testing (updated)
---

./bin/mesos-tests.sh


Thanks,

Jay Guo

Re: Review Request 56530: Prevent resource of old agents being allocated to MULTI_ROLE frameworks.

[Jay Guo]

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

(Updated Feb. 20, 2017, 12:46 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
Michael Park.


Changes
---

address benb and benm's comments


Summary (updated)
-

Prevent resource of old agents being allocated to MULTI_ROLE frameworks.


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


Repository: mesos


Description (updated)
---

In a mixed cluster, when an old agent (not MULTI_ROLE capable)
registers with new master (MULTI_ROLE capable), it cannot correctly
handle tasks from a MULTI_ROLE framework. Therefore, we should
disallow resources from these agents being sent to MULTI_ROLE
frameworks.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
  src/tests/hierarchical_allocator_tests.cpp 
5441fa9d1fad1ca7819038db49c6d88e40571e4a 

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


Testing
---

./bin/mesos-tests.sh 
--gtest_filter="MasterTest.MultiRoleFrameworkDoesNotReceiveOfferFromOldAgent" 
--gtest_break_on_failure --gtest_repeat=-1


Thanks,

Jay Guo

Review Request 56837: Added slave capabilities to HierarchicalAllocatorProcess.

[Jay Guo]

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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
Michael Park.


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


Repository: mesos


Description
---

Allocator should be aware of slave capabilities in order to make
correct/smarter decisions on resource allocations. The interface
`addSlave` is augmented to pass in agent capabilities and we store
them in HierarchicalAllocatorProcess. Tests are updated accordingly.


Diffs
-

  include/mesos/allocator/allocator.hpp 
23413d1f9d76009c999b35d2bc98afc52c136404 
  src/master/allocator/mesos/allocator.hpp 
38fbf16a0500fc9f6ce7a498a6af3d81d63fc215 
  src/master/allocator/mesos/hierarchical.hpp 
d33306745a7287b750cb4a5242c7527369d58d65 
  src/master/allocator/mesos/hierarchical.cpp 
eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
  src/master/master.cpp 0f9fc39c1bd2af032f908d0263ed6a592ef1771c 
  src/tests/allocator.hpp c741a985660e1540ad8e3a9c387d513247e56714 
  src/tests/api_tests.cpp 7715da9072e7be81eb19e742857ff7ab3fd60ea5 
  src/tests/hierarchical_allocator_tests.cpp 
5441fa9d1fad1ca7819038db49c6d88e40571e4a 
  src/tests/master_allocator_tests.cpp 25c67d32eec5fede78eb9fcbc1009eeff7da0dad 
  src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5 
  src/tests/persistent_volume_endpoints_tests.cpp 
ec8df33a3f0d4ad790d5cc753e1691c5517c39c0 
  src/tests/reservation_endpoints_tests.cpp 
7432d752120b43560aa96cad8bf3981ee8102e67 
  src/tests/reservation_tests.cpp 309ce8b9acc9131110198c14c655427d7fe9d603 
  src/tests/slave_recovery_tests.cpp 0e295915fea0a7314e173857249bd8726eeccd76 

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


Testing
---

make check


Thanks,

Jay Guo

Re: Review Request 51053: Update leveldb to 1.19.

[Tomasz Janiszewski]

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

(Updated Feb. 20, 2017, 4:41 a.m.)


Review request for mesos, Bing Li, Benjamin Mahler, Zhiwei Chen, haosdent 
huang, and Vinod Kone.


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


Repository: mesos


Description
---

Leveldb in modern version is required to support s390x and arm64.
It's also required to replace default byte-wise comparator
with varint comparator in \`src/log/leveldb.cpp\`.


Diffs
-

  3rdparty/Makefile.am bbf9cfe7125b193003115f440a00c91e2e68f404 
  3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
eeb27860f6f95d297ccfe273ed76de5355b50ff8 
  3rdparty/cmake/Versions.cmake ad23f3895b49d623709c0939d63a0529654788ef 
  3rdparty/leveldb-1.19.patch PRE-CREATION 
  3rdparty/leveldb-1.19.tar.gz PRE-CREATION 
  3rdparty/leveldb-1.4.patch b899f0141d633b1ffb2321e573395256fc893b16 
  3rdparty/leveldb-1.4.tar.gz 2ddbc0c2e02054406ff0ea43ddc10d14979de8d8 
  3rdparty/versions.am 26f839cd8d454de91f10c2e616e78decc30eff1b 
  LICENSE 7946d1d46174ff30459a33b3d0b44ab62672af0a 
  src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
  src/python/native_common/ext_modules.py.in 
2d4a45efa224b32f80ace4542a00062c5ccb06d5 

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


Testing (updated)
---

make -j 8 distcheck

Test document: 
https://docs.google.com/document/d/1fv2OMvH6hVm6waacOejSrTJwUuDQeXlqqPDZjBmbcKU/edit?usp=sharing


Thanks,

Tomasz Janiszewski

Re: Review Request 56809: Added test for nested container machine reboot case.

[Jie Yu]

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




src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 998 - 1012)


Instead of that, can we call 'provisioner->provision' and use DockerArchive 
as the local docker store?



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (line 1014)


Do you want to check if provisioner dir has been cleaned up?


- Jie Yu


On Feb. 18, 2017, 1:55 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56809/
> ---
> 
> (Updated Feb. 18, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for nested container machine reboot case.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> ea01fe55a28d17105157004d8cf0976202a49b7c 
> 
> Diff: https://reviews.apache.org/r/56809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 56808: Fixed nested container agent flapping issue after reboot.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/provisioner/provisioner.hpp (line 134)


Any reason you need this state? Can 'state' just been a boolean (i.e., 
`destorying`)?



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 498)


Why remove case (1) in the original comment? I think case 1) and case 2) in 
the original comment are for different cases.



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 547)


Not yours, please use const ref here.


- Jie Yu


On Feb. 18, 2017, 1:54 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56808/
> ---
> 
> (Updated Feb. 18, 2017, 1:54 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed nested container agent flapping issue after reboot.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> ff52e35ac44fea70fe2031e6ac537c613c57f50f 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8c20d4077859d437999467d045dfd500c1e576dd 
> 
> Diff: https://reviews.apache.org/r/56808/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 56195: Fixed ContainerLogger / IOSwitchboard FD leaks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56814, 56195]

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 Feb. 19, 2017, 4:37 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56195/
> ---
> 
> (Updated Feb. 19, 2017, 4:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gastón Kleiman, Gilbert Song, 
> Jie Yu, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the containizer launch path would leak FDs if the
> containerizer launch path failed between successfully calling
> prepare() on either the ContainerLogger (in the case of the Docker
> containerizer) or the IOSwitchboard (in the case of the mesos
> containerizer) and forking the actual container.
> 
> These components relied on the Subprocess call inside launcher->fork()
> to close these FDS on their behalf. If the containerizer launch path
> failed somewhere between calling prepare() and making this fork()
> call, these FDs would never be closed.
> 
> In the case of the IOSwitchboard, this would lead to deadlock in the
> destroy path because the future returned by the IOSwitchboard's
> cleanup function would never be satisfied. The IOSwitchboard doesn't
> shutdown until the FDs it allocates to the container have been closed.
> 
> This commit fixes this problem by updating the
> ContainerLogger::SubprocessInfo::FD abstraction to change the way
> manages FDS. Instead of tagging each FD with the Subprocess::IO::OWNED
> label and forcing the launcher->fork() call to deal with closing the
> FDs once it's forked a new subprocess, we now do things slightly
> differently now.
> 
> We now keep the default DUP label on each FD (instead fo changing it
> to OWNED) to cause launcher->fork() to dup it before mapping it onto
> the stdin/stdout/stderr of the subprocess. It then only closes the
> duped FD, leaving the original one open.
> 
> In doing so, it s now the contaienrizer's responsibility to ensure
> that these FDs are closed properly (whether that's between a
> successful prepare() call and launcher->fork()) or after
> launcher->fork() has completed successfully. While this has the
> potential to complicate things slightly on the SUCCESS path,
> at least it is now the containerizers's responsibility to close these
> FDS in *all* cases, rather than splitting that responsibility across
> components.
> 
> In order to simplify this, we've also modified the
> ContainerLogger::SubprocessInfo::FD abstraction to hold a Shared
> pointer to its underlying file descriptor and (optionally) close it on
> destruction. With this, we can ensure that all file descriptors
> created through this abstraction will be automatically closed onced
> their final reference goes out of scope (even if its been copied
> around several times).
> 
> In essence, this release the containerizer from the burden of manually
> closing these FDS itself. So long as it holds the final reference to
> these FDs (which it does), they will be automatically closed along
> *any* path out of containerizer->launch(). These are exactly the
> semantics we wanted to achieve.
> 
> In the case of the the ContainerLogger, ownership of these FDs (and
> thus their final reference) is passed to the containerizer in the
> SubprocessInfo struct returned by prepare(). In the case of the
> IOSwitchboard, we had to add a new API call to transfer ownership
> (since it is an isolator and prepare() can only return a protobuf),
> but the end result is the same.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
>   include/mesos/slave/containerizer.proto 
> c70d437ac3f8f813fcb71c04275cc8eeeb946065 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 10a9b57660388ac2409458a4d07af64cc3b185e2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 7a7ad2da047ef316f4382e45526d503c87a903a0 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> b948f3c44dd2e1af2228ca1579f24ae3a94160b0 
> 
> Diff: https://reviews.apache.org/r/56195/diff/
> 
> 
> Testing
> ---
> 
> Linux CentOS 7:
> ```
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 56195: Fixed ContainerLogger / IOSwitchboard FD leaks.

[Kevin Klues]

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

(Updated Feb. 19, 2017, 4:36 p.m.)


Review request for mesos, Alexander Rukletsov, Gastón Kleiman, Gilbert Song, 
Jie Yu, and Vinod Kone.


Changes
---

Fixed memory leak.


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


Repository: mesos


Description
---

Previously, the containizer launch path would leak FDs if the
containerizer launch path failed between successfully calling
prepare() on either the ContainerLogger (in the case of the Docker
containerizer) or the IOSwitchboard (in the case of the mesos
containerizer) and forking the actual container.

These components relied on the Subprocess call inside launcher->fork()
to close these FDS on their behalf. If the containerizer launch path
failed somewhere between calling prepare() and making this fork()
call, these FDs would never be closed.

In the case of the IOSwitchboard, this would lead to deadlock in the
destroy path because the future returned by the IOSwitchboard's
cleanup function would never be satisfied. The IOSwitchboard doesn't
shutdown until the FDs it allocates to the container have been closed.

This commit fixes this problem by updating the
ContainerLogger::SubprocessInfo::FD abstraction to change the way
manages FDS. Instead of tagging each FD with the Subprocess::IO::OWNED
label and forcing the launcher->fork() call to deal with closing the
FDs once it's forked a new subprocess, we now do things slightly
differently now.

We now keep the default DUP label on each FD (instead fo changing it
to OWNED) to cause launcher->fork() to dup it before mapping it onto
the stdin/stdout/stderr of the subprocess. It then only closes the
duped FD, leaving the original one open.

In doing so, it s now the contaienrizer's responsibility to ensure
that these FDs are closed properly (whether that's between a
successful prepare() call and launcher->fork()) or after
launcher->fork() has completed successfully. While this has the
potential to complicate things slightly on the SUCCESS path,
at least it is now the containerizers's responsibility to close these
FDS in *all* cases, rather than splitting that responsibility across
components.

In order to simplify this, we've also modified the
ContainerLogger::SubprocessInfo::FD abstraction to hold a Shared
pointer to its underlying file descriptor and (optionally) close it on
destruction. With this, we can ensure that all file descriptors
created through this abstraction will be automatically closed onced
their final reference goes out of scope (even if its been copied
around several times).

In essence, this release the containerizer from the burden of manually
closing these FDS itself. So long as it holds the final reference to
these FDs (which it does), they will be automatically closed along
*any* path out of containerizer->launch(). These are exactly the
semantics we wanted to achieve.

In the case of the the ContainerLogger, ownership of these FDs (and
thus their final reference) is passed to the containerizer in the
SubprocessInfo struct returned by prepare(). In the case of the
IOSwitchboard, we had to add a new API call to transfer ownership
(since it is an isolator and prepare() can only return a protobuf),
but the end result is the same.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp 
a3f619b79ca0188df9e231c600dfa396f39ab29a 
  include/mesos/slave/containerizer.proto 
c70d437ac3f8f813fcb71c04275cc8eeeb946065 
  src/slave/containerizer/mesos/containerizer.hpp 
10a9b57660388ac2409458a4d07af64cc3b185e2 
  src/slave/containerizer/mesos/containerizer.cpp 
d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
  src/slave/containerizer/mesos/io/switchboard.hpp 
7a7ad2da047ef316f4382e45526d503c87a903a0 
  src/slave/containerizer/mesos/io/switchboard.cpp 
b948f3c44dd2e1af2228ca1579f24ae3a94160b0 

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


Testing
---

Linux CentOS 7:
```
GTEST_FILTER="" make -j check
sudo src/mesos-tests
```


Thanks,

Kevin Klues