Re: Review Request 53624: Add a content type option to mesos-execute.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53624]

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. 11, 2016, 12:49 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53624/
> ---
> 
> (Updated Nov. 11, 2016, 12:49 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6562
> https://issues.apache.org/jira/browse/MESOS-6562
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using the JSON content type in mesos-execute makes packet tracing
> protocol interactions easier. Add a --content_type option that
> switched between protobuf (the default) and JSON content types.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 
> 
> Diff: https://reviews.apache.org/r/53624/diff/
> 
> 
> Testing
> ---
> 
> make check. Manually verified with tcpdump.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53645: Added '--task' into mesos-execute.

[Qian Zhang]

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

(Updated Nov. 11, 2016, 11:13 a.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Vinod Kone.


Changes
---

Addressed Avinash's comments.


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


Repository: mesos


Description
---

Added '--task' into mesos-execute.


Diffs (updated)
-

  src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 

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


Testing
---

On Ubuntu 16.04, manually ran the command below to successfuly launch a 
container which will be attached to a CNI network "net1".
```
# sudo src/mesos-execute --master=192.168.122.216:5050 
--task=file:///home/stack/workspace/mesos/build/task.json

# cat /home/stack/workspace/mesos/build/task.json
{
  "name": "test",
  "task_id": {"value" : "test"},
  "agent_id": {"value" : ""},
  "resources": [
{
  "name": "cpus",
  "type": "SCALAR",
  "scalar": {
"value": 0.1
  },
  "role": "*"
},
{
  "name": "mem",
  "type": "SCALAR",
  "scalar": {
"value": 32
  },
  "role": "*"
}
  ],
  "command": {
"value": "ifconfig"
  },
  "container": {
"type": "MESOS",
"mesos": {
  "image": {
"type": "DOCKER",
"docker": {
  "name": "busybox"
}
  }
},
"network_infos": [
  {
"name": "net1"
  }
]
  }
}
```


Thanks,

Qian Zhang

Re: Review Request 53645: Added '--task' into mesos-execute.

[Qian Zhang]

> On Nov. 11, 2016, 12:01 a.m., Avinash sridharan wrote:
> > src/cli/execute.cpp, line 1065
> > 
> >
> > Instead of this why can't we just do:
> > 
> > Option taskInfo = flags.task
> > 
> > if (flags.task.isNone() && flags.task_group.isNone()) {
> > 
> > }
> > 
> > This would be more explicit than using flags.name.isSome() for implying 
> > the above condition.

Yes, we can do it, thanks for the comments!


- Qian


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


On Nov. 10, 2016, 9:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53645/
> ---
> 
> (Updated Nov. 10, 2016, 9:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6571
> https://issues.apache.org/jira/browse/MESOS-6571
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added '--task' into mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 
> 
> Diff: https://reviews.apache.org/r/53645/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04, manually ran the command below to successfuly launch a 
> container which will be attached to a CNI network "net1".
> ```
> # sudo src/mesos-execute --master=192.168.122.216:5050 
> --task=file:///home/stack/workspace/mesos/build/task.json
> 
> # cat /home/stack/workspace/mesos/build/task.json
> {
>   "name": "test",
>   "task_id": {"value" : "test"},
>   "agent_id": {"value" : ""},
>   "resources": [
> {
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
> "value": 0.1
>   },
>   "role": "*"
> },
> {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
> "value": 32
>   },
>   "role": "*"
> }
>   ],
>   "command": {
> "value": "ifconfig"
>   },
>   "container": {
> "type": "MESOS",
> "mesos": {
>   "image": {
> "type": "DOCKER",
> "docker": {
>   "name": "busybox"
> }
>   }
> },
> "network_infos": [
>   {
> "name": "net1"
>   }
> ]
>   }
> }
> ```
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 326 - 346)


In fact, I'd prefer moving this up before we create the Info struct in 
'infos' map. We only keep stuff in 'infos' map if cleanup is needed for the 
container.



src/slave/containerizer/mesos/isolators/gpu/isolator.cpp (lines 279 - 282)


GPU isolator should depends on filesystem/linux isolator so that mount in 
the child do not propagate back to the host mount table.

Once you have this dependency, no need to explicitly set 'enter_namespaces' 
here because it's implied by filesystem/linux isolator. Probably return None() 
here because no additional namespaces are needed.



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


I'd remove CLONE_NEWNS here because this is implied by the dependency to 
filesystem/linux isolator.



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


Given that this isolator depends on filesystem/linux isolator, let's remove 
the CLONE_NEWNS here.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 524 - 530)


This does not look right. network/cni isolator might be used with posix 
launcher or filesystem/posix isolator on linux. It'll be turn on by default on 
linux. The purpose of that is because we want to reject the cases where the 
user specifies a named network in NetworkInfo.

Therefore, we need to consider the case where the host network is used and 
filesystem/posix isolator and posix launcher is used. To debug such a 
container, we should not specify any namespaces to enter or clone because it'll 
not be supported by posix launcher.

So we should not shortcut here and bindly assume that we have namespace 
support. We need to do this down below where we know 
`!containerNetworks.empty()`.



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


Why PID namespace here? Also, where is UTS namesapce?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 638 - 645)


If control reaches here, it's on linux and linux launcher was used. It's 
still possible that filesystem/posix is used.

We don't want to create 'info' for DEBUG container, so you need skip this 
block.



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


we need to distinguish between debug class or default class here.

In both cases, we need to enter the network, uts and mnt namespace of the 
parent.

For default nested container, we need to clone a mount namespace.



src/slave/containerizer/mesos/isolators/volume/image.cpp (lines 87 - 100)


I'd suggest for this volume isolator, we put a dependency on 
filesystem/linux isolator (in 'create').

Since filesystem/linux isolator will properly set 'enter_namespaces', no 
need to set it here.

I think the namesapces set in LaunchInfo should be the additional 
namesspaces needed by this isolator.



src/slave/containerizer/mesos/isolators/volume/image.cpp (line 115)


I would add a check here. Image volume is not supported for DEBUG 
container. We should return an Error if container is DEBUG class.



src/slave/containerizer/mesos/isolators/volume/image.cpp (line 195)


If we add the dependency of filesystem/linux isolator, we should probably 
just remove this because this is implied by the dependency.



src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp (lines 132 - 
138)


I would return None() here. This is consistnet with the code above in this 
function:
```
if (!containerConfig.has_container_info()) {
  return None();
}
```

If bindMountSupported is true, that implies the dependency on 
filesystem/linux isolator, which will properly set the 'enter_namespace'.

Otherwise, the logic is a bit weird becasue why not set 'enter_namespace' 
if container_info is not set?



src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp (line 152)


I would actually add a check here and return Error if 

Re: Review Request 53654: Avoided unnecessary copies of `HttpConnection`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53654]

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. 10, 2016, 6:46 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53654/
> ---
> 
> (Updated Nov. 10, 2016, 6:46 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes two places where we were needlessly copying an
> `HttpConnection`. Aside from the performance consequences, making a copy
> suggests to the reader that a copy is semantically necessary, which is
> not the case here.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 87186c6e733a686f96528b1722fda1c287e9c881 
>   src/master/master.cpp f0b629737ace71f14f5c21eb918c3ef1da3c0837 
> 
> Diff: https://reviews.apache.org/r/53654/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53457: Updated Socket::Impl::accept to return std::shared_ptr.

[Benjamin Mahler]

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


Fix it, then Ship it!




Modulo the leak path described below:


3rdparty/libprocess/src/poll_socket.cpp (lines 104 - 110)


This introduces a leakable path for the 's' file descriptor: if create 
fails, 's' is not closed.
It turns out that create cannot fail currently, but of course we don't want 
to rely on this :)

Seems we could either continue to have accept return a Socket::Impl to 
avoid the caller here having to think about closing, or we can perform an 
os::close if create fails.


- Benjamin Mahler


On Nov. 4, 2016, 6:40 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53457/
> ---
> 
> (Updated Nov. 4, 2016, 6:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Socket::Impl::accept to return std::shared_ptr.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> f798af7879546d71e8ef4a295c9cf489a70cb61f 
>   3rdparty/libprocess/src/poll_socket.hpp 
> d04f3f2d1bcf70464ac659b29f96574bbd233414 
>   3rdparty/libprocess/src/poll_socket.cpp 
> f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 
> 
> Diff: https://reviews.apache.org/r/53457/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 53461: Inlined function only used one place.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Nov. 4, 2016, 6:42 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53461/
> ---
> 
> (Updated Nov. 4, 2016, 6:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Inlined function only used one place.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/net.hpp 
> 083b8d1959dcd305d0aa8892082b39cb03786ebf 
> 
> Diff: https://reviews.apache.org/r/53461/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

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

[Benjamin Mahler]

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



I've thought about this before as well, since Future is a shared object, adding 
a non-const operator isn't very safe. There can be multiple sites holding this 
Future and it's not clear who has the ownership necessary to perform non-const 
operations. My thinking about this in the past was that we'd need to introduce 
an explicit type (e.g. OwnedFuture) in order to enforce that there is a single 
owner that can perform non-const operations, move out the underyling object, 
etc.

- Benjamin Mahler


On Nov. 4, 2016, 5:30 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53492/
> ---
> 
> (Updated Nov. 4, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Michael Park.
> 
> 
> 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 53624: Add a content type option to mesos-execute.

[James Peach]

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

(Updated Nov. 11, 2016, 12:49 a.m.)


Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.


Changes
---

Added the --content_type option.


Summary (updated)
-

Add a content type option to mesos-execute.


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


Repository: mesos


Description (updated)
---

Using the JSON content type in mesos-execute makes packet tracing
protocol interactions easier. Add a --content_type option that
switched between protobuf (the default) and JSON content types.


Diffs (updated)
-

  src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 

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


Testing
---

make check. Manually verified with tcpdump.


Thanks,

James Peach

Re: Review Request 53366: Introduced a streaming gzip::Decompressor.

[Anand Mazumdar]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/gzip.hpp (line 22)


Kill this since it's not needed anymore.


- Anand Mazumdar


On Nov. 11, 2016, 12:36 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53366/
> ---
> 
> (Updated Nov. 11, 2016, 12:36 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6530
> https://issues.apache.org/jira/browse/MESOS-6530
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This enables incremental decompression of compressed input.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gzip.hpp 
> b78a8a31204ee585f8e4a88eaefe7346faa46b8d 
>   3rdparty/stout/tests/gzip_tests.cpp 
> 814fb99da193cf950ba48817824acdabda2c2dfc 
> 
> Diff: https://reviews.apache.org/r/53366/diff/
> 
> 
> Testing
> ---
> 
> Wrote a test for decompressing a byte at a time. Also ran this test manually 
> with random chunk sizes.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 53366: Introduced a streaming gzip::Decompressor.

[Benjamin Mahler]

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

(Updated Nov. 11, 2016, 12:36 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Moved `finish` to `finished() const` per discussion.


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


Repository: mesos


Description
---

This enables incremental decompression of compressed input.


Diffs (updated)
-

  3rdparty/stout/include/stout/gzip.hpp 
b78a8a31204ee585f8e4a88eaefe7346faa46b8d 
  3rdparty/stout/tests/gzip_tests.cpp 814fb99da193cf950ba48817824acdabda2c2dfc 

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


Testing
---

Wrote a test for decompressing a byte at a time. Also ran this test manually 
with random chunk sizes.


Thanks,

Benjamin Mahler

Re: Review Request 53365: Fixed an issue in the gzip error handling.

[Benjamin Mahler]

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

(Updated Nov. 11, 2016, 12:33 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Updated the description to reflect the latest changes.


Repository: mesos


Description (updated)
---

It turns out that zlib does not always set the `msg` field, the
calling code is expected to handle the case where `msg` is NULL.
I discovered this while I was playing with the library during
the implementation of a streaming decompressor.

To resolve this issue, this patch introduces an GzipError,
currently hidden to callers within an internal namespace,
in order to produce error messages in a consistent (safe)
manner.

In addition, this patch moves the (in|de)flateInit2 and
(in|de)flateEnd failures to ABORT the program, since these
correspond to programming errors and we will need to invoke
these constructors and destructors once we have streaming
(de)compression support.


Diffs (updated)
-

  3rdparty/stout/include/stout/gzip.hpp 
b78a8a31204ee585f8e4a88eaefe7346faa46b8d 

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


Testing
---

make check


Thanks,

Benjamin Mahler

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

(Updated Nov. 11, 2016, 12:20 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Fixed a typo.


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

  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 53366: Introduced a streaming gzip::Decompressor.

[Benjamin Mahler]

> On Nov. 10, 2016, 11:45 p.m., Anand Mazumdar wrote:
> > 3rdparty/stout/include/stout/gzip.hpp, lines 157-168
> > 
> >
> > hmm, this is a little weird. Why can't this be just be a `bool 
> > failed()` similar to what we have for the decoders i.e., to me decoding 
> > seems similar to decompressing semantically?

Per our discussion offline, this evolved poorly as it turns out that there is 
no need to "finish" the stream. The intention then was for the caller to see if 
gzip considers the stream to be "finished", which means no more input is 
expected. However, I didn't update the method name so I'll update this to be a 
const method and we can call it "finished".


- Benjamin


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


On Nov. 8, 2016, 4:22 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53366/
> ---
> 
> (Updated Nov. 8, 2016, 4:22 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6530
> https://issues.apache.org/jira/browse/MESOS-6530
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This enables incremental decompression of compressed input.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gzip.hpp 
> b78a8a31204ee585f8e4a88eaefe7346faa46b8d 
>   3rdparty/stout/tests/gzip_tests.cpp 
> 814fb99da193cf950ba48817824acdabda2c2dfc 
> 
> Diff: https://reviews.apache.org/r/53366/diff/
> 
> 
> Testing
> ---
> 
> Wrote a test for decompressing a byte at a time. Also ran this test manually 
> with random chunk sizes.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 53366: Introduced a streaming gzip::Decompressor.

[Anand Mazumdar]

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



LGTM minus my comment around renaming `finish()` -> `failed()`.


3rdparty/stout/include/stout/gzip.hpp (lines 157 - 168)


hmm, this is a little weird. Why can't this be just be a `bool failed()` 
similar to what we have for the decoders i.e., to me decoding seems similar to 
decompressing semantically?


- Anand Mazumdar


On Nov. 8, 2016, 4:22 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53366/
> ---
> 
> (Updated Nov. 8, 2016, 4:22 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6530
> https://issues.apache.org/jira/browse/MESOS-6530
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This enables incremental decompression of compressed input.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gzip.hpp 
> b78a8a31204ee585f8e4a88eaefe7346faa46b8d 
>   3rdparty/stout/tests/gzip_tests.cpp 
> 814fb99da193cf950ba48817824acdabda2c2dfc 
> 
> Diff: https://reviews.apache.org/r/53366/diff/
> 
> 
> Testing
> ---
> 
> Wrote a test for decompressing a byte at a time. Also ran this test manually 
> with random chunk sizes.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 53485: Introduced a `readAll()` helper on `http::Pipe::Reader`.

[Benjamin Mahler]

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


Ship it!





3rdparty/libprocess/src/http.cpp (line 436)


Should we do the following?

```
return std::move(*buffer);
```

This would allow us to move into the returned Future once there is a move 
constructor.



3rdparty/libprocess/src/tests/http_tests.cpp (line 415)


Looking at the above tests (e.g. HTTPTest.PipeFailure), looks like you 
don't need the string here? i.e.

```
AWAIT_EXPECT_EQ("helloworld", readAll);
```


- Benjamin Mahler


On Nov. 10, 2016, 10:02 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53485/
> ---
> 
> (Updated Nov. 10, 2016, 10:02 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/http.hpp 
> 9722c6210526479da243ae9945e4c9a89ecb9009 
>   3rdparty/libprocess/src/http.cpp b5ddac9f1970358f8232a481431507242052d294 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 533104c93dd1eaf67bf3752163d2e0cad090078d 
> 
> Diff: https://reviews.apache.org/r/53485/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 53628: Document the namespaces/uts isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53626, 53627, 53628]

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. 10, 2016, 5:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53628/
> ---
> 
> (Updated Nov. 10, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6556
> https://issues.apache.org/jira/browse/MESOS-6556
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document the namespaces/uts isolator.
> 
> 
> Diffs
> -
> 
>   docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 
> 
> Diff: https://reviews.apache.org/r/53628/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53365: Fixed an issue in the gzip error handling.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Nov. 8, 2016, 4:22 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53365/
> ---
> 
> (Updated Nov. 8, 2016, 4:22 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It turns out that zlib does not always set the `msg` field, the
> calling code is expected to handle the case where `msg` is NULL.
> I discovered this while I was playing with the library during
> the implementation of a streaming decompressor.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gzip.hpp 
> b78a8a31204ee585f8e4a88eaefe7346faa46b8d 
> 
> Diff: https://reviews.apache.org/r/53365/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Anand Mazumdar]

> On Nov. 9, 2016, 12:57 a.m., Benjamin Mahler wrote:
> > Very clean update to the test! I left a comment about whether we can do 
> > this similar parameterization on the response side, perhaps you'll want to 
> > leave a TODO for this if you don't want to take it on now?

I left a TODO for now to clean it up later.


- Anand


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


On Nov. 10, 2016, 10:07 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53511/
> ---
> 
> (Updated Nov. 10, 2016, 10:07 p.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 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/
---

(Updated Nov. 10, 2016, 10:31 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Removed code added in last diff.


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

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

(Updated Nov. 10, 2016, 10:20 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments


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

  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 53490: Added a test for request streaming via the connection abstraction.

[Anand Mazumdar]

> On Nov. 9, 2016, 9:50 p.m., Benjamin Mahler wrote:
> > What you have looks good, I would just suggest that you also cover the case 
> > where the request body does not complete to EOF, but fails. In this case 
> > the server should not receive the request at all (if 
> > !RouteOptions.streaming) or should see the request.pipe fail (if 
> > RouteOptions.streaming).
> > 
> > I looked through the existing tests for the response streaming, they are 
> > pretty lacking, you can blame me for that :)
> > Also, they are a bit inconsistent with how you added the test here, so 
> > hopefully at some point we can circle back and clean up the existing tests.

hmm, `socket.shutdown()` only closes the receive end of the socket. It's not 
quite possible to add the test for the latter case as the server never realizes 
that the client went away. We need to fix that before a test can be added. 
Would file an issue and assign to me.


- Anand


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


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

(Updated Nov. 10, 2016, 10:18 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments, NNFR


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

  3rdparty/libprocess/src/http.cpp b5ddac9f1970358f8232a481431507242052d294 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 53624: Use JSON content type in mesos-execute.

[Vinod Kone]

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




src/cli/execute.cpp (line 299)


thinking a bit more, this flag should be "content_type" with "protobuf" as 
default. that way if we support more content types in the future, this would be 
future proof.


- Vinod Kone


On Nov. 10, 2016, 5:46 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53624/
> ---
> 
> (Updated Nov. 10, 2016, 5:46 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6562
> https://issues.apache.org/jira/browse/MESOS-6562
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use JSON content type in mesos-execute so that packet tracing
> protocol interactions is easier.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 
> 
> Diff: https://reviews.apache.org/r/53624/diff/
> 
> 
> Testing
> ---
> 
> make check. Manually verified with tcpdump.
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Anand Mazumdar]

> On Nov. 9, 2016, 3:44 a.m., Benjamin Mahler wrote:
> > Unrelated to this particular change, but can you write up a quick test 
> > (doesn't need to be committed) to ensure that we don't have infinitely 
> > growing future chains here, as benh mentioned offline? I never tested this 
> > for http::Connection, but I suspect we may have a memory growth problem if 
> > we open a connection and loop sending requests on it (try with both pipe 
> > and body).

Sure, as discussed offline, would look up the memory usage of a example test 
framework. Many of them use the `Connection` abstraction under the hood.


> On Nov. 9, 2016, 3:44 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 898-900
> > 
> >
> > Do you want to clear the content length header if they are using a pipe?

We already check that content-length is not set for piped requests and fail the 
request in `send()`. I would add a explicit invariant check.


- Anand


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


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

Re: Review Request 53624: Use JSON content type in mesos-execute.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Nov. 10, 2016, 5:46 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53624/
> ---
> 
> (Updated Nov. 10, 2016, 5:46 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6562
> https://issues.apache.org/jira/browse/MESOS-6562
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use JSON content type in mesos-execute so that packet tracing
> protocol interactions is easier.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 
> 
> Diff: https://reviews.apache.org/r/53624/diff/
> 
> 
> Testing
> ---
> 
> make check. Manually verified with tcpdump.
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Anand Mazumdar]

> On Nov. 9, 2016, 1:16 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/http.cpp, lines 926-928
> > 
> >
> > Before you commit this, can you make sure that this looks consistent 
> > with the request conversion function in your previous review?
> > 
> > Also, this currently copies the response (twice), is it possible to 
> > avoid this, much like we did in your previous review?

As discussed offline, this would need a bit more work to argue around lifetime 
issues of the `Response` object.


- Anand


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


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

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

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


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments, NNFR.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  3rdparty/libprocess/src/http.cpp b5ddac9f1970358f8232a481431507242052d294 

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


Testing
---

make check


Thanks,

Anand Mazumdar

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

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


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments, NNFR.


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

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

(Updated Nov. 10, 2016, 10:04 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments, NNFR


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

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

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


Testing
---


Thanks,

Anand Mazumdar

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

[Anand Mazumdar]

> On Nov. 5, 2016, 1:42 a.m., Benjamin Mahler wrote:
> > Nice commit summary! Would you mind adding a test alongside this? Should be 
> > pretty straightforward to do.

Added.


> On Nov. 5, 2016, 1:42 a.m., Benjamin Mahler wrote:
> > 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.

Dropping since its not relevant due to the function not being there anymore.


- Anand


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


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 53485: Introduced a `readAll()` helper on `http::Pipe::Reader`.

[Anand Mazumdar]

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

(Updated Nov. 10, 2016, 10:02 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments


Summary (updated)
-

Introduced a `readAll()` helper on `http::Pipe::Reader`.


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

  3rdparty/libprocess/include/process/http.hpp 
9722c6210526479da243ae9945e4c9a89ecb9009 
  3rdparty/libprocess/src/http.cpp b5ddac9f1970358f8232a481431507242052d294 
  3rdparty/libprocess/src/tests/http_tests.cpp 
533104c93dd1eaf67bf3752163d2e0cad090078d 

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


Testing
---

make check


Thanks,

Anand Mazumdar

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

[Anand Mazumdar]

> On Nov. 5, 2016, 1:37 a.m., Benjamin Mahler wrote:
> > 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.

Renamed to `requestStreaming`.


- Anand


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


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


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments, NNFR.


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

Re: Review Request 53483: Introduced a reader member to `Request` to support request streaming.

[Anand Mazumdar]

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

(Updated Nov. 10, 2016, 10 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments, NNFR


Summary (updated)
-

Introduced a reader member to `Request` to support request streaming.


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

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

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


Testing
---

make check


Thanks,

Anand Mazumdar

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

[Anand Mazumdar]

> On Nov. 5, 2016, 1:01 a.m., Benjamin Mahler wrote:
> > Actually whoops, we rely on the generated constructor, so this doesn't seem 
> > like an issue?

As discussed offline, we still need this as POD types are not initialized by 
the default generated constructor.


- Anand


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


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

(Updated Nov. 10, 2016, 9:59 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Review comments, NNFR


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

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

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 53623: Windows: Fixed provisioner copy backend build error.

[Joseph Wu]

> On Nov. 9, 2016, 9:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/backends/copy.cpp, lines 269-272
> > 
> >
> > I'd move this to `provision` and bail out early (before mkdir)

Even if we bail out in `::provision`, we still need this chunk (`#ifndef` and 
`#else`) for the method to have a `return` statement.

Alternatively, I'd need to add #ifdef's in the copy backend's header too.


- Joseph


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


On Nov. 9, 2016, 4:38 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53623/
> ---
> 
> (Updated Nov. 9, 2016, 4:38 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The provisioner's `copy` backend has never been supported on Windows,
> but we have included it in the build to reduce the number of `#ifdef`s
> needed to compile the agent.  If the user attempts to specify a rootfs
> on Windows, it will still fail in the containerizer's validation logic.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> 7e98110af9d105c803518a29cd2e74b7464fcb1e 
> 
> Diff: https://reviews.apache.org/r/53623/diff/
> 
> 
> Testing
> ---
> 
> make check (CentOS 7)
> msbuild ... (Windows 10)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53586: Added special case for entering "mnt" namespaces for DEBUG containers.

[Kevin Klues]

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

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


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie's comments.


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


Repository: mesos


Description
---

Until we switch over to the default (a.k.a. "pod" executor) for
launching command tasks, we need to special case which `pid` we use
for entering the `mnt` namespace of a parent container.  Specifically,
we need to enter the `mnt` namespace of the process representing the
command task itself, not the `mnt` namespace of the `init` process of
the container or the `executor` of the container because these run in
the same `mnt` namespace as the agent (not the task).

Unfortunately, there is no easy way to get the `pid` of tasks launched
with the command executor because we only checkpoint the `pid` of the
`init` process of these containers. For now, we compensate for this by
simply walking the process tree from the container's `init` process up
to 2-levels down (where the task process would exist) and look to see
if any process along the way has a different `mnt` namespace. If it
does, we return a reference to its `pid` as the `pid` for entering the
`mnt` namespace of the container.  Otherwise, we return the `init`
process's `pid`.

We then pass this pid to the `mesos-containerizer launch` binary and
have it set the namespace, rather than letting the `ns::clone()` call
do it for us. This is important because otherwise we wouldn't be able
to find the `mesos-containerizer launch` itself (it only exists in the
host mount namespace!).


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
44225ebf63d8dd93be9b60fff496c74dc6c3a5ad 
  src/slave/containerizer/mesos/launch.hpp 
8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
  src/slave/containerizer/mesos/launch.cpp 
377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
  src/slave/containerizer/mesos/utils.hpp 
f24215b52b5fa95321b15b57468660aab4d1aefc 
  src/slave/containerizer/mesos/utils.cpp 
237aea4510dc80e9b3d39d577aa702dfb1f554db 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
e6c690c411f57138207044f31b4816bd4090c1b7 

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


Testing
---

make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 53586: Added special case for entering "mnt" namespaces for DEBUG containers.

[Kevin Klues]

> On Nov. 10, 2016, 8:50 p.m., Jie Yu wrote:
> > src/tests/containerizer/nested_mesos_containerizer_tests.cpp, lines 433-436
> > 
> >
> > Is this necessary? I think once we get a TASK_RUNNING, the subprocess 
> > should have been already launched?

Before I added this, the test was flaky. The debug container would sometimes 
end up looking for `/etc/alpine-release` in the wrong mount namespace. There is 
a (currently) unavoidable race here because the agent only synchronizes with 
the `mesos-containerizer launch` helper in one direction -- i.e. the helper 
will wait for the agent to signal it to continue after all isolators have been 
called, but there is no signal from the helper back to the agent to tell it to 
wait until the helper has entered the proper mount namespace before continuing.


- Kevin


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


On Nov. 10, 2016, 8:25 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53586/
> ---
> 
> (Updated Nov. 10, 2016, 8:25 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6543
> https://issues.apache.org/jira/browse/MESOS-6543
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Until we switch over to the default (a.k.a. "pod" executor) for
> launching command tasks, we need to special case which `pid` we use
> for entering the `mnt` namespace of a parent container.  Specifically,
> we need to enter the `mnt` namespace of the process representing the
> command task itself, not the `mnt` namespace of the `init` process of
> the container or the `executor` of the container because these run in
> the same `mnt` namespace as the agent (not the task).
> 
> Unfortunately, there is no easy way to get the `pid` of tasks launched
> with the command executor because we only checkpoint the `pid` of the
> `init` process of these containers. For now, we compensate for this by
> simply walking the process tree from the container's `init` process up
> to 2-levels down (where the task process would exist) and look to see
> if any process along the way has a different `mnt` namespace. If it
> does, we return a reference to its `pid` as the `pid` for entering the
> `mnt` namespace of the container.  Otherwise, we return the `init`
> process's `pid`.
> 
> We then pass this pid to the `mesos-containerizer launch` binary and
> have it set the namespace, rather than letting the `ns::clone()` call
> do it for us. This is important because otherwise we wouldn't be able
> to find the `mesos-containerizer launch` itself (it only exists in the
> host mount namespace!).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 44225ebf63d8dd93be9b60fff496c74dc6c3a5ad 
>   src/slave/containerizer/mesos/launch.hpp 
> 8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
>   src/slave/containerizer/mesos/launch.cpp 
> 377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
>   src/slave/containerizer/mesos/utils.hpp 
> f24215b52b5fa95321b15b57468660aab4d1aefc 
>   src/slave/containerizer/mesos/utils.cpp 
> 237aea4510dc80e9b3d39d577aa702dfb1f554db 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> e6c690c411f57138207044f31b4816bd4090c1b7 
> 
> Diff: https://reviews.apache.org/r/53586/diff/
> 
> 
> Testing
> ---
> 
> make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 53586: Added special case for entering "mnt" namespaces for DEBUG containers.

[Jie Yu]

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


Fix it, then Ship it!





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


Let's move this function to `src/slave/containerizer/mesos/utils.hpp|cpp`



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


for primitive types, we don't use const ref



src/slave/containerizer/mesos/launch.cpp (line 412)


insert a blank line below



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


Why this? Can you just let the agent detect the resources on the box?



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 401 - 403)


You can use 'createDockerImage'



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 416 - 418)


Can you use 'createContainerInfo'



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 433 - 436)


Is this necessary? I think once we get a TASK_RUNNING, the subprocess 
should have been already launched?


- Jie Yu


On Nov. 10, 2016, 8:25 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53586/
> ---
> 
> (Updated Nov. 10, 2016, 8:25 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6543
> https://issues.apache.org/jira/browse/MESOS-6543
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Until we switch over to the default (a.k.a. "pod" executor) for
> launching command tasks, we need to special case which `pid` we use
> for entering the `mnt` namespace of a parent container.  Specifically,
> we need to enter the `mnt` namespace of the process representing the
> command task itself, not the `mnt` namespace of the `init` process of
> the container or the `executor` of the container because these run in
> the same `mnt` namespace as the agent (not the task).
> 
> Unfortunately, there is no easy way to get the `pid` of tasks launched
> with the command executor because we only checkpoint the `pid` of the
> `init` process of these containers. For now, we compensate for this by
> simply walking the process tree from the container's `init` process up
> to 2-levels down (where the task process would exist) and look to see
> if any process along the way has a different `mnt` namespace. If it
> does, we return a reference to its `pid` as the `pid` for entering the
> `mnt` namespace of the container.  Otherwise, we return the `init`
> process's `pid`.
> 
> We then pass this pid to the `mesos-containerizer launch` binary and
> have it set the namespace, rather than letting the `ns::clone()` call
> do it for us. This is important because otherwise we wouldn't be able
> to find the `mesos-containerizer launch` itself (it only exists in the
> host mount namespace!).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 44225ebf63d8dd93be9b60fff496c74dc6c3a5ad 
>   src/slave/containerizer/mesos/launch.hpp 
> 8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
>   src/slave/containerizer/mesos/launch.cpp 
> 377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
>   src/slave/containerizer/mesos/utils.hpp 
> f24215b52b5fa95321b15b57468660aab4d1aefc 
>   src/slave/containerizer/mesos/utils.cpp 
> 237aea4510dc80e9b3d39d577aa702dfb1f554db 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> e6c690c411f57138207044f31b4816bd4090c1b7 
> 
> Diff: https://reviews.apache.org/r/53586/diff/
> 
> 
> Testing
> ---
> 
> make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 53586: Added special case for entering "mnt" namespaces for DEBUG containers.

[Kevin Klues]

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

(Updated Nov. 10, 2016, 8:25 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on offline comments from Jie to move the 
`getMountNamespaceTarget()` function into `utils.cpp`.


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


Repository: mesos


Description
---

Until we switch over to the default (a.k.a. "pod" executor) for
launching command tasks, we need to special case which `pid` we use
for entering the `mnt` namespace of a parent container.  Specifically,
we need to enter the `mnt` namespace of the process representing the
command task itself, not the `mnt` namespace of the `init` process of
the container or the `executor` of the container because these run in
the same `mnt` namespace as the agent (not the task).

Unfortunately, there is no easy way to get the `pid` of tasks launched
with the command executor because we only checkpoint the `pid` of the
`init` process of these containers. For now, we compensate for this by
simply walking the process tree from the container's `init` process up
to 2-levels down (where the task process would exist) and look to see
if any process along the way has a different `mnt` namespace. If it
does, we return a reference to its `pid` as the `pid` for entering the
`mnt` namespace of the container.  Otherwise, we return the `init`
process's `pid`.

We then pass this pid to the `mesos-containerizer launch` binary and
have it set the namespace, rather than letting the `ns::clone()` call
do it for us. This is important because otherwise we wouldn't be able
to find the `mesos-containerizer launch` itself (it only exists in the
host mount namespace!).


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
44225ebf63d8dd93be9b60fff496c74dc6c3a5ad 
  src/slave/containerizer/mesos/launch.hpp 
8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
  src/slave/containerizer/mesos/launch.cpp 
377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
  src/slave/containerizer/mesos/utils.hpp 
f24215b52b5fa95321b15b57468660aab4d1aefc 
  src/slave/containerizer/mesos/utils.cpp 
237aea4510dc80e9b3d39d577aa702dfb1f554db 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
e6c690c411f57138207044f31b4816bd4090c1b7 

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


Testing
---

make -j check
sudo src/mesos-tests


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


Review request for mesos and Jie Yu.


Changes
---

Updated to remove erroneous `using` directive.


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 
44225ebf63d8dd93be9b60fff496c74dc6c3a5ad 
  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 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

> On Nov. 5, 2016, 2:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1405
> > 
> >
> > I'll be nice to use environment variable for all flags here. I think we 
> > probably need a helper in FlagsBase to return the corresponding environment 
> > variables for the flags:
> > ```
> > // Export flags as environment variables.
> > map FlagsBase::environment(
> > const Option& prefix);
> > ```
> > 
> > And then, we can just merge with os::environment and pass it to 
> > Launcher::fork
> 
> Jiang Yan Xu wrote:
> This helper looks like handy. 
> 
> I am not necessarily against this but wanted us to give it more thought. 
> 
> 1. Should all Mesos libexec binaries work this way?
> 2. Removing all of the flags makes all the helper binary instances look 
> the same through `ps`. Yes privileged users can still get the info in /proc 
> for individual processes but we loose  we lose easy "grep-pability" for 
> debugging. 
> 
> To me the line to draw is between user-supplied info (prefer env vars) 
> and Mesos generated info (e.g., runtime dir which encodes contianerId) which 
> is not sensitive. Thoughts?
> 
> Jie Yu wrote:
> This is just a suggestion. That's the reason I didn't put an issue there.
> 
> For 1, yes, I would say so because libexec binaries are Mesos details, 
> which should not be exposed to users
> For 2, one can still log that in the agent log.
> 
> But we can do that later, we don't have to do it now.

I captured it here: https://issues.apache.org/jira/browse/MESOS-6573


- Jiang Yan


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


On Nov. 8, 2016, 8:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 8:46 a.m.)
> 
> 
> 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 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   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 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

> On Nov. 9, 2016, 2:37 a.m., Gastón Kleiman wrote:
> > The Docker containerizer still passes the env variables to the executor 
> > through cmd line flags, we might want to fix that as well:
> > 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L244
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L351-L372
> 
> Gastón Kleiman wrote:
> I just checked and the env variables are not visible in the 
> `DockerExecutor` cmd line, but the executor passes them to the `docker` 
> binary using the cmd line.
> 
> See: https://issues.apache.org/jira/browse/MESOS-6566

Thanks Gaston for filing the ticket. :)


- Jiang Yan


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


On Nov. 8, 2016, 8:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 8:46 a.m.)
> 
> 
> 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 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   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
> 
>

Review Request 53654: Avoided unnecessary copies of `HttpConnection`.

[Neil Conway]

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

Review request for mesos and Anand Mazumdar.


Repository: mesos


Description
---

This fixes two places where we were needlessly copying an
`HttpConnection`. Aside from the performance consequences, making a copy
suggests to the reader that a copy is semantically necessary, which is
not the case here.


Diffs
-

  src/master/master.hpp 87186c6e733a686f96528b1722fda1c287e9c881 
  src/master/master.cpp f0b629737ace71f14f5c21eb918c3ef1da3c0837 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52919: Updated scripts to allow override of MESOS environment variables.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52856, 52787, 52919]

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. 10, 2016, 3:44 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52919/
> ---
> 
> (Updated Nov. 10, 2016, 3:44 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6399
> https://issues.apache.org/jira/browse/MESOS-6399
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated scripts to allow override of MESOS environment variables.
> 
> 
> Diffs
> -
> 
>   bin/mesos-agent-flags.sh.in 8c936aa06e994a87a8b09b31c907868bf9be38c7 
>   bin/mesos-master-flags.sh.in 951c3e4c714809bcf2364eb28974aeefb7884bcb 
> 
> Diff: https://reviews.apache.org/r/52919/diff/
> 
> 
> Testing
> ---
> 
> ```
> $ MESOS_TEMP_DIR=/tmp/mesos/bar ./bin/mesos-local.sh
> 
> $ tree /tmp/mesos/
> /tmp/mesos/
> ??? bar
> ??? agents
> ?   ??? 0
> ?   ??? runtime_dir
> ?   ??? work_dir
> ?   ??? meta
> ?   ?   ??? boot_id
> ?   ?   ??? slaves
> ?   ?   ??? 6b4ce717-dd6e-4d8c-9f89-66a42e3866a8-S0
> ?   ?   ?   ??? slave.info
> ?   ?   ??? latest -> 
> /tmp/mesos/bar/agents/0/work_dir/meta/slaves/6b4ce717-dd6e-4d8c-9f89-66a42e3866a8-S0
> ?   ??? provisioner
> ??? master
> ??? work_dir
> ??? replicated_log
> ??? 04.log
> ??? CURRENT
> ??? LOCK
> ??? LOG
> ??? MANIFEST-02
> 
> 13 directories, 7 files
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 53626: Add net::setDomainname() helper API.

[James Peach]

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

(Updated Nov. 10, 2016, 5:52 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Add net::setDomainname() helper API.


Diffs (updated)
-

  3rdparty/stout/include/stout/posix/net.hpp 
39b89828d350d83bf1d4b0badcd3e63eb9d1a630 
  3rdparty/stout/include/stout/windows/net.hpp 
1bed115cb848332bf9c31e455b2d001c173face9 

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 53627: Implement a namespaces/uts isolator.

[James Peach]

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

(Updated Nov. 10, 2016, 5:53 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Implement a very simple namespaces/uts isolator that can be used to set
the hostname and domainname of a container without the necessity of a
CNI plugin.

Since we already had a `hostname` field in the ContainerInfo, we can
use that to set the host name once we are in the UTS namespace. Add a
corresponding `domainname` to the ContainerInfo to allow setting the
domain name.


Diffs (updated)
-

  include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
  include/mesos/slave/containerizer.proto 
f4c4ad771b5dead4ea3ee7cd1b4383c4dc2359b4 
  src/Makefile.am 5a47c93388234a68c3c486a021ccdbe3213c5bac 
  src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 
  src/slave/containerizer/mesos/containerizer.cpp 
44225ebf63d8dd93be9b60fff496c74dc6c3a5ad 
  src/slave/containerizer/mesos/isolators/namespaces/uts.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/namespaces/uts.cpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.hpp 
8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
  src/slave/containerizer/mesos/launch.cpp 
377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
  src/tests/containerizer/isolator_tests.cpp 
8fefeef8c83ed2ab01f56a1ec572d3acb307143c 

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


Testing
---

make check.


Thanks,

James Peach

Re: Review Request 53628: Document the namespaces/uts isolator.

[James Peach]

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

(Updated Nov. 10, 2016, 5:53 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Document the namespaces/uts isolator.


Diffs (updated)
-

  docs/mesos-containerizer.md 2bff35f6361f760a9001a249d2c01bbbc9e72932 

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


Testing
---

make check.


Thanks,

James Peach

Re: Review Request 53624: Use JSON content type in mesos-execute.

[James Peach]

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

(Updated Nov. 10, 2016, 5:46 p.m.)


Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.


Changes
---

Add `--json` flag.


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


Repository: mesos


Description
---

Use JSON content type in mesos-execute so that packet tracing
protocol interactions is easier.


Diffs (updated)
-

  src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 

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


Testing
---

make check. Manually verified with tcpdump.


Thanks,

James Peach

Re: Review Request 52295: Added additional unit tests for shared resources.

[Jiang Yan Xu]

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




src/tests/hierarchical_allocator_tests.cpp (lines 1512 - 1513)


The way this test is written, it's not testing special properties of shared 
persistent volumes at all. Persistent volumes have the same behavior...

Shared persistent volumes are offered one-by-one as well but are "offerable 
even if it's already in use". We should test that instead.



src/tests/hierarchical_allocator_tests.cpp (line 1519)


s/with opt/opted in/?



src/tests/hierarchical_allocator_tests.cpp (lines 1530 - 1533)


Is 3 better than 2 in this case? Can we shorten this test?



src/tests/hierarchical_allocator_tests.cpp (lines 1580 - 1581)


We don't use this style. Instead:

```
  EXPECT_EQ(
  allocation.get().resources.get(slave.id()).get().shared(),
  Resources(volume));
```



src/tests/hierarchical_allocator_tests.cpp (line 2158)


This test is basically identitcal to 
'AllocateSharedResourcesMultiFrameworks', can we templatize it?



src/tests/master_validation_tests.cpp (lines 674 - 681)


Use `createTask()`.



src/tests/master_validation_tests.cpp (lines 683 - 690)


The existence of `task2` (and `disk2` for that matter) doesn't seem 
necessary?

They use disctint resources and can't imagine them interfere with each 
other?

Shorten the test?



src/tests/persistent_volume_tests.cpp (lines 1002 - 1003)


Can we add how we verify "the master recovers"? e.g.,

```
// This test verifies that the master recovers after a failover and 
re-offers the shared persistent volume when tasks using the same volume are 
still running.
```



src/tests/persistent_volume_tests.cpp (line 1004)


s/SharedVolume/SharedPersistentVolume/

Full spelling helps with code grepping.



src/tests/persistent_volume_tests.cpp (lines 1006 - 1008)


No `masterFlags` necessary.



src/tests/persistent_volume_tests.cpp (line 1011)


With the new test helper we now use:

```
Owned detector = master.get()->createDetector();
Try slave = StartSlave(detector.get(), slaveFlags);
```



src/tests/persistent_volume_tests.cpp (lines 1050 - 1065)


Intead of actually writing files, we should probably keep them up via 
"sleep 1000" so they are running during the failover.

BTW we need to have expectations for status updates otherwise there are 
going to be warnings?



src/tests/persistent_volume_tests.cpp (lines 1089 - 1090)


Nit: The relevant slave behavior can be inferred from the offer so we don't 
need this expectation.



src/tests/sorter_tests.cpp (line 637)


s/disk 50/disk of 50/



src/tests/sorter_tests.cpp (lines 660 - 661)


This actually fits one line? If not, the indentation should be

```
  Resources additionalAShared =
Resources(sharedDisk) + sharedDisk + sharedDisk;
```



src/tests/sorter_tests.cpp (line 705)


s/the fair/its dominant/



src/tests/sorter_tests.cpp (line 722)


For this test, I think a simple implementation would be to `add()` the same 
resources twice and `remove()` it twice. The `totalScalarQuantities()` only 
drops after the second `remove()`.



src/tests/sorter_tests.cpp (lines 733 - 738)


Mesos wouldn't add the same shared resources in this way. Call 
`sorter->add()` multiple times instead?


- Jiang Yan Xu


On Sept. 26, 2016, 5:39 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52295/
> ---
> 
> (Updated Sept. 26, 2016, 5:39 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6185
> https://issues.apache.org/jira/browse/MESOS-6185
> 
> 
> Repository: mesos
> 
> 

Re: Review Request 53645: Added '--task' into mesos-execute.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53644, 53645]

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. 10, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53645/
> ---
> 
> (Updated Nov. 10, 2016, 1:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6571
> https://issues.apache.org/jira/browse/MESOS-6571
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added '--task' into mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 
> 
> Diff: https://reviews.apache.org/r/53645/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04, manually ran the command below to successfuly launch a 
> container which will be attached to a CNI network "net1".
> ```
> # sudo src/mesos-execute --master=192.168.122.216:5050 
> --task=file:///home/stack/workspace/mesos/build/task.json
> 
> # cat /home/stack/workspace/mesos/build/task.json
> {
>   "name": "test",
>   "task_id": {"value" : "test"},
>   "agent_id": {"value" : ""},
>   "resources": [
> {
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
> "value": 0.1
>   },
>   "role": "*"
> },
> {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
> "value": 32
>   },
>   "role": "*"
> }
>   ],
>   "command": {
> "value": "ifconfig"
>   },
>   "container": {
> "type": "MESOS",
> "mesos": {
>   "image": {
> "type": "DOCKER",
> "docker": {
>   "name": "busybox"
> }
>   }
> },
> "network_infos": [
>   {
> "name": "net1"
>   }
> ]
>   }
> }
> ```
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53644: Added parse function for v1::TaskInfo protobuf.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On Nov. 10, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53644/
> ---
> 
> (Updated Nov. 10, 2016, 1:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6571
> https://issues.apache.org/jira/browse/MESOS-6571
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added parse function for v1::TaskInfo protobuf.
> 
> 
> Diffs
> -
> 
>   src/v1/parse.hpp 86bd7e850ae7a17ed3f48300af1b047a877739c3 
> 
> Diff: https://reviews.apache.org/r/53644/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53645: Added '--task' into mesos-execute.

[Avinash sridharan]

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




src/cli/execute.cpp (line 874)


Add a comment stating either `--task` or `--task_group` is set.



src/cli/execute.cpp (line 1056)


Instead of this why can't we just do:

Option taskInfo = flags.task

if (flags.task.isNone() && flags.task_group.isNone()) {

}

This would be more explicit than using flags.name.isSome() for implying the 
above condition.


- Avinash sridharan


On Nov. 10, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53645/
> ---
> 
> (Updated Nov. 10, 2016, 1:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6571
> https://issues.apache.org/jira/browse/MESOS-6571
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added '--task' into mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 
> 
> Diff: https://reviews.apache.org/r/53645/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04, manually ran the command below to successfuly launch a 
> container which will be attached to a CNI network "net1".
> ```
> # sudo src/mesos-execute --master=192.168.122.216:5050 
> --task=file:///home/stack/workspace/mesos/build/task.json
> 
> # cat /home/stack/workspace/mesos/build/task.json
> {
>   "name": "test",
>   "task_id": {"value" : "test"},
>   "agent_id": {"value" : ""},
>   "resources": [
> {
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
> "value": 0.1
>   },
>   "role": "*"
> },
> {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
> "value": 32
>   },
>   "role": "*"
> }
>   ],
>   "command": {
> "value": "ifconfig"
>   },
>   "container": {
> "type": "MESOS",
> "mesos": {
>   "image": {
> "type": "DOCKER",
> "docker": {
>   "name": "busybox"
> }
>   }
> },
> "network_infos": [
>   {
> "name": "net1"
>   }
> ]
>   }
> }
> ```
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 52787: Reorganized the work directory and the runtime directory in local mode.

[haosdent huang]

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

(Updated Nov. 10, 2016, 3:44 p.m.)


Review request for mesos, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Addres @klueska's comment.


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


Repository: mesos


Description
---

Reorganized the work directory and the runtime directory in local mode.


Diffs (updated)
-

  bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
  src/local/flags.hpp 3d8e5392f770a72354215829022b677caee9d8c6 
  src/local/local.cpp 257179443827ffdfc946ed655a6840fcea70d454 

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


Testing
---

No matter use `bin/mesos-local.sh` or use `src/mesos-local`, the folder 
structure looks like

```
$ tree /tmp/mesos/
/tmp/mesos/
|-- runtime
|   |-- agents
|   |-- 0
|   |-- 1
|-- work
|-- agents
|   |-- 0
|   |   |-- meta
|   |   |   |-- boot_id
|   |   |   |-- slaves
|   |   |   |-- a63fbc0a-268e-4ebe-b31c-90d3a19d2fb3-S0
|   |   |   |   |-- slave.info
|   |   |   |-- latest -> 
/tmp/mesos/work/agents/0/meta/slaves/a63fbc0a-268e-4ebe-b31c-90d3a19d2fb3-S0
|   |   |-- provisioner
|   |-- 1
|   |-- meta
|   |   |-- boot_id
|   |   |-- slaves
|   |   |-- b4d008bc-6be4-4bba-97c3-3a73d3f9611d-S0
|   |   |   |-- slave.info
|   |   |-- latest -> 
/tmp/mesos/work/agents/1/meta/slaves/b4d008bc-6be4-4bba-97c3-3a73d3f9611d-S0
|   |-- provisioner
|-- master
|-- replicated_log
|-- 09.log
|-- 11.sst
|-- CURRENT
|-- LOCK
|-- LOG
|-- LOG.old
|-- MANIFEST-06

20 directories, 11 files
```


Thanks,

haosdent huang

Re: Review Request 52856: Reverted incorrect changes in 1c2ee5c.

[haosdent huang]

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

(Updated Nov. 10, 2016, 3:44 p.m.)


Review request for mesos, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Reverted incorrect changes in 1c2ee5c.


Diffs (updated)
-

  src/local/local.cpp 257179443827ffdfc946ed655a6840fcea70d454 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52919: Updated scripts to allow override of MESOS environment variables.

[haosdent huang]

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

(Updated Nov. 10, 2016, 3:44 p.m.)


Review request for mesos, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Addres @klueska's comment.


Summary (updated)
-

Updated scripts to allow override of MESOS environment variables.


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


Repository: mesos


Description (updated)
---

Updated scripts to allow override of MESOS environment variables.


Diffs (updated)
-

  bin/mesos-agent-flags.sh.in 8c936aa06e994a87a8b09b31c907868bf9be38c7 
  bin/mesos-master-flags.sh.in 951c3e4c714809bcf2364eb28974aeefb7884bcb 

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


Testing
---

```
$ MESOS_TEMP_DIR=/tmp/mesos/bar ./bin/mesos-local.sh

$ tree /tmp/mesos/
/tmp/mesos/
??? bar
??? agents
?   ??? 0
?   ??? runtime_dir
?   ??? work_dir
?   ??? meta
?   ?   ??? boot_id
?   ?   ??? slaves
?   ?   ??? 6b4ce717-dd6e-4d8c-9f89-66a42e3866a8-S0
?   ?   ?   ??? slave.info
?   ?   ??? latest -> 
/tmp/mesos/bar/agents/0/work_dir/meta/slaves/6b4ce717-dd6e-4d8c-9f89-66a42e3866a8-S0
?   ??? provisioner
??? master
??? work_dir
??? replicated_log
??? 04.log
??? CURRENT
??? LOCK
??? LOG
??? MANIFEST-02

13 directories, 7 files
```


Thanks,

haosdent huang

Re: Review Request 53308: Added new hook for modifying the executor environment.

[Till Toenshoff]

> On Nov. 10, 2016, 3:19 p.m., Kapil Arya wrote:
> > Before doing full review, I am wondering if we can create a new protobuf 
> > called `DockerExecutorPrepareInfo` or something similar with the idea that 
> > we can pass on not only environment variables, but also volumes (and any 
> > other options in future). The volumes are interesting because a module 
> > might want to make available certain file paths within Docker executor. Is 
> > this something reasonable?

I like that idea a lot. Let me reiterate with that on this patch.


- Till


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


On Oct. 31, 2016, 3:49 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53308/
> ---
> 
> (Updated Oct. 31, 2016, 3:49 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, and Joseph Wu.
> 
> 
> Bugs: MESOS-6396
> https://issues.apache.org/jira/browse/MESOS-6396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows for modifying the environment effective for the executor itself.
> Additionally allows the hook to receive the location of the sandbox folder.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp c8a58641e0768628a9028a70d8c28f7fb412 
>   src/hook/manager.hpp 5ecfcab48da808c84d36f9bcfcb5a8e0ad2167e5 
>   src/hook/manager.cpp 24885226a788a7abd851e12b527f74fa972ec935 
>   src/slave/containerizer/docker.cpp f720320e112687e8ae972f04159b3b4cb7a58476 
> 
> Diff: https://reviews.apache.org/r/53308/diff/
> 
> 
> Testing
> ---
> 
> make check and functional test in a hook-module
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 53308: Added new hook for modifying the executor environment.

[Kapil Arya]

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



Before doing full review, I am wondering if we can create a new protobuf called 
`DockerExecutorPrepareInfo` or something similar with the idea that we can pass 
on not only environment variables, but also volumes (and any other options in 
future). The volumes are interesting because a module might want to make 
available certain file paths within Docker executor. Is this something 
reasonable?


include/mesos/hook.hpp (line 120)


Can we add comment on the relationship of this hook vs. the 
`slavePreLaunchDockerEnvironmentDecorator` hook above?


- Kapil Arya


On Oct. 31, 2016, 11:49 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53308/
> ---
> 
> (Updated Oct. 31, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, and Joseph Wu.
> 
> 
> Bugs: MESOS-6396
> https://issues.apache.org/jira/browse/MESOS-6396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows for modifying the environment effective for the executor itself.
> Additionally allows the hook to receive the location of the sandbox folder.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp c8a58641e0768628a9028a70d8c28f7fb412 
>   src/hook/manager.hpp 5ecfcab48da808c84d36f9bcfcb5a8e0ad2167e5 
>   src/hook/manager.cpp 24885226a788a7abd851e12b527f74fa972ec935 
>   src/slave/containerizer/docker.cpp f720320e112687e8ae972f04159b3b4cb7a58476 
> 
> Diff: https://reviews.apache.org/r/53308/diff/
> 
> 
> Testing
> ---
> 
> make check and functional test in a hook-module
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 53610: Added health checks documentation.

[Gastón Kleiman]

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



I am not a native speaker, so please take my suggestions with a grain of salt 
and feel free to drop them =).


docs/health-checks.md (line 13)


s/in a way/in the way/



docs/health-checks.md (line 32)


s/docker/Docker/



docs/health-checks.md (line 34)


s/only is/, but is/



docs/health-checks.md (line 43)


Is `HealthCheck` here supposed to be a link to the protobuf



docs/health-checks.md (line 45)


s/only/only the/



docs/health-checks.md (line 49)


"an executor knows what a task means"?



docs/health-checks.md (line 54)


s/and/and to/



docs/health-checks.md (line 56)


s/only reference/only the reference/



docs/health-checks.md (line 67)


s/docker/Docker/g



docs/health-checks.md (line 91)


s/Field `scheme`/The `scheme` field/? I'm not really sure about this one.



docs/health-checks.md (line 101)


Don't they always enter the network namespace?



docs/health-checks.md (line 123)


I'd write something like:

"The task is probed using Mesos' `mesos-tcp-connect` binary, which tries to 
establish a TCP connection to `:port`.



docs/health-checks.md (line 128)


`The health check is considered successful if the connection can be 
established.` ?



docs/health-checks.md (line 130)


Same as in the previous case, it might be clearer to say that it will 
always enter the task's net ns.



docs/health-checks.md (line 161)


s/docker/Docker/ =)



docs/health-checks.md (line 200)


s/Boolean/The boolean/



docs/health-checks.md (line 204)


s/as first/as a first/ ?



docs/health-checks.md (line 211)


s/with `healthy` bit/with the `healthy` field/



docs/health-checks.md (lines 212 - 213)


```
The executor will kill the task after a number of consecutive failures 
defined in the `consecutive_failures` field of the `HealthCheck` protobuf
```?



docs/health-checks.md (line 226)


s/istance/instance/
s/passes health/passes the/



docs/health-checks.md (line 227)


s/paramteres/parameters/



docs/health-checks.md (line 228)


s/about/of/
s/in task's/in the task's/



docs/health-checks.md (line 231)


command or binary?



docs/health-checks.md (line 233)


s/thing/things/



docs/health-checks.md (line 234)


s/appropiate/the appropriate/



docs/health-checks.md (line 235)


`with the` or `as the`?



docs/health-checks.md (line 237)


s/docker/the Docker/



docs/health-checks.md (line 238)


s/^/by /



docs/health-checks.md (lines 242 - 243)


s/docker/the Docker/
s/mesos/the Mesos/



docs/health-checks.md (line 266)


s/depend on/rely on the presence of a/


- Gastón Kleiman


On Nov. 9, 2016, 5:05 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53610/
> ---
> 
> (Updated Nov. 9, 2016, 5:05 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-5597
> https://issues.apache.org/jira/browse/MESOS-5597
> 
> 
> Repository: mesos
> 
> 
> 

Re: Review Request 53641: Refactored `CgroupsAnyHierarchyTest` test cases.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53641]

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. 10, 2016, 12:03 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53641/
> ---
> 
> (Updated Nov. 10, 2016, 12:03 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `CgroupsAnyHierarchyTest` test cases.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0afaec6ae948cabf1472bf01103210d8f9809cb1 
> 
> Diff: https://reviews.apache.org/r/53641/diff/
> 
> 
> Testing
> ---
> 
> This patch attempts to refactor `CgroupsAnyHierarchy` test cases to use a 
> different hierarchy structure.
> If `cpu` have been mounted at `/sys/fs/cgroup/cpu`, it would try to mount all 
> rest subsystems at `/tmp/mesos_test_cgroup`.
> 
> I have several concerns about this proposal. 
> 
> 1. Mount `cpu` at `/sys/fs/cgroup/cpu` while mount `memory` or any other 
> subsystems at `/tmp/mesos_test_cgroup` would fail if cgroups is managed by 
> systemd. In CentOS 7/Ubuntu 16.04, it would failed because cgroups are 
> managed by systemd.
> 
> ```
> ## cpu,cpuacct have been mounted
> # cat /proc/self/mountinfo |grep cgroup
> 25 18 0:20 / /sys/fs/cgroup ro,nosuid,nodev,noexec shared:8 - tmpfs tmpfs 
> ro,mode=755
> 26 25 0:21 / /sys/fs/cgroup/systemd rw,nosuid,nodev,noexec,relatime shared:9 
> - cgroup cgroup 
> rw,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd
> 28 25 0:23 / /sys/fs/cgroup/cpu,cpuacct rw,relatime shared:10 - cgroup cgroup 
> rw,cpuacct,cpu
> 
> ## Mount rest subsystems would failed.
> # mount -t cgroup -o rw,memory,devices cgroup /tmp/mesos_test_cgroup
> mount: cgroup is already mounted or /tmp/mesos_test_cgroup busy
>cgroup is already mounted on /sys/fs/cgroup/systemd
>cgroup is already mounted on /sys/fs/cgroup/cpu,cpuacct
> ```
> 
> 2. I think in current code base, we suppose all the hierarchies under the 
> same directory, instead of supposing that some hierarchies are mounted at 
> `/sys/fs/cgroup/xxx` while some hierarchies are mounted at 
> `/tmp/mesos_test_cgroup`. This is what we suppose in 
> `ContainerizerTest` as well.
> 
> 3. As we see in this patch, change to put hierarchies under different folders 
> looks messy, compare to use a single folder for all cgroup subsystems. 
> cgroups-v2 unified all subsystems under a same hierarchy, which don't allow 
> us mount `cpu` at `/sys/fs/cgroup/cpu` while mount `memory` at 
> `/tmp/mesos_test_cgroup` as well.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 53644: Added parse function for v1::TaskInfo protobuf.

[Qian Zhang]

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

(Updated Nov. 10, 2016, 9:44 p.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Added parse function for v1::TaskInfo protobuf.


Diffs
-

  src/v1/parse.hpp 86bd7e850ae7a17ed3f48300af1b047a877739c3 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 53529: Improved the readability of the master validation tests.

[Alexander Rukletsov]

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


Ship it!





src/tests/master_validation_tests.cpp (lines 87 - 91)


I find it's unfortunate that this test doubles the number of lines it 
touches, but it looks like I'm the minority here.


- Alexander Rukletsov


On Nov. 8, 2016, 9:18 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53529/
> ---
> 
> (Updated Nov. 8, 2016, 9:18 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make all the validation tests in the file follow the same pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> 8697bb2dc05c54553066ace58553f0e40dfcb5f5 
> 
> Diff: https://reviews.apache.org/r/53529/diff/
> 
> 
> Testing
> ---
> 
> `make check`.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 53645: Added '--task' into mesos-execute.

[Qian Zhang]

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

(Updated Nov. 10, 2016, 9:44 p.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Added '--task' into mesos-execute.


Diffs
-

  src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 

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


Testing
---

On Ubuntu 16.04, manually ran the command below to successfuly launch a 
container which will be attached to a CNI network "net1".
```
# sudo src/mesos-execute --master=192.168.122.216:5050 
--task=file:///home/stack/workspace/mesos/build/task.json

# cat /home/stack/workspace/mesos/build/task.json
{
  "name": "test",
  "task_id": {"value" : "test"},
  "agent_id": {"value" : ""},
  "resources": [
{
  "name": "cpus",
  "type": "SCALAR",
  "scalar": {
"value": 0.1
  },
  "role": "*"
},
{
  "name": "mem",
  "type": "SCALAR",
  "scalar": {
"value": 32
  },
  "role": "*"
}
  ],
  "command": {
"value": "ifconfig"
  },
  "container": {
"type": "MESOS",
"mesos": {
  "image": {
"type": "DOCKER",
"docker": {
  "name": "busybox"
}
  }
},
"network_infos": [
  {
"name": "net1"
  }
]
  }
}
```


Thanks,

Qian Zhang

Review Request 53645: Added '--task' into mesos-execute.

[Qian Zhang]

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

Review request for mesos, Avinash sridharan and Jie Yu.


Repository: mesos


Description
---

Added '--task' into mesos-execute.


Diffs
-

  src/cli/execute.cpp b47c427c5ad29dda1985ee8fef6c4efe054df879 

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


Testing
---

On Ubuntu 16.04, manually ran the command below to successfuly launch a 
container which will be attached to a CNI network "net1".
```
# sudo src/mesos-execute --master=192.168.122.216:5050 
--task=file:///home/stack/workspace/mesos/build/task.json

# cat /home/stack/workspace/mesos/build/task.json
{
  "name": "test",
  "task_id": {"value" : "test"},
  "agent_id": {"value" : ""},
  "resources": [
{
  "name": "cpus",
  "type": "SCALAR",
  "scalar": {
"value": 0.1
  },
  "role": "*"
},
{
  "name": "mem",
  "type": "SCALAR",
  "scalar": {
"value": 32
  },
  "role": "*"
}
  ],
  "command": {
"value": "ifconfig"
  },
  "container": {
"type": "MESOS",
"mesos": {
  "image": {
"type": "DOCKER",
"docker": {
  "name": "busybox"
}
  }
},
"network_infos": [
  {
"name": "net1"
  }
]
  }
}
```


Thanks,

Qian Zhang

Re: Review Request 53641: Refactored `CgroupsAnyHierarchyTest` test cases.

[haosdent huang]

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

(Updated Nov. 10, 2016, 12:03 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Jiang Yan Xu.


Repository: mesos


Description
---

Refactored `CgroupsAnyHierarchyTest` test cases.


Diffs
-

  src/tests/containerizer/cgroups_tests.cpp 
0afaec6ae948cabf1472bf01103210d8f9809cb1 

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


Testing (updated)
---

This patch attempts to refactor `CgroupsAnyHierarchy` test cases to use a 
different hierarchy structure.
If `cpu` have been mounted at `/sys/fs/cgroup/cpu`, it would try to mount all 
rest subsystems at `/tmp/mesos_test_cgroup`.

I have several concerns about this proposal. 

1. Mount `cpu` at `/sys/fs/cgroup/cpu` while mount `memory` or any other 
subsystems at `/tmp/mesos_test_cgroup` would fail if cgroups is managed by 
systemd. In CentOS 7/Ubuntu 16.04, it would failed because cgroups are managed 
by systemd.

```
## cpu,cpuacct have been mounted
# cat /proc/self/mountinfo |grep cgroup
25 18 0:20 / /sys/fs/cgroup ro,nosuid,nodev,noexec shared:8 - tmpfs tmpfs 
ro,mode=755
26 25 0:21 / /sys/fs/cgroup/systemd rw,nosuid,nodev,noexec,relatime shared:9 - 
cgroup cgroup 
rw,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd
28 25 0:23 / /sys/fs/cgroup/cpu,cpuacct rw,relatime shared:10 - cgroup cgroup 
rw,cpuacct,cpu

## Mount rest subsystems would failed.
# mount -t cgroup -o rw,memory,devices cgroup /tmp/mesos_test_cgroup
mount: cgroup is already mounted or /tmp/mesos_test_cgroup busy
   cgroup is already mounted on /sys/fs/cgroup/systemd
   cgroup is already mounted on /sys/fs/cgroup/cpu,cpuacct
```

2. I think in current code base, we suppose all the hierarchies under the same 
directory, instead of supposing that some hierarchies are mounted at 
`/sys/fs/cgroup/xxx` while some hierarchies are mounted at 
`/tmp/mesos_test_cgroup`. This is what we suppose in 
`ContainerizerTest` as well.

3. As we see in this patch, change to put hierarchies under different folders 
looks messy, compare to use a single folder for all cgroup subsystems. 
cgroups-v2 unified all subsystems under a same hierarchy, which don't allow us 
mount `cpu` at `/sys/fs/cgroup/cpu` while mount `memory` at 
`/tmp/mesos_test_cgroup` as well.


Thanks,

haosdent huang

Review Request 53641: Refactored `CgroupsAnyHierarchyTest` test cases.

[haosdent huang]

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

Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Jiang Yan Xu.


Repository: mesos


Description
---

Refactored `CgroupsAnyHierarchyTest` test cases.


Diffs
-

  src/tests/containerizer/cgroups_tests.cpp 
0afaec6ae948cabf1472bf01103210d8f9809cb1 

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


Testing
---

This patch attempts to refactor `CgroupsAnyHierarchy` test cases to use a 
different hierarchy structure.
If `cpu` have been mounted at `/sys/fs/cgroup/cpu`, it would try to mount all 
rest subsystems at `/tmp/mesos_test_cgroup`.

I have several concerns about this proposal. 

1. Mount `cpu` at `/sys/fs/cgroup/cpu` while mount `memory` or any other 
subsystems at `/tmp/mesos_test_cgroup` would fail if cgroups is managed by 
systemd.

I test this in CentOS 7.

```
## cpu,cpuacct have been mounted
# cat /proc/self/mountinfo |grep cgroup
25 18 0:20 / /sys/fs/cgroup ro,nosuid,nodev,noexec shared:8 - tmpfs tmpfs 
ro,mode=755
26 25 0:21 / /sys/fs/cgroup/systemd rw,nosuid,nodev,noexec,relatime shared:9 - 
cgroup cgroup 
rw,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd
28 25 0:23 / /sys/fs/cgroup/cpu,cpuacct rw,relatime shared:10 - cgroup cgroup 
rw,cpuacct,cpu

## Mount rest subsystems would failed.
# mount -t cgroup -o rw,memory,devices cgroup /tmp/mesos_test_cgroup
mount: cgroup is already mounted or /tmp/mesos_test_cgroup busy
   cgroup is already mounted on /sys/fs/cgroup/systemd
   cgroup is already mounted on /sys/fs/cgroup/cpu,cpuacct
```

2. I think in current code base, we suppose all the hierarchies under the same 
directory, instead of supposing that some hierarchies are mounted at 
`/sys/fs/cgroup/xxx` while some hierarchies are mounted at 
`/tmp/mesos_test_cgroup`. This is what we suppose in 
`ContainerizerTest` as well.

3. As we see in this patch, change to put hierarchies under different folders 
looks messy, compare to use a single folder for all cgroup subsystems. 
cgroups-v2 unified all subsystems under a same hierarchy, which don't allow us 
mount `cpu` at `/sys/fs/cgroup/cpu` while mount `memory` at 
`/tmp/mesos_test_cgroup` as well.


Thanks,

haosdent huang

Re: Review Request 53533: Extended docker flags to pass devices to mesos-docker-executor.

[Yubo Li]

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

(Updated Nov. 10, 2016, 8:15 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and Rajat 
Phull.


Repository: mesos


Description
---

Extended a new docker flag in docker containerizer to pass devices
to mesos-docker-executor.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 50127: Added DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_Launch.

[Yubo Li]

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

(Updated Nov. 10, 2016, 8:15 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
---

This added a testing case for end-to-end GPU support for docker
containerizer.


Diffs (updated)
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
c478d56ffd734b26fd2dbd7bb1ca02ce929f3f16 

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


Testing
---

GTEST_FILTER="DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_Launch" make -j 
check


Thanks,

Yubo Li

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

[Yubo Li]

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

(Updated Nov. 10, 2016, 8:15 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.


Diffs (updated)
-

  src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
  src/docker/executor.cpp eefbc0c2936eb93044c2d8a1b50155171f398562 

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.

[Yubo Li]

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

(Updated Nov. 10, 2016, 8:14 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 (updated)
-

  src/slave/containerizer/docker.hpp 006f929eca0e0a6b1de941821ac72869ba393d2d 
  src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 53532: Added parse helper function to 'Docker::Device'.

[Yubo Li]

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

(Updated Nov. 10, 2016, 8:14 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and Rajat 
Phull.


Repository: mesos


Description
---

Added helper function to parse string input to 'Docker::Device'
structure.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 

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


Testing
---

make -j4 check


Thanks,

Yubo Li

Re: Review Request 50128: Overloaded the << operator for 'Docker::Device'.

[Yubo Li]

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

(Updated Nov. 10, 2016, 8:13 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
---

This patch overloaded the << operator for 'Docker::Device'.
With such overload, we can just run the global `stringify()`
function to turn 'Docker::Device' into a string.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 50947: Removed isolator flag 'gpu/nvidia' for docker containerizer using GPU.

[Yubo Li]

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

(Updated Nov. 10, 2016, 8:13 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and Rajat 
Phull.


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


Repository: mesos


Description
---

Mesos containerizer uses isolator 'gpu/nvidia' for GPU isolation while
docker and external containerizers do not need it anymore. This removed
the 'gpu/nvidia' isolator check for docker and external containerizers.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
2e722691475c84afae14009014ea70cc0fdd0e65 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 52735: Updated comment message for docker killing.

[Yubo Li]

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

(Updated Nov. 10, 2016, 8:13 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and Rajat 
Phull.


Summary (updated)
-

Updated comment message for docker killing.


Repository: mesos


Description (updated)
---

`Garbage collector` has already enabled so that the docker failed
in killing will be force removed by calling `Self::remove`.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 

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


Testing
---


Thanks,

Yubo Li