Re: Review Request 29889: Recover Docker containers when mesos slave is in a container

[Timothy Chen]

> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/docker/docker.hpp, line 64
> > 
> >
> > Can you add a small comment explaining when 'started' would be 'false'? 
> > I'm guessing you want to use 'started' here to help figure out when a 
> > container has "started" when you do an attached 'docker run', but I don't 
> > know what 'started' implies, and how it is different than checking when 
> > 'pid.isSome()'?

Good point, I cannot use pid because at the time when I inspect again the 
container might already finished and there is no pid available.
So the easiest way to know if the container has actually started is by 
examining the startedAt


> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.hpp, lines 462-464
> > 
> >
> > Any reason not to make these Option as well? It seems a little wierd to 
> > potentially read these and get garbage (or missing) data in the case that 
> > they were actually passed into the constructor as 'None'.

Since we always set it to something in the constructor, I thought there is no 
point to set them to Option. If you look in the constructor code you'll see 
that.


> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.cpp, lines 845-848
> > 
> >
> > Why do you need to associate the promise within the 'onAny' callback? 
> > You should just be able to do:
> > 
> > 
> > Future inspect =
> > docker->inspect(containerName, slave::DOCKER_INSPECT_DELAY);
> > 
> > promise->associate(inspect);
> > 
> > 
> > The other thing that is a bit mysterious to me here is why you actually 
> > want/need an extra Promise? Seems like you can just do:
> > 
> > run.onFailed([=](const string& failure) {
> >   inspect.discard();
> > });
> > 
> > return inspect;
> > 
> > 
> > Maybe it's because you don't want a 'discarded' future to propagate out 
> > from 'launchExecutorContainer'? In which case, I'd comment as much, i.e., 
> > that you explicitly want a failed future rather than let a discarded future 
> > propagate and clearly explain why (hopefully there isn't other code relying 
> > on it someplace because that's definitely a global invariant that we should 
> > avoid!).

I think you're right the simplified version is all I need, in the beginning I 
made a promise because I was thinking I might need to call inspect again later 
as I was thinking to keep inpsect simple and made the retries outside. But now 
with retries happening in inspect, this is a lot easier.


> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.cpp, line 912
> > 
> >
> > So this version is correctly working? Just wanted to call this out 
> > explicitly because the other one is not and that makes me wonder if maybe 
> > this code path isn't being tested?

No this actually works if I just pass the flags straight without slicing, but 
in the other path where I originally store the flags in a 
Option and then pass that into subprocess actually gives me 
bad access from time to time. I didn't want to hard code the type in dockerRun 
to be docker::Flags because I thought docker::run should take a generic 
flagbase in case it wants to pass any arbitrary flags in.


- Timothy


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


On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> ---
> 
> (Updated May 23, 2015, 6:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a one mega patch and contains many reviews that's already on rb.
> 
> This review is not meant to be merged, only provided for easier review.
> 
> 
> Diffs
> -
> 
>   Dockerfile 35abf25 
>   docs/configuration.md 54c4e31 
>   docs/docker-containerizer.md a5438b7 
>   src/Makefile.am 34755cf 
>   src/docker/docker.hpp 3ebbc1f 
>   src/docker/docker.cpp 3a485a2 
>   src/docker/executor.hpp PRE-CREATION 
>   src/docker/executor.cpp PRE-CREATION 
>   src/slave/constants.hpp fd1c1ab 
>   src/slave/constants.cpp 2a99b11 
>   src/slave/containerizer/docker.hpp ed4ee19 
>   src/slave/contai

Review Request 34645: Update existing lambdas to meet style guide

[haosdent huang]

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

Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


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


Repository: mesos


Description
---

Update existing lambdas to meet style guide


Diffs
-

  src/master/master.cpp 1526f59e7c6b135657550eab2ca46216923a01f6 

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


Testing
---


Thanks,

haosdent huang

Review Request 34644: Update existing lambdas to meet style guide

[haosdent huang]

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

Review request for mesos, Benjamin Hindman and Joris Van Remoortere.


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


Repository: mesos


Description
---

Update existing lambdas to meet style guide


Diffs
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
67e582cc250a9767a389e2bd0cc68985477f3ffb 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 29889: Recover Docker containers when mesos slave is in a container

[Timothy Chen]

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



src/slave/containerizer/docker.cpp


Ah, I realize why now I want a promise.
So the semantics I want to have is that I want to able to propagate the 
failure from run back out to the caller, so that it shows up in the TaskStatus 
update, but otherwise return the inspect future when it succeed. So I do want 
to use the promise associate to handle both cases. I'll leave a comment.


- Timothy Chen


On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> ---
> 
> (Updated May 23, 2015, 6:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a one mega patch and contains many reviews that's already on rb.
> 
> This review is not meant to be merged, only provided for easier review.
> 
> 
> Diffs
> -
> 
>   Dockerfile 35abf25 
>   docs/configuration.md 54c4e31 
>   docs/docker-containerizer.md a5438b7 
>   src/Makefile.am 34755cf 
>   src/docker/docker.hpp 3ebbc1f 
>   src/docker/docker.cpp 3a485a2 
>   src/docker/executor.hpp PRE-CREATION 
>   src/docker/executor.cpp PRE-CREATION 
>   src/slave/constants.hpp fd1c1ab 
>   src/slave/constants.cpp 2a99b11 
>   src/slave/containerizer/docker.hpp ed4ee19 
>   src/slave/containerizer/docker.cpp 408a443 
>   src/slave/flags.hpp 5c57478 
>   src/slave/flags.cpp b5e2518 
>   src/tests/docker_containerizer_tests.cpp 154bf98 
>   src/tests/docker_tests.cpp 5520c58 
> 
> Diff: https://reviews.apache.org/r/29889/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 33753: Update pthread and python autoconf macros for Mesos.

[haosdent huang]

> On May 18, 2015, 6:31 p.m., haosdent huang wrote:
> > m4/ax_python_devel.m4, line 268
> > 
> >
> > Could not understand why need add conf('SYSLIBS')) here? Could you 
> > explain this? Thank you
> 
> James Peach wrote:
> ``ax_python_devel.m4`` is taken unmodified from the autoconf archive. It 
> looks like ``SYSLIBS`` was added in commit 
> [6745ad4069f2fab26d72f374079cb8b7a4cf350f 
> ](http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=commitdiff;h=6745ad4069f2fab26d72f374079cb8b7a4cf350f)

got it, Thank you. :-)


- haosdent


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


On May 1, 2015, 4:02 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33753/
> ---
> 
> (Updated May 1, 2015, 4:02 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-2659
> https://issues.apache.org/jira/browse/MESOS-2659
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace acx_pthread with ax_pthread. Update ax_python_devel. Update
> configure.ac to use the autoconf macros.
> 
> 
> Diffs
> -
> 
>   configure.ac 589ae97d0432370b462576cd1985544564893999 
>   m4/acx_pthread.m4 2cf20de144a11be2aa603b04ea511244191037b7 
>   m4/ax_pthread.m4 PRE-CREATION 
>   m4/ax_python_devel.m4 3463d1430e16b7c9722319271e693e56bf1d323d 
> 
> Diff: https://reviews.apache.org/r/33753/diff/
> 
> 
> Testing
> ---
> 
> Bootstrap and verify there are no autotools warnings. Tested on CentOS 7 and 
> Mac OS X 10.10.3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 34646: Return empty task list when current master is not a leader.

[haosdent huang]

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

Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Return empty task list when current master is not a leader.


Diffs
-

  src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 29889: Recover Docker containers when mesos slave is in a container

[Timothy Chen]

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

(Updated May 24, 2015, 6:26 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.


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


Repository: mesos


Description
---

This is a one mega patch and contains many reviews that's already on rb.

This review is not meant to be merged, only provided for easier review.


Diffs (updated)
-

  Dockerfile 35abf25 
  docs/configuration.md 54c4e31 
  docs/docker-containerizer.md a5438b7 
  src/Makefile.am 34755cf 
  src/docker/docker.hpp 3ebbc1f 
  src/docker/docker.cpp 3a485a2 
  src/docker/executor.hpp PRE-CREATION 
  src/docker/executor.cpp PRE-CREATION 
  src/slave/constants.hpp fd1c1ab 
  src/slave/constants.cpp 2a99b11 
  src/slave/containerizer/docker.hpp ed4ee19 
  src/slave/containerizer/docker.cpp 408a443 
  src/slave/flags.hpp 5c57478 
  src/slave/flags.cpp b5e2518 
  src/tests/docker_containerizer_tests.cpp 154bf98 
  src/tests/docker_tests.cpp 5520c58 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 29889: Recover Docker containers when mesos slave is in a container

[Timothy Chen]

> On May 24, 2015, 6:07 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, lines 845-848
> > 
> >
> > Ah, I realize why now I want a promise.
> > So the semantics I want to have is that I want to able to propagate the 
> > failure from run back out to the caller, so that it shows up in the 
> > TaskStatus update, but otherwise return the inspect future when it succeed. 
> > So I do want to use the promise associate to handle both cases. I'll leave 
> > a comment.

I'm keeping the current version since I cannot just use associate, as I no 
longer can override the failure once associated. I'm leaving comments on why.


- Timothy


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


On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> ---
> 
> (Updated May 23, 2015, 6:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a one mega patch and contains many reviews that's already on rb.
> 
> This review is not meant to be merged, only provided for easier review.
> 
> 
> Diffs
> -
> 
>   Dockerfile 35abf25 
>   docs/configuration.md 54c4e31 
>   docs/docker-containerizer.md a5438b7 
>   src/Makefile.am 34755cf 
>   src/docker/docker.hpp 3ebbc1f 
>   src/docker/docker.cpp 3a485a2 
>   src/docker/executor.hpp PRE-CREATION 
>   src/docker/executor.cpp PRE-CREATION 
>   src/slave/constants.hpp fd1c1ab 
>   src/slave/constants.cpp 2a99b11 
>   src/slave/containerizer/docker.hpp ed4ee19 
>   src/slave/containerizer/docker.cpp 408a443 
>   src/slave/flags.hpp 5c57478 
>   src/slave/flags.cpp b5e2518 
>   src/tests/docker_containerizer_tests.cpp 154bf98 
>   src/tests/docker_tests.cpp 5520c58 
> 
> Diff: https://reviews.apache.org/r/29889/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 29889: Recover Docker containers when mesos slave is in a container

[Timothy Chen]

> On April 28, 2015, 10:11 p.m., Bernd Mathiske wrote:
> > Dockerfile, line 18
> > 
> >
> > It seems that docker.io could be on its own line, since it is a kinda 
> > separate topic, which seems to be the grouping criterium here?

I'll revert this since it's not reall related.


- Timothy


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


On May 24, 2015, 6:26 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> ---
> 
> (Updated May 24, 2015, 6:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a one mega patch and contains many reviews that's already on rb.
> 
> This review is not meant to be merged, only provided for easier review.
> 
> 
> Diffs
> -
> 
>   Dockerfile 35abf25 
>   docs/configuration.md 54c4e31 
>   docs/docker-containerizer.md a5438b7 
>   src/Makefile.am 34755cf 
>   src/docker/docker.hpp 3ebbc1f 
>   src/docker/docker.cpp 3a485a2 
>   src/docker/executor.hpp PRE-CREATION 
>   src/docker/executor.cpp PRE-CREATION 
>   src/slave/constants.hpp fd1c1ab 
>   src/slave/constants.cpp 2a99b11 
>   src/slave/containerizer/docker.hpp ed4ee19 
>   src/slave/containerizer/docker.cpp 408a443 
>   src/slave/flags.hpp 5c57478 
>   src/slave/flags.cpp b5e2518 
>   src/tests/docker_containerizer_tests.cpp 154bf98 
>   src/tests/docker_tests.cpp 5520c58 
> 
> Diff: https://reviews.apache.org/r/29889/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 34193: Refactored common functionality into BaseFlags

[Benjamin Hindman]

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

Ship it!


Everything looks good but let's please resolve the issue with 'printUsage' and 
then I'll commit.


3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


Adhering to the Google Style Guide, we do not use non-const reference 
arguments to functions: 
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arguments

As the style guide points out, there are exceptions here, most notably 
'operator <<' which, by no coincidence, is relevant here and perhaps why this 
slipped through? ;-)

Here's my suggestion to resolve this issue: let's add the 'std::string 
message' to 'usage()'. There wasn't anything special about that function that 
didn't include it originally, so I don't see any reason to not add it now. 
Here's how this would look:

std::string usage(const Option& message = None()) const;

Now in the body of 'usage' we can add our default message or replace with 
whatever someone else passed in. And then here is how it would look like when 
used:

if (flags.help) {
  std::cerr << flags.usage("This is: TestFlags [options]") << std::endl;
}

The alternative here would be to take std::ostream as a pointer, but where 
ever we can cleanly prefer the functional style we should as it leads to more 
composable and easier to reason about code (i.e., avoiding non-local 
manipulation of variables, etc).

Moreover, I actually think it yields cleaner code at the call sites where 
we are already using std::cerr directly, for example:

if (flags.master.isNone()) {
  std::cerr << "Missing required option --master" << std::endl;
  flags.printUsage(std::cerr);
  return EXIT_FAILURE;
}

Would be cleaner to me as:

if (flags.master.isNone()) {
  std::cerr << "Missing required option --master" << std::endl
<< flags.usage() << std::endl;
  return EXIT_FAILURE;
}

Finally, to cover all the cases here, I could imagine killing 'setUsageMsg' 
and letting folks define their own constants that they always pass in, for 
example:

const string USAGE_MESSAGE = "This is: TestFlags [Options]";

...

std::cerr << flags.usage(USAGE_MESSAGE) << std::endl;

To be clear I'm not opposed to having a usage message setter, but I'm 
always in favor of having less ways, or ideally one way, of doing the same 
thing. And regardless, s/setUsageMsg/setUsageMessage/ (it looks like Joris 
mentioned this in a previous review, so just following up here, thanks guys!).



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


I think it would be great if we could consistently use the trailing 
underscore it for any single class, rather than some fields using it and some 
not as this could confuse people when to use it. In this case, any reason not 
to change all the fields or not use the trailing underscore for now?



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


Any reason not to make 'programName_' be an Option rather than setting it 
to the empty string if argv[0] doesn't exist? While I understand in this case 
we'd probably print the same thing, we definitely have more information with an 
Option versus someone passing us an empty string for argv[0].



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp


Two newlines between all top-level definitions/declarations please! I see 
Joris called this out in a previous review but he just addressed a single place 
instead of all the places. ;-) Thanks guys!



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp


With the revised 'usage()' this will just be:

out << flags.usage("This is: TestFlags [options]");


- Benjamin Hindman


On May 20, 2015, 11:22 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34193/
> ---
> 
> (Updated May 20, 2015, 11:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2711
> 
> Every program that uses stout's `BaseFlags` ends up
> re-implementing the `printUsage()` function, and adding
> a `bool help` (and associated --help flag); this functional

Re: Review Request 29889: Recover Docker containers when mesos slave is in a container

[Timothy Chen]

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

(Updated May 25, 2015, 5:25 a.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.


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


Repository: mesos


Description
---

This is a one mega patch and contains many reviews that's already on rb.

This review is not meant to be merged, only provided for easier review.


Diffs (updated)
-

  Dockerfile 35abf25 
  docs/configuration.md 54c4e31 
  docs/docker-containerizer.md a5438b7 
  src/Makefile.am 34755cf 
  src/docker/docker.hpp 3ebbc1f 
  src/docker/docker.cpp 3a485a2 
  src/docker/executor.hpp PRE-CREATION 
  src/docker/executor.cpp PRE-CREATION 
  src/slave/constants.hpp fd1c1ab 
  src/slave/constants.cpp 2a99b11 
  src/slave/containerizer/docker.hpp ed4ee19 
  src/slave/containerizer/docker.cpp 408a443 
  src/slave/flags.hpp 5c57478 
  src/slave/flags.cpp b5e2518 
  src/tests/docker_containerizer_tests.cpp 154bf98 
  src/tests/docker_tests.cpp 5520c58 

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


Testing
---

make check


Thanks,

Timothy Chen