Re: Review Request 53253: Fixed the bug when search base hierarchy in `cgroups_tests.cpp`.

[Jiang Yan Xu]

> On Oct. 31, 2016, 6:20 p.m., Jiang Yan Xu wrote:
> > We unfortunately have to copy the cleanup code from 
> > `ContainerizerTest::SetUp()` to here. FWIW the 
> > test fixture originally was written with co-mounted subsystems in a single 
> > hierarchy (hence *Any*), later altered when Mesos started to use separate 
> > hierarchies for each subsystem, but not in a clean way IMO.
> > 
> > The `CgroupsAnyHierarchyTest` fixture and its children AFAICT still works 
> > with one hierarchy with multiple co-mounted subsystems. In fact Mesos does 
> > support co-mounted subsystems.
> > 
> > So IMO it wouldn't be a bad idea to have tests in cgroups_tests.cpp to only 
> > **create** one hierarchy for all the specified subsystems (most tests 
> > assume one). Of course it's fine if the subsystems are already mounted in 
> > separate hierarchies but the tests only manages (create/cleanup) a single 
> > hierachy.
> > 
> > This would make fixes for related problems (e.g., MESOS-6422) cleaner as 
> > well.
> > 
> > Thoughts?
> 
> haosdent huang wrote:
> To be honest, I prefer to follow what Linux do now which mount subsystems 
> in different folders. Suppose the Linux have already mounted 
> `/sys/fs/cgroups/cpu`, `/sys/fs/cgroups/memory`. And in our test case, we 
> create `/sys/fs/cgroups/unified-mount` and mount rest subsystems(`devices`, 
> `net_cls`, `perf_event`) on it. Then it looks wried. If we choose to mount 
> them separately in our test cases, e.g, `/sys/fs/cgroups/devices`, 
> `/sys/fs/cgroups/perf_event` and etc. This is more consistent with current 
> operate system behaviour.
> 
> Co-mounted all hierarchies simplfy work if there is not any exist 
> subsystems mounted in current machine. But it would become tricky if some 
> subystems have been mounted. Anything I miss here?
> 
> Jiang Yan Xu wrote:
> Let me clarify:
> 
> 1. Mesos works with co-mounted subsystems: 
> https://github.com/apache/mesos/commit/31337348cef29719890bffb59fbf8df8b18b39d0
> 2. At the abstraction level of Mesos agent and Mesos containerizer, it's 
> OK if we **require** separately mounted subsystems in tests since it's what 
> we **currently** prefer.
> 3. However, at the abstraction level of linux/cgroups.cpp and 
> cgroups_tests.cpp, it's **OK** for the tests to **create** a single hierarchy 
> (so it only needs to cleanup one) but **allow** existing hierarhies (e.g., 
> `/sys/fs/cgroup/*`) with single subsystems because neither the logic in 
> linux/cgroups.cpp nor cgroups_tests.cpp require separately mounted 
> subsystems; that's above its abstraction level; linux/cgroups.cpp is just a 
> utility and as such doesn't have preferences on this.
> 
> The benefit is the simplicity in cleanups in cgroups_tests.cpp: you only 
> need to recursively destroy `TEST_CGROUPS_ROOT` and you clean up 
> `TEST_CGROUPS_HIERARCHY` (because it's always a hierarchy and never a base 
> hierarchy) and you are done. If `CgroupsAnyHierarchyTest` and its descendants 
> ever use existing hierarhies (e.g., `/sys/fs/cgroup/*`), easy, just destroy 
> `TEST_CGROUPS_ROOT`. It would be cleaner than copying the code over from 
> mesos.cpp and MESOS-6422 is fixed as well.
> 
> haosdent huang wrote:
> Suppose we have mount `cpu` and `memory` subsystems, and we choose the 
> single hierarchy way in our test cases.
> Then we still need to find out the base hierarchy first and use it to 
> create `TEST_CGROUPS_HIERARCHY` under that base hierarchy.
> Because we don't support multiple cgroups base hierarchies now. Is this 
> fine? If so, I would update the patch chain and fix MESOS-6422 together.

re "we don't support multiple cgroups base hierarchies now": As I mentioned in 
my comment above, Mesos works with co-mounted subsystems. To be more concrete, 
systemd [mounts cpu,cpuacct together in one 
hierarchy](https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Resource_Management_Guide/br-Resource_Controllers_in_Linux_Kernel.html)
 and Mesos supports it.

Now, even if `cpu,cpuacct` were the only legit scenario that Mesos supports 
co-mounted subsystems, it's still a good reason we don't rule them out in our 
tests, right?

re "we still need to find out the base hierarchy first": Why find the "base 
hierarchy"? Why don't we directly `cgroups::mount(TEST_CGROUPS_HIERARCHY)`?

The bottom line is that we don't need to artificially restrict the 
`CgroupsAnyHierarchyTest` to be one hierarchy per subsystem. After all, it's an 
"Any" (i.e., one) hierarchy test. The tests in the fixtures conform to this. 

If becomes necessary to have a `CgroupsMultipleHierarchiesTest`, then we could 
explicitly require and create multiple hierarchies there (which doesn't have to 
be a subclass of `CgroupsAnyHierarchyTest`). 

BTW, I am not suggesting that we change all the tests including the 
`ContainerizerTest::SetUp/TearDown`, these are tests at different abstraction 
levels.


- Jiang Yan

Re: Review Request 53511: Parameterized existing decoder tests on the type of decoder.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53481, 53482, 53483, 53484, 53485, 53486, 53487, 53488, 
53489, 53490, 53491, 53510, 53511]

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 Nov. 5, 2016, 1:32 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53511/
> ---
> 
> (Updated Nov. 5, 2016, 1:32 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to not duplicate tests for the streaming request
> decoder.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> 4535614312373b0515025f09f9f8495f9ce353a3 
> 
> Diff: https://reviews.apache.org/r/53511/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53509: Flag —master now required and parsed by MasterDetector.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [53509]

Failed command: ./support/apply-review.sh -n -r 53509

Error:
2016-11-05 04:26:27 URL:https://reviews.apache.org/r/53509/diff/raw/ 
[1539/1539] -> "53509.patch" [1]
Traceback (most recent call last):
  File "support/apply-reviews.py", line 342, in 
reviewboard()
  File "support/apply-reviews.py", line 321, in reviewboard
apply_review()
  File "support/apply-reviews.py", line 139, in apply_review
commit_patch()
  File "support/apply-reviews.py", line 180, in commit_patch
data = patch_data()
  File "support/apply-reviews.py", line 199, in patch_data
return reviewboard_data()
  File "support/apply-reviews.py", line 258, in reviewboard_data
review_url=url)
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2014' in position 
5: ordinal not in range(128)

Full log: https://builds.apache.org/job/mesos-reviewbot/15956/console

- Mesos ReviewBot


On Nov. 5, 2016, 12:51 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53509/
> ---
> 
> (Updated Nov. 5, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Till Toenshoff.
> 
> 
> Bugs: MESOS-2723
> https://issues.apache.org/jira/browse/MESOS-2723
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit makes the --master argument mandatory.
> The parsing of the argument now happens in src/master/detector.cpp.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b265bc6390ed8329a200408ef45512f900f9b999 
> 
> Diff: https://reviews.apache.org/r/53509/diff/
> 
> 
> Testing
> ---
> 
> Testing done by running a master with `--zk=zk://localhost:2181/mesos` then 
> an agent with `--master=zk://localhost:2181/mesos` and then executing a sleep 
> task on Mesos with `--master=zk://localhost:2181/mesos`.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 53354: Updated namespace isolators to customize based on 'ContainerClass'.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [53354, 53353, 53352, 53351, 53350]

Failed command: ./support/apply-review.sh -n -r 53353

Error:
2016-11-05 03:20:30 URL:https://reviews.apache.org/r/53353/diff/raw/ [397/397] 
-> "53353.patch" [1]
error: patch failed: include/mesos/slave/containerizer.proto:30
error: include/mesos/slave/containerizer.proto: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/15955/console

- Mesos ReviewBot


On Nov. 4, 2016, 9:34 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53354/
> ---
> 
> (Updated Nov. 4, 2016, 9:34 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6464
> https://issues.apache.org/jira/browse/MESOS-6464
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The namespace-related isolators now do different things depending on
> whether they are launching a "normal" nested container or a "debug"
> nested container. Normal nested containers clone a new mount namespace
> as well as a new pid namespace. Debug nested cotnainers do not -- they
> simply inherit these namespaces from their parent.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> af9f3736b487b595e8768e56ce60dc4823db28a1 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> df16b8fee6799a69c7d96f33a5049bd9787c48f5 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> a1283e5ee92c916baaf9fca8ce314d597e8421b3 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> e3756c920081f2944bf4b640edf0a83f42784586 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
> 0d9ec57d9aa83bcc6cc2e5a8d75f2e2251179b1b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 939142e36b926d9e4201d35dedd25e32e9f8c63c 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 48202fb5bf1ede71b80760844c6d8a36ca7c700c 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> 210e67ad0d84f52135e77184f21e574c9e31628d 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 7b976d29226c3e0a4d52922e9d2f7e685de72297 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 0305d14c1f791c93edcd3b32786b483b15f40a2d 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> e6c690c411f57138207044f31b4816bd4090c1b7 
> 
> Diff: https://reviews.apache.org/r/53354/diff/
> 
> 
> Testing
> ---
> 
> make -j check
> (Some tests are still fialing though -- need to debug)
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 53501: Added a test to catch regressions to MESOS-4975.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53501]

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 Nov. 4, 2016, 10:12 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53501/
> ---
> 
> (Updated Nov. 4, 2016, 10:12 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.
> 
> 
> Bugs: MESOS-4975
> https://issues.apache.org/jira/browse/MESOS-4975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to catch regressions to MESOS-4975.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 
> 
> Diff: https://reviews.apache.org/r/53501/diff/
> 
> 
> Testing
> ---
> 
> make check with this test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53485: Introduced a `io::read()` overload for reading from piped reader.

[Benjamin Mahler]

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



Nice commit summary! Would you mind adding a test alongside this? Should be 
pretty straightforward to do.


3rdparty/libprocess/include/process/io.hpp (line 100)


Can you remove the const& here given that you need non-const access and 
it's what you take in the actual `_read` implementation? As it stands the const 
may be a bit misleading.


- Benjamin Mahler


On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53485/
> ---
> 
> (Updated Nov. 4, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The helper reads from the pipe till EOF. This is used later to
> read BODY requests from the streaming request decoder.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/io.hpp 
> eec5efd7e6b71a783f2bb40826054d0488cee71f 
>   3rdparty/libprocess/src/io.cpp d930ceebc90468e082b984e41385549f906dc6ae 
> 
> Diff: https://reviews.apache.org/r/53485/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53484: Introduced `RouteOptions` to support streaming requests.

[Benjamin Mahler]

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


Fix it, then Ship it!




How about updating the summary to say "Introduced options for HTTP routes, 
initially for streaming requests."

In the ideal, we could just say "Introduced options for HTTP routes." which 
would mean we only add the struct in this patch and not the streaming bit. 
Then, in the patch where we support streaming we add the 'streaming' option to 
the struct here. This approach would be a bit cleaner for posterity when 
looking at the committed patches, but it's up to you.


3rdparty/libprocess/include/process/process.hpp (line 262)


I wonder if this should be 'streamingRequest' or Request::Type? As it 
stands I wonder if it some might misinterpret it as being related to streaming 
responses (since that is more common).

Up to you.



3rdparty/libprocess/include/process/process.hpp (lines 279 - 285)


How about using a default argument rather than an overload, since that 
expresses the intent: if you don't provide us with RouteOptions, we will give 
you defaults.



3rdparty/libprocess/include/process/process.hpp (line 485)


Since we define a notion of what the "default" options are, we can just 
remove the Option<> here? Seems a bit simpler to reason about if there are 
always options, since "no options" and "default options" are equivalent AFAICT.


- Benjamin Mahler


On Nov. 5, 2016, 1:33 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53484/
> ---
> 
> (Updated Nov. 5, 2016, 1:33 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows routes to specify configuration options. Currently, it
> only has one member `streaming` i.e, if the route supports request
> streaming. This also enables us to add more options in the future
> without polluting overloads.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> f389d3d3b671e301a7ac911ad87ab13289e8c82f 
>   3rdparty/libprocess/src/process.cpp 
> ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
> 
> Diff: https://reviews.apache.org/r/53484/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53484: Introduced `RouteOptions` to support streaming requests.

[Anand Mazumdar]

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

(Updated Nov. 5, 2016, 1:33 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Updated desc


Summary (updated)
-

Introduced `RouteOptions` to support streaming requests.


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


Repository: mesos


Description
---

This allows routes to specify configuration options. Currently, it
only has one member `streaming` i.e, if the route supports request
streaming. This also enables us to add more options in the future
without polluting overloads.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
f389d3d3b671e301a7ac911ad87ab13289e8c82f 
  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53511: Parameterized existing decoder tests on the type of decoder.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This allows us to not duplicate tests for the streaming request
decoder.


Diffs
-

  3rdparty/libprocess/src/tests/decoder_tests.cpp 
4535614312373b0515025f09f9f8495f9ce353a3 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53510: Removed extraneous socket argument from `DataDecoder` constructor.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This argument is not used anywhere in the code. This makes it
consistent with the streaming request decoder.


Diffs
-

  3rdparty/libprocess/src/decoder.hpp 76dca0b272af8591880ef220ec2dc006906fbc36 

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


Testing
---


Thanks,

Anand Mazumdar

Re: Review Request 53482: Initialized the POD type in the `Request` struct.

[Benjamin Mahler]

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



Actually whoops, we rely on the generated constructor, so this doesn't seem 
like an issue?

- Benjamin Mahler


On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53482/
> ---
> 
> (Updated Nov. 4, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the `keepAlive` member was not initialized correctly,
> the behavior is undefined if POD types are not correctly
> initialized.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 9722c6210526479da243ae9945e4c9a89ecb9009 
> 
> Diff: https://reviews.apache.org/r/53482/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53483: Added members type/reader to `Request`.

[Benjamin Mahler]

> On Nov. 5, 2016, 12:54 a.m., Benjamin Mahler wrote:
> >

Also can you clarify the commit summary for those that don't have context?

For example: "Introduced a PIPE-type http::Request to support streaming 
requests."

With this I can tell what the intention is.


- Benjamin


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


On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53483/
> ---
> 
> (Updated Nov. 4, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These new members are needed for supporting request streaming i.e.,
> the caller can use the writer to stream chunks to the server if
> the request body is not known in advance.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 9722c6210526479da243ae9945e4c9a89ecb9009 
> 
> Diff: https://reviews.apache.org/r/53483/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53483: Added members type/reader to `Request`.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/http.hpp (lines 445 - 448)


Note that when I wrote the `http::Response` documentation, I hadn't yet 
added `http::Connection` which means I only documented the server-side 
semantics. I should have added the client-side semantics to the documentation 
when I added `http::Connection`.

Given that we now have `http::Connection` and want to document both client 
and server side usages of `http::Request` and `http::Response`, how about the 
following?

```
  // Clients can choose to provide the entire body at once
  // via BODY or can choose to stream the body over to the
  // server via PIPE.
  //
  // On the server-side, Processes setting up routes can specify
  // whether they want to process the incoming request once it is
  // complete or in a streaming manner (see `ProcessBase::route`
  // for handler options).
  enum
  {
BODY,
PIPE
  } type;
```

Ideally we should commit the client-side of the documentation in this 
patch, and add the server side of the documentation in whichever of your 
subsequent patches adds the route options.



3rdparty/libprocess/include/process/http.hpp (lines 449 - 453)


How about we move the type related variables right below type, much like we 
did this http::Response? This way it's clearer for the consumer to see how 
these relate:

```
  enum
  {
BODY,
PIPE
  } type;
  
  std::string body;
  Option reader;
```


- Benjamin Mahler


On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53483/
> ---
> 
> (Updated Nov. 4, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These new members are needed for supporting request streaming i.e.,
> the caller can use the writer to stream chunks to the server if
> the request body is not known in advance.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 9722c6210526479da243ae9945e4c9a89ecb9009 
> 
> Diff: https://reviews.apache.org/r/53483/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53473: Add new param user to logrotate's prepare function.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52308, 52310, 53473]

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 Nov. 4, 2016, 2:58 p.m., Sivaram Kannan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53473/
> ---
> 
> (Updated Nov. 4, 2016, 2:58 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5856
> https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add new param user to logrotate's prepare function.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> 939974736f9eb744c83036e074718d2a1eba8b0a 
>   src/slave/container_loggers/lib_logrotate.hpp 
> 28fdf3bdcc66d473521b377f66ab0b48f6900f58 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 53698d339f0f4d2dc916b53239ca0c36bbebcd42 
>   src/slave/container_loggers/logrotate.hpp 
> d1db69236f5a9b1dbb3113ad02218a512afdb46b 
>   src/slave/container_loggers/sandbox.hpp 
> e0aeb32a9ec83af049af8a10010b819c1d8b25d8 
>   src/slave/container_loggers/sandbox.cpp 
> cc263ebef7e0c3e778fabafa49faa6dd315adc45 
>   src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/tests/container_logger_tests.cpp 
> 1bb94a8461e481983f25a44737e4011ed5fc4b1f 
> 
> Diff: https://reviews.apache.org/r/53473/diff/
> 
> 
> Testing
> ---
> 
> Run the mesos-logrotate-logger with un-priviledged user and verify whether 
> the logs are getting rotated.
> Run the mesos-logrotate-logger as root user and verify whether the logs are 
> getting rotated.
> 
> 
> Thanks,
> 
> Sivaram Kannan
> 
>

Re: Review Request 53509: Flag —master now required and parsed by MasterDetector.

[Armand Grillet]

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

(Updated Nov. 5, 2016, 12:51 a.m.)


Review request for mesos, Joseph Wu and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

This commit makes the --master argument mandatory.
The parsing of the argument now happens in src/master/detector.cpp.


Diffs
-

  src/cli/execute.cpp b265bc6390ed8329a200408ef45512f900f9b999 

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


Testing
---

Testing done by running a master with `--zk=zk://localhost:2181/mesos` then an 
agent with `--master=zk://localhost:2181/mesos` and then executing a sleep task 
on Mesos with `--master=zk://localhost:2181/mesos`.


Thanks,

Armand Grillet

Review Request 53509: Flag —master now required and parsed by MasterDetector.

[Armand Grillet]

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

Review request for mesos.


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


Repository: mesos


Description
---

This commit makes the —master argument mandatory.
The parsing of the argument now happens in src/master/detector.cpp.


Diffs
-

  src/cli/execute.cpp b265bc6390ed8329a200408ef45512f900f9b999 

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


Testing
---

Testing done by running a master with `--zk=zk://localhost:2181/mesos` then an 
agent with `--master=zk://localhost:2181/mesos` and then executing a sleep task 
on Mesos with `--master=zk://localhost:2181/mesos`.


Thanks,

Armand Grillet

Re: Review Request 53482: Initialized the POD type in the `Request` struct.

[Benjamin Mahler]

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


Ship it!





3rdparty/libprocess/include/process/http.hpp (lines 434 - 437)


Maybe something more like an additional TODO:

```
// TODO(anand): Ideally this could default to 'true' since
// persistent connections are the default since HTTP 1.1.
// Perhaps we need to go from `keepAlive` to `closeConnection`
// to reflect the header more accurately, and to have an
// intuitive default of false.
```

I noticed go stores this variable as `Close` which means the intuitive 
default (false) makes sense.


- Benjamin Mahler


On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53482/
> ---
> 
> (Updated Nov. 4, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the `keepAlive` member was not initialized correctly,
> the behavior is undefined if POD types are not correctly
> initialized.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 9722c6210526479da243ae9945e4c9a89ecb9009 
> 
> Diff: https://reviews.apache.org/r/53482/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53481: Moved around the `Request` struct in the file.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Nov. 4, 2016, 5:55 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53481/
> ---
> 
> (Updated Nov. 4, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This serves two purposes:
> - This brings it near the `Response` struct thereby making it easy
> to switch between them for readability.
> - In the future, `Request` would have a `Pipe::Reader` member
> for supporting request streaming. Otherwise, we would need to
> add another forward declaration and forward declaring nested
> structs is not quite possible.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 9722c6210526479da243ae9945e4c9a89ecb9009 
> 
> Diff: https://reviews.apache.org/r/53481/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53239: Changed master to make use of "retired" agent IDs.

[Vinod Kone]

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




src/master/master.cpp (lines 5144 - 5148)


If the slave thinks an ID needs to be retired, I think it makes sense for 
the master to send a shutdown to that slave and remove it? If we just ignore it 
how is the situation going to resolve?



src/master/master.cpp (line 5165)


why are we marking it unreachable instead of removing it directly?



src/master/master.cpp (line 5170)


Retirement



src/master/master.cpp (line 5187)


what's the plan for including old agent ids when the agent doesn't reboot? 
also, should we rename it to "old_slave_ids" instead of "retired_slave_ids" ?


- Vinod Kone


On Oct. 27, 2016, 10:46 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53239/
> ---
> 
> (Updated Oct. 27, 2016, 10:46 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5396
> https://issues.apache.org/jira/browse/MESOS-5396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A retired agent ID will never attempt to re-register in the future;
> moreover, any tasks/executors being managed by that agent ID are no
> longer running. We can take advantage of this knowledge to avoid waiting
> for `agent_reregister_timeout` to expire after master failover.
> 
> This is particularly important when agent removal rate-limiting is in
> use: if a power failure causes the master to fail at the same time that
> many agent hosts lose power, when power returns the master will failover
> and all the agents will register anew and receive new agent IDs. With
> agent removal rate-limiting, it may take a long time for the master to
> mark all the old agent IDs as unreachable; in the meantime, explicit
> reconciliation will not return any results, potentially leaving
> frameworks in limbo for an extended period.
> 
> Note that we currently mark retired agents as unreachable; in the near
> future, that will change to marking such agents "gone", once support for
> that feature is completed.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 87186c6e733a686f96528b1722fda1c287e9c881 
>   src/master/master.cpp 8692726d21812827f9e1fd9093d80fd260588ecb 
>   src/tests/slave_recovery_tests.cpp 65fc18bc2732dc53581d39ee23368e347f0b2ca4 
> 
> Diff: https://reviews.apache.org/r/53239/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> NOTE: Current implementation reuses `Master::markUnreachableAfterFailover`, 
> which means we emit misleading log messages and increment the wrong metrics. 
> Will adjust based on initial review comments.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53238: Changed agent to report "retired" agent IDs on registration.

[Vinod Kone]

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




src/slave/slave.cpp (lines 1373 - 1375)


we don't do this anywhere else, can we just do?

```
foreach(const SlaveID& slaveId, retiredSlaveIds) {
  message.add_retired_slave_ids()->CopyFrom(slaveId);
}

```



src/slave/state.cpp (line 58)


s/findAllSlaveIDs/recoverSlaveIDs/


- Vinod Kone


On Oct. 27, 2016, 6:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53238/
> ---
> 
> (Updated Oct. 27, 2016, 6:40 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5396
> https://issues.apache.org/jira/browse/MESOS-5396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the agent observes that the boot ID has changed since the last time
> it successfully (re-)registered with the master, we know that any agent
> IDs that were previously in use in this `work_dir` must be associated
> with different boot IDs; hence, any executors/tasks associated with
> those agent IDs are no longer running, and those agent ID will never
> attempt to re-register with the master. The agent now reports these
> retired agent IDs to the master; the master will later take advantage of
> this information, e.g., to learn that an agent that we think might
> re-register after master failover will never re-register.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 7cbac56099689ffc378d83bae3a16abb49b50dd9 
>   src/slave/slave.hpp c0a17657e684e87d555731d2e35ac15894c3c05b 
>   src/slave/slave.cpp 881c10ac61d1e4fdeabdc42c0a41508c36f49040 
>   src/slave/state.hpp a1c849690a5c2b3fc5ea3eb2e782e99a2b0f8044 
>   src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 
> 
> Diff: https://reviews.apache.org/r/53238/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52735: Removed TODO message for docker killing.

[Guangya Liu]

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



Can you make this as the first patch for the patch chain?


src/slave/containerizer/docker.cpp 


Instead of killing this directly, can you please update the comments a bit 
and move it right before #2213

```
// We've failed to do a Docker::kill, which means that the
// container is still running! Here we invoke `Self::remove`
// after `docker_remove_delay` to force remove the docker
// container again.
```


- Guangya Liu


On 十一月 2, 2016, 7:29 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52735/
> ---
> 
> (Updated 十一月 2, 2016, 7:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed TODO message for docker killing.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp f720320e112687e8ae972f04159b3b4cb7a58476 
> 
> Diff: https://reviews.apache.org/r/52735/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 53237: Improved slave recovery tests to check reconciliation, metrics.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/slave_recovery_tests.cpp (line 2886)


why? can you add more info for posterity?


- Vinod Kone


On Oct. 27, 2016, 6:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53237/
> ---
> 
> (Updated Oct. 27, 2016, 6:40 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5396
> https://issues.apache.org/jira/browse/MESOS-5396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved slave recovery tests to check reconciliation, metrics.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 65fc18bc2732dc53581d39ee23368e347f0b2ca4 
> 
> Diff: https://reviews.apache.org/r/53237/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53236: Cleaned up Master::markUnreachableAfterFailover.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 27, 2016, 6:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53236/
> ---
> 
> (Updated Oct. 27, 2016, 6:40 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5396
> https://issues.apache.org/jira/browse/MESOS-5396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up Master::markUnreachableAfterFailover.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 87186c6e733a686f96528b1722fda1c287e9c881 
>   src/master/master.cpp 23ddb995b4ad0fcdb589974308a2e81ececdad31 
> 
> Diff: https://reviews.apache.org/r/53236/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53466: Added test to verify container status for nested containers.

[Jie Yu]

> On Nov. 4, 2016, 5:53 p.m., Kevin Klues wrote:
> > src/tests/mesos.hpp, lines 1867-1875
> > 
> >
> > This seems like a logically different change that should have its own 
> > commit message explaining why this is changed this way.

Yup, i'll separate it and explain why.


- Jie


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


On Nov. 4, 2016, 7:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53466/
> ---
> 
> (Updated Nov. 4, 2016, 7:08 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is used to verify that container status is properly set in
> task status update for nested containers.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> db953d39a95e7e95871054114a0a9bbeded46224 
>   src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 
> 
> Diff: https://reviews.apache.org/r/53466/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53466: Added test to verify container status for nested containers.

[Jie Yu]

> On Nov. 4, 2016, 5:53 p.m., Kevin Klues wrote:
> > src/tests/mesos.hpp, line 365
> > 
> >
> > Why is this pulled in if it's now used anywhere? Does some of the macro 
> > magic below actually reference this type?

It's used in the test because we want to do v1::ContainerStatus in the test and 
this is necessary to disambguate with internal::tests::v1 namespace.


- Jie


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


On Nov. 4, 2016, 7:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53466/
> ---
> 
> (Updated Nov. 4, 2016, 7:08 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is used to verify that container status is properly set in
> task status update for nested containers.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> db953d39a95e7e95871054114a0a9bbeded46224 
>   src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 
> 
> Diff: https://reviews.apache.org/r/53466/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53235: Cleaned up header includes and `using`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 27, 2016, 6:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53235/
> ---
> 
> (Updated Oct. 27, 2016, 6:40 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5396
> https://issues.apache.org/jira/browse/MESOS-5396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up header includes and `using`.
> 
> 
> Diffs
> -
> 
>   src/slave/state.hpp a1c849690a5c2b3fc5ea3eb2e782e99a2b0f8044 
>   src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 
> 
> Diff: https://reviews.apache.org/r/53235/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53234: Logged warning message if reading previous boot ID fails.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 27, 2016, 6:39 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53234/
> ---
> 
> (Updated Oct. 27, 2016, 6:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5396
> https://issues.apache.org/jira/browse/MESOS-5396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Not much we can do here, but as a general rule, we should never silently
> ignore I/O errors.
> 
> 
> Diffs
> -
> 
>   src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 
> 
> Diff: https://reviews.apache.org/r/53234/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Tested manually by doing `chmod 000` of the boot-id file.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53465: Fixed the container status for nested containers.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53465/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, the ContainerStatus inside the TaskStatus for a
> task in the default executor is not correct. The agent always gets the
> container status of the top level container, even if the task might
> correspond to a nested container.
> 
> This patch fixed this issue by letting the executor set the container
> ID for the task (because only it has the knowledge about this
> mapping), and the agent will use that container ID to get the proper
> container status.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp b8a88ad6e361720dc7d02c9423db3e67226f779f 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 2f6723c64261fb3295626d6479fe844fb23b0650 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 939142e36b926d9e4201d35dedd25e32e9f8c63c 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> Diff: https://reviews.apache.org/r/53465/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53233: Simplified `State::recover` interface in the agent.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 27, 2016, 6:39 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53233/
> ---
> 
> (Updated Oct. 27, 2016, 6:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5396
> https://issues.apache.org/jira/browse/MESOS-5396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `State::recover` previously returned `Result`, which implied
> three possibilities: Error, None (if the `work_dir` doesn't exist), or a
> `State` value. However, the agent code treated the latter two cases the
> same way: if the `work_dir` doesn't exist, it is simpler to represent
> this case by returning a `State` value whose fields are `None()`.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp c0a17657e684e87d555731d2e35ac15894c3c05b 
>   src/slave/slave.cpp 881c10ac61d1e4fdeabdc42c0a41508c36f49040 
>   src/slave/state.hpp a1c849690a5c2b3fc5ea3eb2e782e99a2b0f8044 
>   src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 
> 
> Diff: https://reviews.apache.org/r/53233/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53464: Added ContainerID to ContainerStatus.

[Jie Yu]

> On Nov. 4, 2016, 5:40 p.m., Kevin Klues wrote:
> > include/mesos/mesos.proto, line 2193
> > 
> >
> > Why is this the first element in the message? I would think since we 
> > already have others taht are ordered 1,2,3, we would put this at the bottom 
> > with 4.
> > 
> > Is it so that we keep the "ContainerID" of the "ContainerStatus" at the 
> > top? If so, that makes sense to me.

We don't sort according to tag number typically. `id` sounds to me should be 
logically the first entry.


- Jie


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


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53464/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ContainerID to ContainerStatus.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 905a34bb13c2fcd1b22ef4e9605405ef84711c25 
>   include/mesos/v1/mesos.proto b72069a6caff0b163d9d9cae45c3caa0b25de38f 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53464/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53065: Removed unused function `paths::getArchiveDir`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 20, 2016, 3:38 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53065/
> ---
> 
> (Updated Oct. 20, 2016, 3:38 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unused function `paths::getArchiveDir`.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp ebff3a9fab3f4a8d38502fe02bd5a34cf95c1830 
>   src/slave/paths.cpp eb414caae5847949854743d4e3c603217a897898 
>   src/tests/paths_tests.cpp 0671ee25b484cacf08c9a20ee6eba88e6f14fa97 
> 
> Diff: https://reviews.apache.org/r/53065/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53354: Updated namespace isolators to customize based on 'ContainerClass'.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 267 - 
282)


This is not necessary for now because docker volume isolator is not nesting 
aware yet. So this method won't be called for a debug container.

I'll simply remove it for now. When we add support for nesting to Docker 
volume isolator, we'll address this in an atomic patch.



src/slave/containerizer/mesos/isolators/filesystem/shared.cpp (lines 81 - 96)


Ditto on removing this. Shared filesystem isolator is not nesting aware. We 
also plan to just remove this isolator in favor of using linux filesystem 
isolator.



src/slave/containerizer/mesos/isolators/gpu/isolator.cpp (line 286)


What if `!containerConfig.has_container_class` (meaning using the default), 
do you need to short cut by `return _prepare(...);`?



src/slave/containerizer/mesos/isolators/namespaces/pid.cpp (line 88)


You want to enter the MNT namespace as well, right? Because otherwise, the 
/proc might not be for the host.



src/slave/containerizer/mesos/isolators/namespaces/pid.cpp (lines 90 - 98)


I'd prefer the following logic for now. Let's add switch later once we have 
another type. It's unknown to me if we'll follow the same pattern here.
```
if (containerConfig.has_container_class() &&
containerConfig.container_class() == ContainerClass::DEBUG) {
  return launchInfo;
}
```



src/slave/containerizer/mesos/isolators/namespaces/pid.cpp (line 91)


Want to remove this?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 614 - 618)


I'll also add a NOTE saying that if the parent container has a rootfs, the 
filesystem/linux isolator will properly set the namespace to enter (MNT 
namespace). If the parent does not have a rootfs, since it joins host network, 
no namespace needs to enter.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 620 - 627)


For DEBUG containers, we don't need to create 'info' because we don't need 
to prepare /etc/ network files for the container.

The reason we create 'info' for normal nested container is because we need 
to setup network files later in isolate().



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 664)


I would add a NOTE saying that for debug class, we need to enter the mount 
namespace of the parent container as well. We rely on filesystem/linux isolator 
to do that.

Alternatively, we can make it explicit here.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 672 - 678)


I prefer:
```
if (containerConfig.has_container_class() &&
containerConfig.container_class() == ContainerClass::DEBUG) {
  launchInfo.set_enter_namespaces(CLONE_NEWNS);
} else {
  launchInfo.set_clone_namespaces(CLONE_NEWNS);
}
```



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 2517 - 
2533)


No need for this. port mapping isolator is not nesting aware.



src/slave/containerizer/mesos/isolators/volume/image.cpp (lines 91 - 101)


I'd prefer:
```
if (containerConfig.has_container_class() &&
containerConfig.container_class() == ContainerClass::DEBUG) {
  ContainerLaunchInfo launchInfo;
  launchInfo.set_enter_namespaces(CLONE_NEWNS);
  return launchInfo;
}
```



src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp (lines 130 - 
140)


I'd actually tweek the logics here. You want to short circut for the case 
where bindMount is not supported as well as volume is not supported for DEBUG 
containers.

```
if (containerId.has_parent() &&
containerConfig.has_container_class() &&
containerConfig.container_class() == ContainerClass::DEBUG) {
  if (bindMountSupported) {
launchInfo.set_enter_namespaces(CLONE_NEWNS);
  }
  
  // No need to proceed because volumes are not supported
  // for DEBUG container currently.
  return launchInfo;
}
  
if (bindMountSupported) {
  launchInfo.set_clone_namespaces(C

Re: Review Request 53451: Added agent API protos for executing and attaching nested containers.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 4, 2016, 10:07 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53451/
> ---
> 
> (Updated Nov. 4, 2016, 10:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-6525
> https://issues.apache.org/jira/browse/MESOS-6525
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch only contains the proto definitions. The implementation is
> not yet present.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 0e9e071d1b97ba41c1636e41f242d564998e8f0b 
>   include/mesos/v1/agent/agent.proto c2f282c05d15ae1fa099cd26ceb66d41e52b1d95 
>   src/slave/http.cpp a32aca437cffd52bc2bcde859eedddca2038e3f1 
>   src/slave/validation.cpp 974bf24f1e139e409cc4b6363b646ff50d57df1e 
> 
> Diff: https://reviews.apache.org/r/53451/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53269: Avoided needless copy inside `foreach` loop.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Oct. 31, 2016, 5:45 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53269/
> ---
> 
> (Updated Oct. 31, 2016, 5:45 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided needless copy inside `foreach` loop.
> 
> 
> Diffs
> -
> 
>   src/master/registrar.cpp 529013fb54cf0c007467425ece4c4d8c7b9f83a9 
> 
> Diff: https://reviews.apache.org/r/53269/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 53501: Added a test to catch regressions to MESOS-4975.

[Jiang Yan Xu]

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

Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.


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


Repository: mesos


Description
---

Added a test to catch regressions to MESOS-4975.


Diffs
-

  src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 

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


Testing
---

make check with this test.


Thanks,

Jiang Yan Xu

Re: Review Request 53451: Added agent API protos for executing and attaching nested containers.

[Vinod Kone]

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

(Updated Nov. 4, 2016, 10:07 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Jie Yu, and Kevin 
Klues.


Changes
---

kevin's comments.


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


Repository: mesos


Description
---

This patch only contains the proto definitions. The implementation is
not yet present.


Diffs (updated)
-

  include/mesos/agent/agent.proto 0e9e071d1b97ba41c1636e41f242d564998e8f0b 
  include/mesos/v1/agent/agent.proto c2f282c05d15ae1fa099cd26ceb66d41e52b1d95 
  src/slave/http.cpp a32aca437cffd52bc2bcde859eedddca2038e3f1 
  src/slave/validation.cpp 974bf24f1e139e409cc4b6363b646ff50d57df1e 

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


Testing
---

make


Thanks,

Vinod Kone

Re: Review Request 53491: WIP Disabled tests relying on filtering HTTP events.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53481, 53482, 53483, 53484, 53485, 53486, 53487, 53488, 
53489, 53490, 53491]

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 Nov. 4, 2016, 5:54 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53491/
> ---
> 
> (Updated Nov. 4, 2016, 5:54 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
> https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a placeholder patch to make review bot happy. Some tests
> that rely on filtering HTTP events based on type won't work now
> since the request body is not yet known when `visit()` is invoked.
> I am working on fixing these.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 
>   src/tests/slave_recovery_tests.cpp cc50498040986e9aca1031df3622b13e7a44218a 
> 
> Diff: https://reviews.apache.org/r/53491/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53499: Add ceph framework to the list.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Nov. 4, 2016, 9:36 p.m., Tim Harper wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53499/
> ---
> 
> (Updated Nov. 4, 2016, 9:36 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ceph framework to the list.
> 
> 
> Diffs
> -
> 
>   docs/frameworks.md d8a0ac93c11b689b67ba99a9c3a9bfe01a51db77 
> 
> Diff: https://reviews.apache.org/r/53499/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tim Harper
> 
>

Re: Review Request 53435: Fixed header include order.

[Joseph Wu]

> On Nov. 4, 2016, 9:06 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/docker_volume_isolator_tests.cpp, lines 36-37
> > 
> >
> > That's what I understood, but I made an error in my comment: in that 
> > ordering it should be at the same level as `runtime.hpp` since both of 
> > these a 6 levels deep. Currently `linux.hpp` is preceeded by headers 7 
> > levels deep.
> > 
> > I seems this ruleis rather complicated in pratice.
> 
> Neil Conway wrote:
> Yeah, there are a lot of corner cases. Personally I think this isn't 
> worth devoting a lot of attention to in the absence of tool support.
> 
> I'd personally vote for (a) coming up with a reasonable sort order (b) 
> once we get consensus, add that to the style guide (c) writing a clang-tidy 
> check that implements the sort order.

This order (alphabetical, with extra newlines between different nesting levels) 
is reasonable enough... 

But a counter example is how we sort `stout` headers (by nesting level first, 
then alphabetical):
```
#include 
#include 
#include 
#include 

#include 
#include 
#include 
```


- Joseph


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


On Nov. 4, 2016, 8:15 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53435/
> ---
> 
> (Updated Nov. 4, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed header include order.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53435/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 53499: Add ceph framework to the list.

[Tim Harper]

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

Review request for mesos.


Repository: mesos


Description
---

Add ceph framework to the list.


Diffs
-

  docs/frameworks.md d8a0ac93c11b689b67ba99a9c3a9bfe01a51db77 

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


Testing
---


Thanks,

Tim Harper

Re: Review Request 53354: Updated namespace isolators to customize based on 'ContainerClass'.

[Kevin Klues]

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

(Updated Nov. 4, 2016, 9:34 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased with changes in previous patches.


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


Repository: mesos


Description
---

The namespace-related isolators now do different things depending on
whether they are launching a "normal" nested container or a "debug"
nested container. Normal nested containers clone a new mount namespace
as well as a new pid namespace. Debug nested cotnainers do not -- they
simply inherit these namespaces from their parent.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
af9f3736b487b595e8768e56ce60dc4823db28a1 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
df16b8fee6799a69c7d96f33a5049bd9787c48f5 
  src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
a1283e5ee92c916baaf9fca8ce314d597e8421b3 
  src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
e3756c920081f2944bf4b640edf0a83f42784586 
  src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
0d9ec57d9aa83bcc6cc2e5a8d75f2e2251179b1b 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
939142e36b926d9e4201d35dedd25e32e9f8c63c 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
48202fb5bf1ede71b80760844c6d8a36ca7c700c 
  src/slave/containerizer/mesos/isolators/volume/image.cpp 
210e67ad0d84f52135e77184f21e574c9e31628d 
  src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
7b976d29226c3e0a4d52922e9d2f7e685de72297 
  src/slave/containerizer/mesos/linux_launcher.cpp 
0305d14c1f791c93edcd3b32786b483b15f40a2d 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
e6c690c411f57138207044f31b4816bd4090c1b7 

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


Testing
---

make -j check
(Some tests are still fialing though -- need to debug)


Thanks,

Kevin Klues

Re: Review Request 53351: Added 'ContainerClass' to help decide how best to launch a container.

[Kevin Klues]

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

(Updated Nov. 4, 2016, 9:32 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated to address Jie's comments.


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


Repository: mesos


Description
---

A 'ContainerClass' must now be passed to all *nested*
containerizer->launch() calls in order to specify the class of
container being launched. For now, we simply have the default nested
container class (called 'NESTED') in a subsequent commit we will
introduce the 'DEBUG' class to help classify debug-based nested
containers that should be launched slightly differently than default
nested containers.

The 'ContainerClass' specified is passed through to each isolator in
'ContainerConfig' so they have a chance to custiomize their isolation
policies based on the class as well.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
f4c4ad771b5dead4ea3ee7cd1b4383c4dc2359b4 
  src/slave/containerizer/composing.hpp 
68c0523bb96e1078bd95c6094bce04c38c2fc9dd 
  src/slave/containerizer/composing.cpp 
0c5489f8dffaf75bce26a042000eaf7376f05ff4 
  src/slave/containerizer/containerizer.hpp 
7554446ac51a9063bd52b8b99845c73650740849 
  src/slave/containerizer/mesos/containerizer.hpp 
c4fea8b56c39d5a363f6ea80bd109fd2d3db52d9 
  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/slave/http.cpp a32aca437cffd52bc2bcde859eedddca2038e3f1 
  src/tests/api_tests.cpp 57981f12d429614a537733ac4cec9c0e72ff6d6f 
  src/tests/containerizer.hpp 080b8fa695d4c0dd2ed2bf028f1fc617dad23943 
  src/tests/containerizer.cpp c3bcb85d245a0e95b377059802cd89617eb5e25c 

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


Testing
---

make -j check


Thanks,

Kevin Klues

Re: Review Request 53352: Updated 'LinuxLauncher->fork()` with *enter* and *clone* namespaces.

[Kevin Klues]

> On Nov. 2, 2016, 9:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1223-1230
> > 
> >
> > This is pretty much `ContainerLaunchInfo`. I wanted to cleanup this 
> > part for quite a long time. Let's try to clean this up in a follow up patch 
> > maybe.

Makes sense, I filed a ticket.


- Kevin


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


On Nov. 1, 2016, 10:28 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53352/
> ---
> 
> (Updated Nov. 1, 2016, 10:28 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6464
> https://issues.apache.org/jira/browse/MESOS-6464
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of only taking the namespaces to *clone* inside a newly forked
> container, 'LinuxLauncher->fork()' now takes a list of namespaces to
> enter inside a parent container before forking. When both an *enter*
> and a *clone* for the same namespace are passed, we will first enter
> the namespace of the parent, and then clone a new one.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> f4c4ad771b5dead4ea3ee7cd1b4383c4dc2359b4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> af9f3736b487b595e8768e56ce60dc4823db28a1 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> df16b8fee6799a69c7d96f33a5049bd9787c48f5 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> a1283e5ee92c916baaf9fca8ce314d597e8421b3 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> e3756c920081f2944bf4b640edf0a83f42784586 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
> 0d9ec57d9aa83bcc6cc2e5a8d75f2e2251179b1b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 939142e36b926d9e4201d35dedd25e32e9f8c63c 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 48202fb5bf1ede71b80760844c6d8a36ca7c700c 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> 210e67ad0d84f52135e77184f21e574c9e31628d 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 7b976d29226c3e0a4d52922e9d2f7e685de72297 
>   src/slave/containerizer/mesos/launcher.hpp 
> 6ceb02de5dc143e545e2fec43e2608916e46b898 
>   src/slave/containerizer/mesos/launcher.cpp 
> b45313fd717f9553ccb0cbe9e8ac095e2536944a 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> d2353055a838c872d5852982cfede8e38c6e8701 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 0305d14c1f791c93edcd3b32786b483b15f40a2d 
>   src/tests/containerizer/isolator_tests.cpp 
> 8fefeef8c83ed2ab01f56a1ec572d3acb307143c 
>   src/tests/containerizer/launcher.hpp 
> 773b458f19e11b219c3f13a43f2b751a4bbe7b85 
>   src/tests/containerizer/launcher.cpp 
> a92d9890f0931425d69ef8ce0896d081b8722079 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
>   src/tests/containerizer/port_mapping_tests.cpp 
> fbdc0db9238c85d2f6eaba7d13ee5ce23342b527 
> 
> Diff: https://reviews.apache.org/r/53352/diff/
> 
> 
> Testing
> ---
> 
> make -j check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

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

Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Used an environment variable to pass command environment.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/slave/containerizer/mesos/launch.hpp 
8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
  src/slave/containerizer/mesos/launch.cpp 
377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
  src/slave/containerizer/mesos/main.cpp 
1a0e765ddb6d8519426b8d47067efdfa3432e07a 

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


Testing
---

make check.

No new tests are written but the fact the existing tests that run exectuors 
depends on this to work correctly is a proof.


Thanks,

Jiang Yan Xu

Re: Review Request 53387: Avoided memory leaks in exception/error paths in tests.

[Joseph Wu]

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


Ship it!




LGTM :)

- Joseph Wu


On Nov. 3, 2016, 9:23 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53387/
> ---
> 
> (Updated Nov. 3, 2016, 9:23 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `clang-tidy` rightly points out that certain `ASSERT` failures can
> result in leaking some heap-allocated values that aren't wrapped in a
> smart pointer. This commit fixes the cases that `clang-tidy` complains
> about by wrapping the values in `Owned`.
> 
> Note that there are many other places in the tests that leak resources
> if an exception occurs. The proper fix is usually to use a smart pointer
> rather than a raw pointer; several of these APIs should be changed to
> return Owned, for example. However, this is not always easy/clean, in
> part because of limitations of the current `Owned` and `Shared`
> types (e.g., MESOS-5122, MESOS-6496). So for now, just fix the cases
> that clang-tidy complains about and a few other similar instances.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53387/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Verified that observed `clang-tidy` warnings go away with this change.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53386: Fixed memory leak in JVM code.

[Joseph Wu]

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


Ship it!




LGTM!

- Joseph Wu


On Nov. 3, 2016, 8:31 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53386/
> ---
> 
> (Updated Nov. 3, 2016, 8:31 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed memory leak in JVM code.
> 
> 
> Diffs
> -
> 
>   src/jvm/jvm.cpp 62f7857180162c510269b5d10f5add94ab354fa2 
> 
> Diff: https://reviews.apache.org/r/53386/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Verified that observed `clang-tidy` leak warning goes away with this change.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53450: Added TTYInfo to ContainerInfo.

[Kevin Klues]

> On Nov. 4, 2016, 8:16 p.m., Kevin Klues wrote:
> > include/mesos/mesos.proto, line 2115
> > 
> >
> > I see you changed s/TtyInfo/TTYInfo/
> > 
> > Is this intentional? I know @bmahler has lamented in the past about 
> > decisions like this. (e.g. `HTTPScheduler` he would prefer to be 
> > `HttpScheduler`).
> 
> Vinod Kone wrote:
> I might be missing context but why do we prefer HttpScheduler to 
> HTTPScheduler? HTTP is an abbrevation, so all caps seems better to me. Note 
> that we have TCPCheckInfo, HTTPCheckInfo, URI, URL, etc protos already.

You'd have to ask @bmahler his thoughts on this. I just know it came up a few 
times during my GPU reviews. For consistency though (since so many other 
protobufs are already written like this), I think it's probably better to leave 
it as ALL CAPS.


- Kevin


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


On Nov. 4, 2016, 12:35 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53450/
> ---
> 
> (Updated Nov. 4, 2016, 12:35 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-6525
> https://issues.apache.org/jira/browse/MESOS-6525
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added to ContainerInfo because we want to be able to attach to
> containers that are launched without a CommandInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 905a34bb13c2fcd1b22ef4e9605405ef84711c25 
>   include/mesos/v1/mesos.proto b72069a6caff0b163d9d9cae45c3caa0b25de38f 
> 
> Diff: https://reviews.apache.org/r/53450/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 52587: Allow CREATE of shared volumes based on capability of framework.

[Jiang Yan Xu]

> On Nov. 4, 2016, 12:30 p.m., Neil Conway wrote:
> > src/master/validation.cpp, line 1532
> > 
> >
> > Can we use consistent tense here? The other volume error messages say 
> > "has been attempted" rather than "being attempted".
> > 
> > Also, the other error messages don't include the volume that failed 
> > validation, so we should probably be consistent.

+1 on the tense.

but re: "the other error messages don't include the volume that failed 
validation" they don't include the volume but they include other things this 
error message doesn't include. Whether the additional information is useful 
depends on the failure IMO. Nevertheless I guess it's true we don't have to say 
it's a shared volume separately because `stringify(volume)` reveals it.


So overall the following look good?

```
...

"Create volume operation for '" + stringify(volume) +
"' has been attempted by framework '" +

...
```


- Jiang Yan


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


On Nov. 1, 2016, 9:54 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52587/
> ---
> 
> (Updated Nov. 1, 2016, 9:54 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6316
> https://issues.apache.org/jira/browse/MESOS-6316
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a framework issues a CREATE for a shared volume, we allow that
> operation only if the framework has opted in for the capability of
> SHARED_RESOURCES. However, for HTTP operator (/create-volumes), we
> do not check as the operator API is not related to a specific
> framework.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 013bb592ba47b785c552e199633e4784e8aa71b1 
>   src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
>   src/master/validation.cpp f690a9eacd278b51a52f5588dbeea377df074435 
>   src/tests/master_validation_tests.cpp 
> a5d8610bd61822cdf55cbc8d7056e5cf8fecfa54 
>   src/tests/persistent_volume_tests.cpp 
> 6289009fe9ed0a57ba5eff46dbbe0633a75d2616 
> 
> Diff: https://reviews.apache.org/r/52587/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53450: Added TTYInfo to ContainerInfo.

[Vinod Kone]

> On Nov. 4, 2016, 8:16 p.m., Kevin Klues wrote:
> > include/mesos/mesos.proto, line 2115
> > 
> >
> > I see you changed s/TtyInfo/TTYInfo/
> > 
> > Is this intentional? I know @bmahler has lamented in the past about 
> > decisions like this. (e.g. `HTTPScheduler` he would prefer to be 
> > `HttpScheduler`).

I might be missing context but why do we prefer HttpScheduler to HTTPScheduler? 
HTTP is an abbrevation, so all caps seems better to me. Note that we have 
TCPCheckInfo, HTTPCheckInfo, URI, URL, etc protos already.


- Vinod


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


On Nov. 4, 2016, 12:35 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53450/
> ---
> 
> (Updated Nov. 4, 2016, 12:35 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-6525
> https://issues.apache.org/jira/browse/MESOS-6525
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added to ContainerInfo because we want to be able to attach to
> containers that are launched without a CommandInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 905a34bb13c2fcd1b22ef4e9605405ef84711c25 
>   include/mesos/v1/mesos.proto b72069a6caff0b163d9d9cae45c3caa0b25de38f 
> 
> Diff: https://reviews.apache.org/r/53450/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53473: Add new param user to logrotate's prepare function.

[Joseph Wu]

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




include/mesos/slave/container_logger.hpp (line 154)


With your current patch, the logrotate logger will always specify the 
`--user` flag, even if `switch_user` is false.  If we had a test with the 
logrotate logger & `flags.switch_user = false`, it would likely fail.

An `Option& user` is more accurate here.

Note: For flags parsing, `--user=""` explicitly sets the user to empty 
string (rather than setting `Option user` to `None`).



src/slave/containerizer/docker.cpp (lines 1282 - 1285)


If the `prepare` function takes an `Option` instead, you can simply 
pass along `container->user` (with no `.get()`).



src/slave/containerizer/mesos/containerizer.cpp (lines 1364 - 1367)


Ditto here.


- Joseph Wu


On Nov. 4, 2016, 7:58 a.m., Sivaram Kannan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53473/
> ---
> 
> (Updated Nov. 4, 2016, 7:58 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5856
> https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add new param user to logrotate's prepare function.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> 939974736f9eb744c83036e074718d2a1eba8b0a 
>   src/slave/container_loggers/lib_logrotate.hpp 
> 28fdf3bdcc66d473521b377f66ab0b48f6900f58 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 53698d339f0f4d2dc916b53239ca0c36bbebcd42 
>   src/slave/container_loggers/logrotate.hpp 
> d1db69236f5a9b1dbb3113ad02218a512afdb46b 
>   src/slave/container_loggers/sandbox.hpp 
> e0aeb32a9ec83af049af8a10010b819c1d8b25d8 
>   src/slave/container_loggers/sandbox.cpp 
> cc263ebef7e0c3e778fabafa49faa6dd315adc45 
>   src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/tests/container_logger_tests.cpp 
> 1bb94a8461e481983f25a44737e4011ed5fc4b1f 
> 
> Diff: https://reviews.apache.org/r/53473/diff/
> 
> 
> Testing
> ---
> 
> Run the mesos-logrotate-logger with un-priviledged user and verify whether 
> the logs are getting rotated.
> Run the mesos-logrotate-logger as root user and verify whether the logs are 
> getting rotated.
> 
> 
> Thanks,
> 
> Sivaram Kannan
> 
>

Re: Review Request 53456: Avoid 'using namespace' clause in the test containerizer.

[Joseph Wu]

> On Nov. 4, 2016, 1:11 p.m., Joseph Wu wrote:
> > src/tests/containerizer.cpp, lines 402-403
> > 
> >
> > I personally prefer to not import any of the `process` namespace.  Many 
> > of the functions there have very generic sounding names, which makes code 
> > like this slightly harder to read.
> 
> Benjamin Mahler wrote:
> Do you mean functions or types? Not importing the functions sounds ok to 
> me.

The functions in this case.

I also prefer, say, `process::Future` over `Future`.  But both are valid 
stylistically (I think).


- Joseph


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


On Nov. 3, 2016, 11:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53456/
> ---
> 
> (Updated Nov. 3, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoid 'using namespace' clause in the test containerizer.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.cpp c2208660352e5cf62cec5c7b92ec67fea62aea49 
> 
> Diff: https://reviews.apache.org/r/53456/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 53456: Avoid 'using namespace' clause in the test containerizer.

[Benjamin Mahler]

> On Nov. 4, 2016, 8:11 p.m., Joseph Wu wrote:
> > src/tests/containerizer.cpp, lines 402-403
> > 
> >
> > I personally prefer to not import any of the `process` namespace.  Many 
> > of the functions there have very generic sounding names, which makes code 
> > like this slightly harder to read.

Do you mean functions or types? Not importing the functions sounds ok to me.


- Benjamin


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


On Nov. 4, 2016, 6:15 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53456/
> ---
> 
> (Updated Nov. 4, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoid 'using namespace' clause in the test containerizer.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.cpp c2208660352e5cf62cec5c7b92ec67fea62aea49 
> 
> Diff: https://reviews.apache.org/r/53456/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 53451: Added agent API protos for executing and attaching nested containers.

[Kevin Klues]

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




include/mesos/agent/agent.proto (line 159)


s/in streaming response/in a streaming response/



include/mesos/agent/agent.proto (line 338)


It is also the streaming response to `Call::ATTACH_CONTAINER_OUTPUT`



include/mesos/v1/agent/agent.proto (line 35)


s/Call::NestedContainerSession/Call::LaunchNestedContainerSession

Also, we should add a similar comment in the *non-v1* agent.proto file.



include/mesos/v1/agent/agent.proto (line 159)


s/in streaming response/in a streaming response/



include/mesos/v1/agent/agent.proto (line 338)


Also true for `Call::ATTACH_CONTAINER_OUTPUT`


- Kevin Klues


On Nov. 4, 2016, 12:36 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53451/
> ---
> 
> (Updated Nov. 4, 2016, 12:36 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-6525
> https://issues.apache.org/jira/browse/MESOS-6525
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch only contains the proto definitions. The implementation is
> not yet present.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 0e9e071d1b97ba41c1636e41f242d564998e8f0b 
>   include/mesos/v1/agent/agent.proto c2f282c05d15ae1fa099cd26ceb66d41e52b1d95 
>   src/slave/http.cpp a32aca437cffd52bc2bcde859eedddca2038e3f1 
>   src/slave/validation.cpp 974bf24f1e139e409cc4b6363b646ff50d57df1e 
> 
> Diff: https://reviews.apache.org/r/53451/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53330: Tracked layers and pull latency in docker store.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [53330, 53105]

Failed command: ./support/apply-review.sh -n -r 53105

Error:
2016-11-04 20:15:28 URL:https://reviews.apache.org/r/53105/diff/raw/ 
[1733/1733] -> "53105.patch" [1]
error: patch failed: src/slave/containerizer/docker.hpp:125
error: src/slave/containerizer/docker.hpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/15951/console

- Mesos ReviewBot


On Nov. 4, 2016, 5:16 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53330/
> ---
> 
> (Updated Nov. 4, 2016, 5:16 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added several metrics for docker store:
> - a counter for number of layers pulled
> - a gauge for total number of layers
> - a timer for pull latency distribution in the last hour
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
> 232c027f8f96da0cb30b957bce4607d3695050d2 
>   src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
> cd684b33eb308ce1eeb4539a5b2d51985d835db7 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> e192f86a1848b373f3aa73d29688a96375cac313 
> 
> Diff: https://reviews.apache.org/r/53330/diff/
> 
> 
> Testing
> ---
> 
> Use mesos-execute to pull several different images, and verified following 
> counters in agent's metrics/snapshot:
> ```
> curl localhost:5051/metrics/snapshot | jq . | grep 
> containerizer/mesos/docker_pull
>   "containerizer/mesos/docker_store/layers_pulled": 47,
>   "containerizer/mesos/docker_store/pull_ms/p50": 7080.760064,
>   "containerizer/mesos/docker_store/pull_ms/p90": 11653.6390912,
>   "containerizer/mesos/docker_store/pull_ms/p": 12528.3066648832,
>   "containerizer/mesos/docker_store/pull_ms": 4550.814976,
>   "containerizer/mesos/docker_store/pull_ms/count": 4,
>   "containerizer/mesos/docker_store/pull_ms/max": 12529.182208,
>   "containerizer/mesos/docker_store/pull_ms/p999": 12520.426776832,
>   "containerizer/mesos/docker_store/pull_ms/min": 3167.337984,
>   "containerizer/mesos/docker_store/pull_ms/p95": 12091.4106496,
>   "containerizer/mesos/docker_store/pull_ms/p99": 12441.62789632,
>   "containerizer/mesos/docker_store/layers": 47,
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52310: Switch the uid of the binary if a user is passed from the lib_logrotate.

[Joseph Wu]

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



My `Ship It!` from earlier still stands.  Just need to check the remainder of 
the chain.


src/slave/container_loggers/logrotate.cpp (line 246)


Oops, there's a double-space in the middle of that comment.


- Joseph Wu


On Nov. 4, 2016, 7:45 a.m., Sivaram Kannan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52310/
> ---
> 
> (Updated Nov. 4, 2016, 7:45 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5856
> https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Switch the uid of the binary if a user is passed from the lib_logrotate.
> 
> 
> Diffs
> -
> 
>   src/slave/container_loggers/logrotate.cpp 
> 431bc3cbb54e94359078e4dae0b32ad301393640 
> 
> Diff: https://reviews.apache.org/r/52310/diff/
> 
> 
> Testing
> ---
> 
> 1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
> the logs are getting rotated. 
> 2. Run the mesos-logrotate-logger as root user and verify whether the logs 
> are getting rotated.
> 
> 
> Thanks,
> 
> Sivaram Kannan
> 
>

Re: Review Request 53450: Added TTYInfo to ContainerInfo.

[Kevin Klues]

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


Fix it, then Ship it!





include/mesos/mesos.proto (line 2115)


I see you changed s/TtyInfo/TTYInfo/

Is this intentional? I know @bmahler has lamented in the past about 
decisions like this. (e.g. `HTTPScheduler` he would prefer to be 
`HttpScheduler`).


- Kevin Klues


On Nov. 4, 2016, 12:35 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53450/
> ---
> 
> (Updated Nov. 4, 2016, 12:35 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-6525
> https://issues.apache.org/jira/browse/MESOS-6525
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added to ContainerInfo because we want to be able to attach to
> containers that are launched without a CommandInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 905a34bb13c2fcd1b22ef4e9605405ef84711c25 
>   include/mesos/v1/mesos.proto b72069a6caff0b163d9d9cae45c3caa0b25de38f 
> 
> Diff: https://reviews.apache.org/r/53450/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 53456: Avoid 'using namespace' clause in the test containerizer.

[Joseph Wu]

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


Ship it!





src/tests/containerizer.cpp (lines 402 - 403)


I personally prefer to not import any of the `process` namespace.  Many of 
the functions there have very generic sounding names, which makes code like 
this slightly harder to read.


- Joseph Wu


On Nov. 3, 2016, 11:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53456/
> ---
> 
> (Updated Nov. 3, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoid 'using namespace' clause in the test containerizer.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.cpp c2208660352e5cf62cec5c7b92ec67fea62aea49 
> 
> Diff: https://reviews.apache.org/r/53456/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52587: Allow CREATE of shared volumes based on capability of framework.

[Neil Conway]

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




src/master/validation.cpp (line 1530)


Can we use consistent tense here? The other volume error messages say "has 
been attempted" rather than "being attempted".

Also, the other error messages don't include the volume that failed 
validation, so we should probably be consistent.


- Neil Conway


On Nov. 1, 2016, 4:54 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52587/
> ---
> 
> (Updated Nov. 1, 2016, 4:54 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6316
> https://issues.apache.org/jira/browse/MESOS-6316
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a framework issues a CREATE for a shared volume, we allow that
> operation only if the framework has opted in for the capability of
> SHARED_RESOURCES. However, for HTTP operator (/create-volumes), we
> do not check as the operator API is not related to a specific
> framework.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 013bb592ba47b785c552e199633e4784e8aa71b1 
>   src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
>   src/master/validation.cpp f690a9eacd278b51a52f5588dbeea377df074435 
>   src/tests/master_validation_tests.cpp 
> a5d8610bd61822cdf55cbc8d7056e5cf8fecfa54 
>   src/tests/persistent_volume_tests.cpp 
> 6289009fe9ed0a57ba5eff46dbbe0633a75d2616 
> 
> Diff: https://reviews.apache.org/r/52587/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53435: Fixed header include order.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53386, 53387, 53435]

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 Nov. 4, 2016, 3:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53435/
> ---
> 
> (Updated Nov. 4, 2016, 3:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed header include order.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53435/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53455: Fixed the TestContainerizer to be thread-safe.

[Joseph Wu]

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


Fix it, then Ship it!




LGTM.  Might have preferred a few smaller patches (such as one to combine the 
`TestContainerizer` constructors), but I'm not going to block the review on 
that.


src/tests/containerizer.hpp (line 137)


s/mocks/mock/



src/tests/containerizer.cpp (lines 246 - 251)


This function seems to be un-used (or I'm just not seeing it).


- Joseph Wu


On Nov. 3, 2016, 11:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53455/
> ---
> 
> (Updated Nov. 3, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6544 and MESOS-6545
> https://issues.apache.org/jira/browse/MESOS-6544
> https://issues.apache.org/jira/browse/MESOS-6545
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The TestContainerizer is currently not backed by a Process and
> does not do any explicit synchronization and so is not thread safe.
> 
> Most tests currently cannot trip the concurrency issues, but
> this surfaced recently in MESOS-6544.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.hpp d418ecebee155f894187e24549f42497372d8a5f 
>   src/tests/containerizer.cpp c2208660352e5cf62cec5c7b92ec67fea62aea49 
> 
> Diff: https://reviews.apache.org/r/53455/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50974: Documented all the API calls for Operator HTTP API.

[Vinod Kone]

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



Thanks for doing this. Awesome! Some minor suggestions before we can commit 
this.


docs/operator-http-api.md 


Instead of just the JSON, we need to add some text on what these calls will 
do. Look at scheduler and executor docs.



docs/operator-http-api.md (line 32)


Can follow the same order as the enums in call? The first one should be 
GET_HEALTH



docs/operator-http-api.md (line 66)


I would pull to the `type` to the top of the JSON like we did the scheduler 
and executor api docs.



docs/operator-http-api.md (line 72)


202 Accepted? here and everywhere else.


- Vinod Kone


On Aug. 11, 2016, 8:33 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50974/
> ---
> 
> (Updated Aug. 11, 2016, 8:33 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-5992
> https://issues.apache.org/jira/browse/MESOS-5992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented all the API calls for Operator HTTP API.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md 4f4c39e7b4b6de32af1933c34eba21f126fae8ac 
> 
> Diff: https://reviews.apache.org/r/50974/diff/
> 
> 
> Testing
> ---
> 
> Checked the generated page through "rake dev".
> Validated and formatted all the JSON snippets with:
> http://jsonlint.com/
> http://jsonviewer.stack.hu/
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Review Request 53483: Added members type/reader to `Request`.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

These new members are needed for supporting request streaming i.e.,
the caller can use the writer to stream chunks to the server if
the request body is not known in advance.


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
9722c6210526479da243ae9945e4c9a89ecb9009 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 53467: Added ContainerStatus.container_id to JSON endpoints.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 4, 2016, 7:40 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53467/
> ---
> 
> (Updated Nov. 4, 2016, 7:40 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ContainerStatus.container_id to JSON endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp c2398177c9e5af12c7a20c02e92d3f3036a7a39a 
>   src/common/http.cpp fb8454a97ad78c306d17b4222c14d68b3598175d 
>   src/slave/http.cpp a32aca437cffd52bc2bcde859eedddca2038e3f1 
>   src/tests/slave_tests.cpp 8f1dbef081171421bf2761e24a4f267cbd015d0d 
> 
> Diff: https://reviews.apache.org/r/53467/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 53485: Introduced a `io::read()` overload for reading from piped reader.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

The helper reads from the pipe till EOF. This is used later to
read BODY requests from the streaming request decoder.


Diffs
-

  3rdparty/libprocess/include/process/io.hpp 
eec5efd7e6b71a783f2bb40826054d0488cee71f 
  3rdparty/libprocess/src/io.cpp d930ceebc90468e082b984e41385549f906dc6ae 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53481: Moved around the `Request` struct in the file.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This serves two purposes:
- This brings it near the `Response` struct thereby making it easy
to switch between them for readability.
- In the future, `Request` would have a `Pipe::Reader` member
for supporting request streaming. Otherwise, we would need to
add another forward declaration and forward declaring nested
structs is not quite possible.


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
9722c6210526479da243ae9945e4c9a89ecb9009 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53484: Introduced `RouteOptions`.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This allows routes to specify configuration options. Currently, it
only has one member `streaming` i.e, if the route supports request
streaming. This also enables us to add more options in the future
without polluting overloads.


Diffs
-

  3rdparty/libprocess/include/process/process.hpp 
f389d3d3b671e301a7ac911ad87ab13289e8c82f 
  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53482: Initialized the POD type in the `Request` struct.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Previously, the `keepAlive` member was not initialized correctly,
the behavior is undefined if POD types are not correctly
initialized.


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
9722c6210526479da243ae9945e4c9a89ecb9009 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53486: Introduced a streaming request decoder in libprocess.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This would become the default de facto decoder used by libprocess
and replace the existing `DataDecoder`.


Diffs
-

  3rdparty/libprocess/src/decoder.hpp 76dca0b272af8591880ef220ec2dc006906fbc36 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53487: Wired the libprocess code to use the streaming decoder.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Old libprocess style messages and routes not supporting request
streaming read the body from the piped reader. Otherwise, the
request is forwarded to the handler when the route supports
streaming.


Diffs
-

  3rdparty/libprocess/include/process/process.hpp 
f389d3d3b671e301a7ac911ad87ab13289e8c82f 
  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53488: Removed `convert()` continuations in favor of using `io::read()`.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Removed `convert()` continuations in favor of using `io::read()`.


Diffs
-

  3rdparty/libprocess/src/http.cpp b5ddac9f1970358f8232a481431507242052d294 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53491: WIP Disabled tests relying on filtering HTTP events.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This is a placeholder patch to make review bot happy. Some tests
that rely on filtering HTTP events based on type won't work now
since the request body is not yet known when `visit()` is invoked.
I am working on fixing these.


Diffs
-

  src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 
  src/tests/slave_recovery_tests.cpp cc50498040986e9aca1031df3622b13e7a44218a 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53489: Added support for request streaming to the connection abstraction.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This required modifications to the `encode()` method to return
a `Pipe::Reader` instead of the request body. The `send()` then
reads from this pipe to send the request via the socket.


Diffs
-

  3rdparty/libprocess/src/http.cpp b5ddac9f1970358f8232a481431507242052d294 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 53490: Added a test for request streaming via the connection abstraction.

[Anand Mazumdar]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added a test for request streaming via the connection abstraction.


Diffs
-

  3rdparty/libprocess/src/tests/http_tests.cpp 
533104c93dd1eaf67bf3752163d2e0cad090078d 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 53466: Added test to verify container status for nested containers.

[Kevin Klues]

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




src/tests/mesos.hpp (line 365)


Why is this pulled in if it's now used anywhere? Does some of the macro 
magic below actually reference this type?



src/tests/mesos.hpp (lines 1867 - 1875)


This seems like a logically different change that should have its own 
commit message explaining why this is changed this way.


- Kevin Klues


On Nov. 4, 2016, 7:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53466/
> ---
> 
> (Updated Nov. 4, 2016, 7:08 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is used to verify that container status is properly set in
> task status update for nested containers.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> db953d39a95e7e95871054114a0a9bbeded46224 
>   src/tests/mesos.hpp f521d22eeafd1901d1acf42c168824b922b68743 
> 
> Diff: https://reviews.apache.org/r/53466/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53465: Fixed the container status for nested containers.

[Jie Yu]

> On Nov. 4, 2016, 5:49 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 747
> > 
> >
> > Is this correct? Why is it sufficient to recursively call this on the 
> > status of the parent? Is it because (for now) we always share the same 
> > cgroups as our parent?

Yes. I can add a TODO here to change that once we move to a world where cgroup 
is a nested container is not shared with its parent.


- Jie


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


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53465/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, the ContainerStatus inside the TaskStatus for a
> task in the default executor is not correct. The agent always gets the
> container status of the top level container, even if the task might
> correspond to a nested container.
> 
> This patch fixed this issue by letting the executor set the container
> ID for the task (because only it has the knowledge about this
> mapping), and the agent will use that container ID to get the proper
> container status.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp b8a88ad6e361720dc7d02c9423db3e67226f779f 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 2f6723c64261fb3295626d6479fe844fb23b0650 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 939142e36b926d9e4201d35dedd25e32e9f8c63c 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> Diff: https://reviews.apache.org/r/53465/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53465: Fixed the container status for nested containers.

[Jie Yu]

> On Nov. 4, 2016, 5:49 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1276
> > 
> >
> > Ditto on this one.

Yes, I can add a TODO.


- Jie


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


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53465/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, the ContainerStatus inside the TaskStatus for a
> task in the default executor is not correct. The agent always gets the
> container status of the top level container, even if the task might
> correspond to a nested container.
> 
> This patch fixed this issue by letting the executor set the container
> ID for the task (because only it has the knowledge about this
> mapping), and the agent will use that container ID to get the proper
> container status.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp b8a88ad6e361720dc7d02c9423db3e67226f779f 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 2f6723c64261fb3295626d6479fe844fb23b0650 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 939142e36b926d9e4201d35dedd25e32e9f8c63c 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> Diff: https://reviews.apache.org/r/53465/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53127: Added the test `ProvisionerDockerWhiteoutTest`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53041, 53042, 53053, 53161, 53115, 53116, 53127]

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 Nov. 4, 2016, 9:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53127/
> ---
> 
> (Updated Nov. 4, 2016, 9:40 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the test `ProvisionerDockerWhiteoutTest`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d8fb7abb55410cfd0835f59bfc0f35566aeeae37 
> 
> Diff: https://reviews.apache.org/r/53127/diff/
> 
> 
> Testing
> ---
> 
> [==] Running 3 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 3 tests from BackendFlag/ProvisionerDockerWhiteoutTest
> [ RUN  ] 
> BackendFlag/ProvisionerDockerWhiteoutTest.ROOT_INTERNET_CURL_Whiteout/0
> Executing pre-exec command 
> '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/stack\/workspace\/mesos\/build\/src\/mesos-containerizer"}'
> Executing pre-exec command 
> '{"arguments":["mount","-n","--rbind","\/tmp\/BackendFlag_ProvisionerDockerWhiteoutTest_ROOT_INTERNET_CURL_Whiteout_0_42lvlX\/slaves\/5ec98305-4a7d-4351-976e-7c7f4023329c-S0\/frameworks\/5ec98305-4a7d-4351-976e-7c7f4023329c-\/executors\/7bea80dc-b644-430f-abb0-cabe2af3ded8\/runs\/2f4f0799-8fc1-4260-b1d3-422aa40f8ef1","\/tmp\/BackendFlag_ProvisionerDockerWhiteoutTest_ROOT_INTERNET_CURL_Whiteout_0_42lvlX\/provisioner\/containers\/2f4f0799-8fc1-4260-b1d3-422aa40f8ef1\/backends\/aufs\/rootfses\/8837b468-156f-4e04-96b2-ab201626ad3c\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> I1024 16:17:23.245787 13269 exec.cpp:162] Version: 1.2.0
> I1024 16:17:23.256351 13271 exec.cpp:237] Executor registered on agent 
> 5ec98305-4a7d-4351-976e-7c7f4023329c-S0
> Received SUBSCRIBED event
> Subscribed executor on workstation
> Received LAUNCH event
> Starting task 7bea80dc-b644-430f-abb0-cabe2af3ded8
> /home/stack/workspace/mesos/build/src/mesos-containerizer launch 
> --command="{"arguments":["test","!","-f","\/etc\/rc3.d\/S40-network"],"shell":false,"value":"\/usr\/bin\/test"}"
>  --help="false" 
> --rootfs="/tmp/BackendFlag_ProvisionerDockerWhiteoutTest_ROOT_INTERNET_CURL_Whiteout_0_42lvlX/provisioner/containers/2f4f0799-8fc1-4260-b1d3-422aa40f8ef1/backends/aufs/rootfses/8837b468-156f-4e04-96b2-ab201626ad3c"
>  --unshare_namespace_mnt="true" --user="root" 
> --working_directory="/mnt/mesos/sandbox"
> Forked command at 13278
> Changing root to 
> /tmp/BackendFlag_ProvisionerDockerWhiteoutTest_ROOT_INTERNET_CURL_Whiteout_0_42lvlX/provisioner/containers/2f4f0799-8fc1-4260-b1d3-422aa40f8ef1/backends/aufs/rootfses/8837b468-156f-4e04-96b2-ab201626ad3c
> Command exited with status 0 (pid: 13278)
> I1024 16:17:23.500792 13273 exec.cpp:414] Executor asked to shutdown
> Received SHUTDOWN event
> Shutting down
> [   OK ] 
> BackendFlag/ProvisionerDockerWhiteoutTest.ROOT_INTERNET_CURL_Whiteout/0 (1365 
> ms)
> [ RUN  ] 
> BackendFlag/ProvisionerDockerWhiteoutTest.ROOT_INTERNET_CURL_Whiteout/1
> Executing pre-exec command 
> '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/stack\/workspace\/mesos\/build\/src\/mesos-containerizer"}'
> Executing pre-exec command 
> '{"arguments":["mount","-n","--rbind","\/tmp\/BackendFlag_ProvisionerDockerWhiteoutTest_ROOT_INTERNET_CURL_Whiteout_1_Zm1mK7\/slaves\/9d49d96c-189d-47e9-9370-72146356af7c-S0\/frameworks\/9d49d96c-189d-47e9-9370-72146356af7c-\/executors\/ae8fda3e-db36-49f7-8699-9a4768d4af27\/runs\/49f24d09-6474-4a39-9da6-8c8da0ad4238","\/tmp\/BackendFlag_ProvisionerDockerWhiteoutTest_ROOT_INTERNET_CURL_Whiteout_1_Zm1mK7\/provisioner\/containers\/49f24d09-6474-4a39-9da6-8c8da0ad4238\/backends\/copy\/rootfses\/db73608a-614b-49be-bebe-cd9aa42e31f5\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> I1024 16:17:24.971269 13344 exec.cpp:162] Version: 1.2.0
> I1024 16:17:24.981901 13364 exec.cpp:237] Executor registered on agent 
> 9d49d96c-189d-47e9-9370-72146356af7c-S0
> Received SUBSCRIBED event
> Subscribed executor on workstation
> Received LAUNCH event
> Starting task ae8fda3e-db36-49f7-8699-9a4768d4af27
> /home/stack/workspace/mesos/build/

Re: Review Request 53465: Fixed the container status for nested containers.

[Kevin Klues]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 747)


Is this correct? Why is it sufficient to recursively call this on the 
status of the parent? Is it because (for now) we always share the same cgroups 
as our parent?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1276)


Ditto on this one.


- Kevin Klues


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53465/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, the ContainerStatus inside the TaskStatus for a
> task in the default executor is not correct. The agent always gets the
> container status of the top level container, even if the task might
> correspond to a nested container.
> 
> This patch fixed this issue by letting the executor set the container
> ID for the task (because only it has the knowledge about this
> mapping), and the agent will use that container ID to get the proper
> container status.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp b8a88ad6e361720dc7d02c9423db3e67226f779f 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 2f6723c64261fb3295626d6479fe844fb23b0650 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 939142e36b926d9e4201d35dedd25e32e9f8c63c 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> Diff: https://reviews.apache.org/r/53465/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53449: Added mesos test v1 scheduler default actions.

[Gilbert Song]

> On Nov. 3, 2016, 9:49 p.m., Jie Yu wrote:
> > src/tests/mesos.hpp, line 1832
> > 
> >
> > To be consistent, I'll move those to v1::scheduler namespace.

Thanks. I should have moved them.


- Gilbert


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


On Nov. 3, 2016, 5:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53449/
> ---
> 
> (Updated Nov. 3, 2016, 5:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mesos test v1 scheduler default actions.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 45dba233d9db39ead5f562e8090d07b5e0c6d42d 
> 
> Diff: https://reviews.apache.org/r/53449/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 53464: Added ContainerID to ContainerStatus.

[Kevin Klues]

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


Fix it, then Ship it!





include/mesos/mesos.proto (line 2193)


Why is this the first element in the message? I would think since we 
already have others taht are ordered 1,2,3, we would put this at the bottom 
with 4.

Is it so that we keep the "ContainerID" of the "ContainerStatus" at the 
top? If so, that makes sense to me.


- Kevin Klues


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53464/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6465 and MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6465
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ContainerID to ContainerStatus.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 905a34bb13c2fcd1b22ef4e9605405ef84711c25 
>   include/mesos/v1/mesos.proto b72069a6caff0b163d9d9cae45c3caa0b25de38f 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53464/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53463: Inlined '_status' method in MesosContainerizer as a lambda.

[Kevin Klues]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/containerizer.cpp (line 1907)


Do you need the '=' here or is just `containerID` sufficient?


- Kevin Klues


On Nov. 4, 2016, 7:07 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53463/
> ---
> 
> (Updated Nov. 4, 2016, 7:07 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-6528
> https://issues.apache.org/jira/browse/MESOS-6528
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Inlined '_status' method in MesosContainerizer as a lambda.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
> 
> Diff: https://reviews.apache.org/r/53463/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 53460: Refactored network::Address into inet::Address.

[Benjamin Hindman]

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

(Updated Nov. 4, 2016, 5:30 p.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Also added unix::Address.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
  3rdparty/libprocess/include/process/address.hpp 
04e3155d65f476208fa852e83b79d173b66288fd 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/socket.hpp 
f798af7879546d71e8ef4a295c9cf489a70cb61f 
  3rdparty/libprocess/include/process/ssl/gtest.hpp 
21a0fc45b55a368a21b3e616c751ab43eebd4902 
  3rdparty/libprocess/src/address.cpp PRE-CREATION 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 
  3rdparty/libprocess/src/poll_socket.cpp 
f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 
  3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
  3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
  3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
55c8c309571b1892b0acc4d766eda9bb98085a6f 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 53492: Added non-const operator-> to Future.

[Benjamin Hindman]

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

Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Added non-const operator-> to Future.


Diffs
-

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

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 53479: Perform GC asynchronously.

[Jacob Janco]

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

(Updated Nov. 4, 2016, 5:25 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Perform GC asynchronously.


Diffs
-

  src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
  src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 

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


Testing
---

`make check`


Thanks,

Jacob Janco

Re: Review Request 53330: Tracked layers and pull latency in docker store.

[Zhitao Li]

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

(Updated Nov. 4, 2016, 5:16 p.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, Jason Lai, and Jie Yu.


Summary (updated)
-

Tracked layers and pull latency in docker store.


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


Repository: mesos


Description (updated)
---

This patch added several metrics for docker store:
- a counter for number of layers pulled
- a gauge for total number of layers
- a timer for pull latency distribution in the last hour


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
232c027f8f96da0cb30b957bce4607d3695050d2 
  src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
cd684b33eb308ce1eeb4539a5b2d51985d835db7 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
e192f86a1848b373f3aa73d29688a96375cac313 

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


Testing (updated)
---

Use mesos-execute to pull several different images, and verified following 
counters in agent's metrics/snapshot:
```
curl localhost:5051/metrics/snapshot | jq . | grep 
containerizer/mesos/docker_pull
  "containerizer/mesos/docker_store/layers_pulled": 47,
  "containerizer/mesos/docker_store/pull_ms/p50": 7080.760064,
  "containerizer/mesos/docker_store/pull_ms/p90": 11653.6390912,
  "containerizer/mesos/docker_store/pull_ms/p": 12528.3066648832,
  "containerizer/mesos/docker_store/pull_ms": 4550.814976,
  "containerizer/mesos/docker_store/pull_ms/count": 4,
  "containerizer/mesos/docker_store/pull_ms/max": 12529.182208,
  "containerizer/mesos/docker_store/pull_ms/p999": 12520.426776832,
  "containerizer/mesos/docker_store/pull_ms/min": 3167.337984,
  "containerizer/mesos/docker_store/pull_ms/p95": 12091.4106496,
  "containerizer/mesos/docker_store/pull_ms/p99": 12441.62789632,
  "containerizer/mesos/docker_store/layers": 47,
```


Thanks,

Zhitao Li

Re: Review Request 53266: Fixed `DefaultExecutorTest.*` tests on hosts without Docker.

[Gastón Kleiman]

> On Oct. 31, 2016, 11:11 p.m., Vinod Kone wrote:
> > src/tests/default_executor_tests.cpp, lines 219-229
> > 
> >
> > do we use this pattern anywhere else in our test suite? if not, i would 
> > spend some time to think about alternate patterns.

https://github.com/apache/mesos/blob/master/src/tests/state_tests.cpp#L355 
follows a similar pattern.

I followed Yan's advice, updating this RR and discarding the rest of the chain.


- Gastón


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


On Nov. 4, 2016, 5:12 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53266/
> ---
> 
> (Updated Nov. 4, 2016, 5:12 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6455
> https://issues.apache.org/jira/browse/MESOS-6455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some of the tests are parameterized by the containerizers.
> 
> Fixed the naming schema, so that the tests using the Docker
> containerizer run only if Docker is available.
> 
> Now the tests with the Mesos containerizer also run if the tests
> are started as a non-root user.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> db953d39a95e7e95871054114a0a9bbeded46224 
> 
> Diff: https://reviews.apache.org/r/53266/diff/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*' 
> --docker=/tmp/foo`
> `sudo bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*'`
> `bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*'`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 53266: Fixed `DefaultExecutorTest.*` tests on hosts without Docker.

[Gastón Kleiman]

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

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


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


Changes
---

Addressed all the feedback.


Summary (updated)
-

Fixed `DefaultExecutorTest.*` tests on hosts without Docker.


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


Repository: mesos


Description (updated)
---

Some of the tests are parameterized by the containerizers.

Fixed the naming schema, so that the tests using the Docker
containerizer run only if Docker is available.

Now the tests with the Mesos containerizer also run if the tests
are started as a non-root user.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp db953d39a95e7e95871054114a0a9bbeded46224 

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


Testing (updated)
---

`sudo bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*' 
--docker=/tmp/foo`
`sudo bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*'`
`bin/mesos-tests.sh --gtest_filter='*DefaultExecutorTest*'`


Thanks,

Gastón Kleiman

Re: Review Request 53479: Perform GC asynchronously.

[Jacob Janco]

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

(Updated Nov. 4, 2016, 5 p.m.)


Review request for mesos.


Repository: mesos


Description
---

Perform GC asynchronously.


Diffs
-

  src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
  src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 

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


Testing (updated)
---

`make check`


Thanks,

Jacob Janco

Review Request 53479: Perform GC asynchronously.

[Jacob Janco]

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

Review request for mesos.


Repository: mesos


Description
---

Perform GC asynchronously.


Diffs
-

  src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
  src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 

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


Testing
---


Thanks,

Jacob Janco

Re: Review Request 53435: Fixed header include order.

[Neil Conway]

> On Nov. 4, 2016, 4:06 p.m., Benjamin Bannier wrote:
> > src/tests/containerizer/docker_volume_isolator_tests.cpp, lines 36-37
> > 
> >
> > That's what I understood, but I made an error in my comment: in that 
> > ordering it should be at the same level as `runtime.hpp` since both of 
> > these a 6 levels deep. Currently `linux.hpp` is preceeded by headers 7 
> > levels deep.
> > 
> > I seems this ruleis rather complicated in pratice.

Yeah, there are a lot of corner cases. Personally I think this isn't worth 
devoting a lot of attention to in the absence of tool support.

I'd personally vote for (a) coming up with a reasonable sort order (b) once we 
get consensus, add that to the style guide (c) writing a clang-tidy check that 
implements the sort order.


- Neil


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


On Nov. 4, 2016, 3:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53435/
> ---
> 
> (Updated Nov. 4, 2016, 3:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed header include order.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53435/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53435: Fixed header include order.

[Benjamin Bannier]

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




src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 33 - 34)


That's what I understood, but I made an error in my comment: in that 
ordering it should be at the same level as `runtime.hpp` since both of these a 
6 levels deep. Currently `linux.hpp` is preceeded by headers 7 levels deep.

I seems this ruleis rather complicated in pratice.


- Benjamin Bannier


On Nov. 4, 2016, 4:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53435/
> ---
> 
> (Updated Nov. 4, 2016, 4:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed header include order.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53435/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53468: Show the pailer at the right side in WebUI.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [53468, 53289, 53288, 53287, 53286]

Failed command: ./support/apply-review.sh -n -r 53289

Error:
2016-11-04 15:58:33 URL:https://reviews.apache.org/r/53289/diff/raw/ [208/208] 
-> "53289.patch" [1]
error: unrecognized binary patch at line 3
fatal: patch with only garbage at line 4

Full log: https://builds.apache.org/job/mesos-reviewbot/15948/console

- Mesos ReviewBot


On Nov. 4, 2016, 8:47 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53468/
> ---
> 
> (Updated Nov. 4, 2016, 8:47 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6418
> https://issues.apache.org/jira/browse/MESOS-6418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Show the pailer at the right side in WebUI.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 
> b9849197227b06df348789a49348e2b5d4cfd2ae 
>   src/webui/master/static/css/mesos.css 
> 5b1227e9d64757f9fc106e497f7fa3ed72112c10 
>   src/webui/master/static/js/controllers.js 
> dd0cc810748a9bd378d9c6b15e9fe89b7c0f11d9 
>   src/webui/master/static/js/jquery.pailer.js 
> 537fe002166167748a6357a50f4b7dd04b1f4257 
>   src/webui/master/static/pailer.html 
> 19e0981143bd7e8372b49f4f036867e9dd05727a 
> 
> Diff: https://reviews.apache.org/r/53468/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 53435: Fixed header include order.

[Neil Conway]

> On Nov. 4, 2016, 2:41 p.m., Benjamin Bannier wrote:
> > src/tests/containerizer/docker_volume_isolator_tests.cpp, lines 36-37
> > 
> >
> > I am unsure what style is preferred here. We could (a) order header 
> > groups by directory depth (that's e.g., the reason 
> > `slave/containerizer/mesos/linux_launcher.hpp` is on l.29, not below l.36, 
> > or (b) keep related headers together. I can see both patterns in the code 
> > base. Maybe you have an idea.
> 
> Neil Conway wrote:
> I'd prefer the former (sort by directory depth, then lexicographically).
> 
> Benjamin Bannier wrote:
> In that case you should move `#include 
> "slave/containerizer/mesos/isolators/filesystem/linux.hpp"` just below 
> `#include "slave/containerizer/mesos/isolators/docker/runtime.hpp"` (with a 
> newline).

Sorry: I meant something like:

```
#include "a.hpp"
#include "b.hpp"

#include "a/c.hpp"
#include "a/d.hpp"

#include "a/c/e.hpp"

#include "a/d/f.hpp"
```


- Neil


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


On Nov. 4, 2016, 3:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53435/
> ---
> 
> (Updated Nov. 4, 2016, 3:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed header include order.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53435/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53435: Fixed header include order.

[Benjamin Bannier]

> On Nov. 4, 2016, 3:41 p.m., Benjamin Bannier wrote:
> > src/tests/containerizer/docker_volume_isolator_tests.cpp, lines 36-37
> > 
> >
> > I am unsure what style is preferred here. We could (a) order header 
> > groups by directory depth (that's e.g., the reason 
> > `slave/containerizer/mesos/linux_launcher.hpp` is on l.29, not below l.36, 
> > or (b) keep related headers together. I can see both patterns in the code 
> > base. Maybe you have an idea.
> 
> Neil Conway wrote:
> I'd prefer the former (sort by directory depth, then lexicographically).

In that case you should move `#include 
"slave/containerizer/mesos/isolators/filesystem/linux.hpp"` just below 
`#include "slave/containerizer/mesos/isolators/docker/runtime.hpp"` (with a 
newline).


- Benjamin


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


On Nov. 4, 2016, 4:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53435/
> ---
> 
> (Updated Nov. 4, 2016, 4:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed header include order.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53435/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50125: Added mesos-docker-executor support for devices control.

[Guangya Liu]

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



Seems spliting this to two patches is more simple for review: One patch for 
adding a new flag '--devices' to mesos docker executor, and the other is 
passing 'devices' as parameter for `dockerFlags` when launching mesos docker 
executor.

- Guangya Liu


On 十一月 2, 2016, 7:28 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50125/
> ---
> 
> (Updated 十一月 2, 2016, 7:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new flag '--devices' to mesos-docker-executor, and gave its
> feature to control devices exposition, isolation, and access permission.
> Also, passed GPUs assignment to mesos-docker-executor.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
>   src/docker/executor.cpp ab3f0473fdc9105d1c425f0dbe7b81c566d541e8 
>   src/slave/containerizer/docker.cpp f720320e112687e8ae972f04159b3b4cb7a58476 
> 
> Diff: https://reviews.apache.org/r/50125/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50599: Assigned Nvidia GPU devices to docker container.

[Guangya Liu]

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



What about split this to two patches, one is for adding helper function to 
parse a string to a 'Docker::Device', another is assign Nvidia GPU devices to 
docker container.

- Guangya Liu


On 十一月 2, 2016, 7:28 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50599/
> ---
> 
> (Updated 十一月 2, 2016, 7:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Assigned Nvidia GPU devices to docker container based on
> GPUs allocated by Nvidia GPU allocator.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
>   src/slave/containerizer/docker.cpp f720320e112687e8ae972f04159b3b4cb7a58476 
> 
> Diff: https://reviews.apache.org/r/50599/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 53435: Fixed header include order.

[Neil Conway]

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

(Updated Nov. 4, 2016, 3:15 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Tweak, per review.


Repository: mesos


Description
---

Fixed header include order.


Diffs (updated)
-

  src/tests/containerizer/docker_volume_isolator_tests.cpp 
ca7bffd3b1773a11a4679d114885d3edd977b02b 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
4df537747d84daa68c29e2d05b22fa386a4a16db 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 53435: Fixed header include order.

[Neil Conway]

> On Nov. 4, 2016, 2:41 p.m., Benjamin Bannier wrote:
> > src/tests/containerizer/docker_volume_isolator_tests.cpp, lines 36-37
> > 
> >
> > I am unsure what style is preferred here. We could (a) order header 
> > groups by directory depth (that's e.g., the reason 
> > `slave/containerizer/mesos/linux_launcher.hpp` is on l.29, not below l.36, 
> > or (b) keep related headers together. I can see both patterns in the code 
> > base. Maybe you have an idea.

I'd prefer the former (sort by directory depth, then lexicographically).


- Neil


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


On Nov. 3, 2016, 4:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53435/
> ---
> 
> (Updated Nov. 3, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed header include order.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4df537747d84daa68c29e2d05b22fa386a4a16db 
> 
> Diff: https://reviews.apache.org/r/53435/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 53475: Use explicit error codes in ErrnoError and SocketError.

[James Peach]

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

Review request for mesos and switched to 'mcypark'.


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


Repository: mesos


Description
---

Pass the error code down to the Error object explicitly, rather than
setting the per-thread error and using it implicitly.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 
  3rdparty/libprocess/src/poll_socket.cpp 
f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 

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


Testing
---

make check on Fedora 24


Thanks,

James Peach

Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

[James Peach]

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

Review request for mesos and switched to 'mcypark'.


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


Repository: mesos


Description
---

Support explicit error codes in ErrnoError and SocketError.


Diffs
-

  3rdparty/stout/include/stout/errorbase.hpp 
96d395d37cdad706c2f23fdd9caeefc2a33e0541 
  3rdparty/stout/include/stout/os/posix/rmdir.hpp 
5657b5a18aaedf842b7b780ea7eada73118788cb 
  3rdparty/stout/include/stout/windows/error.hpp 
f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
  3rdparty/stout/tests/error_tests.cpp 25b50141dd9bbf163aa816750210b298456740a8 

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


Testing
---

make check on Fedora 24


Thanks,

James Peach