Review Request 54363: Added a test to ensure multi-role framework being removed properly.

[Jay Guo]

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

Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.


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


Repository: mesos


Description
---

Added a test to ensure multi-role framework being removed properly.


Diffs
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

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


Testing
---

make
make check GTEST_FILTER="MasterTest.RemoveMultiRoleFramework"
make check


Thanks,

Jay Guo

Review Request 54362: Changed master to remove roles for a multi-role framework.

[Jay Guo]

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

Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.


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


Repository: mesos


Description
---

When a multi-role framework is torn down, it should be removed from
internal data structure for every role in this framework.


Diffs
-

  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54359, 54360, 54361]

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 Dec. 5, 2016, 6:13 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated Dec. 5, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6684
> https://issues.apache.org/jira/browse/MESOS-6684
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure roles from multi-role framework are added.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
> 
> Diff: https://reviews.apache.org/r/54361/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54356: Added implementation of `watch()` to the 'IOSwitchboard' isolator.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:19 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Needed to rebase again because I accidentally included something in this commit 
that should have been included in a previous one.


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


Repository: mesos


Description
---

We use watch() to trigger the destruction of a container if the
io switchboard server process ever exits unexpectedly.


Diffs (updated)
-

  include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
  src/slave/containerizer/mesos/io/switchboard.hpp 
839665a22aca9b1c1c1cf4992406bc924ee2b065 
  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 
  src/tests/containerizer/io_switchboard_tests.cpp 
ebf9bc75b8a256847a507954615241cff1da4dc3 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 54355: Added implementation of `recover()` to the IOSwitchboard isolator.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:11 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added implementation of `recover()` to the IOSwitchboard isolator.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.hpp 
839665a22aca9b1c1c1cf4992406bc924ee2b065 
  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 

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


Testing (updated)
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests

Test added in follow-on patch.


Thanks,

Kevin Klues

Re: Review Request 54356: Added implementation of `watch()` to the 'IOSwitchboard' isolator.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:11 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Now I have tests.


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


Repository: mesos


Description
---

We use watch() to trigger the destruction of a container if the
io switchboard server process ever exits unexpectedly.


Diffs
-

  include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
  src/slave/containerizer/mesos/io/switchboard.hpp 
839665a22aca9b1c1c1cf4992406bc924ee2b065 
  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 
  src/tests/containerizer/io_switchboard_tests.cpp 
ebf9bc75b8a256847a507954615241cff1da4dc3 

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


Testing (updated)
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 54354: Added path helpers for checkpointing the io switchboard pid.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:10 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

Added path helpers for checkpointing the io switchboard pid.


Diffs (updated)
-

  src/slave/containerizer/mesos/paths.hpp 
c0fe2a4ae8d8b6de24ab265d483d3edc11c68a0e 
  src/slave/containerizer/mesos/paths.cpp 
e090392ed079993968f8664d89ad6c49eca3f3c4 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 54356: Added implementation of `watch()` to the 'IOSwitchboard' isolator.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:10 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

We use watch() to trigger the destruction of a container if the
io switchboard server process ever exits unexpectedly.


Diffs (updated)
-

  include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
  src/slave/containerizer/mesos/io/switchboard.hpp 
839665a22aca9b1c1c1cf4992406bc924ee2b065 
  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 
  src/tests/containerizer/io_switchboard_tests.cpp 
ebf9bc75b8a256847a507954615241cff1da4dc3 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests

Unfortunately, I don't have any new tests for this yet. That will follow in a 
subsequent commit.


Thanks,

Kevin Klues

Re: Review Request 54355: Added implementation of `recover()` to the IOSwitchboard isolator.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:10 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

Added implementation of `recover()` to the IOSwitchboard isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.hpp 
839665a22aca9b1c1c1cf4992406bc924ee2b065 
  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests

Unfortunately, I don't have any new tests for this yet. That will follow in a 
subsequent commit.


Thanks,

Kevin Klues

Re: Review Request 54352: Added removal of unix domain socket path in IOSwitchboard::cleanup.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:10 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

Added removal of unix domain socket path in IOSwitchboard::cleanup.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:09 a.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

Added support for ATTACH_CONTAINER_INPUT to the io switchboard.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 
  src/tests/containerizer/io_switchboard_tests.cpp 
ebf9bc75b8a256847a507954615241cff1da4dc3 

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


Testing
---

make -j check


Thanks,

Kevin Klues

Re: Review Request 54353: Added short circuit for `local` mode in `IOSwitchboard::connect()'.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:10 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased on master.


Repository: mesos


Description
---

Added short circuit for `local` mode in `IOSwitchboard::connect()'.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 54347: Cleaned up the 'IOSwitchboard.RedirectLog' test.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:09 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Rebased on master.


Repository: mesos


Description
---

Cleaned up the 'IOSwitchboard.RedirectLog' test.


Diffs (updated)
-

  src/tests/containerizer/io_switchboard_tests.cpp 
ebf9bc75b8a256847a507954615241cff1da4dc3 

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


Testing
---

make check


Thanks,

Kevin Klues

Re: Review Request 54348: Fixed small bug in `IOSwitchboardServerProcess::acceptLoop()`.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:09 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased on master.


Repository: mesos


Description
---

Fixed small bug in `IOSwitchboardServerProcess::acceptLoop()`.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 

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


Testing
---

```
GTEST_FILTER="" make -j check
GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
```


Thanks,

Kevin Klues

Re: Review Request 54344: Added `setWindowSize()` to stout for setting a TTY's window size.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:09 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase on master.


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


Repository: mesos


Description
---

Added `setWindowSize()` to stout for setting a TTY's window size.


Diffs (updated)
-

  3rdparty/stout/include/stout/posix/os.hpp 
09196be9d359ef79c9cbafeec25d965f2db35d42 

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


Testing
---

make check


Thanks,

Kevin Klues

Re: Review Request 54344: Added `setWindowSize()` to stout for setting a TTY's window size.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:06 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

Added `setWindowSize()` to stout for setting a TTY's window size.


Diffs (updated)
-

  3rdparty/stout/include/stout/posix/os.hpp 
09196be9d359ef79c9cbafeec25d965f2db35d42 

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


Testing
---

make check


Thanks,

Kevin Klues

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

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


Ship it!




I can take care of the addressing remaining minor comments.


src/examples/persistent_volume_framework.cpp (line 219)


s/consumer/producer/.



src/examples/persistent_volume_framework.cpp (line 242)


The empty line here doesn't comply with the [style 
guide](https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements).
 Also for long blocks of switch case it looks better with `{}`.



src/examples/persistent_volume_framework.cpp (line 279)


`)~");` should be aligned with `tasks` above but more usually we just put 
one the same line where the string ends.



src/examples/persistent_volume_framework.cpp (line 431)


s/and terminates/(terminated)/ since the last state is already TERMINATING.



src/examples/persistent_volume_framework.cpp (line 438)


s/initialized/initial/



src/examples/persistent_volume_framework.cpp (line 439)


Due to the change to add the INIT state, I think we have to revise this a 
bit:

```
STAGING, // The shard has started but haven't finished launching tasks.
```


- Jiang Yan Xu


On Dec. 2, 2016, 1:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Dec. 2, 2016, 1:43 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Jay Guo]

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

(Updated Dec. 5, 2016, 6:13 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.


Changes
---

rename test case


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


Repository: mesos


Description
---

Added a test to ensure roles from multi-role framework are added.


Diffs (updated)
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

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


Testing
---

make
make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
make check


Thanks,

Jay Guo

Re: Review Request 54354: Added path helpers for checkpointing the io switchboard pid.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 5, 2016, 12:38 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54354/
> ---
> 
> (Updated Dec. 5, 2016, 12:38 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6688
> https://issues.apache.org/jira/browse/MESOS-6688
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added path helpers for checkpointing the io switchboard pid.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/paths.hpp 
> c0fe2a4ae8d8b6de24ab265d483d3edc11c68a0e 
>   src/slave/containerizer/mesos/paths.cpp 
> e090392ed079993968f8664d89ad6c49eca3f3c4 
> 
> Diff: https://reviews.apache.org/r/54354/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54353: Added short circuit for `local` mode in `IOSwitchboard::connect()'.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 5, 2016, 12:38 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54353/
> ---
> 
> (Updated Dec. 5, 2016, 12:38 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added short circuit for `local` mode in `IOSwitchboard::connect()'.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
> 
> Diff: https://reviews.apache.org/r/54353/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54352: Added removal of unix domain socket path in IOSwitchboard::cleanup.

[Jie Yu]

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




src/slave/containerizer/mesos/io/switchboard.cpp (lines 407 - 417)


I would probably wait until the io switchboard process has been terminated. 
Otherwise, rm here might get EBUSY?

In other words, move this logic to `status.then`



src/slave/containerizer/mesos/io/switchboard.cpp (line 416)


I would still LOG(ERROR) if rm fails.


- Jie Yu


On Dec. 5, 2016, 12:39 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54352/
> ---
> 
> (Updated Dec. 5, 2016, 12:39 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6689
> https://issues.apache.org/jira/browse/MESOS-6689
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added removal of unix domain socket path in IOSwitchboard::cleanup.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
> 
> Diff: https://reviews.apache.org/r/54352/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54294: Added an I/O switchboard test with TTY enabled.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:38 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Rebased. Addressed comments.


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


Repository: mesos


Description
---

Added an I/O switchboard test with TTY enabled.


Diffs (updated)
-

  src/tests/containerizer/io_switchboard_tests.cpp 
d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54293: Supported TTY in I/O switchboard.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:37 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Rebased. Addressed comments.


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


Repository: mesos


Description
---

Supported TTY in I/O switchboard.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
33b4c231cc88a048f3de8ab4b2e0e9eb2140132d 
  src/slave/containerizer/mesos/containerizer.cpp 
a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
  src/slave/containerizer/mesos/io/switchboard.hpp 
23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
5800217d3562a53218d966c57a14e8b768643c34 
  src/slave/containerizer/mesos/launch.cpp 
d78ca4df694712e60a15f1795878653eb5e0414e 
  src/tests/containerizer/io_switchboard_tests.cpp 
d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:36 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Addressed comments. Rebased.


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


Repository: mesos


Description
---

Allowed subprocess to take duplicated FDs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
  3rdparty/libprocess/include/process/windows/subprocess.hpp 
f452f6743d01f0b99010fa5e5bcbaae1358c8241 
  3rdparty/libprocess/src/subprocess.cpp 
284e22e28ae8d2c1486e4a6bea743b8663ce2023 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54350: Added a DUP2 child hook to Subprocess.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:36 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a DUP2 child hook to Subprocess.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
1d02454d5541f96cb4928bf027fcae3764989d67 
  3rdparty/libprocess/src/subprocess.cpp 
284e22e28ae8d2c1486e4a6bea743b8663ce2023 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54349: Added os::dup2 to stout.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:36 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added os::dup2 to stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54292: Added os::setctty to stout.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:35 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Rebased. Addressed comments.


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


Repository: mesos


Description
---

Added os::setctty to stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54289: Added a check to require I/O switchboard server for TTY support.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:33 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a check to require I/O switchboard server for TTY support.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54291: Added os::ptsname to stout.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:34 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Addressed comments. Rebased.


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


Repository: mesos


Description
---

Added os::ptsname to stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54290: Added a ErrnoFailure similar to ErrnoError.

[Jie Yu]

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

(Updated Dec. 5, 2016, 5:34 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a ErrnoFailure similar to ErrnoError.


Diffs (updated)
-

  3rdparty/libprocess/include/process/future.hpp 
175214a9090f8cc8241a81e535c87370102f3011 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54294: Added an I/O switchboard test with TTY enabled.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 5, 2016, 12:17 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54294/
> ---
> 
> (Updated Dec. 5, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an I/O switchboard test with TTY enabled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 
> 
> Diff: https://reviews.apache.org/r/54294/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54293: Supported TTY in I/O switchboard.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> ---
> 
> (Updated Dec. 4, 2016, 9:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported TTY in I/O switchboard.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> 5800217d3562a53218d966c57a14e8b768643c34 
>   src/slave/containerizer/mesos/launch.cpp 
> d78ca4df694712e60a15f1795878653eb5e0414e 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 
> 
> Diff: https://reviews.apache.org/r/54293/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54294: Added an I/O switchboard test with TTY enabled.

[Jie Yu]

> On Dec. 5, 2016, 2:10 a.m., Kevin Klues wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 308-309
> > 
> >
> > Why not put this at the top with the other declaration for 
> > `IOSwitchboardServerTest`?

We typically put fixture close to its tests.


- Jie


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


On Dec. 5, 2016, 12:17 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54294/
> ---
> 
> (Updated Dec. 5, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an I/O switchboard test with TTY enabled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 
> 
> Diff: https://reviews.apache.org/r/54294/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54293: Supported TTY in I/O switchboard.

[Jie Yu]

> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 248
> > 
> >
> > Can you add a comment here about the difference between the two 
> > hashsets? Reading top to bottom, it's not clear exactly what each one 
> > represents just by looking at the name of the variable.
> 
> Kevin Klues wrote:
> Maybe it's just enough to rename the second one to `childFds`.

I added a comment to explain this.


> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 279
> > 
> >
> > Does it make sense to eventually move this function into stout?

yeah, I'll add a TODO. We should add a stout/posix/tty.hpp.


> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.hpp, line 126
> > 
> >
> > It's somewhat arbitrary, but can we make this the second to last 
> > parameter, in order to group the booleans together? If so, can we do it 
> > throughout?

Let me do that in a separate patch.


> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 392-397
> > 
> >
> > I guess you don't need to set the other flags anymore because you rely 
> > on their default values now?

Default is not STDOUT_FILENO and STDERR_FILENO. I'll add a comment here.


- Jie


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


On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> ---
> 
> (Updated Dec. 4, 2016, 9:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported TTY in I/O switchboard.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> 5800217d3562a53218d966c57a14e8b768643c34 
>   src/slave/containerizer/mesos/launch.cpp 
> d78ca4df694712e60a15f1795878653eb5e0414e 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 
> 
> Diff: https://reviews.apache.org/r/54293/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Benjamin Hindman]

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


Ship it!




The changes all make sense, but I agree with Kevin that moving the "ownership" 
for Windows into `internal::createChildProcess` would be more symetrical. 
Alternatively, could you move the close out from `cloneChild` on Linux and do 
the closes immediately after you return from both the Linux and Windows 
functions before you even check if the child creation was an error?

- Benjamin Hindman


On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54351/
> ---
> 
> (Updated Dec. 5, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed subprocess to take duplicated FDs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> f452f6743d01f0b99010fa5e5bcbaae1358c8241 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54351/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 54359: Removed redundant empty statements.

[Jay Guo]

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

Review request for mesos, Benjamin Mahler and Guangya Liu.


Repository: mesos


Description
---

Removed redundant empty statements.


Diffs
-

  src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 

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


Testing
---


Thanks,

Jay Guo

Review Request 54360: Changed master to add roles for a multi-role framework.

[Jay Guo]

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

Review request for mesos, Benjamin Mahler, Guangya Liu, and Qian Zhang.


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


Repository: mesos


Description
---

When a multi-role framework registers, master should add roles
from 'roles' field of FrameworkInfo to internal data structure
`activeRoles`.


Diffs
-

  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 

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


Testing
---


Thanks,

Jay Guo

Review Request 54361: Added a test to ensure roles from multi-role framework are added.

[Jay Guo]

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

Review request for mesos, Benjamin Mahler, Guangya Liu, and Qiang Zhang.


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


Repository: mesos


Description
---

Added a test to ensure roles from multi-role framework are added.


Diffs
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

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


Testing
---

make
make check GTEST_FILTER="MasterTest.AcceptMultiRoleFramework"
make check


Thanks,

Jay Guo

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

> On Dec. 5, 2016, 1:36 a.m., Kevin Klues wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 330-344
> > 
> >
> > I feel like moving this into the windows `createChildProcess()` 
> > function would make it more semetrical with the change above. It seems odd 
> > that we would ahve to close out here on windows, but not on posix.
> > 
> > Also, lines 318 and 326 above should either be removed (if you follow 
> > my suggestion) or changed to be the same as this line (if you don't).

This part the code needs to be refactored. The logic is really hard to follow 
when I did the change. I'll add a TODO for now because I don't have a good way 
to test windows code.


- Jie


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


On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54351/
> ---
> 
> (Updated Dec. 5, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed subprocess to take duplicated FDs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> f452f6743d01f0b99010fa5e5bcbaae1358c8241 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54351/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54291: Added os::ptsname to stout.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54350: Added a DUP2 child hook to Subprocess.

[Benjamin Hindman]

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


Ship it!




Ship It!

- Benjamin Hindman


On Dec. 4, 2016, 9:18 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54350/
> ---
> 
> (Updated Dec. 4, 2016, 9:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a DUP2 child hook to Subprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> 1d02454d5541f96cb4928bf027fcae3764989d67 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54350/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54292: Added os::setctty to stout.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54292/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::setctty to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54292/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

> On Dec. 5, 2016, 1:36 a.m., Kevin Klues wrote:
> > 3rdparty/libprocess/include/process/posix/subprocess.hpp, lines 332-384
> > 
> >
> > Is this move actually related to this commit, or is it just an existing 
> > bug fix / simplification?

I need to change those anyway because those FDs might be equal. Therefore, I 
did this simplication.


- Jie


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


On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54351/
> ---
> 
> (Updated Dec. 5, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed subprocess to take duplicated FDs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> f452f6743d01f0b99010fa5e5bcbaae1358c8241 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54351/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54349: Added os::dup2 to stout.

[Benjamin Hindman]

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


Ship it!




Ship It!

- Benjamin Hindman


On Dec. 4, 2016, 9:17 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54349/
> ---
> 
> (Updated Dec. 4, 2016, 9:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::dup2 to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54349/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54291: Added os::ptsname to stout.

[Benjamin Hindman]

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


Ship it!




Adding the TODO for `ptsname_r` on Linux SGTM as well as using `const char*` if 
possible. Otherwise LGTM.

- Benjamin Hindman


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54349: Added os::dup2 to stout.

[Jie Yu]

> On Dec. 5, 2016, 1:02 a.m., Kevin Klues wrote:
> > Should we add an equivalent function for windows? I assume we will need one 
> > to make any child hooks we add to `Subprocess` cross platform.

dup is posix specific. i think we will have posix specific hooks anyway (e.g., 
supervised and setsid are all posix only hooks). Will try to make them not 
available to windows in a followup


- Jie


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


On Dec. 4, 2016, 9:17 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54349/
> ---
> 
> (Updated Dec. 4, 2016, 9:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::dup2 to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54349/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54290: Added a ErrnoFailure similar to ErrnoError.

[Benjamin Hindman]

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


Ship it!




Ship It!

- Benjamin Hindman


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54290/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a ErrnoFailure similar to ErrnoError.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> 175214a9090f8cc8241a81e535c87370102f3011 
> 
> Diff: https://reviews.apache.org/r/54290/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54289: Added a check to require I/O switchboard server for TTY support.

[Benjamin Hindman]

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


Ship it!




Ship It!

- Benjamin Hindman


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54289/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a check to require I/O switchboard server for TTY support.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 594c37e531c5b26599989a126aede56954d460fa 
> 
> Diff: https://reviews.apache.org/r/54289/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54295: Added a synchronous version of loop for io::read/write/redirect.

[Benjamin Hindman]

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

(Updated Dec. 5, 2016, 4:35 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Added a synchronous version of loop for io::read/write/redirect.


Diffs (updated)
-

  3rdparty/libprocess/include/process/loop.hpp 
a78ea7da8a5a778d816a64fb46d9479cc2a4ed70 
  3rdparty/libprocess/src/http.cpp fc55bda0fae8a43794fb938718b3a720dc342473 
  3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 54291: Added os::ptsname to stout.

[Jie Yu]

> On Dec. 2, 2016, 9:16 a.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/posix/os.hpp, line 480
> > 
> >
> > We should be able to use a simple function-local static here and avoid 
> > the global leak of `mutex`, e.g.,
> > 
> > static std::mutex mutex;

We use pointer here to avoid global non-POD data.


- Jie


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


On Dec. 2, 2016, 6:29 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54291/
> ---
> 
> (Updated Dec. 2, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::ptsname to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54291/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54356: Added implementation of `watch()` to the 'IOSwitchboard' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54352, 54353, 54354, 54355, 54356]

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 Dec. 5, 2016, 12:38 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54356/
> ---
> 
> (Updated Dec. 5, 2016, 12:38 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6663
> https://issues.apache.org/jira/browse/MESOS-6663
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We use watch() to trigger the destruction of a container if the
> io switchboard server process ever exits unexpectedly.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
> 
> Diff: https://reviews.apache.org/r/54356/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> Unfortunately, I don't have any new tests for this yet. That will follow in a 
> subsequent commit.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54294: Added an I/O switchboard test with TTY enabled.

[Kevin Klues]

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




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


Why not put this at the top with the other declaration for 
`IOSwitchboardServerTest`?



src/tests/containerizer/io_switchboard_tests.cpp (line 344)


Can you add a quick comment about what's going on here (and why it's 
important for testing the tty redirection).


- Kevin Klues


On Dec. 5, 2016, 12:17 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54294/
> ---
> 
> (Updated Dec. 5, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an I/O switchboard test with TTY enabled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 
> 
> Diff: https://reviews.apache.org/r/54294/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54293: Supported TTY in I/O switchboard.

[Kevin Klues]

> On Dec. 5, 2016, 2:02 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 248
> > 
> >
> > Can you add a comment here about the difference between the two 
> > hashsets? Reading top to bottom, it's not clear exactly what each one 
> > represents just by looking at the name of the variable.

Maybe it's just enough to rename the second one to `childFds`.


- Kevin


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


On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> ---
> 
> (Updated Dec. 4, 2016, 9:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported TTY in I/O switchboard.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> 5800217d3562a53218d966c57a14e8b768643c34 
>   src/slave/containerizer/mesos/launch.cpp 
> d78ca4df694712e60a15f1795878653eb5e0414e 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 
> 
> Diff: https://reviews.apache.org/r/54293/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54293: Supported TTY in I/O switchboard.

[Kevin Klues]

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




src/slave/containerizer/mesos/io/switchboard.hpp (line 126)


It's somewhat arbitrary, but can we make this the second to last parameter, 
in order to group the booleans together? If so, can we do it throughout?



src/slave/containerizer/mesos/io/switchboard.hpp (line 178)


s/termial/terminal



src/slave/containerizer/mesos/io/switchboard.cpp (line 247)


Can you add a comment here about the difference between the two hashsets? 
Reading top to bottom, it's not clear exactly what each one represents just by 
looking at the name of the variable.



src/slave/containerizer/mesos/io/switchboard.cpp (line 258)


s/the pseudo/a pseudo



src/slave/containerizer/mesos/io/switchboard.cpp (line 277)


Does it make sense to eventually move this function into stout?



src/slave/containerizer/mesos/io/switchboard.cpp (line 283)


Does it make sense to eventually move this function into stout?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 388 - 391)


I guess you don't need to set the other flags anymore because you rely on 
their default values now?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 439 - 441)


Can you add a comment about why we do this? When I first looked at it, it 
wasn't obvious why it was necessary, but I see why it's needed later on (i.e. 
for future failures).



src/slave/containerizer/mesos/io/switchboard.cpp (lines 749 - 750)


Can you add a quick comment about why we don't need to redirect to `stderr` 
if we have a tty set up.


- Kevin Klues


On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> ---
> 
> (Updated Dec. 4, 2016, 9:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported TTY in I/O switchboard.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> 5800217d3562a53218d966c57a14e8b768643c34 
>   src/slave/containerizer/mesos/launch.cpp 
> d78ca4df694712e60a15f1795878653eb5e0414e 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 
> 
> Diff: https://reviews.apache.org/r/54293/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Kevin Klues]

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




3rdparty/libprocess/include/process/posix/subprocess.hpp (line 188)


What if `stdoutfds.write == stderrfds.write`? I guess you will close it 
here, and then *not* close it in the next if statement?

Looking more closely at it, it seems like you just extend the (implicit) 
logic described in the comment for *not* closing stdin/stdout/stderr because 
they have already been closed by the dup call.

I'd extend the comment to make it clear what's going on.

Simething like:
```
...
We also need to ensure that we don't "double close" any file descriptors in 
the case where one of stdinfds.read, stdoutfds.write, or stdoutfds.write are 
equal.
```
```



3rdparty/libprocess/include/process/posix/subprocess.hpp (lines 329 - 370)


Is this move actually related to this commit, or is it just an existing bug 
fix / simplification?



3rdparty/libprocess/src/subprocess.cpp (lines 330 - 338)


I feel like moving this into the windows `createChildProcess()` function 
would make it more semetrical with the change above. It seems odd that we would 
ahve to close out here on windows, but not on posix.

Also, lines 318 and 326 above should either be removed (if you follow my 
suggestion) or changed to be the same as this line (if you don't).


- Kevin Klues


On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54351/
> ---
> 
> (Updated Dec. 5, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed subprocess to take duplicated FDs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> f452f6743d01f0b99010fa5e5bcbaae1358c8241 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54351/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54350: Added a DUP2 child hook to Subprocess.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 4, 2016, 9:18 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54350/
> ---
> 
> (Updated Dec. 4, 2016, 9:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a DUP2 child hook to Subprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> 1d02454d5541f96cb4928bf027fcae3764989d67 
>   3rdparty/libprocess/src/subprocess.cpp 
> 284e22e28ae8d2c1486e4a6bea743b8663ce2023 
> 
> Diff: https://reviews.apache.org/r/54350/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54349: Added os::dup2 to stout.

[Kevin Klues]

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


Ship it!




Should we add an equivalent function for windows? I assume we will need one to 
make any child hooks we add to `Subprocess` cross platform.

- Kevin Klues


On Dec. 4, 2016, 9:17 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54349/
> ---
> 
> (Updated Dec. 4, 2016, 9:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
> https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::dup2 to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
> 
> Diff: https://reviews.apache.org/r/54349/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54344: Added `setWindowSize()` to stout for setting a TTY's window size.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 12:41 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added `setWindowSize()` to stout for setting a TTY's window size.


Diffs
-

  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 

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


Testing
---

make check


Thanks,

Kevin Klues

Re: Review Request 54352: Added removal of unix domain socket path in IOSwitchboard::cleanup.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 12:39 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added removal of unix domain socket path in IOSwitchboard::cleanup.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Review Request 54356: Added implementation of `watch()` to the 'IOSwitchboard' isolator.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

We use watch() to trigger the destruction of a container if the
io switchboard server process ever exits unexpectedly.


Diffs
-

  include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
  src/slave/containerizer/mesos/io/switchboard.hpp 
23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests

Unfortunately, I don't have any new tests for this yet. That will follow in a 
subsequent commit.


Thanks,

Kevin Klues

Review Request 54352: Added removal of unix domain socket path in IOSwitchboard::cleanup.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


Bugs: 6689
https://issues.apache.org/jira/browse/6689


Repository: mesos


Description
---

Added removal of unix domain socket path in IOSwitchboard::cleanup.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Review Request 54354: Added path helpers for checkpointing the io switchboard pid.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added path helpers for checkpointing the io switchboard pid.


Diffs
-

  src/slave/containerizer/mesos/paths.hpp 
c0fe2a4ae8d8b6de24ab265d483d3edc11c68a0e 
  src/slave/containerizer/mesos/paths.cpp 
e090392ed079993968f8664d89ad6c49eca3f3c4 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Review Request 54355: Added implementation of `recover()` to the IOSwitchboard isolator.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added implementation of `recover()` to the IOSwitchboard isolator.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.hpp 
23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests

Unfortunately, I don't have any new tests for this yet. That will follow in a 
subsequent commit.


Thanks,

Kevin Klues

Review Request 54353: Added short circuit for `local` mode in `IOSwitchboard::connect()'.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added short circuit for `local` mode in `IOSwitchboard::connect()'.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 54294: Added an I/O switchboard test with TTY enabled.

[Jie Yu]

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

(Updated Dec. 5, 2016, 12:17 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


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


Repository: mesos


Description
---

Added an I/O switchboard test with TTY enabled.


Diffs (updated)
-

  src/tests/containerizer/io_switchboard_tests.cpp 
d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

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

(Updated Dec. 5, 2016, 12:16 a.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


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


Repository: mesos


Description
---

Allowed subprocess to take duplicated FDs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
  3rdparty/libprocess/include/process/windows/subprocess.hpp 
f452f6743d01f0b99010fa5e5bcbaae1358c8241 
  3rdparty/libprocess/src/subprocess.cpp 
284e22e28ae8d2c1486e4a6bea743b8663ce2023 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54293: Supported TTY in I/O switchboard.

[Jie Yu]

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

(Updated Dec. 4, 2016, 9:21 p.m.)


Review request for mesos, Benjamin Hindman and Kevin Klues.


Changes
---

Used well known numbers to pass FDs.


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


Repository: mesos


Description
---

Supported TTY in I/O switchboard.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
33b4c231cc88a048f3de8ab4b2e0e9eb2140132d 
  src/slave/containerizer/mesos/containerizer.cpp 
a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
  src/slave/containerizer/mesos/io/switchboard.hpp 
23c66bb2b05310a12fec24a5e5eebddd370ba0f0 
  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
5800217d3562a53218d966c57a14e8b768643c34 
  src/slave/containerizer/mesos/launch.cpp 
d78ca4df694712e60a15f1795878653eb5e0414e 
  src/tests/containerizer/io_switchboard_tests.cpp 
d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 54351: Allowed subprocess to take duplicated FDs.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Kevin Klues.


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


Repository: mesos


Description
---

Allowed subprocess to take duplicated FDs.


Diffs
-

  3rdparty/libprocess/include/process/posix/subprocess.hpp 
aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 
  3rdparty/libprocess/include/process/windows/subprocess.hpp 
f452f6743d01f0b99010fa5e5bcbaae1358c8241 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 54350: Added a DUP2 child hook to Subprocess.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Kevin Klues.


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


Repository: mesos


Description
---

Added a DUP2 child hook to Subprocess.


Diffs
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
1d02454d5541f96cb4928bf027fcae3764989d67 
  3rdparty/libprocess/src/subprocess.cpp 
284e22e28ae8d2c1486e4a6bea743b8663ce2023 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 54349: Added os::dup2 to stout.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Kevin Klues.


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


Repository: mesos


Description
---

Added os::dup2 to stout.


Diffs
-

  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54013: Added user doc for nested container and task group.

[Vinod Kone]

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


Fix it, then Ship it!




LGTM.


docs/nested-container-and-task-group.md (line 28)


this does not sound like they have the same life cycle?



docs/nested-container-and-task-group.md (line 41)


abstractions



docs/nested-container-and-task-group.md (line 70)


Both authorized operators and executors...



docs/nested-container-and-task-group.md (line 105)


Maybe link to 
https://github.com/apache/mesos/blob/master/docs/app-framework-development-guide.md
 for details about default executor?



docs/nested-container-and-task-group.md (line 209)


mention that only 2 levels of nesting is supported as of 1.1?


- Vinod Kone


On Nov. 30, 2016, 7:11 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Nov. 30, 2016, 7:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, Qian 
> Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6335
> https://issues.apache.org/jira/browse/MESOS-6335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added user doc for nested container and task group.
> 
> 
> Diffs
> -
> 
>   docs/nested-container-and-task-group.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54013/diff/
> 
> 
> Testing
> ---
> 
> Tested by gist view. Here is the link:
> 
> https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

(Updated Dec. 4, 2016, 7:17 p.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Updated to address newest comments.


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


Repository: mesos


Description
---

Added support for ATTACH_CONTAINER_INPUT to the io switchboard.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
778367a268ec350ed438bc9fe9d359d63bdb5503 
  src/tests/containerizer/io_switchboard_tests.cpp 
d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 

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


Testing
---

make -j check


Thanks,

Kevin Klues

Re: Review Request 54348: Fixed small bug in `IOSwitchboardServerProcess::acceptLoop()`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 4, 2016, 7:35 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54348/
> ---
> 
> (Updated Dec. 4, 2016, 7:35 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed small bug in `IOSwitchboardServerProcess::acceptLoop()`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
> 
> Diff: https://reviews.apache.org/r/54348/diff/
> 
> 
> Testing
> ---
> 
> ```
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Anand Mazumdar]

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


Fix it, then Ship it!




I would file a follow up issue to add tests for the validation code itself.


src/slave/containerizer/mesos/io/switchboard.cpp (lines 492 - 493)


How about:

```cpp
// TODO(klueska): Move this to `src/slave/validation.hpp` and make the 
agent validate all the calls before forwarding them to the switchboard.




src/slave/containerizer/mesos/io/switchboard.cpp (lines 847 - 849)


Kill this, you already have an explicit `CHECK` for this before you invoke 
this.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 852 - 866)


For this handler, it should *only* validate that the second record is a 
`ProcessIO` record. It shouldn't validate the other types and just return an 
error if the record is of some other type.

Can you just do:

```cpp
case agent::Call::AttachContainerInput::UNKNOWN:
case agent::Call::AttachContainerInput::CONTAINER_ID: {
  return Error("Expecting 'attach_container_input.type' to be 'PROCESS_IO'"
   " instead of: '" + call.type() + "'");
```



src/slave/containerizer/mesos/io/switchboard.cpp (lines 934 - 936)


We rely on the compiler failing if a `condition` is missed in the case 
statements `-Wswitch`.

Kill this and just have `UNREACHABLE()` in the outer scope.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 939 - 941)


Kill this and replace with unreachable outside.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 984 - 985)


Kill this invariant check.

The agent does not validate if the second record type is `PROCESS_IO`. We 
need to validate it in the handler itself for now as per our earlier discussion.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 1044 - 1046)


Kill this and just have a `UNREACHABLE()`.


- Anand Mazumdar


On Dec. 4, 2016, 6:55 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> ---
> 
> (Updated Dec. 4, 2016, 6:55 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jie Yu.
> 
> 
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for ATTACH_CONTAINER_INPUT to the io switchboard.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 778367a268ec350ed438bc9fe9d359d63bdb5503 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 
> 
> Diff: https://reviews.apache.org/r/54297/diff/
> 
> 
> Testing
> ---
> 
> make -j check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54013: Added user doc for nested container and task group.

[Anand Mazumdar]

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



Gilbert, can you add [~neilc] as a reviewer too?

- Anand Mazumdar


On Nov. 30, 2016, 7:11 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Nov. 30, 2016, 7:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, Qian 
> Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6335
> https://issues.apache.org/jira/browse/MESOS-6335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added user doc for nested container and task group.
> 
> 
> Diffs
> -
> 
>   docs/nested-container-and-task-group.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54013/diff/
> 
> 
> Testing
> ---
> 
> Tested by gist view. Here is the link:
> 
> https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 54013: Added user doc for nested container and task group.

[haosdent huang]

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




docs/nested-container-and-task-group.md (line 303)


Why we refer aurora document here? Since could not find anything associated 
with POD in this link.


- haosdent huang


On Nov. 30, 2016, 7:11 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Nov. 30, 2016, 7:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, Qian 
> Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6335
> https://issues.apache.org/jira/browse/MESOS-6335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added user doc for nested container and task group.
> 
> 
> Diffs
> -
> 
>   docs/nested-container-and-task-group.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54013/diff/
> 
> 
> Testing
> ---
> 
> Tested by gist view. Here is the link:
> 
> https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>