Re: Review Request 54365: Fixed indentation of a function argument in master.cpp.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54359, 54360, 54361, 54365]

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. 7, 2016, 4:02 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54365/
> ---
> 
> (Updated Dec. 7, 2016, 4:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indentation of a function argument in master.cpp.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 
> 
> Diff: https://reviews.apache.org/r/54365/diff/
> 
> 
> Testing
> ---
> 
> line 3902 should be aligned with other arguments.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 53897: Changed how master represents "recovered" frameworks.

[Neil Conway]

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

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


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

After master failover, the new master doesn't know which frameworks were
registered with the previous master (because this information is not
currently stored in the registry). In the period after the master fails
over but before the framework scheduler has re-registered, the master
learns about the frameworks in the cluster when agents re-register (an
agent reports the FrameworkInfo for all of the frameworks it is running
when it re-registers).

Such frameworks were previously represented separately from the normal
list of frameworks in the master: the master kept a collection of
`FrameworkInfo` for these "recovered" frameworks.

This commit removes this separate collection of "recovered" frameworks.
Instead, the master now treats recovered frameworks very similarly to
frameworks that are registered but currently disconnected. For example,
recovered frameworks will now have a `Framework` object which tracks the
tasks/executors running under that framework; recovered frameworks will
be reported via the normal "frameworks" key when querying HTTP
endpoints. Similarly, "teardown" operations on recovered frameworks will
now work correctly (MESOS-6419).

This means there is no longer a concept of "orphan tasks" [1]: if the
master knows about a task, the task will be running under a framework
(albeit the framework might be recovered or disconnected). A new
"recovered" key has been added to various HTTP endpoints/APIs to
determine if a framework hasn't yet re-registered after master failover.

Another behavioral change is that, previously, a framework
re-registering after master failover was allowed to make arbitrary
changes to its `FrameworkInfo`. The master now only applies updates to
FrameworkInfo fields that can be safely changed (as was already done
when a framework fails over to a master that hasn't failed over). The
previous behavior qualifies as a bug.

[1] The exception here is if the cluster contains Mesos agents older
than 1.0, because old Mesos agents don't report `FrameworkInfo`s when
they re-register.


Diffs (updated)
-

  include/mesos/master/master.proto 966105cf14bf92ad15a38653cd5c7f3322821289 
  include/mesos/v1/master/master.proto 0e5a8674a76a46c6f2cc17e11cd735f756d94fd1 
  src/master/http.cpp ac560d1fdd219d0de0c5d987a32a7112e149602f 
  src/master/master.hpp b444b2352360fb4f7179acd97dffc0cd81cc7afa 
  src/master/master.cpp 67f32229470da4cf7953881d1c5dcb99393002de 
  src/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
  src/tests/fault_tolerance_tests.cpp 59f68f65fac9fca4a6941793b712bfe7bb30c880 
  src/tests/master_allocator_tests.cpp bb94e38d5bb472801366c172cfc036f2eecdcbcb 
  src/tests/master_authorization_tests.cpp 
06c6e47250003cd467bf37e1daa8bd636ef8fef5 
  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
  src/tests/partition_tests.cpp 5a0d4bd2de6a5aa0e9fdf0d34cd10d16fd4e34a1 
  src/tests/teardown_tests.cpp 125e1808e51da53fdf1bb1babc956ff062cbb102 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54472: Waited for the domain socket file in IOSwitchboard::connect.

[Jie Yu]

> On Dec. 7, 2016, 5:29 a.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 636-637
> > 
> >
> > @bmahler alwasy wanted me to put the " " on the next line instead of 
> > ending a line with a space. We seem to be inconsistent about this overall 
> > so I've adopted his way so that I mayself am consistent.

OK, sorry about that. I reverted this change.


- Jie


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


On Dec. 7, 2016, 5:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54472/
> ---
> 
> (Updated Dec. 7, 2016, 5:21 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6737
> https://issues.apache.org/jira/browse/MESOS-6737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Do not try to connect to the domain socket until the socket file has
> been created by the server.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c54d64efe7dc526473427bca89a1b65205e16018 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 851a1015294882ca5401cb8fd60ceff6f933249d 
> 
> Diff: https://reviews.apache.org/r/54472/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --gtest_filter=IOSwitchboardTest.ContainerAttach 
> --gtest_repeat=100
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54472: Waited for the domain socket file in IOSwitchboard::connect.

[Kevin Klues]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/io/switchboard.cpp (lines 635 - 636)


@bmahler alwasy wanted me to put the " " on the next line instead of ending 
a line with a space. We seem to be inconsistent about this overall so I've 
adopted his way so that I mayself am consistent.


- Kevin Klues


On Dec. 7, 2016, 5:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54472/
> ---
> 
> (Updated Dec. 7, 2016, 5:21 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6737
> https://issues.apache.org/jira/browse/MESOS-6737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Do not try to connect to the domain socket until the socket file has
> been created by the server.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c54d64efe7dc526473427bca89a1b65205e16018 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 851a1015294882ca5401cb8fd60ceff6f933249d 
> 
> Diff: https://reviews.apache.org/r/54472/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --gtest_filter=IOSwitchboardTest.ContainerAttach 
> --gtest_repeat=100
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54465: Added synchronization between agent and IOSwitchboard server for listen.

[Jie Yu]

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



I posted a simple work around in https://reviews.apache.org/r/54472/

I think we anyway need a time based loop.


src/slave/containerizer/mesos/io/switchboard.cpp (lines 185 - 190)


My main concern is that after agent restarted, we don't have a way to do 
synchronization.

We probably still need a time based loop here.



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


I would probably return a failure in .then if the container is being 
destroyed (i.e., not in 'infos')


- Jie Yu


On Dec. 7, 2016, 3:21 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54465/
> ---
> 
> (Updated Dec. 7, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-6737
> https://issues.apache.org/jira/browse/MESOS-6737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We use a pipe to synchronize with the io switchboard process to
> determine when it is ready to listen for incoming connections. We do
> not block the launch path with this pipe, but rather poll on it and
> block incoming connections until it is ready.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> f691b182d4029a15bbb3b1c158b176d43f265cc1 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c54d64efe7dc526473427bca89a1b65205e16018 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> 6ce4cdb08e3e15eda21dcb740c4390613a88f5c5 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 851a1015294882ca5401cb8fd60ceff6f933249d 
> 
> Diff: https://reviews.apache.org/r/54465/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 54472: Waited for the domain socket file in IOSwitchboard::connect.

[Jie Yu]

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

Review request for mesos, Kevin Klues and Vinod Kone.


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


Repository: mesos


Description
---

Do not try to connect to the domain socket until the socket file has
been created by the server.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
c54d64efe7dc526473427bca89a1b65205e16018 
  src/tests/containerizer/io_switchboard_tests.cpp 
851a1015294882ca5401cb8fd60ceff6f933249d 

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


Testing
---

bin/mesos-tests.sh --gtest_filter=IOSwitchboardTest.ContainerAttach 
--gtest_repeat=100


Thanks,

Jie Yu

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54381]

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. 6, 2016, 2:15 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54381/
> ---
> 
> (Updated Dec. 6, 2016, 2:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces authorization support for the newly introduced debug API
> calls: `AttachContainerInput` and `AttachContainerOutput`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 91eebe117d3dc86dd5b1fd47156c01e147165cef 
>   src/slave/slave.hpp 4b94dff9c1f9f212f84984674268ef38b43d93bd 
>   src/slave/slave.cpp 7eb45036f49289972c0b706c3c8e2ed96553d279 
>   src/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
> 
> Diff: https://reviews.apache.org/r/54381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54465: Added synchronization between agent and IOSwitchboard server for listen.

[Vinod Kone]

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



LGTM overall


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


why is this optional? shouldn't this be required?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 631 - 645)


can this be?

```
infos[containerId]->listening = process::io::poll(..)
  .then([serverListeningPipe]() {
os::close(serverListeningPipe[0]);
return Nothing();
  })
```



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


why is this note no longer valid?



src/slave/containerizer/mesos/io/switchboard_main.cpp (line 97)


looks like at this point the server has called `listen` but not `accept`. i 
guess that should be ok?


- Vinod Kone


On Dec. 7, 2016, 3:21 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54465/
> ---
> 
> (Updated Dec. 7, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-6737
> https://issues.apache.org/jira/browse/MESOS-6737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We use a pipe to synchronize with the io switchboard process to
> determine when it is ready to listen for incoming connections. We do
> not block the launch path with this pipe, but rather poll on it and
> block incoming connections until it is ready.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> f691b182d4029a15bbb3b1c158b176d43f265cc1 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c54d64efe7dc526473427bca89a1b65205e16018 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> 6ce4cdb08e3e15eda21dcb740c4390613a88f5c5 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 851a1015294882ca5401cb8fd60ceff6f933249d 
> 
> Diff: https://reviews.apache.org/r/54465/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 54470: Windows: Fixed default isolators in Agent.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


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


Repository: mesos


Description
---

Windows: Fixed default isolators in Agent.

This commit partially addresses MESOS-6717, specifically the fact that
some of the default isolators currently used by the Agent are
POSIX-specific, and will cause every Agent test to crash.

In particular, this commit will transition Windows builds of the agent
away from using the `posix/cpu`, `posix/mem`, and `filesystem/posix`
isolators by default, replacing them with `windows/cpu` and
`filesystem/windows` (sadly, there is not yet a memory isolator for
Windows).


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
6d50755fa24eb67bb72a78f9acc69c7e5eecabe2 
  src/slave/flags.cpp 67326eece05e6300d1407ed8887aabb2f06fe9cd 
  src/slave/http.cpp 91eebe117d3dc86dd5b1fd47156c01e147165cef 
  src/tests/mesos.cpp 8fd8bcb033f47e2538aa36cd373c892a882afdfd 

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


Testing
---


Thanks,

Alex Clemmer

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

[Benjamin Mahler]

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


Fix it, then Ship it!





src/master/master.cpp (line 7033)


Thinking about this again, we probably should avoid capturing the framework 
pointer and instead take it explicitly:

```
auto addFrameworkRole = [this](Framework* framework, const string& role) {

auto removeFrameworkRole = [this](Framework* framework, const string& role) 
{
```


- Benjamin Mahler


On Dec. 7, 2016, 3:58 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54360/
> ---
> 
> (Updated Dec. 7, 2016, 3:58 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
> ---
> 
> When a multi-role framework registers, master should add roles
> from 'roles' field of FrameworkInfo to internal data structure
> `activeRoles`. On the other hand, master should remove framework
> from all the roles it subscribed with.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 
> 
> Diff: https://reviews.apache.org/r/54360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54380: Don't send PIDs of disconnected frameworks to re-registering agents.

[Neil Conway]

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

(Updated Dec. 7, 2016, 4:25 a.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Don't send PIDs of disconnected frameworks to re-registering agents.


Diffs
-

  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54408: Replaced `Master::Framework::active` with a new `state` enum value.

[Neil Conway]

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

(Updated Dec. 7, 2016, 4:20 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak commit message.


Summary (updated)
-

Replaced `Master::Framework::active` with a new `state` enum value.


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


Repository: mesos


Description
---

That is, the master previously tracked two separate things about a
framework: its "state" (CONNECTED, DISCONNECTED, or RECOVERED), and
whether the framework is considered active. It is simpler to represent
the latter value as just another state: a framework can now be ACTIVE,
INACTIVE, DISCONNECTED, or RECOVERED. A framework is "connected" if it
is either ACTIVE or INACTIVE. This rules out a few combinations that
never made sense, such as "state = DISCONNECTED and active = TRUE".


Diffs (updated)
-

  src/master/http.cpp ac560d1fdd219d0de0c5d987a32a7112e149602f 
  src/master/master.hpp b444b2352360fb4f7179acd97dffc0cd81cc7afa 
  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 
  src/master/quota_handler.cpp 5578663f26d9b737499dc4f5a286c34c37645442 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 54467: Fixed incorrect warning messages.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

These warning messages printed the old value of the `FrameworkInfo`
field, not the new (ignored) value.


Diffs
-

  src/master/master.hpp b444b2352360fb4f7179acd97dffc0cd81cc7afa 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 54468: Added some additional CHECKs to the master.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added some additional CHECKs to the master.


Diffs
-

  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 53897: Changed how master represents "recovered" frameworks.

[Neil Conway]

> On Dec. 3, 2016, 2:12 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 7132-7137
> > 
> >
> > hmm. it's a bit weird that activating a framework also updates it. this 
> > should be done by the callers before calling this method.
> 
> Neil Conway wrote:
> Hmmm, why is this weird? To me it seems reasonable that when we activate 
> a recovered framework, we're given the `FrameworkInfo` that was presented by 
> the framework on re-registration; we use that `FrameworkInfo` to update 
> whatever `FrameworkInfo` we previously had for the recovered framework.
> 
> If we move this outside of `activateRecoveredFramework`, we'd need 
> identical code in both of its call-sites -- that's not too bad, but I'm not 
> sure why it is an improvement.
> 
> Vinod Kone wrote:
> i meant the name "activateRecoveredFramework" doesn't seemt to suggest 
> that it also "updates" in addition to "activate"? usually they are done by 
> different functions (e.g., Allocator::activateFramework(), 
> Allocator::updateFramework(), Allocator::activateSlave(), 
> Allocator::updateSlave). when i think of activating a recovered framework, my 
> intuition is that it goes from RECOVERED to ACTIVATED state in the master and 
> maybe gets activated in the allocator (basically things that need to be done 
> to get a framework activated).
> 
> More importanly, i realized that previously a framework could update its 
> FrameworkInfo willy nilly after a master failover, but now it cannot because 
> `updateFrameworkInfo` doesn't allow updates to all fields. That's probably ok 
> but maybe worth calling out?

I added a note about the behavioral change to the commit message. I also added 
a test to validate this behavior.


- Neil


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


On Dec. 4, 2016, 2:06 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53897/
> ---
> 
> (Updated Dec. 4, 2016, 2:06 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6419
> https://issues.apache.org/jira/browse/MESOS-6419
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> After master failover, the new master doesn't know which frameworks were
> registered with the previous master (because this information is not
> currently stored in the registry). In the period after the master fails
> over but before the framework scheduler has re-registered, the master
> learns about the frameworks in the cluster when agents re-register (an
> agent reports the FrameworkInfo for all of the frameworks it is running
> when it re-registers).
> 
> Such frameworks were previously represented separately from the normal
> list of frameworks in the master: the master kept a collection of
> `FrameworkInfo` for these "recovered" frameworks.
> 
> This commit removes this separate collection of "recovered" frameworks.
> Instead, the master now treats recovered frameworks very similarly to
> frameworks that are registered but currently disconnected. For example,
> recovered frameworks will now have a `Framework` object which tracks the
> tasks/executors running under that framework; recovered frameworks will
> be reported via the normal "frameworks" key when querying HTTP
> endpoints. Similarly, "teardown" operations on recovered frameworks will
> now work correctly (MESOS-6419).
> 
> This means there is no longer a concept of "orphan tasks" [1]: if the
> master knows about a task, the task will be running under a framework
> (albeit the framework might be recovered or disconnected). A new
> "recovered" key has been added to various HTTP endpoints/APIs to
> determine if a framework hasn't yet re-registered after master failover.
> 
> [1] The exception here is if the cluster contains Mesos agents older
> than 1.0, because old Mesos agents don't report `FrameworkInfo`s when
> they re-register.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 966105cf14bf92ad15a38653cd5c7f3322821289 
>   include/mesos/v1/master/master.proto 
> 0e5a8674a76a46c6f2cc17e11cd735f756d94fd1 
>   src/master/http.cpp ac560d1fdd219d0de0c5d987a32a7112e149602f 
>   src/master/master.hpp b444b2352360fb4f7179acd97dffc0cd81cc7afa 
>   src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 
>   src/tests/api_tests.cpp afae6a7e0809174f48f280f170fad9315e80a906 
>   src/tests/fault_tolerance_tests.cpp 
> 59f68f65fac9fca4a6941793b712bfe7bb30c880 
>   src/tests/master_allocator_tests.cpp 
> bb94e38d5bb472801366c172cfc036f2eecdcbcb 
>   src/tests/master_authorization_tests.cpp 
> 

Re: Review Request 53897: Changed how master represents "recovered" frameworks.

[Neil Conway]

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

(Updated Dec. 7, 2016, 4:14 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Add additional CHECKs, add new unit test, tweak a few tests.


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


Repository: mesos


Description (updated)
---

After master failover, the new master doesn't know which frameworks were
registered with the previous master (because this information is not
currently stored in the registry). In the period after the master fails
over but before the framework scheduler has re-registered, the master
learns about the frameworks in the cluster when agents re-register (an
agent reports the FrameworkInfo for all of the frameworks it is running
when it re-registers).

Such frameworks were previously represented separately from the normal
list of frameworks in the master: the master kept a collection of
`FrameworkInfo` for these "recovered" frameworks.

This commit removes this separate collection of "recovered" frameworks.
Instead, the master now treats recovered frameworks very similarly to
frameworks that are registered but currently disconnected. For example,
recovered frameworks will now have a `Framework` object which tracks the
tasks/executors running under that framework; recovered frameworks will
be reported via the normal "frameworks" key when querying HTTP
endpoints. Similarly, "teardown" operations on recovered frameworks will
now work correctly (MESOS-6419).

This means there is no longer a concept of "orphan tasks" [1]: if the
master knows about a task, the task will be running under a framework
(albeit the framework might be recovered or disconnected). A new
"recovered" key has been added to various HTTP endpoints/APIs to
determine if a framework hasn't yet re-registered after master failover.

Another behavioral change is that, previously, a framework
re-registering after master failover was allowed to make arbitrary
changes to its `FrameworkInfo`. The master now only applies updates to
FrameworkInfo fields that can be safely changed (as was already done
when a framework fails over to a master that hasn't failed over). The
previous behavior qualifies as a bug.

[1] The exception here is if the cluster contains Mesos agents older
than 1.0, because old Mesos agents don't report `FrameworkInfo`s when
they re-register.


Diffs (updated)
-

  include/mesos/master/master.proto 966105cf14bf92ad15a38653cd5c7f3322821289 
  include/mesos/v1/master/master.proto 0e5a8674a76a46c6f2cc17e11cd735f756d94fd1 
  src/master/http.cpp ac560d1fdd219d0de0c5d987a32a7112e149602f 
  src/master/master.hpp b444b2352360fb4f7179acd97dffc0cd81cc7afa 
  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 
  src/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
  src/tests/fault_tolerance_tests.cpp 59f68f65fac9fca4a6941793b712bfe7bb30c880 
  src/tests/master_allocator_tests.cpp bb94e38d5bb472801366c172cfc036f2eecdcbcb 
  src/tests/master_authorization_tests.cpp 
06c6e47250003cd467bf37e1daa8bd636ef8fef5 
  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
  src/tests/partition_tests.cpp 5a0d4bd2de6a5aa0e9fdf0d34cd10d16fd4e34a1 
  src/tests/teardown_tests.cpp 125e1808e51da53fdf1bb1babc956ff062cbb102 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54177: Simplified some test code.

[Neil Conway]

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

(Updated Dec. 7, 2016, 4:07 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Simplified more test code.


Summary (updated)
-

Simplified some test code.


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


Repository: mesos


Description (updated)
---

Simplified some test code.


Diffs (updated)
-

  src/tests/fault_tolerance_tests.cpp 59f68f65fac9fca4a6941793b712bfe7bb30c880 
  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
  src/tests/slave_tests.cpp 3d06c3a4a74bbcba4c9b6bdba4258c5aefdab845 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54365: Fixed indentation of a function argument in master.cpp.

[Jay Guo]

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

(Updated Dec. 7, 2016, 4:02 a.m.)


Review request for mesos, Benjamin Mahler and Guangya Liu.


Repository: mesos


Description
---

Fixed indentation of a function argument in master.cpp.


Diffs
-

  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 

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


Testing
---

line 3902 should be aligned with other arguments.


Thanks,

Jay Guo

Re: 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/
---

(Updated Dec. 7, 2016, 4:01 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 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

Re: Review Request 54361: Added tests for add/remove multi-role framework.

[Benjamin Mahler]

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




src/tests/master_tests.cpp (lines 4916 - 4917)


Ditto guangya's suggestion about testing the addition and removal in a 
single test. Also, do you know how the testing was done when implicit roles 
were added?

See: https://reviews.apache.org/r/41225/

Should we place this test alongside the existing role tests? Seems that 
r/41225 should have been testing this already, is it not the case?


- Benjamin Mahler


On Dec. 7, 2016, 3:59 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated Dec. 7, 2016, 3:59 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 tests for add/remove multi-role framework.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
> 
> Diff: https://reviews.apache.org/r/54361/diff/
> 
> 
> Testing
> ---
> 
> make
> make check GTEST_FILTER="MasterTest.AddFrameworkWithMultipleRoles"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: 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/
---

(Updated Dec. 7, 2016, 4 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
---

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 tests for add/remove multi-role framework.

[Jay Guo]

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

(Updated Dec. 7, 2016, 3:59 a.m.)


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


Changes
---

Merge two tests into one.


Summary (updated)
-

Added tests for add/remove multi-role framework.


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


Repository: mesos


Description (updated)
---

Added tests for add/remove multi-role framework.


Diffs (updated)
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

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


Testing
---

make
make check GTEST_FILTER="MasterTest.AddFrameworkWithMultipleRoles"
make check


Thanks,

Jay Guo

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

[Jay Guo]

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

(Updated Dec. 7, 2016, 3:58 a.m.)


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


Changes
---

Addressed guangya's comments. Also squashed patches for `addFramework` and 
`removeFramework` into one. Otherwise, `addFramework` alone introduces some 
flakyness for tests where multi-role framework is involved.


Summary (updated)
-

Changed master to add/remove roles for a multi-role framework.


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


Repository: mesos


Description (updated)
---

When a multi-role framework registers, master should add roles
from 'roles' field of FrameworkInfo to internal data structure
`activeRoles`. On the other hand, master should remove framework
from all the roles it subscribed with.


Diffs (updated)
-

  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 54465: Added synchronization between agent and IOSwitchboard server for listen.

[Kevin Klues]

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

(Updated Dec. 7, 2016, 3:21 a.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Changes
---

Added a test and fixed a small bug in `main` (which was never writing anything 
to the pipe originally!).


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


Repository: mesos


Description
---

We use a pipe to synchronize with the io switchboard process to
determine when it is ready to listen for incoming connections. We do
not block the launch path with this pipe, but rather poll on it and
block incoming connections until it is ready.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.hpp 
f691b182d4029a15bbb3b1c158b176d43f265cc1 
  src/slave/containerizer/mesos/io/switchboard.cpp 
c54d64efe7dc526473427bca89a1b65205e16018 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
6ce4cdb08e3e15eda21dcb740c4390613a88f5c5 
  src/tests/containerizer/io_switchboard_tests.cpp 
851a1015294882ca5401cb8fd60ceff6f933249d 

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


Testing
---

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


Thanks,

Kevin Klues

Re: Review Request 54449: Check quotas are enabled in the XFS disk isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54449]

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. 6, 2016, 9:21 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> ---
> 
> (Updated Dec. 6, 2016, 9:21 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
> 
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
> 
> 
> Diffs
> -
> 
>   configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> dd4df86bf90bfa9cbf4664d89274cf3c64c2e374 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> 7602fe3b6ab069db643397418732e773d0417f8a 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
> b9d8e7dc999ba3064bee7105eff0f9553d825df8 
> 
> Diff: https://reviews.apache.org/r/54449/diff/
> 
> 
> Testing
> ---
> 
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 54465: Added synchronization between agent and IOSwitchboard server for listen.

[Kevin Klues]

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

(Updated Dec. 7, 2016, 2:47 a.m.)


Review request for mesos, Jie Yu and Vinod Kone.


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


Repository: mesos


Description
---

We use a pipe to synchronize with the io switchboard process to
determine when it is ready to listen for incoming connections. We do
not block the launch path with this pipe, but rather poll on it and
block incoming connections until it is ready.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.hpp 
f691b182d4029a15bbb3b1c158b176d43f265cc1 
  src/slave/containerizer/mesos/io/switchboard.cpp 
c54d64efe7dc526473427bca89a1b65205e16018 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
6ce4cdb08e3e15eda21dcb740c4390613a88f5c5 

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


Testing
---

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


Thanks,

Kevin Klues

Review Request 54465: Added synchronization between agent and IOSwitchboard server for listen.

[Kevin Klues]

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

Review request for mesos, Jie Yu and Vinod Kone.


Repository: mesos


Description
---

We use a pipe to synchronize with the io switchboard process to
determine when it is ready to listen for incoming connections. We do
not block the launch path with this pipe, but rather poll on it and
block incoming connections until it is ready.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.hpp 
f691b182d4029a15bbb3b1c158b176d43f265cc1 
  src/slave/containerizer/mesos/io/switchboard.cpp 
c54d64efe7dc526473427bca89a1b65205e16018 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
6ce4cdb08e3e15eda21dcb740c4390613a88f5c5 

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


Testing
---

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


Thanks,

Kevin Klues

Review Request 54462: Windows: Added APR include path to libprocess configuration.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


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


Repository: mesos


Description
---

Windows: Added APR include path to libprocess configuration.

Partially addresses MESOS-3447, as APR is a dependency of the SVN
facilities of Stout.

On Unix builds, APR is expected to have been installed on the system
prior to building Mesos (usually by a package manager). Since Windows
does not have a package manager or a reasonble way of automatically
discovering where a package is installed (aside from the registry), our
CMake build system takes it upon itself to manage these system
dependencies.  manage this dependency itself. This means that on
Windows, we need to configure the build to look for the APR headers in
our custom-downloaded APR repository.  Currently, though, we are not
doing this, so when we'll hit a compile-time error if we try to build
(e.g.) `svn.hpp`.

This commit will introduce the APR include paths as part of the build
against Stout. Since Stout is a header-only library, it is (right now)
incumbent on whoever is bundling Stout up to manage the third-party
dependencies of Stout. In our current implementation, libprocess manages
the APR dependency for Stout, so (just as we do for other dependencies
like cURL) we will have it manage this logic as well.


Diffs
-

  3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
786e47e63dc03ab4851c93ec2030f85c049cebe9 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 53550: Rename symbols in log.proto to avoid naming collision in win32 API.

[Alex Clemmer]

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

(Updated Dec. 7, 2016, 2:16 a.m.)


Review request for mesos, Daniel Pravat and Joseph Wu.


Changes
---

Changed `IGNORE_RESPONSE` -> `IGNORED` and `IGNORE_PROMISE` -> `IGNORED`.


Repository: mesos


Description
---

The standard win32 headers define a macro, `IGNORE`, which presently
collides with two uses of the same symbol in log.proto. This causes a
compile error on Windows.

In this commit, we rename the symbol in the log.proto files. There are
two primary reasons for this.

The first is because the trick we have previously applied to get around
similar problems (in which we `#undef` the macro, and re-define as a
global constant) is made somewaht more complex by the fact that the
log.proto headers are generated by protocol buffers. To implement this
effectively, we'd have to individually `#undef` at every site we include
log.pb.h, which is not a huge deal given the number of #include sites,
but doesn't protect us against future build breaks.

The second is that the approach of re-naming the symbol in log.proto is
not very invasive: we need to change a handful of places where the
system is used, and we never have to think of the issue again. And,
because it is an internal API, we don't require a major version bump to
implement the change.


Diffs (updated)
-

  src/log/consensus.cpp 94ddf245c07ccd38d4fe828bc47c98ecf540243f 
  src/log/coordinator.cpp 72ef0366633b4e4137fd303fa49f9efb166c80c9 
  src/log/replica.cpp d596e617d4b4eab91245e0a88a3b9479fc75b813 
  src/messages/log.proto 6a2c26be2afe411e6927cddebcfd9634b2b1b884 
  src/tests/log_tests.cpp 99954388eb0fad2acde0cedfd7daa3c9379bfb03 

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


Testing
---


Thanks,

Alex Clemmer

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

[Neil Conway]

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



This should be linked from elsewhere in the docs. e.g., `home.md`.

- Neil Conway


On Dec. 6, 2016, 4:28 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Dec. 6, 2016, 4:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Artem Harutyunyan, 
> Jie Yu, Neil Conway, 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.

[Gilbert Song]

> On Dec. 6, 2016, 9:23 a.m., Jie Yu wrote:
> > docs/nested-container-and-task-group.md, line 125
> > 
> >
> > Can you follow up to update mesos-containerizer.md to include this 
> > isolator and provide a link here?

yes, I can do that.


- Gilbert


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


On Dec. 6, 2016, 8:28 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Dec. 6, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Artem Harutyunyan, 
> Jie Yu, Neil Conway, 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 54434: Used UNSET_CLOEXEC instead of DUP2 hook for I/O switchboard.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54432, 54433, 54434]

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. 6, 2016, 6:44 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54434/
> ---
> 
> (Updated Dec. 6, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used UNSET_CLOEXEC instead of DUP2 hook for I/O switchboard.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
> 
> Diff: https://reviews.apache.org/r/54434/diff/
> 
> 
> Testing
> ---
> 
> make check on both osx and linux
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54434: Used UNSET_CLOEXEC instead of DUP2 hook for I/O switchboard.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 6, 2016, 6:44 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54434/
> ---
> 
> (Updated Dec. 6, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used UNSET_CLOEXEC instead of DUP2 hook for I/O switchboard.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
> 
> Diff: https://reviews.apache.org/r/54434/diff/
> 
> 
> Testing
> ---
> 
> make check on both osx and linux
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54433: Added a UNSET_CLOEXEC child hook to subprocess.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 6, 2016, 6:44 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54433/
> ---
> 
> (Updated Dec. 6, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a UNSET_CLOEXEC child hook to subprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> 0d9c74a7d01795868b06479c794b8b7521f9c973 
>   3rdparty/libprocess/src/subprocess.cpp 
> 340fc32bdffb083357a886ece31f1353885e8642 
> 
> Diff: https://reviews.apache.org/r/54433/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54432: Added os::unsetCloexec to stout.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 6, 2016, 6:43 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54432/
> ---
> 
> (Updated Dec. 6, 2016, 6:43 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added os::unsetCloexec to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/fcntl.hpp 
> daaca0da959b8ad04bc5042b85e5b9e3e6653344 
>   3rdparty/stout/include/stout/os/windows/fcntl.hpp 
> 2bc794a405e59d80c1e8308c0049d2306347adfa 
>   3rdparty/stout/tests/os_tests.cpp 446282373aebb06dae32209a9b0d148cb15eced9 
> 
> Diff: https://reviews.apache.org/r/54432/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

[Adam B]

> On Dec. 5, 2016, 4:55 p.m., Vinod Kone wrote:
> > src/tests/api_tests.cpp, lines 3728-3793
> > 
> >
> > why did you change these tests? the idea with the tests was to verify 
> > that `Containerizer::attach()` was getting called from the API handler.
> 
> Alexander Rojas wrote:
> Because one needs to find the containers to perform authorization, and 
> with these tests there was no container to be found, so no authz to be 
> performed and therefor not mockContainerizer to be called.
> 
> Adam B wrote:
> Alexander's code adds extra verification that the provided containerId 
> maps to a known executor before reaching the containerizer, which is why a 
> `NotFound` is now returned and `attach` is never called in these tests, since 
> they use a completely random/invalid containerId. If you want the tests to 
> verify that `Containerizer::attach()` is getting called from the API handler, 
> then we should modify the tests to use a valid containerId, perhaps just the 
> first one listed in `/containers`, although that may require us to launch a 
> task so that there is a container running.

@vinodkone We have updated the tests to not use authz, and updated the code to 
work around the new containerID verification check, so your tests continue to 
work "as expected". Please take a look.


- Adam


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


On Dec. 6, 2016, 6:15 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54381/
> ---
> 
> (Updated Dec. 6, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces authorization support for the newly introduced debug API
> calls: `AttachContainerInput` and `AttachContainerOutput`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 91eebe117d3dc86dd5b1fd47156c01e147165cef 
>   src/slave/slave.hpp 4b94dff9c1f9f212f84984674268ef38b43d93bd 
>   src/slave/slave.cpp 7eb45036f49289972c0b706c3c8e2ed96553d279 
>   src/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
> 
> Diff: https://reviews.apache.org/r/54381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

[Adam B]

> On Dec. 6, 2016, 12:50 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 2465-2468
> > 
> >
> > Can you please comment on the need for std::move here?
> > Why is decoder a `&&` in the first place? @vinodkone may be able to 
> > comment

>From Anand: It’s passing ownership of the `decoder` to the 
>`attachContainerInput` function. It could have been a constant reference too 
>based on how we use the `Owned` pointer in our code but it would break when we 
>move `Owned` to rely on move semantics ala `unique_ptr`.
Adam: So then it's ok for us to `return _attachContainerInput(call, 
std::move(decoder_), contentType, acceptType);` to pass ownership of the 
`decoder` from `attachContainerInput()` to `_attachContainerInput()`?
Anand: yes

Sounds like a quick `Pass on ownership of decoder to the continuation.` comment 
would be sufficient here.


- Adam


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


On Dec. 6, 2016, 6:15 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54381/
> ---
> 
> (Updated Dec. 6, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces authorization support for the newly introduced debug API
> calls: `AttachContainerInput` and `AttachContainerOutput`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 91eebe117d3dc86dd5b1fd47156c01e147165cef 
>   src/slave/slave.hpp 4b94dff9c1f9f212f84984674268ef38b43d93bd 
>   src/slave/slave.cpp 7eb45036f49289972c0b706c3c8e2ed96553d279 
>   src/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
> 
> Diff: https://reviews.apache.org/r/54381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54446: Windows: Added `authentication_tests.cpp` build.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Dec. 6, 2016, 12:51 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54446/
> ---
> 
> (Updated Dec. 6, 2016, 12:51 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6697
> https://issues.apache.org/jira/browse/MESOS-6697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will partially address MESOS-6697 by adding
> `authentication_tests.cpp` to the Windows build, but disabling all
> tests in the file, as the Windows implementation of the Master currently
> does not support authentication (see MESOS-6733).
> 
> Our goal in turning on the tests is to ensure that the tests remain able
> to compile on Windows as we continue to work on MESOS-6733. When this
> issue is resolved, we do expect to come back and turn on these tests,
> which should resolve MESOS-6697.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4d8658bf4e2e4ec07a56542539c9409a1d9079d3 
>   src/tests/CMakeLists.txt 2ce9d37a75fdec3468aef8aba91db3a0a40e0220 
>   src/tests/authentication_tests.cpp fc7afae0abde8e9274685b9a2121f5a91d59f3d6 
> 
> Diff: https://reviews.apache.org/r/54446/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 54395: Enabled build of Agent test harness on Windows.

[Joseph Wu]

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


Ship it!




Looks good on the OSX and Ubuntu.

- Joseph Wu


On Dec. 6, 2016, 1:11 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54395/
> ---
> 
> (Updated Dec. 6, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6696 and MESOS-6717
> https://issues.apache.org/jira/browse/MESOS-6696
> https://issues.apache.org/jira/browse/MESOS-6717
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled build of Agent test harness on Windows.
> 
> This commit adds the Agent test harness to the Windows builds. This is
> the first major step towards lighting up comprehensive Agent testing on
> agent builds, an important step that has been prevented by a variety of
> issues (e.g., things like getting the Master to work for at least
> developer scenarios, and so on).
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 4d8658bf4e2e4ec07a56542539c9409a1d9079d3 
>   src/tests/CMakeLists.txt 2ce9d37a75fdec3468aef8aba91db3a0a40e0220 
>   src/tests/cluster.cpp 6a9d94b8ac3c8fd0428b7a67d1cb3f99a658fa9b 
>   src/tests/environment.cpp e08678c5610332b80ce7db47e697651fa0109502 
>   src/tests/main.cpp c10eeac335b0c8cdb2ea6a0701915ec33f76a2b2 
>   src/tests/test_helper_main.cpp a7d511ce71e2789df50aef02d2d50b4c94f38a50 
> 
> Diff: https://reviews.apache.org/r/54395/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 54381: Adds authorization support when attaching containers input/output.

[Adam B]

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



A few nits, but we really just need Vinod's approval of the test updates, and 
his (or somebody's) input on the `&&` and `std::move`


src/slave/http.cpp (line 2398)


Nit: You use this value 3 times. Maybe worth pulling it out into a variable.



src/slave/http.cpp (lines 2427 - 2428)


Nit: Based on the `locateExecutor` logic, if executor != NULL, then 
framework != NULL. In fact, maybe you don't even need to calculate the 
frameworkId/Info until you're ready to set it in the Object. Then this logic is 
just `if (executor.isSome() && executor == nullptr)`



src/slave/http.cpp (line 2712)


Nit: container_id is `required` in `AttachContainerOutput` so it's not 
really necessary to CHECK it here.


- Adam B


On Dec. 6, 2016, 6:15 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54381/
> ---
> 
> (Updated Dec. 6, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Adam B, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces authorization support for the newly introduced debug API
> calls: `AttachContainerInput` and `AttachContainerOutput`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 91eebe117d3dc86dd5b1fd47156c01e147165cef 
>   src/slave/slave.hpp 4b94dff9c1f9f212f84984674268ef38b43d93bd 
>   src/slave/slave.cpp 7eb45036f49289972c0b706c3c8e2ed96553d279 
>   src/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
> 
> Diff: https://reviews.apache.org/r/54381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54367: Added support to destroy running DEBUG containers on agent recovery.

[Kevin Klues]

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

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


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie's comments.


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


Repository: mesos


Description
---

Added support to destroy running DEBUG containers on agent recovery.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
cb2312205bf49963e35b95a6bfd71fd0960b837b 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
6c341e14cc383410baa76ea8eea294c627d1c028 

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


Testing
---

TEST_FILTER="" make -j40 check
sudo GTEST_FILTER="*DestroyDebugContainerOnRecover*" src/mesos-tests


Thanks,

Kevin Klues

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

[Benjamin Mahler]

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




src/master/master.cpp (lines 7033 - 7056)


How about using a lambda here to simplify things?

```
auto addRole = [&](const string& role) {
  CHECK(isWhitelistedRole(role))
<< "Unknown role '" << role << "'"
<< " of framework " << *framework;

  if (!activeRoles.contains(role)) {
activeRoles[role] = new Role();
  }
  activeRoles.at(role)->addFramework(framework);
};

if (protobuf::frameworkHasCapability(
framework->info,
FrameworkInfo::Capability::MULTI_ROLE)) {
  foreach (const string& role, framework->info.roles()) {
addRole(role);
  }
} else {
  addRole(framework->info.role();
}
```

You could make a similar change in your next patch I believe.


- Benjamin Mahler


On Dec. 5, 2016, 4:57 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54360/
> ---
> 
> (Updated Dec. 5, 2016, 4:57 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
> ---
> 
> 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
> 
>

Re: Review Request 54455: Added test for IOSwitchboard `recovery()`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 6, 2016, 11:03 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54455/
> ---
> 
> (Updated Dec. 6, 2016, 11:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6688
> https://issues.apache.org/jira/browse/MESOS-6688
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for IOSwitchboard `recovery()`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> c8fe8768db705d97299215ab69b91babb8bbc2f9 
> 
> Diff: https://reviews.apache.org/r/54455/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*RecoverThenKillSwitchboardContainerDestroyed*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

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

[Benjamin Mahler]

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



Can you combine this change with https://reviews.apache.org/r/54362/ into a 
single patch?

- Benjamin Mahler


On Dec. 5, 2016, 4:57 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54360/
> ---
> 
> (Updated Dec. 5, 2016, 4:57 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
> ---
> 
> 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
> 
>

Re: Review Request 54391: Added ability for client to signal EOF on ATTACH_CONTAINER_INPUT.

[Jie Yu]

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


Ship it!




LGTM.

- Jie Yu


On Dec. 5, 2016, 10:18 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54391/
> ---
> 
> (Updated Dec. 5, 2016, 10:18 p.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 ability for client to signal EOF on ATTACH_CONTAINER_INPUT.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 4258e861357225d7f9613b66f66121e28961dbf1 
> 
> Diff: https://reviews.apache.org/r/54391/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> No new unit tests added for this, but tested manually with the CLI and it 
> works as expected.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54410: Made the style of `.navbar-text` consistent with others.

[Benjamin Mahler]

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



Is this just changing the color? What did the added margins do?

- Benjamin Mahler


On Dec. 6, 2016, 9:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54410/
> ---
> 
> (Updated Dec. 6, 2016, 9:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-6725
> https://issues.apache.org/jira/browse/MESOS-6725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the style of `.navbar-text` consistent with other texts in the
> navbar.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/css/mesos.css 
> c817aaee0fd2d2579ff2e4a152d47e83ce80d68c 
> 
> Diff: https://reviews.apache.org/r/54410/diff/
> 
> 
> Testing
> ---
> 
> # Before:
> 
> ![](https://issues.apache.org/jira/secure/attachment/12841913/before.png)
> 
> # After:
> 
> ![](https://issues.apache.org/jira/secure/attachment/12841912/after.png)
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 54411: Fixed the colspan number of task tables in WebUI.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Dec. 6, 2016, 9:42 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54411/
> ---
> 
> (Updated Dec. 6, 2016, 9:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-6728
> https://issues.apache.org/jira/browse/MESOS-6728
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the colspan number of task tables in WebUI.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/home.html 4109e89e783aa75beb83c1d3affbdf3ec4230bc2 
> 
> Diff: https://reviews.apache.org/r/54411/diff/
> 
> 
> Testing
> ---
> 
> # Before:
> 
> ![](https://issues.apache.org/jira/secure/attachment/12841917/before_54411.png)
> 
> # After:
> 
> ![](https://issues.apache.org/jira/secure/attachment/12841916/after_54411.png)
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 54367: Added support to destroy running DEBUG containers on agent recovery.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/containerizer.cpp (lines 757 - 765)


I would also check if `containerId.has_parent()` here (i.e., only for 
nested container). The semantics for top level executor container with this 
file is a bit weird at the moment. Should we remove it from the 'alive' list?

My suggestion is that, we add an extra check below:
```
if (containerId.has_parent() &&
alive.contains(getRootContainerId(containerId)) &&
pid.isSome() &&
!containerizer::paths::getContainerForceDestroyOnRecovery(
flags.runtime_dir, containerId)) {
  // Add to 'recoverable'.
}
```



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


launch a debug container which should be destroyed on recovery


- Jie Yu


On Dec. 5, 2016, 9:44 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54367/
> ---
> 
> (Updated Dec. 5, 2016, 9:44 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6718
> https://issues.apache.org/jira/browse/MESOS-6718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support to destroy running DEBUG containers on agent recovery.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 13cf75735f3e78a9725904a886e0f537f795b33e 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6c341e14cc383410baa76ea8eea294c627d1c028 
> 
> Diff: https://reviews.apache.org/r/54367/diff/
> 
> 
> Testing
> ---
> 
> TEST_FILTER="" make -j40 check
> sudo GTEST_FILTER="*DestroyDebugContainerOnRecover*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54368: Added helpers to checkpoint a 'destroy-on-recovery' file for containers.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 6, 2016, 11:04 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54368/
> ---
> 
> (Updated Dec. 6, 2016, 11:04 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6718
> https://issues.apache.org/jira/browse/MESOS-6718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existence of this file causes the containerizer to mark the
> container for destruction after agent restarts. This is currently
> useful for DEBUG containers launched by a
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/paths.hpp 
> dd197f92c39c80d73326a1507ad1850086830e88 
>   src/slave/containerizer/mesos/paths.cpp 
> c5542873b6a313db2405caf9b3d25fead30ba216 
> 
> Diff: https://reviews.apache.org/r/54368/diff/
> 
> 
> Testing
> ---
> 
> Tested in subsequent patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.

[Joseph Wu]

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


Fix it, then Ship it!





src/slave/flags.cpp (lines 214 - 222)


Since we want to (eventually) add something like an `os::var()`, you should 
add a TODO here to note that.


- Joseph Wu


On Dec. 5, 2016, 5:06 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> ---
> 
> (Updated Dec. 5, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Alex Clemmer.
> 
> 
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit fixes MESOS-6677, which breaks the ability
> to run any agent on Windows, and thus is blocking all
> Windows development progress on the `master` branch.
> 
> The cause was that the default `runtime_dir` value was POSIX specific,
> and used `os::user()` which is deleted on Windows.
> The fix is to guard the POSIX code, and add a
> Windows implementation.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 
>   src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 
> 
> Diff: https://reviews.apache.org/r/54336/diff/
> 
> 
> Testing
> ---
> 
> make && make check on Linux: 1411 tests passed, no failures.
> 
> msbuild and attached to Linux master: no runtime failures.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 54368: Added helpers to checkpoint a 'destroy-on-recovery' file for containers.

[Kevin Klues]

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

(Updated Dec. 6, 2016, 11:04 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated to fix patch dependency in chain.


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


Repository: mesos


Description
---

The existence of this file causes the containerizer to mark the
container for destruction after agent restarts. This is currently
useful for DEBUG containers launched by a
`LAUNCH_NESTED_CONTAINER_SESSION` call.


Diffs (updated)
-

  src/slave/containerizer/mesos/paths.hpp 
dd197f92c39c80d73326a1507ad1850086830e88 
  src/slave/containerizer/mesos/paths.cpp 
c5542873b6a313db2405caf9b3d25fead30ba216 

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


Testing
---

Tested in subsequent patch.


Thanks,

Kevin Klues

Review Request 54455: Added test for IOSwitchboard `recovery()`.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added test for IOSwitchboard `recovery()`.


Diffs
-

  src/tests/containerizer/io_switchboard_tests.cpp 
c8fe8768db705d97299215ab69b91babb8bbc2f9 

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


Testing
---

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


Thanks,

Kevin Klues

Re: Review Request 54415: Stout: Fixed two bugs in `mkdtemp` that block agent tests.

[Joseph Wu]

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


Ship it!





3rdparty/stout/include/Makefile.am (line 159)


Now that I think about it... it's funny how we've added all these Windows 
sources to this makefile, but we'll never actually use them in the autotools 
build.


- Joseph Wu


On Dec. 6, 2016, 3:29 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54415/
> ---
> 
> (Updated Dec. 6, 2016, 3:29 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6697
> https://issues.apache.org/jira/browse/MESOS-6697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently there are two bugs in the Windows implementation of
> `os::mkdtemp`, which together cause all agent tests to fail. We describe
> these bugs below and explain the steps this commit will take to address
> them.
> 
> The first concerns a mismatch between the POSIX specification of
> `mkdtemp` and the Windows implementation of `os::mkdtemp`. Specifically,
> the POSIX specification indicates that, if any component of the path
> prefix in the directory pattern passed to `mkdtemp` does not exist, we
> are to set `errno` to `ENOENT` and return a null pointer. In the Stout
> POSIX wrapper, `os::mkdtemp`, we will return an `ErrnoError`. In
> contrast, the Windows implementation will erroneously recursively create
> all components of the path prefix. This commit will address this by
> explicitly setting the `recursive` flag of the call to `os::mkdir` to
> `false`.
> 
> The second concerns a failure to parse Unix-like paths when creating the
> temporary directory. Specifically, both the POSIX and Windows
> implementations of `os::mkdtemp` have a default argument: `/tmp/XX`.
> When we replace the 'X' characters with random alphanumeric characters
> and pass the result to `os::mkdir`, and when the `recursive` flag is
> errorneously set to `true`, we will fail to parse the path on Windows,
> because `os::mkdir` can only pass Windows-style paths, with the '\'
> character. This causes all Agent tests to fail, e.g., when we try to
> create sandbox directories.
> 
> Technically, this second issue is actually resolved by setting the
> `recursive` flag to `false` in the call to `os::mkdir`, but here we
> observe that `/tmp` is not the "right place" to put temporary files on
> Windows anyway, and so we take the time to clean it up here, by
> replacing the default string literal path `/tmp/XX` with the
> platform-agnostic `path::join(os::temp(), "XX)`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am d5bc728ce378586e84173b98499b0d52047a83e1 
>   3rdparty/stout/include/stout/os.hpp 
> bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba 
>   3rdparty/stout/include/stout/os/posix/mkdtemp.hpp 
> 90866dba8e6be206c64f21204d936c1bed808c9a 
>   3rdparty/stout/include/stout/os/posix/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> 2cb73bbb996775e3764ad852ccda5076b41aef41 
>   3rdparty/stout/include/stout/os/windows/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/posix/os.hpp 
> 1f4d155e4399b955d0ca884f5336c78d8ceb56b5 
>   3rdparty/stout/include/stout/windows/os.hpp 
> de9b04ad82443038a0f4408bc72cae1540a1beaf 
> 
> Diff: https://reviews.apache.org/r/54415/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 54441: Added function to unblock IOSwitchboard when waiting for connection.

[Kevin Klues]

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

(Updated Dec. 6, 2016, 11:01 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie's comments.


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


Repository: mesos


Description
---

Added function to unblock IOSwitchboard when waiting for connection.


Diffs (updated)
-

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

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


Testing
---

make check


Thanks,

Kevin Klues

Re: Review Request 54442: Added SIGTERM handler to gracefully shutdown IOSwitchboard server.

[Kevin Klues]

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

(Updated Dec. 6, 2016, 11:01 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie's comments.


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


Repository: mesos


Description
---

When receiving a SIGTERM, the io switchboard process will forcibly
unblock the server from waiting on a connection before attempting to
drain its `stdoutFromFd` and `stderrFromFd` file descriptors. Once
these fds are drained (or they become invalid), the server will shut
itself down as per the normal exit route.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
aff6c2150bfe8086fd51b548cb6339acc23f78c9 

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


Testing
---

make check

The real test is encapsulated in https://reviews.apache.org/r/54367


Thanks,

Kevin Klues

Re: Review Request 54426: Avoided building glog tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54426]

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. 6, 2016, 3:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54426/
> ---
> 
> (Updated Dec. 6, 2016, 3:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Kapil Arya, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided building glog tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am d12946c37be6e179cb622b5486e4896ac7d717e9 
> 
> Diff: https://reviews.apache.org/r/54426/diff/
> 
> 
> Testing
> ---
> 
> `make check` (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 54081: Added `--pidfile` option to master and agent binaries.

[Benjamin Mahler]

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



Thanks Ilya!

Have you looked at other pidfile related libraries? Looks like BSD provides 
some functions for this (they're also available on Linux):
https://www.freebsd.org/cgi/man.cgi?query=pidfile=3=FreeBSD+6.1-RELEASE
https://linux.die.net/man/3/pidfile

It would be great to provide an os-agnostic library within stout for this, e.g.:

```
pidfile::open()
pidfile::close()
pidfile::remove()
```

I haven't looked too deeply at what we should have these take and return, but I 
noticed the use of RAII seemed unnecessary in your patch (only the .cpp 
implementation uses the exposed RAII class? If so, perhaps we can just leave 
RAII out for now since the callers do not use it). Also it would be great to 
which process is already running (which is provided by the BSD/Linux 
functions). Also, another suggestion as a first step here is to avoid Windows 
support entirely for now if we don't implement the locking aspect of this (as 
that seems necessary for this to be safely usable?)

We could structure the patches like this:

(1) Addition of pidfile utilities to stout.
(2) Tests of the pidfile utilities in stout (should be possible?)
(3) Integration of the pidfile utilities into mesos (manually test since the 
logic will reside in the mains?)

- Benjamin Mahler


On Nov. 28, 2016, 12:16 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54081/
> ---
> 
> (Updated Nov. 28, 2016, 12:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1648
> https://issues.apache.org/jira/browse/MESOS-1648
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The PID file is created and kept locked while master / agent is running to 
> prevent other instances with the same PID file location from starting.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt d6e213686a44fbca0ed841594ce803151224a5a7 
>   src/Makefile.am dd1626d177b38a6613f18f32bb0668abbb5100e0 
>   src/common/pid_file.hpp PRE-CREATION 
>   src/common/pid_file.cpp PRE-CREATION 
>   src/logging/logging.cpp 70d66a5c396f709e8f27ad0d51315ed6d257f73b 
>   src/master/main.cpp fa7ba1310142a3bef71379ba37fded9b8390aae9 
>   src/slave/main.cpp 8010f8e229e2d820649750c9db0456ecd1b854d3 
>   src/tests/common/pid_file_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54081/diff/
> 
> 
> Testing
> ---
> 
> Added test to verify that PID file is created upon `PIDFile` object creation 
> and deleted upon its destruction. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Review Request 54453: Transitioned to `in_memory` log as default in tests involving Master.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, Joris Van 
Remoortere, and Joseph Wu.


Bugs: MESOS-6695 and MESOS-6697
https://issues.apache.org/jira/browse/MESOS-6695
https://issues.apache.org/jira/browse/MESOS-6697


Repository: mesos


Description
---

Currently, nearly all instances of the Master in our tests have the
`replicated_log` flag turned on. On Windows, this will cause all tests
that involve instantiating a Master to fail immediately.

This commit will solve this problem by setting the default master flags
to set `in_memory` instead, and then selectively turning on
`replicated_log` when it's necessary.

NOTE: This commit is largely a resurrection of #41665. And, although it
is meant specifically to address MESOS-6697, this issue actually affects
almost all open issues in MESOS-6695.


Diffs
-

  src/tests/dynamic_weights_tests.cpp 6f1e249e51e41aee7fdb22a2ccbfa9be71774e6d 
  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 
  src/tests/mesos.cpp 8fd8bcb033f47e2538aa36cd373c892a882afdfd 
  src/tests/partition_tests.cpp 5a0d4bd2de6a5aa0e9fdf0d34cd10d16fd4e34a1 
  src/tests/reconciliation_tests.cpp 71073a05a702dffd4c8e10c3c125a144ac8ca41d 

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


Testing
---


Thanks,

Alex Clemmer

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

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 6, 2016, 8:44 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54356/
> ---
> 
> (Updated Dec. 6, 2016, 8:44 p.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 8279225eda3042c7842b765c5db1d685755db86f 
>   include/mesos/v1/mesos.proto 4628cc6c4f3efc2553bb180b1a4c8e1e19af8711 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> c8fe8768db705d97299215ab69b91babb8bbc2f9 
> 
> Diff: https://reviews.apache.org/r/54356/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.

[Joseph Wu]

> On Dec. 5, 2016, 6:33 p.m., Joseph Wu wrote:
> > src/slave/constants.hpp, lines 141-144
> > 
> >
> > There isn't any need to get rid of this constant, but we could update 
> > the comment.
> > 
> > i.e. This is the _desired_ default, but if the path is inaccessible 
> > (posix non-root), the default will be a temporary folder.
> 
> Alex Clemmer wrote:
> I have a different view, actually. To me it seems like there's no 
> particular reason to have it, while there _are_ in my view a few good reasons 
> to not have it (and, I tend to believe that you should have to justify why 
> something exists rather than why it shouldn't exist). And, since it is used 
> only once, it costs nothing to change.
> 
> * We should really try to keep path literals out of the codebase as much 
> as possible, because of the subtle bugs and problems with combining paths 
> with '\' and '/' on Windows. Even if it is safe in this case to use, I think 
> we should always think carefully about whether we have a good reason for 
> having a string literal path, and in this case, I don't see a clear benefit, 
> while I do see a clear risk of someone accidentally `path::join`'ing onto the 
> end at some point in the future. It seems like a better choice (as Jie 
> suggests in #54335) would be to implement this as `os::runstatedir`. See my 
> next point for more on this.
> * To me, it makes sense to have a platform-indepenent variable data root 
> in a new function `os::var()` (_i.e._ `/var` on POSIX, and something like 
> `ProgramData` on Windows), and to use something like `path::join(os::var(), 
> ...)` to implement a function, `os::runstatedir()`. I think this is 
> reasonable in particular since (per conversation with Jie) we don't want to 
> change the default of this path away from `/var`.
> 
> What are your thoughts? Am I missing something?
> 
> Alex Clemmer wrote:
> Oh, also, on the two points above, it's worth reading my next comment to 
> see a more complete picture for our current plan to accomplish the second 
> part in particular.
> 
> Andrew Schwartzmeyer wrote:
> I would like to point out that, as far as I can tell, this is the only 
> hardcoded absolute path in any of the `constants.hpp` headers. I think that 
> it should be removed as a matter of preventing its accidental use on a 
> platform for which it is incorrect.
> 
> That said, I can see why you'd be not want to remove it given just this 
> commit, as it only moves the definition into the `Flags::runtime_dir`, which 
> is *also* not the right place for this. Perhaps it'd be appropriate to add 
> `os::runstate_dir` as Alex and Jie suggest to hold this platform-depenent 
> constant (keeping in mind that this commit also does not introduce the 
> correct Windows location of `ProgramData`).

Ok, sounds like a reasonable workaround for now.


- Joseph


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


On Dec. 5, 2016, 5:06 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> ---
> 
> (Updated Dec. 5, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Alex Clemmer.
> 
> 
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit fixes MESOS-6677, which breaks the ability
> to run any agent on Windows, and thus is blocking all
> Windows development progress on the `master` branch.
> 
> The cause was that the default `runtime_dir` value was POSIX specific,
> and used `os::user()` which is deleted on Windows.
> The fix is to guard the POSIX code, and add a
> Windows implementation.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 
>   src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 
> 
> Diff: https://reviews.apache.org/r/54336/diff/
> 
> 
> Testing
> ---
> 
> make && make check on Linux: 1411 tests passed, no failures.
> 
> msbuild and attached to Linux master: no runtime failures.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Review Request 54449: Check quotas are enabled in the XFS disk isolator.

[James Peach]

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

Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.

Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.


Diffs
-

  configure.ac 5fb2efffaf62652d59c832da9c0f7e1b43a64ee4 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
dd4df86bf90bfa9cbf4664d89274cf3c64c2e374 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
7602fe3b6ab069db643397418732e773d0417f8a 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
b9d8e7dc999ba3064bee7105eff0f9553d825df8 

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


Testing
---

Make check on Fedora 25. Manual test on F25 with mesos-execute.


Thanks,

James Peach

Re: Review Request 54442: Added SIGTERM handler to gracefully shutdown IOSwitchboard server.

[Jie Yu]

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


Fix it, then Ship it!





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


This has an unfortunate race condition. If the io switchboard terminates 
and the pid is reused by some other process, we might be sending SIGTERM to a 
random process. This might be a problem under high load.

Sounds to me maybe a message through domain socket is safer. You can add a 
TODO here and fix it later.



src/slave/containerizer/mesos/io/switchboard_main.cpp (line 51)


write an `\n` in the end.



src/slave/containerizer/mesos/io/switchboard_main.cpp (line 53)


User `_exit` here. `exit` is not async safe.



src/slave/containerizer/mesos/io/switchboard_main.cpp (line 95)


Add `namespace io = process::io` above and use `io::poll` here.



src/slave/containerizer/mesos/io/switchboard_main.cpp (line 97)


Can you handle failure case as well. Like io::poll returns a Failure or 
Discarded future.

os::close can be moved to onAny.


- Jie Yu


On Dec. 6, 2016, 8:03 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54442/
> ---
> 
> (Updated Dec. 6, 2016, 8:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6664
> https://issues.apache.org/jira/browse/MESOS-6664
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When receiving a SIGTERM, the io switchboard process will forcibly
> unblock the server from waiting on a connection before attempting to
> drain its `stdoutFromFd` and `stderrFromFd` file descriptors. Once
> these fds are drained (or they become invalid), the server will shut
> itself down as per the normal exit route.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
>   src/slave/containerizer/mesos/io/switchboard_main.cpp 
> aff6c2150bfe8086fd51b548cb6339acc23f78c9 
> 
> Diff: https://reviews.apache.org/r/54442/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> The real test is encapsulated in https://reviews.apache.org/r/54367
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54395: Enabled build of Agent test harness on Windows.

[Alex Clemmer]

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

(Updated Dec. 6, 2016, 9:11 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Changes
---

Ignoring the `GTEST_IS_THREADSAFE` check in the Agent test harness.


Bugs: MESOS-6696 and MESOS-6717
https://issues.apache.org/jira/browse/MESOS-6696
https://issues.apache.org/jira/browse/MESOS-6717


Repository: mesos


Description
---

Enabled build of Agent test harness on Windows.

This commit adds the Agent test harness to the Windows builds. This is
the first major step towards lighting up comprehensive Agent testing on
agent builds, an important step that has been prevented by a variety of
issues (e.g., things like getting the Master to work for at least
developer scenarios, and so on).


Diffs (updated)
-

  src/CMakeLists.txt 4d8658bf4e2e4ec07a56542539c9409a1d9079d3 
  src/tests/CMakeLists.txt 2ce9d37a75fdec3468aef8aba91db3a0a40e0220 
  src/tests/cluster.cpp 6a9d94b8ac3c8fd0428b7a67d1cb3f99a658fa9b 
  src/tests/environment.cpp e08678c5610332b80ce7db47e697651fa0109502 
  src/tests/main.cpp c10eeac335b0c8cdb2ea6a0701915ec33f76a2b2 
  src/tests/test_helper_main.cpp a7d511ce71e2789df50aef02d2d50b4c94f38a50 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 54328: Fixed HttpServeTests to connect to a non-ANY address.

[Joseph Wu]

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

(Updated Dec. 6, 2016, 1:08 p.m.)


Review request for mesos, Benjamin Mahler, Alex Clemmer, and Jie Yu.


Changes
---

Renamed a variable and expanded a comment.


Repository: mesos


Description
---

Connecting to the ANY address (0.0.0.0) is legal in BSD sockets,
but results in an ambiguous connection.  The outgoing connection
could literally be anywhere on the network.

These tests aim to connect to a specific socket on the local
machine, and as such, should supply a valid IP address.
This commit coincidentally fixes these tests on Windows,
which disallows connecting to invalid IP addresses.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/http_tests.cpp 
7f5425742d554b78fd4788c4a453068a604e1f46 

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


Testing
---

CMake:
make process_tests
3rdparty/libprocess/src/tests/process_tests --gtest_filter="HttpServeTest*"

msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:process_tests
3rdparty\libprocess\src\tests\Debug\process_tests.exe 
--gtest_filter="HttpServeTest.*"


Thanks,

Joseph Wu

Re: Review Request 54441: Added function to unblock IOSwitchboard when waiting for connection.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/io/switchboard.cpp (lines 892 - 894)


I would just call 'set' here. No need for the if check here.


- Jie Yu


On Dec. 6, 2016, 7:58 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54441/
> ---
> 
> (Updated Dec. 6, 2016, 7:58 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6664
> https://issues.apache.org/jira/browse/MESOS-6664
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added function to unblock IOSwitchboard when waiting for connection.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 19f2b625f2aa4b790fbe80b8dfad44b219f2c24e 
> 
> Diff: https://reviews.apache.org/r/54441/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 54446: Windows: Added `authentication_tests.cpp` build.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


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


Repository: mesos


Description
---

This commit will partially address MESOS-6697 by adding
`authentication_tests.cpp` to the Windows build, but disabling all
tests in the file, as the Windows implementation of the Master currently
does not support authentication (see MESOS-6733).

Our goal in turning on the tests is to ensure that the tests remain able
to compile on Windows as we continue to work on MESOS-6733. When this
issue is resolved, we do expect to come back and turn on these tests,
which should resolve MESOS-6697.


Diffs
-

  src/CMakeLists.txt 4d8658bf4e2e4ec07a56542539c9409a1d9079d3 
  src/tests/CMakeLists.txt 2ce9d37a75fdec3468aef8aba91db3a0a40e0220 
  src/tests/authentication_tests.cpp fc7afae0abde8e9274685b9a2121f5a91d59f3d6 

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


Testing
---


Thanks,

Alex Clemmer

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. 6, 2016, 8:44 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie's comments.


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 8279225eda3042c7842b765c5db1d685755db86f 
  include/mesos/v1/mesos.proto 4628cc6c4f3efc2553bb180b1a4c8e1e19af8711 
  src/slave/containerizer/mesos/io/switchboard.hpp 
839665a22aca9b1c1c1cf4992406bc924ee2b065 
  src/slave/containerizer/mesos/io/switchboard.cpp 
0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
  src/tests/containerizer/io_switchboard_tests.cpp 
c8fe8768db705d97299215ab69b91babb8bbc2f9 

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


Testing
---

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


Thanks,

Kevin Klues

Re: Review Request 54422: Removed test assumptions about arrival order of status updates.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54422]

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. 6, 2016, 2:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54422/
> ---
> 
> (Updated Dec. 6, 2016, 2:09 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Task status updates can arrive in any order. Adjusted the code to not
> assume a particual order.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 3d06c3a4a74bbcba4c9b6bdba4258c5aefdab845 
> 
> Diff: https://reviews.apache.org/r/54422/diff/
> 
> 
> Testing
> ---
> 
> * `make check` on OS X, clang-trunk w/o optimizations
> * `SlaveTest.*` in repetation
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 54442: Added SIGTERM handler to gracefully shutdown IOSwitchboard server.

[Kevin Klues]

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

(Updated Dec. 6, 2016, 8:03 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

When receiving a SIGTERM, the io switchboard process will forcibly
unblock the server from waiting on a connection before attempting to
drain its `stdoutFromFd` and `stderrFromFd` file descriptors. Once
these fds are drained (or they become invalid), the server will shut
itself down as per the normal exit route.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
aff6c2150bfe8086fd51b548cb6339acc23f78c9 

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


Testing
---

make check

The real test is encapsulated in https://reviews.apache.org/r/54367


Thanks,

Kevin Klues

Re: Review Request 54442: Added SIGTERM handler to gracefully shutdown IOSwitchboard server.

[Kevin Klues]

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

(Updated Dec. 6, 2016, 8:01 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

When receiving a SIGTERM, the io switchboard process will forcibly
unblock the server from waiting on a connection before attempting to
drain its `stdoutFromFd` and `stderrFromFd` file descriptors. Once
these fds are drained (or they become invalid), the server will shut
itself down as per the normal exit route.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
aff6c2150bfe8086fd51b548cb6339acc23f78c9 

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


Testing
---

make check

The real test is encapsulated in https://reviews.apache.org/r/54367


Thanks,

Kevin Klues

Re: Review Request 54442: Added SIGTERM handler to gracefully shutdown IOSwitchboard server.

[Kevin Klues]

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

(Updated Dec. 6, 2016, 7:58 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

When receiving a SIGTERM, the io switchboard process will forcibly
unblock the server from waiting on a connection before attempting to
drain its `stdoutFromFd` and `stderrFromFd` file descriptors. Once
these fds are drained (or they become invalid), the server will shut
itself down as per the normal exit route.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
aff6c2150bfe8086fd51b548cb6339acc23f78c9 

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


Testing
---

make check

The real test is encapsulated in https://reviews.apache.org/r/54367


Thanks,

Kevin Klues

Re: Review Request 54441: Added function to unblock IOSwitchboard when waiting for connection.

[Kevin Klues]

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

(Updated Dec. 6, 2016, 7:58 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added function to unblock IOSwitchboard when waiting for connection.


Diffs
-

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

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


Testing
---

make check


Thanks,

Kevin Klues

Re: Review Request 54442: Added SIGTERM handler to gracefully shutdown IOSwitchboard server.

[Kevin Klues]

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

(Updated Dec. 6, 2016, 7:55 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

When receiving a SIGTERM, the io switchboard process will forcibly
unblock the server from waiting on a connection before attempting to
drain its `stdoutFromFd` and `stderrFromFd` file descriptors. Once
these fds are drained (or they become invalid), the server will shut
itself down as per the normal exit route.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
0254bd2f37cedd90f37d7b3e38a9b47e3d0fc3a6 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
aff6c2150bfe8086fd51b548cb6339acc23f78c9 

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


Testing
---

make check

The real test is encapsulated in https://reviews.apache.org/r/54367


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. 6, 2016, 7:46 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on all but 1 of Jie's comments.


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

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


Testing
---

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

Test added in follow-on patch.


Thanks,

Kevin Klues

Review Request 54442: Added SIGTERM handler to gracefully shutdown IOSwitchboard server.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

When receiving a SIGTERM, the io switchboard process will forcibly
unblock the server from waiting on a connection before attempting to
drain its `stdoutFromFd` and `stderrFromFd` file descriptors. Once
these fds are drained (or they become invalid), the server will shut
itself down as per the normal exit route.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
19f2b625f2aa4b790fbe80b8dfad44b219f2c24e 
  src/slave/containerizer/mesos/io/switchboard_main.cpp 
aff6c2150bfe8086fd51b548cb6339acc23f78c9 

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


Testing
---

make check

The real test is encapsulated in https://reviews.apache.org/r/54367


Thanks,

Kevin Klues

Review Request 54441: Added function to unblock IOSwitchboard when waiting for connection.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added function to unblock IOSwitchboard when waiting for connection.


Diffs
-

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

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


Testing
---

make check


Thanks,

Kevin Klues

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

[Kevin Klues]

> On Dec. 6, 2016, 5:31 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 161
> > 
> >
> > or agent crashes right before we create the directory and fork the 
> > server.

Would we actually have an active container state in this case though? If the 
agent crashed at that point, then you would never have gotten the cahnce to 
even try and launch the container itself.


- Kevin


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


On Dec. 6, 2016, 4:55 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54355/
> ---
> 
> (Updated Dec. 6, 2016, 4:55 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 
> 19f2b625f2aa4b790fbe80b8dfad44b219f2c24e 
> 
> Diff: https://reviews.apache.org/r/54355/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> Test added in follow-on patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 53803: Added a new libprocess HTTP test.

[Greg Mann]

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

(Updated Dec. 6, 2016, 7:28 p.m.)


Review request for mesos, Benjamin Mahler and Joseph Wu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This patch adds HTTPTest.SocketEOF to verify that EOFs are
reliably received whether or not there is a pending recv()
request at the time the EOF is received.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/http_tests.cpp 
7f5425742d554b78fd4788c4a453068a604e1f46 

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


Testing
---

Testing details are at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 52349: Add protobuf messages for OCI image spec.

[Avinash sridharan]

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




include/mesos/oci/spec.proto (line 24)


Might want to add a comment highlgihting the possible media types:
https://github.com/opencontainers/image-spec/blob/master/media-types.md



include/mesos/oci/spec.proto (line 59)


This represents a media-type. In the comments we should highlight the OCI 
MIME type of this message. Will be easier for readers to correlate the message.



include/mesos/oci/spec.proto (line 93)


Ditto on highlighting the OCI MIME type.



include/mesos/oci/spec.proto (line 106)


Ditto on highlighting the OCI MIME type.


- Avinash sridharan


On Dec. 3, 2016, 1:50 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> ---
> 
> (Updated Dec. 3, 2016, 1:50 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add protobuf messages for OCI image spec.
> 
> 
> Diffs
> -
> 
>   include/mesos/oci/spec.hpp PRE-CREATION 
>   include/mesos/oci/spec.proto PRE-CREATION 
>   src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
> 
> Diff: https://reviews.apache.org/r/52349/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.

[Andrew Schwartzmeyer]

> On Dec. 6, 2016, 2:33 a.m., Joseph Wu wrote:
> > src/slave/flags.cpp, lines 214-216
> > 
> >
> > Seems like the problem on Windows is `os::user()` rather than the value 
> > of the `--runtime_dir` flag.  As long as `os::user()` returns some value, 
> > we'll get an appropriate default runtime directory for Windows.  In most 
> > cases, `/var/run/mesos` should work on Windows (unless there are permission 
> > issues?), because our code does a recursive `mkdir` before using the 
> > runtime directory.
> > 
> > If this is the case, this review is probably the approach we want to 
> > take: https://reviews.apache.org/r/53706/
> 
> Alex Clemmer wrote:
> I actually don't see it this way. As I said in the comments of #54335, 
> the reason we are switching on the `os::user` at all is because `/var` is 
> accessible only from `root` in some POSIXes. Since Windows does not suffer 
> from this problem, I think we should put it in a place that is sensible for 
> Windows, which should eventually be something like `path::join(os::var(), 
> "mesos", "runtime")`. Until we have `os::var` (sometime in the next couple 
> days), I think it's reasonable to put this data in the default non-`root` 
> Unix folder to unblock the rest of the Windows team. What do you think?
> 
> Alex Clemmer wrote:
> Also, I'm not a super big fan of putting it in `/var` on Windows. :) I'd 
> personally rather find the right place for it on different platforms and put 
> it there instead of just mirroring what we have on POSIX.

To add to this, there is a subtle bug with *just* fixing `os::user()`. While 
that would prevent the crash, it would introduce a strange edge case where the 
folder used most of the time is `path::join(os::temp(), "mesos", "runtime")` 
except when the Windows user is named `root` (edge case, but valid), and it 
becomes `/var/run/mesos`. This is not what the the code was intending to mean, 
so I think it would be remiss to introduce this bug.

And I agree that `/var/run/mesos` is not the appropriate path on Windows. The 
analogous path would be `ProgramData`, which I'm working changing correctly.


> On Dec. 6, 2016, 2:33 a.m., Joseph Wu wrote:
> > src/slave/constants.hpp, lines 141-144
> > 
> >
> > There isn't any need to get rid of this constant, but we could update 
> > the comment.
> > 
> > i.e. This is the _desired_ default, but if the path is inaccessible 
> > (posix non-root), the default will be a temporary folder.
> 
> Alex Clemmer wrote:
> I have a different view, actually. To me it seems like there's no 
> particular reason to have it, while there _are_ in my view a few good reasons 
> to not have it (and, I tend to believe that you should have to justify why 
> something exists rather than why it shouldn't exist). And, since it is used 
> only once, it costs nothing to change.
> 
> * We should really try to keep path literals out of the codebase as much 
> as possible, because of the subtle bugs and problems with combining paths 
> with '\' and '/' on Windows. Even if it is safe in this case to use, I think 
> we should always think carefully about whether we have a good reason for 
> having a string literal path, and in this case, I don't see a clear benefit, 
> while I do see a clear risk of someone accidentally `path::join`'ing onto the 
> end at some point in the future. It seems like a better choice (as Jie 
> suggests in #54335) would be to implement this as `os::runstatedir`. See my 
> next point for more on this.
> * To me, it makes sense to have a platform-indepenent variable data root 
> in a new function `os::var()` (_i.e._ `/var` on POSIX, and something like 
> `ProgramData` on Windows), and to use something like `path::join(os::var(), 
> ...)` to implement a function, `os::runstatedir()`. I think this is 
> reasonable in particular since (per conversation with Jie) we don't want to 
> change the default of this path away from `/var`.
> 
> What are your thoughts? Am I missing something?
> 
> Alex Clemmer wrote:
> Oh, also, on the two points above, it's worth reading my next comment to 
> see a more complete picture for our current plan to accomplish the second 
> part in particular.

I would like to point out that, as far as I can tell, this is the only 
hardcoded absolute path in any of the `constants.hpp` headers. I think that it 
should be removed as a matter of preventing its accidental use on a platform 
for which it is incorrect.

That said, I can see why you'd be not want to remove it given just this commit, 
as it only moves the definition into the `Flags::runtime_dir`, which is *also* 
not the right place for this. Perhaps it'd be appropriate to add 
`os::runstate_dir` as Alex and Jie suggest to hold this platform-depenent 
constant (keeping in mind that this commit also does not 

Re: Review Request 54335: Add os::var() to stout.

[Alex Clemmer]

> On Dec. 5, 2016, 6:31 p.m., Jie Yu wrote:
> > Flying by. I am checking 
> > https://www.gnu.org/prep/standards/html_node/Directory-Variables.html
> > 
> > Looks like, in retrospect, we should call the current `runtime_dir` 
> > `runstate_dir` instead. So we probably should use `os::runstatedir`?
> 
> Alex Clemmer wrote:
> +1, thanks for the helpful suggestion Jie. We were debating what to call 
> this anyway. :)
> 
> Alex Clemmer wrote:
> But, actually, I think I spoke too soon. The idea is actually to use this 
> for all the places we use a directory rooted at `/var`, _i.e._, for all 
> places we're dealing with variable data. I think the final picture of what 
> the disk isolators and persistent volumes stuff will end up looking like is 
> not yet fully developed, but the idea here is that we will want all of the 
> places those things manage variable data to be managed out of a sensible 
> `/var` on Windows, too. This is also why I think it's important to consider 
> the implication of choosing user-specific directories (as I say below).
> 
> Thoughts?
> 
> Jie Yu wrote:
> Looks like according to GNU standard, this should be `localstatedir` 
> (e.g., /var or /usr/local/var). And `runstatedir` is under `localstatedir` 
> (i.e., `$(localstatedir)/run`). And for volumes which normally under 
> `/var/lib`, should be called `libdir` according to GNU standard.
> 
> Note that `/var/run` will normally cleared by OS upon reboot, while 
> `/var/lib` contains persisent information across reboot.

Ok, those are good points. To make this stuff work in Windows, I think we will 
still need an `os::var`, but your suggestion to implement a series of Stout 
functions following the form `os::runstatedir`, _etc_. make sense and IMO are 
worth adopting. Does that sound good to you?


- Alex


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


On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> ---
> 
> (Updated Dec. 5, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Alex Clemmer.
> 
> 
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
>   3rdparty/stout/include/stout/windows/os.hpp 
> de9b04ad82443038a0f4408bc72cae1540a1beaf 
> 
> Diff: https://reviews.apache.org/r/54335/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 54328: Fixed HttpServeTests to connect to a non-ANY address.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/libprocess/src/tests/http_tests.cpp (lines 1781 - 1782)


Is there a better name for `_address` like `any_address`?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1784 - 1785)


Why not? Can you explain in the comment? Also, can you comment a bit on 
what libprocess is doing that makes it ok to use process::address but not ok to 
use the server bound address?

Ditto below.


- Benjamin Mahler


On Dec. 3, 2016, 12:28 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54328/
> ---
> 
> (Updated Dec. 3, 2016, 12:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Alex Clemmer, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Connecting to the ANY address (0.0.0.0) is legal in BSD sockets,
> but results in an ambiguous connection.  The outgoing connection
> could literally be anywhere on the network.
> 
> These tests aim to connect to a specific socket on the local
> machine, and as such, should supply a valid IP address.
> This commit coincidentally fixes these tests on Windows,
> which disallows connecting to invalid IP addresses.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 7f5425742d554b78fd4788c4a453068a604e1f46 
> 
> Diff: https://reviews.apache.org/r/54328/diff/
> 
> 
> Testing
> ---
> 
> CMake:
> make process_tests
> 3rdparty/libprocess/src/tests/process_tests --gtest_filter="HttpServeTest*"
> 
> msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:process_tests
> 3rdparty\libprocess\src\tests\Debug\process_tests.exe 
> --gtest_filter="HttpServeTest.*"
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 54434: Used UNSET_CLOEXEC instead of DUP2 hook for I/O switchboard.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Kevin Klues.


Repository: mesos


Description
---

Used UNSET_CLOEXEC instead of DUP2 hook for I/O switchboard.


Diffs
-

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

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


Testing
---

make check on both osx and linux


Thanks,

Jie Yu

Review Request 54433: Added a UNSET_CLOEXEC child hook to subprocess.

[Jie Yu]

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

Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Added a UNSET_CLOEXEC child hook to subprocess.


Diffs
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
0d9c74a7d01795868b06479c794b8b7521f9c973 
  3rdparty/libprocess/src/subprocess.cpp 
340fc32bdffb083357a886ece31f1353885e8642 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 54432: Added os::unsetCloexec to stout.

[Jie Yu]

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

Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Added os::unsetCloexec to stout.


Diffs
-

  3rdparty/stout/include/stout/os/posix/fcntl.hpp 
daaca0da959b8ad04bc5042b85e5b9e3e6653344 
  3rdparty/stout/include/stout/os/windows/fcntl.hpp 
2bc794a405e59d80c1e8308c0049d2306347adfa 
  3rdparty/stout/tests/os_tests.cpp 446282373aebb06dae32209a9b0d148cb15eced9 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 54414: Cleaned up rlimits documentation.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 6, 2016, 12:06 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54414/
> ---
> 
> (Updated Dec. 6, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Jie Yu.
> 
> 
> Bugs: MESOS-6427
> https://issues.apache.org/jira/browse/MESOS-6427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * added front matter,
> * add section headers,
> * remove stray double spaces,
> * used canonical delimiter for guarded code blocks, and
> * used HTML table to enable canonical styling, see also MESOS-6729.
> 
> 
> Diffs
> -
> 
>   docs/posix_rlimits.md 0a0e6a8d20eb87852e0ce635b2de2caed94b7020 
> 
> Diff: https://reviews.apache.org/r/54414/diff/
> 
> 
> Testing
> ---
> 
> Checked with bundled site setup.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Jie Yu]

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


Fix it, then Ship it!





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


Can you follow up to update mesos-containerizer.md to include this isolator 
and provide a link here?


- Jie Yu


On Dec. 6, 2016, 4:28 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Dec. 6, 2016, 4:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Artem Harutyunyan, 
> Jie Yu, Neil Conway, 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 54335: Add os::var() to stout.

[Jie Yu]

> On Dec. 5, 2016, 6:31 p.m., Jie Yu wrote:
> > Flying by. I am checking 
> > https://www.gnu.org/prep/standards/html_node/Directory-Variables.html
> > 
> > Looks like, in retrospect, we should call the current `runtime_dir` 
> > `runstate_dir` instead. So we probably should use `os::runstatedir`?
> 
> Alex Clemmer wrote:
> +1, thanks for the helpful suggestion Jie. We were debating what to call 
> this anyway. :)
> 
> Alex Clemmer wrote:
> But, actually, I think I spoke too soon. The idea is actually to use this 
> for all the places we use a directory rooted at `/var`, _i.e._, for all 
> places we're dealing with variable data. I think the final picture of what 
> the disk isolators and persistent volumes stuff will end up looking like is 
> not yet fully developed, but the idea here is that we will want all of the 
> places those things manage variable data to be managed out of a sensible 
> `/var` on Windows, too. This is also why I think it's important to consider 
> the implication of choosing user-specific directories (as I say below).
> 
> Thoughts?

Looks like according to GNU standard, this should be `localstatedir` (e.g., 
/var or /usr/local/var). And `runstatedir` is under `localstatedir` (i.e., 
`$(localstatedir)/run`). And for volumes which normally under `/var/lib`, 
should be called `libdir` according to GNU standard.

Note that `/var/run` will normally cleared by OS upon reboot, while `/var/lib` 
contains persisent information across reboot.


- Jie


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


On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> ---
> 
> (Updated Dec. 5, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Alex Clemmer.
> 
> 
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows API to look up correct location for persistent,
> app-local variable data. Returns standard location on POSIX.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 
> c37e64db662ba3cee83d2f55de0f9d71ad72c038 
>   3rdparty/stout/include/stout/windows/os.hpp 
> de9b04ad82443038a0f4408bc72cae1540a1beaf 
> 
> Diff: https://reviews.apache.org/r/54335/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 54365: Fixed indentation of a function argument in master.cpp.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 5, 2016, 8:48 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54365/
> ---
> 
> (Updated 十二月 5, 2016, 8:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indentation of a function argument in master.cpp.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 
> 
> Diff: https://reviews.apache.org/r/54365/diff/
> 
> 
> Testing
> ---
> 
> line 3902 should be aligned with other arguments.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

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

[Guangya Liu]

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



One minor comment is we *must* to commit this one together with 
https://reviews.apache.org/r/54360 , otherwise, the test 
`FrameworkInfoValidationTest.AcceptMultiRoleFramework` will be failed when 
`removeFramework` by `teardown`.

- Guangya Liu


On 十二月 6, 2016, 1:48 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54362/
> ---
> 
> (Updated 十二月 6, 2016, 1:48 p.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
> ---
> 
> 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 52382: Added stubs for OCI store.

[Avinash sridharan]

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




src/slave/containerizer/mesos/provisioner/oci/store.cpp (line 55)


s/images/OCI images store/


- Avinash sridharan


On Dec. 3, 2016, 1:51 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52382/
> ---
> 
> (Updated Dec. 3, 2016, 1:51 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added stubs for OCI store.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d 
>   src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
>   src/slave/containerizer/mesos/provisioner/oci/store.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52382/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Gilbert Song]

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

(Updated Dec. 6, 2016, 8:28 a.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Artem Harutyunyan, 
Jie Yu, Neil Conway, 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 (updated)
-

  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.

[Gilbert Song]

> On Dec. 4, 2016, 11:44 a.m., Vinod Kone wrote:
> > docs/nested-container-and-task-group.md, line 209
> > 
> >
> > mention that only 2 levels of nesting is supported as of 1.1?

We already support arbitrary nested levels. There is a unit test 
(`ROOT_CGROUPS_LaunchNestedThreeLevels`) for that support.


- Gilbert


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


On Nov. 30, 2016, 11:11 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Nov. 30, 2016, 11:11 a.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 52379: Added agent flag '--oci_store_dir'.

[Avinash sridharan]

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




docs/configuration.md (line 1606)


Just checking: Isn't the `configuration.md` auto-generated?
https://issues.apache.org/jira/browse/MESOS-4298



docs/configuration.md (line 1609)


Directory in which the OCI provisioner will store images.



src/slave/flags.cpp (line 167)


Directory in which the OCI provisioner stores images.


- Avinash sridharan


On Dec. 3, 2016, 1:51 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52379/
> ---
> 
> (Updated Dec. 3, 2016, 1:51 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flag '--oci_store_dir'.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md efe3e9bd9d203a7ba44adf4ead24f14b8b577637 
>   docs/endpoints/slave/state.json.md 0f82c1926404e79b281b2ea5f4d0ca21323aeded 
>   docs/endpoints/slave/state.md b34459e8624f0b29e927ff79be7fc845ac88080b 
>   src/slave/flags.hpp c6c3197bbf30ec617751f4a1a34914c0f0e29eb5 
>   src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 
>   src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 
> 
> Diff: https://reviews.apache.org/r/52379/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Gilbert Song]

> On Dec. 4, 2016, 10:11 a.m., Anand Mazumdar wrote:
> > Gilbert, can you add [~neilc] as a reviewer too?

Thanks, Anand!


- Gilbert


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


On Nov. 30, 2016, 11:11 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Nov. 30, 2016, 11:11 a.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 54415: Stout: Fixed two bugs in `mkdtemp` that block agent tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54324, 53550, 53551, 53552, 54395, 54415]

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. 6, 2016, 11:29 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54415/
> ---
> 
> (Updated Dec. 6, 2016, 11:29 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6697
> https://issues.apache.org/jira/browse/MESOS-6697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently there are two bugs in the Windows implementation of
> `os::mkdtemp`, which together cause all agent tests to fail. We describe
> these bugs below and explain the steps this commit will take to address
> them.
> 
> The first concerns a mismatch between the POSIX specification of
> `mkdtemp` and the Windows implementation of `os::mkdtemp`. Specifically,
> the POSIX specification indicates that, if any component of the path
> prefix in the directory pattern passed to `mkdtemp` does not exist, we
> are to set `errno` to `ENOENT` and return a null pointer. In the Stout
> POSIX wrapper, `os::mkdtemp`, we will return an `ErrnoError`. In
> contrast, the Windows implementation will erroneously recursively create
> all components of the path prefix. This commit will address this by
> explicitly setting the `recursive` flag of the call to `os::mkdir` to
> `false`.
> 
> The second concerns a failure to parse Unix-like paths when creating the
> temporary directory. Specifically, both the POSIX and Windows
> implementations of `os::mkdtemp` have a default argument: `/tmp/XX`.
> When we replace the 'X' characters with random alphanumeric characters
> and pass the result to `os::mkdir`, and when the `recursive` flag is
> errorneously set to `true`, we will fail to parse the path on Windows,
> because `os::mkdir` can only pass Windows-style paths, with the '\'
> character. This causes all Agent tests to fail, e.g., when we try to
> create sandbox directories.
> 
> Technically, this second issue is actually resolved by setting the
> `recursive` flag to `false` in the call to `os::mkdir`, but here we
> observe that `/tmp` is not the "right place" to put temporary files on
> Windows anyway, and so we take the time to clean it up here, by
> replacing the default string literal path `/tmp/XX` with the
> platform-agnostic `path::join(os::temp(), "XX)`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am d5bc728ce378586e84173b98499b0d52047a83e1 
>   3rdparty/stout/include/stout/os.hpp 
> bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba 
>   3rdparty/stout/include/stout/os/posix/mkdtemp.hpp 
> 90866dba8e6be206c64f21204d936c1bed808c9a 
>   3rdparty/stout/include/stout/os/posix/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> 2cb73bbb996775e3764ad852ccda5076b41aef41 
>   3rdparty/stout/include/stout/os/windows/temp.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/posix/os.hpp 
> 1f4d155e4399b955d0ca884f5336c78d8ceb56b5 
>   3rdparty/stout/include/stout/windows/os.hpp 
> de9b04ad82443038a0f4408bc72cae1540a1beaf 
> 
> Diff: https://reviews.apache.org/r/54415/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 54426: Avoided building glog tests.

[Benjamin Bannier]

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

Review request for mesos, Alexander Rukletsov, Kapil Arya, and Till Toenshoff.


Repository: mesos


Description
---

Avoided building glog tests.


Diffs
-

  3rdparty/Makefile.am d12946c37be6e179cb622b5486e4896ac7d717e9 

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


Testing
---

`make check` (OS X)


Thanks,

Benjamin Bannier

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

[Guangya Liu]

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




src/tests/master_tests.cpp (line 4995)


This will invoke `removeFramework`, shall we make also add some logic to 
test the `removeFramework` here and deprecate patch 
https://reviews.apache.org/r/54363 ? As I saw that 
https://reviews.apache.org/r/54363 almost have same code as this test.

We can probably rename this as `AddAndRemoveFrameworkWithMultiRoles`?


- Guangya Liu


On 十二月 6, 2016, 1:58 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54361/
> ---
> 
> (Updated 十二月 6, 2016, 1:58 p.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.AddFrameworkWithMultipleRoles"
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

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

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 6, 2016, 1:48 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54362/
> ---
> 
> (Updated 十二月 6, 2016, 1:48 p.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
> ---
> 
> 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 54360: Changed master to add roles for a multi-role framework.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 5, 2016, 4:57 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54360/
> ---
> 
> (Updated 十二月 5, 2016, 4:57 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
> ---
> 
> 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
> 
>