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

[Alexander Rojas]

> On Dec. 6, 2016, 1:55 a.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.

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.


- Alexander


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


On Dec. 5, 2016, 5:19 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54381/
> ---
> 
> (Updated Dec. 5, 2016, 5:19 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/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
> 
> Diff: https://reviews.apache.org/r/54381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

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




src/slave/containerizer/mesos/io/switchboard.cpp (lines 599 - 604)


Same as we did in recovery, this treat this flag as a knob for new 
containers.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 606 - 608)


'watch' might be called for containers that don't have io switchboard 
server. So we should not return a failure here.



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


we probably should not treat it as a failure if 'status' is None(). After 
agent restart, the reap with return None() for all existing io switchboard 
server because the parent/child relationship has been broken. So:
```
return status
  .then([](const Option& status) {
if (status.isNone()) {
  return Future();
}

if (status.isSome() &&
WIFEXITED(status.get()) &&
WEXITSTATUS(status.get()) == 0) {
  return Future();
}

...
  });
```



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


I'll probably add a namespace alias at the beinging
```
namespace paths = mesos::internal::slave::containerizer::paths;
```



src/tests/containerizer/io_switchboard_tests.cpp (lines 587 - 588)


I'd move this test to a separate patch.



src/tests/containerizer/io_switchboard_tests.cpp (lines 634 - 655)


I would probably rewrite this test and create agent instead. It's much 
easier to simulate an agent restart with a full MesosTest with agent and master.


- Jie Yu


On Dec. 6, 2016, 12:27 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54356/
> ---
> 
> (Updated Dec. 6, 2016, 12:27 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6663
> https://issues.apache.org/jira/browse/MESOS-6663
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We use watch() to trigger the destruction of a container if the
> io switchboard server process ever exits unexpectedly.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
>   include/mesos/v1/mesos.proto 36c122784daa190e25903e8bd4a05f7f1507c304 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 19f2b625f2aa4b790fbe80b8dfad44b219f2c24e 
>   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 54335: Add os::var() to stout.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54335]

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

- Mesos ReviewBot


On Dec. 5, 2016, 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 54355: Added implementation of `recover()` to the IOSwitchboard isolator.

[Jie Yu]

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


Fix it, then Ship it!





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


or agent crashes right before we create the directory and fork the server.



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


Failed to get I/O switchboard server pid for ...



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


Pid file does not exist



src/slave/containerizer/mesos/io/switchboard.cpp (lines 334 - 348)


I would probably move this down right before we fork the server so that if 
anything below fails, we don't need to do anything in recovery.


- Jie Yu


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 53853: Expanded the comment around `ContainerInfo` protobuf.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Dec. 5, 2016, 9:08 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53853/
> ---
> 
> (Updated Dec. 5, 2016, 9:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gilbert Song, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
>   include/mesos/v1/mesos.proto 36c122784daa190e25903e8bd4a05f7f1507c304 
> 
> Diff: https://reviews.apache.org/r/53853/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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, 4:55 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on 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

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, 3:36 a.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/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 54391: Added ability for client to signal EOF on ATTACH_CONTAINER_INPUT.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54391]

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

- Mesos ReviewBot


On Dec. 5, 2016, 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 54336: Windows: Fix `Flags::runtime_dir` value.

[Alex Clemmer]

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

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.


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

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.


- Alex


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


On Dec. 6, 2016, 1:06 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> ---
> 
> (Updated Dec. 6, 2016, 1:06 a.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 
> 
> 

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

[Alex Clemmer]

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

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?


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

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


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


On Dec. 6, 2016, 1:06 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> ---
> 
> (Updated Dec. 6, 2016, 1:06 a.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 54359: Removed redundant semicolons.

[Jay Guo]

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

(Updated Dec. 6, 2016, 3:05 a.m.)


Review request for mesos, Benjamin Mahler and Guangya Liu.


Changes
---

address guangya's comment


Summary (updated)
-

Removed redundant semicolons.


Repository: mesos


Description (updated)
---

Removed redundant semicolons.


Diffs (updated)
-

  src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 

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


Testing
---


Thanks,

Jay Guo

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

[Jay Guo]

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

(Updated Dec. 6, 2016, 3:05 a.m.)


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


Changes
---

address guangya's comments


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/tests/master_tests.cpp dfedbbdf78e8054813872e9eeebccc7504097751 

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


Testing
---

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


Thanks,

Jay Guo

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

[Vinod Kone]

> On Dec. 3, 2016, 2:12 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 7124
> > 
> >
> > CHECK_NOTNULL(framework);
> 
> Neil Conway wrote:
> Is there a general rule for when to add `CHECK_NOTNULL` for internal 
> functions that take pointer parameters? `Master::failoverFramework`, 
> `Master::_failoverFramework`, `Master::_exited(Framework*)`, `Master::drop`, 
> and `Master::authorizeTask` could all have `CHECK_NOTNULL`s added, for 
> example.

i think we should do CHEcK_NOTNULL() for all methods that take raw pointers. 
the above ones you cited seem to have missed them.


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

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?


- Vinod


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

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




src/slave/constants.hpp 


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.



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/


- 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 54361: Added a test to ensure roles from multi-role framework are added.

[Jay Guo]

> On Dec. 6, 2016, 12:52 a.m., Guangya Liu wrote:
> > src/tests/master_tests.cpp, line 4957
> > 
> >
> > When this role was added?

https://github.com/apache/mesos/blob/master/src/master/http.cpp#L3254-L3268
Default role `"*"` is always inserted in the response.


> On Dec. 6, 2016, 12:52 a.m., Guangya Liu wrote:
> > src/tests/master_tests.cpp, line 4992
> > 
> >
> > EXPECT_SOME_EQ(expected.get(), parse);

We already did `ASSERT_SOME(parse)` when we get response, so I think 
`EXPECT_EQ` is sufficient here.


- Jay


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


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

Re: Review Request 53896: Refactored representation of framework connectedness.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 5, 2016, 5:06 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53896/
> ---
> 
> (Updated Dec. 5, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6419
> https://issues.apache.org/jira/browse/MESOS-6419
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the master used a bool to track whether a given framework is
> connected. This commit adjusts the master to use an enum instead. The
> enum currently only has two values, CONNECTED and DISCONNECTED, but an
> additional value (RECOVERED) will be introduced shortly.
> 
> 
> Diffs
> -
> 
>   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/53896/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Benjamin Mahler]

> On Dec. 6, 2016, 2:12 a.m., Benjamin Mahler wrote:
> > Is it possible to split the optional pid change from the discard logic 
> > change? If so that would be great!
> > 
> > Looks like a chunk from the next patch slipped into this one?

Also, would be great to update the description to say that this also updates 
the discard logic to resolve racing (and which race in particular).


- Benjamin


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


On Dec. 5, 2016, 4:35 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54295/
> ---
> 
> (Updated Dec. 5, 2016, 4:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a synchronous version of loop for io::read/write/redirect.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/loop.hpp 
> a78ea7da8a5a778d816a64fb46d9479cc2a4ed70 
>   3rdparty/libprocess/src/http.cpp fc55bda0fae8a43794fb938718b3a720dc342473 
>   3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 
> 
> Diff: https://reviews.apache.org/r/54295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53895: Changed the allocator API to allow adding inactive frameworks.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 2, 2016, 9:50 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53895/
> ---
> 
> (Updated Dec. 2, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6419 and MESOS-6675
> https://issues.apache.org/jira/browse/MESOS-6419
> https://issues.apache.org/jira/browse/MESOS-6675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, a newly added framework was assumed to always be
> active. Adding inactive frameworks will be used to support recovered
> frameworks that haven't yet re-registered.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fe24e8e4291c76a015e1f344764ed53b653cd8cd 
>   include/mesos/allocator/allocator.hpp 
> 3fe04f8a4fb331defbe672c739df4e137bdb3627 
>   src/master/allocator/mesos/allocator.hpp 
> 61159a91700270b1aa6fb335b8a41592e03ef8a9 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3b759494071c4cae4b8b7dbcb0028df4146fc30e 
>   src/master/master.cpp c2c08e176ba3b5afa15c5896b58b6545fb508f5b 
>   src/tests/allocator.hpp bce11904f0b8de66a6620b636321568fdc0d113f 
>   src/tests/hierarchical_allocator_tests.cpp 
> f9dcdbfff8823220409b05002418c98137a0696c 
>   src/tests/master_allocator_tests.cpp 
> bb94e38d5bb472801366c172cfc036f2eecdcbcb 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> f35592ae311f49d3e331fa78c8e427f34bf5 
>   src/tests/reservation_tests.cpp dddfbbe0fc3d0940a65a809ab8340d57dfc685c8 
>   src/tests/resource_offers_tests.cpp 
> 3dde7bf236cb88d72dce73da18d3bc55f0f9402f 
>   src/tests/slave_recovery_tests.cpp 324cf597b6aed7373d9018c4bc9de8fdeac0084c 
> 
> Diff: https://reviews.apache.org/r/53895/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Benjamin Mahler]

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


Fix it, then Ship it!




Is it possible to split the optional pid change from the discard logic change? 
If so that would be great!

Looks like a chunk from the next patch slipped into this one?


3rdparty/libprocess/include/process/loop.hpp (line 102)


Where is ValueType defined? It looks like it's in the next review? Did you 
mean to include this chunk in the next review instead of this one? These don't 
seem to be used in this patch?



3rdparty/libprocess/include/process/loop.hpp (line 291)


Maybe add a comment about what the mutex is for?


- Benjamin Mahler


On Dec. 5, 2016, 4:35 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54295/
> ---
> 
> (Updated Dec. 5, 2016, 4:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a synchronous version of loop for io::read/write/redirect.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/loop.hpp 
> a78ea7da8a5a778d816a64fb46d9479cc2a4ed70 
>   3rdparty/libprocess/src/http.cpp fc55bda0fae8a43794fb938718b3a720dc342473 
>   3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 
> 
> Diff: https://reviews.apache.org/r/54295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

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

[Jie Yu]

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




src/slave/containerizer/mesos/io/switchboard.cpp (lines 147 - 148)


Update this comemnts here?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 206 - 210)


Do you need to check the existence of the io_switchboard directory as well. 
It's likely the container we want to recover here does not have io_switchboard 
directory (do not use io switchboard server).



src/slave/containerizer/mesos/io/switchboard.cpp (lines 217 - 220)


I don't think we want to fail the agent recovery if the previous agent run 
crashed at an unfortunate time.

Also, if a container is in active container list (recoverable), that means 
the io switchboard pid must exist with proper value. Otherwise, it should not 
be part of the recoverable list. I'll probably return a Failure for that case.

Also, I would do the recovering of active containers first. Then, deal with 
orphans.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 648 - 650)


`cleanup` might be called for legacy containers that do not have io 
switchboard server. For those containers, we probably don't want to create an 
Info for them.

Therefore, we should probably ignore unknown container here. Please add a 
comment here about why we ignore unknown container here.


- Jie Yu


On Dec. 6, 2016, 12:21 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, 12:21 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
> 
>

Review Request 54408: Replace 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/
---

Review request for mesos and Vinod Kone.


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
-

  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

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, 2:01 a.m.)


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


Changes
---

Libprocess server sockets need to be closed before `WSACleanup` is called. This 
will cause the test harness to terminate gracefully.


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


Repository: mesos


Description (updated)
---

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/main.cpp c10eeac335b0c8cdb2ea6a0701915ec33f76a2b2 
  src/tests/test_helper_main.cpp a7d511ce71e2789df50aef02d2d50b4c94f38a50 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 54407: Refactored Master::removeFramework to use Master::deactivate.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Refactored Master::removeFramework to use Master::deactivate.


Diffs
-

  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 54401: Reorganized location of checkpointed files for the 'IOSwitchboard'.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 6, 2016, 12:17 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54401/
> ---
> 
> (Updated Dec. 6, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6688
> https://issues.apache.org/jira/browse/MESOS-6688
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reorganized location of checkpointed files for the 'IOSwitchboard'.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 19f2b625f2aa4b790fbe80b8dfad44b219f2c24e 
>   src/slave/containerizer/mesos/paths.hpp 
> c0fe2a4ae8d8b6de24ab265d483d3edc11c68a0e 
>   src/slave/containerizer/mesos/paths.cpp 
> e090392ed079993968f8664d89ad6c49eca3f3c4 
> 
> Diff: https://reviews.apache.org/r/54401/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER-"" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

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

[Alex Clemmer]

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


Ship it!




It is debatable that you would want to put `path::join(os::temp(), "mesos", 
"runtime")` into a variable; I don't think this particularly helps the clarity, 
and doesn't make the code safer (since the only two usages would be so close 
together), so I think it's fine the way it is, but I do think it's worth 
pointing out that some people would argue with me on this. :)

- Alex Clemmer


On Dec. 6, 2016, 1:06 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> ---
> 
> (Updated Dec. 6, 2016, 1:06 a.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 54389: Fixed typo.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54389]

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

- Mesos ReviewBot


On Dec. 5, 2016, 9:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54389/
> ---
> 
> (Updated Dec. 5, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typo.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 2c36f8e8982183da0915645a26c200f2a81442ef 
> 
> Diff: https://reviews.apache.org/r/54389/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54389: Fixed typo.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Dec. 5, 2016, 9:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54389/
> ---
> 
> (Updated Dec. 5, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typo.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 2c36f8e8982183da0915645a26c200f2a81442ef 
> 
> Diff: https://reviews.apache.org/r/54389/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Andrew Schwartzmeyer]

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

(Updated Dec. 6, 2016, 1:06 a.m.)


Review request for mesos and Alex Clemmer.


Changes
---

Remove dependency.


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 54381: Adds authorization support when attaching containers input/output.

[Vinod Kone]

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




src/tests/api_tests.cpp (lines 3728 - 3785)


why did you change these tests? the idea with the tests was to verify that 
`Containerizer::attach()` was getting called from the API handler.


- Vinod Kone


On Dec. 5, 2016, 4:19 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54381/
> ---
> 
> (Updated Dec. 5, 2016, 4:19 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/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
> 
> Diff: https://reviews.apache.org/r/54381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

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




src/tests/master_tests.cpp (line 4917)


How about s/AddMultiRoleFramework/AddFrameworkWithMultiRoles



src/tests/master_tests.cpp (line 4947)


s/response.get().body/response->body



src/tests/master_tests.cpp (line 4949)


ditto



src/tests/master_tests.cpp (line 4957)


When this role was added?



src/tests/master_tests.cpp (line 4967)


ditto



src/tests/master_tests.cpp (line 4978)


ditto



src/tests/master_tests.cpp (lines 4991 - 4992)


new line here



src/tests/master_tests.cpp (line 4992)


EXPECT_SOME_EQ(expected.get(), parse);


- Guangya Liu


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

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

[Andrew Schwartzmeyer]

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

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


Review request for mesos and Alex Clemmer.


Changes
---

Fix comment format.


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 (updated)
-

  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 54336: Windows: Fix `Flags::runtime_dir` value.

[Andrew Schwartzmeyer]

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

(Updated Dec. 6, 2016, 12:31 a.m.)


Review request for mesos and Alex Clemmer.


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


Repository: mesos


Description (updated)
---

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 (updated)
-

  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 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, 12:27 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated to remove check for `io_switchboard_enable_server` (which was made 
unnecessary by updated to previous patches).


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
  include/mesos/v1/mesos.proto 36c122784daa190e25903e8bd4a05f7f1507c304 
  src/slave/containerizer/mesos/io/switchboard.hpp 
839665a22aca9b1c1c1cf4992406bc924ee2b065 
  src/slave/containerizer/mesos/io/switchboard.cpp 
19f2b625f2aa4b790fbe80b8dfad44b219f2c24e 
  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.

[Andrew Schwartzmeyer]

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

(Updated Dec. 6, 2016, 12:21 a.m.)


Review request for mesos and Alex Clemmer.


Changes
---

Fixed replacement of `DEFAULT_ROOT_RUNTIME_DIRECTORY`.


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


Repository: mesos


Description
---

The default `runtime_dir` value was POSIX specific,
and caused https://issues.apache.org/jira/browse/MESOS-6677.

The constant was removed in preparation for `os::runstatedir` refactor.

We unblock the Windows agent by guarding the POSIX code on Windows.


Diffs (updated)
-

  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 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, 12:21 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on comments from Jie.


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

Re: Review Request 54359: Removed redundant empty statements.

[Guangya Liu]

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


Ship it!




I think that you can removing redundant colon?

- Guangya Liu


On 十二月 5, 2016, 4:58 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54359/
> ---
> 
> (Updated 十二月 5, 2016, 4:58 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed redundant empty statements.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 
> 
> Diff: https://reviews.apache.org/r/54359/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

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

[Andrew Schwartzmeyer]

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

(Updated Dec. 6, 2016, 12:10 a.m.)


Review request for mesos and Alex Clemmer.


Changes
---

Updated with test results.


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


Repository: mesos


Description
---

The default `runtime_dir` value was POSIX specific,
and caused https://issues.apache.org/jira/browse/MESOS-6677.

The constant was removed in preparation for `os::runstatedir` refactor.

We unblock the Windows agent by guarding the POSIX code on Windows.


Diffs
-

  src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 
  src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 

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


Testing (updated)
---

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 54336: Windows: Fix `Flags::runtime_dir` value.

[Andrew Schwartzmeyer]

> On Dec. 5, 2016, 7:56 p.m., Alex Clemmer wrote:
> > src/slave/flags.cpp, lines 213-231
> > 
> >
> > In general, I do question whether this should be in the CLI code. It 
> > seems like this lambda should be wrapped up into a function, 
> > `Try os::runstatedir` (as Jie suggests in #54335)? Then you 
> > can `CHECK_SOME` on that result here.
> > 
> > Just as a note, since this review is blocking the agent code working, I 
> > do think it's fine to not have it block on the `os::var` review, and just 
> > have it return a directory in `temp` on Windows until we sort out the 
> > issues in the `os::var` review. The questions about what where we should 
> > put variable data on Windows is (IMO) worth considering somewhat carefully, 
> > but they are not necessary to complete here.
> 
> Andrew Schwartzmeyer wrote:
> Agreed, rewrote to not be dependent on `os::var`. Waiting on tests.

I'd note that the POSIX version as well shouldn't be in the CLI code; but the 
refactor to `os::runstatedir` will be part of the the longer task, #54335, and 
this review is just to unblock the Windows agent.


- Andrew


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


On Dec. 5, 2016, 11:38 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, 11:38 p.m.)
> 
> 
> Review request for mesos and Alex Clemmer.
> 
> 
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default `runtime_dir` value was POSIX specific,
> and caused https://issues.apache.org/jira/browse/MESOS-6677.
> 
> The constant was removed in preparation for `os::runstatedir` refactor.
> 
> We unblock the Windows agent by guarding the POSIX code on Windows.
> 
> 
> 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 (testing second 
> iteration now).
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[Andrew Schwartzmeyer]

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

(Updated Dec. 5, 2016, 11:38 p.m.)


Review request for mesos and Alex Clemmer.


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


Repository: mesos


Description (updated)
---

The default `runtime_dir` value was POSIX specific,
and caused https://issues.apache.org/jira/browse/MESOS-6677.

The constant was removed in preparation for `os::runstatedir` refactor.

We unblock the Windows agent by guarding the POSIX code on Windows.


Diffs (updated)
-

  src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 
  src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 

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


Testing (updated)
---

make && make check on Linux: 1411 tests passed, no failures.

msbuild and attached to Linux master: no runtime failures (testing second 
iteration now).


Thanks,

Andrew Schwartzmeyer

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

[Jie Yu]

> On Dec. 5, 2016, 9:11 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 537-547
> > 
> >
> > It's likely that the io switchboard server has been forked, but the 
> > agent crashes before it was able to checkpoint the pid.
> > 
> > If that happens, during recovery, we will not maintain Info for that 
> > container. As a result, we won't try to cleanup the socket file potentially 
> > created?
> > 
> > I think we probably need to createa directory for io switchboard 
> > related files (sock and pid files). When we create the directory, it 
> > indicates that the io switchboard server might or might not be created. 
> > During recovery, if we find the directory exists, but pid file does not 
> > exist, we should still create the Info with pid set to None(), and cleanup 
> > the socket file in 'cleanup' method.
> > 
> > Thoughts?
> 
> Kevin Klues wrote:
> That seems reasonable, what should we call the directory? As below?
> ```
> io_switchboard
> |-pid
> -socket
> ```
> 
> Kevin Klues wrote:
> That said, note that the io-switchboard itself creates the sock file, so 
> the only time this would ever happen is if an agent restarted between 
> successfully launching the io-switchboard and checkpointing its pid.

yeah, the layout sounds good to me.


- Jie


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


On Dec. 5, 2016, 9:46 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54355/
> ---
> 
> (Updated Dec. 5, 2016, 9:46 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6688
> https://issues.apache.org/jira/browse/MESOS-6688
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation of `recover()` to the IOSwitchboard isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> d5211b98616e72a27ca6b472a5ee83505c227f22 
> 
> Diff: https://reviews.apache.org/r/54355/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> Test added in follow-on patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

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

[Kevin Klues]

> On Dec. 5, 2016, 9:11 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 537-547
> > 
> >
> > It's likely that the io switchboard server has been forked, but the 
> > agent crashes before it was able to checkpoint the pid.
> > 
> > If that happens, during recovery, we will not maintain Info for that 
> > container. As a result, we won't try to cleanup the socket file potentially 
> > created?
> > 
> > I think we probably need to createa directory for io switchboard 
> > related files (sock and pid files). When we create the directory, it 
> > indicates that the io switchboard server might or might not be created. 
> > During recovery, if we find the directory exists, but pid file does not 
> > exist, we should still create the Info with pid set to None(), and cleanup 
> > the socket file in 'cleanup' method.
> > 
> > Thoughts?
> 
> Kevin Klues wrote:
> That seems reasonable, what should we call the directory? As below?
> ```
> io_switchboard
> |-pid
> -socket
> ```

That said, note that the io-switchboard itself creates the sock file, so the 
only time this would ever happen is if an agent restarted between successfully 
launching the io-switchboard and checkpointing its pid.


- Kevin


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


On Dec. 5, 2016, 9:46 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54355/
> ---
> 
> (Updated Dec. 5, 2016, 9:46 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6688
> https://issues.apache.org/jira/browse/MESOS-6688
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation of `recover()` to the IOSwitchboard isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> d5211b98616e72a27ca6b472a5ee83505c227f22 
> 
> Diff: https://reviews.apache.org/r/54355/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> Test added in follow-on patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

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

[Andrew Schwartzmeyer]

> On Dec. 5, 2016, 7:56 p.m., Alex Clemmer wrote:
> > src/slave/flags.cpp, lines 213-231
> > 
> >
> > In general, I do question whether this should be in the CLI code. It 
> > seems like this lambda should be wrapped up into a function, 
> > `Try os::runstatedir` (as Jie suggests in #54335)? Then you 
> > can `CHECK_SOME` on that result here.
> > 
> > Just as a note, since this review is blocking the agent code working, I 
> > do think it's fine to not have it block on the `os::var` review, and just 
> > have it return a directory in `temp` on Windows until we sort out the 
> > issues in the `os::var` review. The questions about what where we should 
> > put variable data on Windows is (IMO) worth considering somewhat carefully, 
> > but they are not necessary to complete here.

Agreed, rewrote to not be dependent on `os::var`. Waiting on tests.


> On Dec. 5, 2016, 7:56 p.m., Alex Clemmer wrote:
> > src/slave/flags.cpp, line 214
> > 
> >
> > Hmm, shouldn't this be a `Try`?
> > 
> > Also, in #54336 the Unix version of `os::var` is returning a 
> > `std::string`, so unless I'm missing something important, it seems like 
> > this should actually not build on Unix?

Oops. I assumed Result and Try were equivalent, but only 
Result is equivalent to Try. Anyway, it was removed in the 
rewrite without `os::var`.


- Andrew


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


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/54336/
> ---
> 
> (Updated Dec. 5, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default `runtime_dir` value was POSIX specific,
> and caused https://issues.apache.org/jira/browse/MESOS-6677.
> Now uses `os::var()` to get `C:\ProgramData\mesos\runtime` for Windows
> and `/var/lib/mesos/runtime` for POSIX.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 
>   src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 
> 
> Diff: https://reviews.apache.org/r/54336/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 53853: Expanded the comment around `ContainerInfo` protobuf.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53853]

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

- Mesos ReviewBot


On Dec. 5, 2016, 5:08 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53853/
> ---
> 
> (Updated Dec. 5, 2016, 5:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gilbert Song, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
>   include/mesos/v1/mesos.proto 36c122784daa190e25903e8bd4a05f7f1507c304 
> 
> Diff: https://reviews.apache.org/r/53853/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

[Kevin Klues]

> On Dec. 5, 2016, 9:11 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 167-171
> > 
> >
> > I think this flag should apply to containers that are about to be 
> > launched next.
> > 
> > So let's still recover Info for those containers that have io 
> > switchboard server even if this flag has been turned off after agent 
> > restart.
> > 
> > What do you think?

I agree. I removed this check, and added a NOTE about this in the comments 
below.


> On Dec. 5, 2016, 9:11 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 537-547
> > 
> >
> > It's likely that the io switchboard server has been forked, but the 
> > agent crashes before it was able to checkpoint the pid.
> > 
> > If that happens, during recovery, we will not maintain Info for that 
> > container. As a result, we won't try to cleanup the socket file potentially 
> > created?
> > 
> > I think we probably need to createa directory for io switchboard 
> > related files (sock and pid files). When we create the directory, it 
> > indicates that the io switchboard server might or might not be created. 
> > During recovery, if we find the directory exists, but pid file does not 
> > exist, we should still create the Info with pid set to None(), and cleanup 
> > the socket file in 'cleanup' method.
> > 
> > Thoughts?

That seems reasonable, what should we call the directory? As below?
```
io_switchboard
|-pid
-socket
```


- Kevin


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


On Dec. 5, 2016, 9:46 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54355/
> ---
> 
> (Updated Dec. 5, 2016, 9:46 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6688
> https://issues.apache.org/jira/browse/MESOS-6688
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation of `recover()` to the IOSwitchboard isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> d5211b98616e72a27ca6b472a5ee83505c227f22 
> 
> Diff: https://reviews.apache.org/r/54355/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> Test added in follow-on patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 53982: Added documentation for posix/rlimit isolator.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 5, 2016, 10:14 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53982/
> ---
> 
> (Updated Dec. 5, 2016, 10:14 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Bugs: MESOS-6427
> https://issues.apache.org/jira/browse/MESOS-6427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for posix/rlimit isolator.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 
>   docs/posix_rlimits.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53982/diff/
> 
> 
> Testing
> ---
> 
> Tested with github markdown renderer, and with build setup from `site/`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Andrew Schwartzmeyer]

> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/posix/os.hpp, line 463
> > 
> >
> > We probably want this function to return `Try`, because we 
> > want to be able to use `os::var` in code that touches both the Windows and 
> > Unix codepaths.

Thanks, this was a bug I didn't catch since I hadn't built on Linux just yet.


> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 770
> > 
> >
> > Technically the spec for this function does say that the second 
> > argument can be `0`, but I personally prefer the more self-documenting 
> > `KF_FLAG_DEFAULT`.
> > 
> > Let me also give you something else to consider, that slightly 
> > contradicts what I'd thought before. Upon reading this code, I'd like to 
> > suggest that it is worth considering whether it is desirable to have 
> > `nullptr` passed in as the third argument, as Mesos is constantly launching 
> > processes with different users, and it might lead to unintended 
> > consequences to have `os::var` return different directories under different 
> > circumstances. The Windows implementation of the agent obviously has a lot 
> > of problems with `su`, but it I think the spirit of the POSIX spec of 
> > `/var` is a globally-accessible path that might be permissioned so that 
> > only `root` can access it, and I think it's worth thinking about 
> > duplicating those semantics here, too, perhaps by passing in the handle of 
> > the administrator, and (especially if getting that handle requires other 
> > permissions) perhaps finding a more appropriate path for this.
> > 
> > If you'd like to punt on this point, that's fine, but I would encourage 
> > you then to document this with a comment to make the consequences of this 
> > design decision clear, as well as file a bug into our backlog to follow up 
> > on this later.

Agreed on `KF_FLAG_DEFAULT` over `0`.

It would be a major problem if this folder changed per user. Fortunately, the 
[`KNOWNFOLDERID`](https://msdn.microsoft.com/en-us/library/windows/desktop/dd378457(v=vs.85).aspx)
 `ProgramData` has folder type [`KF_CATEGORY 
enumeration`](https://msdn.microsoft.com/en-us/library/windows/desktop/bb762512(v=vs.85).aspx)
 `FIXED` which does not change:
> Fixed file system folders are not managed by the Shell and are usually given 
> a permanent path when the system is installed. For example, the Windows and 
> Program Files folders are fixed folders. A number of features such as 
> redirection do not apply to this category.

So we are safe to use `nullptr` here. I've added a comment explaining such to 
the code.


> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 772
> > 
> >
> > Typically we would want a relevant error message here. Although our 
> > error package does not report things like the line number, a good error 
> > message will at the very least give you something to `grep` for when a 
> > function crashes your process.
> > 
> > (Also if you saw use returning just `WindowsError()` somewhere else, 
> > that's probably my fault, and you should learn from my pain :).)

Haha, I did indeed. Added a descriptive error message.


> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 782
> > 
> >
> > I think it's worth considering whether this should be canonized and 
> > encoded as its own function in `stout/strings.hpp` or something. It seems 
> > like we don't want to be hand-rolling a unicode conversion in every place 
> > we need to convert from `wchar` -> `char`, as it's super error prone. Also, 
> > it will make it easier to manage locale changes later.

Absolutely it should be. The other problem with this is that it's assuming none 
of the characters in the original `wstring` were actually multi-byte. While 
this is *probably* true, it is **not** guaranteed per 
[MSDN](https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx),
 a path may:
> Use any character in the current code page for a name, including Unicode 
> characters and characters in the extended character set (128–255)

So I'll need to fix this.


> On Dec. 5, 2016, 7:30 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 769
> > 
> >
> > Although cumbersome, I would personally like to see this RAII'd 
> > somehow, perhaps with a `unique_ptr` or even something incidentally RAII'd, 
> > like a `vector`.
> > 
> > My reason for suggesting this is not just safety, but also 

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

[Kevin Klues]

---
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 54077: Made sure parser settings member is properly initialized.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Nov. 25, 2016, 4:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54077/
> ---
> 
> (Updated Nov. 25, 2016, 4:07 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6646
> https://issues.apache.org/jira/browse/MESOS-6646
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made sure parser settings member is properly initialized.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 13db2eeea6f4b564c970c0cddcdebbe789aba65d 
> 
> Diff: https://reviews.apache.org/r/54077/diff/
> 
> 
> Testing
> ---
> 
> `make check` (OS X, clang trunk w/o optimizations, SSL build)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Kevin Klues]

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

Review request for mesos, Anand Mazumdar and Jie Yu.


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 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. 5, 2016, 10:06 p.m.)


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

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

There is one important caveat to share here: the test harness will
currently error out immediately if you try to run it. In subsequent
reviews, we will add code that addresses these concerns directly, but
this is out of scope for the current review.


Diffs
-

  src/CMakeLists.txt ea6e399c40d3b2cda195091dc7e74230ff3f68fd 
  src/tests/CMakeLists.txt 2ce9d37a75fdec3468aef8aba91db3a0a40e0220 
  src/tests/cluster.cpp 6a9d94b8ac3c8fd0428b7a67d1cb3f99a658fa9b 
  src/tests/test_helper_main.cpp a7d511ce71e2789df50aef02d2d50b4c94f38a50 

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


Testing
---


Thanks,

Alex Clemmer

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

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

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

There is one important caveat to share here: the test harness will
currently error out immediately if you try to run it. In subsequent
reviews, we will add code that addresses these concerns directly, but
this is out of scope for the current review.


Diffs
-

  src/CMakeLists.txt ea6e399c40d3b2cda195091dc7e74230ff3f68fd 
  src/tests/CMakeLists.txt 2ce9d37a75fdec3468aef8aba91db3a0a40e0220 
  src/tests/cluster.cpp 6a9d94b8ac3c8fd0428b7a67d1cb3f99a658fa9b 
  src/tests/test_helper_main.cpp a7d511ce71e2789df50aef02d2d50b4c94f38a50 

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


Testing
---


Thanks,

Alex Clemmer

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

[Kevin Klues]

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

(Updated Dec. 5, 2016, 10:05 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Fixed syntax error in last patch.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

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


Thanks,

Kevin Klues

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

[Kevin Klues]

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

(Updated Dec. 5, 2016, 9:40 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie's comments.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

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


Thanks,

Kevin Klues

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

[Kevin Klues]

> On Dec. 5, 2016, 6:39 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 539
> > 
> >
> > We should not capture 'this' without a defer. Either use a 'defer' 
> > here, or capture `flags.runtime_dir`.

Duh. That was a stupid mistake.


- Kevin


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


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

Review Request 54389: Fixed typo.

[Neil Conway]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Fixed typo.


Diffs
-

  docs/getting-started.md 2c36f8e8982183da0915645a26c200f2a81442ef 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 53689: Use a common fixture for the PID namespace test.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 5, 2016, 8:44 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53689/
> ---
> 
> (Updated Dec. 5, 2016, 8:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-6557
> https://issues.apache.org/jira/browse/MESOS-6557
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a common fixture for the PID namespace test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> da4627846730abd3a817c3d538ff5676c3c1ef45 
> 
> Diff: https://reviews.apache.org/r/53689/diff/
> 
> 
> Testing
> ---
> 
> Make check in Fedora 24.
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Jie Yu]

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




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


Can we also log the error case? Like:
```
if (pid.isError()) {
  LOG(ERROR) << ...
} else if (pid.None()) {
  // Add a comment about when this will happen.
  // E.g., container that do not enable io switchboard server.
} else {
  ...
}
```



src/slave/containerizer/mesos/io/switchboard.cpp (lines 154 - 162)


orphans will be killed by containerizer. So we don't have to kill the io 
switchboard here. Just recover the pid and rely on 'cleanup' to clean it up.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 167 - 171)


I think this flag should apply to containers that are about to be launched 
next.

So let's still recover Info for those containers that have io switchboard 
server even if this flag has been turned off after agent restart.

What do you think?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 536 - 546)


It's likely that the io switchboard server has been forked, but the agent 
crashes before it was able to checkpoint the pid.

If that happens, during recovery, we will not maintain Info for that 
container. As a result, we won't try to cleanup the socket file potentially 
created?

I think we probably need to createa directory for io switchboard related 
files (sock and pid files). When we create the directory, it indicates that the 
io switchboard server might or might not be created. During recovery, if we 
find the directory exists, but pid file does not exist, we should still create 
the Info with pid set to None(), and cleanup the socket file in 'cleanup' 
method.

Thoughts?


- Jie Yu


On Dec. 5, 2016, 9:46 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54355/
> ---
> 
> (Updated Dec. 5, 2016, 9:46 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6688
> https://issues.apache.org/jira/browse/MESOS-6688
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation of `recover()` to the IOSwitchboard isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 839665a22aca9b1c1c1cf4992406bc924ee2b065 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> d5211b98616e72a27ca6b472a5ee83505c227f22 
> 
> Diff: https://reviews.apache.org/r/54355/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> Test added in follow-on patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

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



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. 5, 2016, 4:19 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54381/
> ---
> 
> (Updated Dec. 5, 2016, 4:19 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/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 
> 
> Diff: https://reviews.apache.org/r/54381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 53690: Add namespaces/ipc documentation.

[James Peach]

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

(Updated Dec. 5, 2016, 8:44 p.m.)


Review request for mesos, Avinash sridharan and Jie Yu.


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


Repository: mesos


Description
---

Add namespaces/ipc documentation.


Diffs (updated)
-

  docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 

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


Testing
---

Make check on Fedora 24.


Thanks,

James Peach

Re: Review Request 53688: Implement a namespaces/ipc isolator.

[James Peach]

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

(Updated Dec. 5, 2016, 8:44 p.m.)


Review request for mesos, Avinash sridharan and Jie Yu.


Summary (updated)
-

Implement a namespaces/ipc isolator.


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


Repository: mesos


Description (updated)
---

Implement a namespaces/ipc isolator.


Diffs (updated)
-

  src/CMakeLists.txt ea6e399c40d3b2cda195091dc7e74230ff3f68fd 
  src/Makefile.am 9177ea6a478620f97c692f9da4c92b475cae78cd 
  src/slave/containerizer/mesos/containerizer.cpp 
a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
  src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp PRE-CREATION 
  src/tests/containerizer/isolator_tests.cpp 
da4627846730abd3a817c3d538ff5676c3c1ef45 

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


Testing
---

Make check on Fedora 24.


Thanks,

James Peach

Re: Review Request 53689: Use a common fixture for the PID namespace test.

[James Peach]

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

(Updated Dec. 5, 2016, 8:44 p.m.)


Review request for mesos, Avinash sridharan and Jie Yu.


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


Repository: mesos


Description
---

Use a common fixture for the PID namespace test.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
da4627846730abd3a817c3d538ff5676c3c1ef45 

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


Testing
---

Make check in Fedora 24.


Thanks,

James Peach

Re: Review Request 54177: Slightly simplified two test cases.

[Neil Conway]

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

(Updated Dec. 5, 2016, 8:05 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Change dep.


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


Repository: mesos


Description
---

Slightly simplified two test cases.


Diffs (updated)
-

  src/tests/slave_tests.cpp 3d06c3a4a74bbcba4c9b6bdba4258c5aefdab845 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 54387: Removed an inaccurate comment.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Removed an inaccurate comment.


Diffs
-

  src/master/master.cpp b0670d993348d189fafff0f83f9da0c5b18d1c51 

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


Testing
---

`make check` (no functional change)


Thanks,

Neil Conway

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

[Alex Clemmer]

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




src/slave/flags.cpp (lines 213 - 231)


In general, I do question whether this should be in the CLI code. It seems 
like this lambda should be wrapped up into a function, `Try 
os::runstatedir` (as Jie suggests in #54335)? Then you can `CHECK_SOME` on that 
result here.

Just as a note, since this review is blocking the agent code working, I do 
think it's fine to not have it block on the `os::var` review, and just have it 
return a directory in `temp` on Windows until we sort out the issues in the 
`os::var` review. The questions about what where we should put variable data on 
Windows is (IMO) worth considering somewhat carefully, but they are not 
necessary to complete here.



src/slave/flags.cpp (line 214)


Hmm, shouldn't this be a `Try`?

Also, in #54336 the Unix version of `os::var` is returning a `std::string`, 
so unless I'm missing something important, it seems like this should actually 
not build on Unix?


- Alex Clemmer


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/54336/
> ---
> 
> (Updated Dec. 5, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default `runtime_dir` value was POSIX specific,
> and caused https://issues.apache.org/jira/browse/MESOS-6677.
> Now uses `os::var()` to get `C:\ProgramData\mesos\runtime` for Windows
> and `/var/lib/mesos/runtime` for POSIX.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 
>   src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 
> 
> Diff: https://reviews.apache.org/r/54336/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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. :)

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?


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

+1, thanks for the helpful suggestion Jie. We were debating what to call this 
anyway. :)


- 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 54335: Add os::var() to stout.

[Alex Clemmer]

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




3rdparty/stout/include/stout/posix/os.hpp (line 463)


We probably want this function to return `Try`, because we 
want to be able to use `os::var` in code that touches both the Windows and Unix 
codepaths.



3rdparty/stout/include/stout/windows/os.hpp (line 769)


Although cumbersome, I would personally like to see this RAII'd somehow, 
perhaps with a `unique_ptr` or even something incidentally RAII'd, like a 
`vector`.

My reason for suggesting this is not just safety, but also a matter of 
reasoning about the code: I tend to find it easier to reason about code when 
it's clear when something is done initializing, and when it's clear what the 
spec for destruction is, and the easiest way to accomplish this is to use RAII.



3rdparty/stout/include/stout/windows/os.hpp (line 770)


Technically the spec for this function does say that the second argument 
can be `0`, but I personally prefer the more self-documenting `KF_FLAG_DEFAULT`.

Let me also give you something else to consider, that slightly contradicts 
what I'd thought before. Upon reading this code, I'd like to suggest that it is 
worth considering whether it is desirable to have `nullptr` passed in as the 
third argument, as Mesos is constantly launching processes with different 
users, and it might lead to unintended consequences to have `os::var` return 
different directories under different circumstances. The Windows implementation 
of the agent obviously has a lot of problems with `su`, but it I think the 
spirit of the POSIX spec of `/var` is a globally-accessible path that might be 
permissioned so that only `root` can access it, and I think it's worth thinking 
about duplicating those semantics here, too, perhaps by passing in the handle 
of the administrator, and (especially if getting that handle requires other 
permissions) perhaps finding a more appropriate path for this.

If you'd like to punt on this point, that's fine, but I would encourage you 
then to document this with a comment to make the consequences of this design 
decision clear, as well as file a bug into our backlog to follow up on this 
later.



3rdparty/stout/include/stout/windows/os.hpp (line 772)


Typically we would want a relevant error message here. Although our error 
package does not report things like the line number, a good error message will 
at the very least give you something to `grep` for when a function crashes your 
process.

(Also if you saw use returning just `WindowsError()` somewhere else, that's 
probably my fault, and you should learn from my pain :).)



3rdparty/stout/include/stout/windows/os.hpp (line 782)


I think it's worth considering whether this should be canonized and encoded 
as its own function in `stout/strings.hpp` or something. It seems like we don't 
want to be hand-rolling a unicode conversion in every place we need to convert 
from `wchar` -> `char`, as it's super error prone. Also, it will make it easier 
to manage locale changes later.


- Alex Clemmer


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 54015: Added V1 API protos to python bindings.

[Anand Mazumdar]

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


Fix it, then Ship it!




Sorry for the delay on this! I would fix these minor comments before committing.


src/Makefile.am (line 345)


Reorder alphabetically.



src/Makefile.am (lines 453 - 456)


Reorder alphabetically.



src/Makefile.am (line 460)


Alignment of the trailing character (``) needs to be fixed.



src/Makefile.am (line 490)


Alignment of the trailing character (``) needs to be fixed.


- Anand Mazumdar


On Nov. 23, 2016, 4:39 a.m., Vijay Srinivasaraghavan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54015/
> ---
> 
> (Updated Nov. 23, 2016, 4:39 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6597
> https://issues.apache.org/jira/browse/MESOS-6597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added V1 API protos to python bindings.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
> 
> Diff: https://reviews.apache.org/r/54015/diff/
> 
> 
> Testing
> ---
> 
> Ran "make check"
> 
> 
> Thanks,
> 
> Vijay Srinivasaraghavan
> 
>

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

[Jie Yu]

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


Fix it, then Ship it!





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


We should not capture 'this' without a defer. Either use a 'defer' here, or 
capture `flags.runtime_dir`.


- Jie Yu


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

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

[Jie Yu]

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



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

- Jie Yu


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 53690: Add namespaces/ipc documentation.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 5, 2016, 5:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53690/
> ---
> 
> (Updated Dec. 5, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-6557
> https://issues.apache.org/jira/browse/MESOS-6557
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add namespaces/ipc documentation.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 
> 
> Diff: https://reviews.apache.org/r/53690/diff/
> 
> 
> Testing
> ---
> 
> Make check on Fedora 24.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53688: Implement a namespace/ipc isolator.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp (line 56)


why 'pid-ipc'? should we be consistent, and calling it 
ipc-namespace-isolator?



src/tests/containerizer/isolator_tests.cpp (lines 81 - 105)


Hum, I'd still suggest we inline those code in the test for now. I don't 
see a strong reason we need to do that yet. 'executorCommand' here just check 
if the command succeeds or not. We might want to check if a command 'failed' or 
not in the future. So let's don't do this yet.

The reason for that is that I found the test itself is more readable if all 
those logics are self contained.



src/tests/containerizer/isolator_tests.cpp (line 108)


This is nice. Let's keep this in the fixture.



src/tests/containerizer/isolator_tests.cpp (lines 138 - 142)


If you want to do the refactor of the test fixture, you probably should 
update this test as well. But as I mentioned above, let's not do it for now.



src/tests/containerizer/isolator_tests.cpp (lines 210 - 212)


This is just a question. Looking at this test. I am wondering if you need 
to remount the proc or not for IPC namespace?



src/tests/containerizer/isolator_tests.cpp (line 211)


I'd use `stringify(shmmaxValue)` here


- Jie Yu


On Dec. 5, 2016, 5:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53688/
> ---
> 
> (Updated Dec. 5, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-6557
> https://issues.apache.org/jira/browse/MESOS-6557
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement a namespace/ipc isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ea6e399c40d3b2cda195091dc7e74230ff3f68fd 
>   src/Makefile.am 9177ea6a478620f97c692f9da4c92b475cae78cd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
>   src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp PRE-CREATION 
>   src/tests/containerizer/isolator_tests.cpp 
> da4627846730abd3a817c3d538ff5676c3c1ef45 
> 
> Diff: https://reviews.apache.org/r/53688/diff/
> 
> 
> Testing
> ---
> 
> Make check on Fedora 24.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53689: Use a common fixture for the PID namespace test.

[Jie Yu]

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



Aha, just saw this. I still suggest that we inline those methods so the test 
looks more self contained.

- Jie Yu


On Dec. 5, 2016, 5:29 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53689/
> ---
> 
> (Updated Dec. 5, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-6557
> https://issues.apache.org/jira/browse/MESOS-6557
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a common fixture for the PID namespace test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> da4627846730abd3a817c3d538ff5676c3c1ef45 
> 
> Diff: https://reviews.apache.org/r/53689/diff/
> 
> 
> Testing
> ---
> 
> Make check in Fedora 24.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 54133: Fixed includes in default_executor_tests.cpp.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Nov. 28, 2016, 6:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54133/
> ---
> 
> (Updated Nov. 28, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed includes in default_executor_tests.cpp.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 5bddcc3d21523a83301a7c410e8f96dbd8e8e104 
> 
> Diff: https://reviews.apache.org/r/54133/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of r/54134.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Anand Mazumdar]

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


Ship it!




Can you also do a sweep in `src/tests/slave_tests.cpp` for other task group 
related tests that have a similar wrong assumption?

I would go ahead and commit this for now.

- Anand Mazumdar


On Nov. 28, 2016, 6:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54134/
> ---
> 
> (Updated Nov. 28, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Just like TASK_KILLED updates, TASK_RUNNING updates can arrive in any
> order. Adjust the code to not assume a particual order.
> 
> Also cleaned up the code to minimize the mutated state.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 5bddcc3d21523a83301a7c410e8f96dbd8e8e104 
> 
> Diff: https://reviews.apache.org/r/54134/diff/
> 
> 
> Testing
> ---
> 
> * `make check` (OS X), internal CI on various Linux configurations.
> * also tested with `*DefaultExecutor*` tests selected.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Anand Mazumdar]

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


Ship it!





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


Related to this, we need to fix the agent's `api/v1` handler default 
'Accept' to be `application/json` as it was before. Otherwise, an old client 
relying on it i.e., not setting the header would break un-neccesarily after an 
upgrade.


- Anand Mazumdar


On Dec. 5, 2016, 7:09 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54347/
> ---
> 
> (Updated Dec. 5, 2016, 7:09 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up the 'IOSwitchboard.RedirectLog' test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> ebf9bc75b8a256847a507954615241cff1da4dc3 
> 
> Diff: https://reviews.apache.org/r/54347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

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

[Andrew Schwartzmeyer]

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

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

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

[Andrew Schwartzmeyer]

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

Review request for mesos.


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


Repository: mesos


Description
---

The default `runtime_dir` value was POSIX specific,
and caused https://issues.apache.org/jira/browse/MESOS-6677.
Now uses `os::var()` to get `C:\ProgramData\mesos\runtime` for Windows
and `/var/lib/mesos/runtime` for POSIX.


Diffs
-

  src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 
  src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 54311: Fixed whitespace, code style in Network::Address.

[Alexander Rukletsov]

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


Ship it!





3rdparty/libprocess/include/process/address.hpp (lines 365 - 366)


I'll preserve `// __WINDOWS__` to preserve local consistency.


- Alexander Rukletsov


On Dec. 5, 2016, 5:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54311/
> ---
> 
> (Updated Dec. 5, 2016, 5:23 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed whitespace, code style in Network::Address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/address.hpp 
> a2eeb390db360b9441a19315ee697bfe9b138cf8 
> 
> Diff: https://reviews.apache.org/r/54311/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54311: Fixed whitespace, code style in Network::Address.

[Neil Conway]

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

(Updated Dec. 5, 2016, 5:23 p.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Rebase.


Summary (updated)
-

Fixed whitespace, code style in Network::Address.


Repository: mesos


Description (updated)
---

Fixed whitespace, code style in Network::Address.


Diffs (updated)
-

  3rdparty/libprocess/include/process/address.hpp 
a2eeb390db360b9441a19315ee697bfe9b138cf8 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 53690: Add namespaces/ipc documentation.

[James Peach]

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

(Updated Dec. 5, 2016, 5:29 p.m.)


Review request for mesos, Avinash sridharan and Jie Yu.


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


Repository: mesos


Description
---

Add namespaces/ipc documentation.


Diffs (updated)
-

  docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 

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


Testing
---

Make check on Fedora 24.


Thanks,

James Peach

Re: Review Request 53689: Use a common fixture for the PID namespace test.

[James Peach]

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

(Updated Dec. 5, 2016, 5:29 p.m.)


Review request for mesos, Avinash sridharan and Jie Yu.


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


Repository: mesos


Description
---

Use a common fixture for the PID namespace test.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
da4627846730abd3a817c3d538ff5676c3c1ef45 

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


Testing
---

Make check in Fedora 24.


Thanks,

James Peach

Re: Review Request 53688: Implement a namespace/ipc isolator.

[James Peach]

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

(Updated Dec. 5, 2016, 5:29 p.m.)


Review request for mesos, Avinash sridharan and Jie Yu.


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


Repository: mesos


Description
---

Implement a namespace/ipc isolator.


Diffs (updated)
-

  src/CMakeLists.txt ea6e399c40d3b2cda195091dc7e74230ff3f68fd 
  src/Makefile.am 9177ea6a478620f97c692f9da4c92b475cae78cd 
  src/slave/containerizer/mesos/containerizer.cpp 
a7e2665d4c64b7b322d4ae8d5441e8e777236c59 
  src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp PRE-CREATION 
  src/tests/containerizer/isolator_tests.cpp 
da4627846730abd3a817c3d538ff5676c3c1ef45 

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


Testing
---

Make check on Fedora 24.


Thanks,

James Peach

Re: Review Request 53853: Expanded the comment around `ContainerInfo` protobuf.

[Alexander Rukletsov]

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

(Updated Dec. 5, 2016, 5:08 p.m.)


Review request for mesos, Benjamin Bannier, Gilbert Song, Jie Yu, and Jan 
Schlicht.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 657dcb8edde3154efec21bd147663fc704f97482 
  include/mesos/v1/mesos.proto 36c122784daa190e25903e8bd4a05f7f1507c304 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53896: Refactored representation of framework connectedness.

[Neil Conway]

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

(Updated Dec. 5, 2016, 5:06 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Add TODO comment.


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


Repository: mesos


Description
---

Previously, the master used a bool to track whether a given framework is
connected. This commit adjusts the master to use an enum instead. The
enum currently only has two values, CONNECTED and DISCONNECTED, but an
additional value (RECOVERED) will be introduced shortly.


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/53896/diff/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 53896: Refactored representation of framework connectedness.

[Neil Conway]

> On Dec. 2, 2016, 10:54 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 2547
> > 
> >
> > can you convert this into ACTIVE and INACTIVE states? if you want to do 
> > that later, please add a TODO (and maybe a ticket to track).

Created a separate JIRA for this, 
https://issues.apache.org/jira/browse/MESOS-6719


> On Dec. 2, 2016, 10:54 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 2528
> > 
> >
> > I would kill this helper in favor of checking state directly. The 
> > reason being, as we add more and more states having such helpers seems 
> > redundant.

Per discussion, I like the helpers for two reasons:

1. When we do MESOS-6719, whether a framework is "connected" might correspond 
to more than one `state` enumeration value.
2. Checking `if (framework->state == Framework::State::CONNECTED)` is a more 
verbose than checking `if (framework->connected())`


- Neil


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


On Nov. 18, 2016, 7:21 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53896/
> ---
> 
> (Updated Nov. 18, 2016, 7:21 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6419
> https://issues.apache.org/jira/browse/MESOS-6419
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the master used a bool to track whether a given framework is
> connected. This commit adjusts the master to use an enum instead. The
> enum currently only has two values, CONNECTED and DISCONNECTED, but an
> additional value (RECOVERED) will be introduced shortly.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 90cbed1ba411e18906fe9c26bc14576a26d1b7b9 
>   src/master/master.hpp 7829f3f5f6125714b2fa48fe7c2813c26d14e26d 
>   src/master/master.cpp 7ed1d259f02991bcd1389d0529a4bc97b0aa0245 
>   src/master/quota_handler.cpp 5578663f26d9b737499dc4f5a286c34c37645442 
> 
> Diff: https://reviews.apache.org/r/53896/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54014: Fixed missing protobuf java package definition.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Nov. 23, 2016, 5:46 a.m., Vijay Srinivasaraghavan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54014/
> ---
> 
> (Updated Nov. 23, 2016, 5:46 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6597
> https://issues.apache.org/jira/browse/MESOS-6597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed missing protobuf java package definition.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/allocator/allocator.proto 
> 73d45b37a7afc47366a4a01a36912f30b47c30b1 
> 
> Diff: https://reviews.apache.org/r/54014/diff/
> 
> 
> Testing
> ---
> 
> Ran "make check"
> 
> 
> Thanks,
> 
> Vijay Srinivasaraghavan
> 
>

Re: Review Request 53825: MESOS-6597 Enabled java protos generation for all V1 proto files.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Nov. 23, 2016, 5:46 a.m., Vijay Srinivasaraghavan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53825/
> ---
> 
> (Updated Nov. 23, 2016, 5:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zameer Manji.
> 
> 
> Bugs: MESOS-6597
> https://issues.apache.org/jira/browse/MESOS-6597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6597 Enabled java protos generation for all V1 proto files.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
> 
> Diff: https://reviews.apache.org/r/53825/diff/
> 
> 
> Testing
> ---
> 
> Built mesos and confirmed jar file containing new java PB wrappers for V1 
> API. Ran standard unit test (make tests)
> 
> 
> Thanks,
> 
> Vijay Srinivasaraghavan
> 
>

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, line 5602
> > 
> >
> > shouldn't we send a ShutdownFrameworkMessage in this case?

This is implemented in a later review in this chain, 
https://reviews.apache.org/r/54232/


> On Dec. 3, 2016, 2:12 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 5578-5580
> > 
> >
> > yea, i think this should be
> > 
> > ```
> > if (framework != nullptr && framework->connected()) {
> > 
> > }
> > ```

I did this in a separate review, https://reviews.apache.org/r/54380/


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

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.


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

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

[Alexander Rojas]

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

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/tests/api_tests.cpp 7c70f284a9225ca23abc7049f48f3efba314b641 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 54177: Slightly simplified two test cases.

[Neil Conway]

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

(Updated Dec. 5, 2016, 4:12 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak dependency


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


Repository: mesos


Description
---

Slightly simplified two test cases.


Diffs (updated)
-

  src/tests/slave_tests.cpp 3d06c3a4a74bbcba4c9b6bdba4258c5aefdab845 

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


Testing
---

`make check`


Thanks,

Neil Conway

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

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 53982: Added documentation for posix/rlimit isolator.

[Neil Conway]

> On Nov. 29, 2016, 3:08 p.m., Neil Conway wrote:
> > I'd still like to see more discussion of how someone might actually use 
> > this feature. For example, when you say "With these constraints operators 
> > can use a
> > global hard limit to contain resource limits set by users" -- how would the 
> > operator do this? Presumably the operator cannot enforce global resource 
> > limits via a user-specified flag in `ContainerInfo`.
> 
> Benjamin Bannier wrote:
> This section is related to POSIX resource limits in particular, and not 
> how they are mapped in Mesos. I specifically mention _POSIX resource limit_ 
> at the beginning of the paragraph, and also replaced _operator_ with _system 
> administrators_ to make this clearer.

Sure. I still think that there should be more discussion of how to use this 
feature to solve an actual problem that a cluster operator or user might 
encounter.


- Neil


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


On Dec. 5, 2016, 10:14 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53982/
> ---
> 
> (Updated Dec. 5, 2016, 10:14 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Bugs: MESOS-6427
> https://issues.apache.org/jira/browse/MESOS-6427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for posix/rlimit isolator.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 
>   docs/posix_rlimits.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53982/diff/
> 
> 
> Testing
> ---
> 
> Tested with github markdown renderer, and with build setup from `site/`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 53982: Added documentation for posix/rlimit isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53982]

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

- Mesos ReviewBot


On Dec. 5, 2016, 10:14 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53982/
> ---
> 
> (Updated Dec. 5, 2016, 10:14 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Bugs: MESOS-6427
> https://issues.apache.org/jira/browse/MESOS-6427
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for posix/rlimit isolator.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 
>   docs/posix_rlimits.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53982/diff/
> 
> 
> Testing
> ---
> 
> Tested with github markdown renderer, and with build setup from `site/`.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54344, 54347, 54297, 54348, 54352, 54353, 54354, 54355, 
54356, 54368, 54367]

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

- Mesos ReviewBot


On Dec. 5, 2016, 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 53982: Added documentation for posix/rlimit isolator.

[Benjamin Bannier]

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

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


Review request for mesos, Jie Yu and Neil Conway.


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


Repository: mesos


Description
---

Added documentation for posix/rlimit isolator.


Diffs (updated)
-

  docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 
  docs/posix_rlimits.md PRE-CREATION 

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


Testing
---

Tested with github markdown renderer, and with build setup from `site/`.


Thanks,

Benjamin Bannier

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



Patch looks great!

Reviews applied: [54359, 54360, 54361, 54362, 54363, 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. 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 Dec. 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 54355: Added implementation of `recover()` to the IOSwitchboard isolator.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 9:46 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated to properly recover orphans info before killing them.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

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

Test added in follow-on patch.


Thanks,

Kevin Klues

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. 5, 2016, 9:44 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated to force destruction of executor in test.


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

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

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 
c0fe2a4ae8d8b6de24ab265d483d3edc11c68a0e 
  src/slave/containerizer/mesos/paths.cpp 
e090392ed079993968f8664d89ad6c49eca3f3c4 

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


Testing
---

Tested in subsequent patch.


Thanks,

Kevin Klues