Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-13 Thread James DeFelice

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




src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 66)


options must be specified as follows:
`--volumeopts={param.key}={param.value}`


- James DeFelice


On April 13, 2016, 9:17 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 13, 2016, 9:17 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-13 Thread Guangya Liu


> On 四月 5, 2016, 4:55 p.m., haosdent huang wrote:
> >
> 
> haosdent huang wrote:
> By the way, I saw you didn't support `create`, `remove` and `path` 
> methods while it exists in dvdcli. Are they unnecessary here?

We do not need to use those methods but only `mount` and `umount`. 

1) We expect the end user create the volume first before use it.
2) Similar as docker, it is not expected to remove volume by mesos, as the data 
in the volume may be needed in future, this should leave to end user.


> On 四月 5, 2016, 4:55 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 59
> > 
> >
> > Do we need check `dvdcli` version or something first?

The dvdcli currently only have one release and also planning to write a C++ 
library for mesos, so I think that we do not need to check the version now. 
What do you think? https://github.com/emccode/dvdcli/releases


> On 四月 5, 2016, 4:55 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 111
> > 
> >
> > A bit confuse why we continue to launch after unmount.

I need to check the unmount result in `launch`.


> On 四月 5, 2016, 4:55 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 125
> > 
> >
> > I think this msg isn't clear. Actually we failed to launch DVD, but 
> > this message looks like we failed in some general methods like `os::shell`. 
> > Maybe we could make it more specific.

I was following the code style here: 
https://github.com/apache/mesos/blob/master/src/docker/docker.cpp#L163

Can you please show more why do you think the message is not clear? Any 
proposal? ;-)


> On 四月 5, 2016, 4:55 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 146
> > 
> >
> > I suggest to replace `const Owned& externalMount,` to 
> > `Owned& externalMount,` which make it more matched.

What is the problem of using `const` here?


> On 四月 5, 2016, 4:55 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 58
> > 
> >
> > I think we could use `strings::format` here?

Using "+" may be more simple, the `strings::format` will return a `Try`. I saw 
that docker is using such way 
https://github.com/apache/mesos/blob/master/src/docker/docker.cpp#L142 , what 
do you think?


- Guangya


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


On 四月 13, 2016, 9:17 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated 四月 13, 2016, 9:17 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-13 Thread Guangya Liu

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

(Updated 四月 13, 2016, 9:17 a.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
Jie Yu.


Repository: mesos


Description
---

Added dvd client for mount and unmount.


Diffs (updated)
-

  src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
  src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
  src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
PRE-CREATION 

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


Testing
---

make
make check


Thanks,

Guangya Liu



Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-13 Thread Guangya Liu


> On 四月 4, 2016, 11:37 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 54
> > 
> >
> > `driver_options` is an optional field, right? This may lead to segfault.

This type is now changed to `repeated parameter`


- Guangya


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


On 四月 13, 2016, 9:17 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated 四月 13, 2016, 9:17 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-13 Thread Guangya Liu


> On 四月 4, 2016, 11:37 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, lines 
> > 67-71
> > 
> >
> > could we set `dvdcliPath + /dvdcli` as cmd, and append the rest as argv?

I was following the docker executor here because I saw that the cmd is not very 
long, so I was using a simple way here, what do you think?

The docker refererence: 
https://github.com/apache/mesos/blob/master/src/docker/docker.cpp#L733-L742


> On 四月 4, 2016, 11:37 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 77
> > 
> >
> > Should we user await()?
> > 
> > ```
> > reutrn await(
> > s.get().status(),
> > io::read(s.get().out().get()),
> > io::read(s.get().err().get()))
> >   .then(...)
> > ```

Yes, I was a bit confused here, I also think that we should add `await` here, 
but I saw some places using `await` while some not, can you help clarify? ;-)

Not using `await`: 
https://github.com/apache/mesos/blob/master/src/docker/docker.cpp#L748
Using `await`: 
https://github.com/apache/mesos/blob/master/src/common/command_utils.cpp#L62


- Guangya


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


On 四月 2, 2016, 5:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated 四月 2, 2016, 5:56 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-05 Thread haosdent huang


> On April 5, 2016, 4:55 p.m., haosdent huang wrote:
> >

By the way, I saw you didn't support `create`, `remove` and `path` methods 
while it exists in dvdcli. Are they unnecessary here?


- haosdent


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


On April 2, 2016, 5:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 2, 2016, 5:56 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-05 Thread haosdent huang

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




src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 28)


I think add this here looks a bit wired.
```
find . -name '*.hpp'|xargs grep 'using namespace'
./3rdparty/libprocess/3rdparty/stout/include/stout/lambda.hpp:using 
namespace std::placeholders;
./3rdparty/libprocess/include/process/pid.hpp: *using namespace process;
./src/slave/slave.hpp:using namespace process;
```

How about move the class to `messo::internal::slave::dvd` namespace?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 42)


I suggest to follow `doxgen-style-guide.md` to add comments to functions.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 56)


s/how does dvdcli works/how dvdcli works/g.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 58)


I think we could use `strings::format` here?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 59)


Do we need check `dvdcli` version or something first?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 65)


I think could make it more excatly as well. `s/Mount command/Execute Docker 
Volume Driver mount command:`.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 74)


I suggest use `return Failure("Failed to mount Docker Volume Driver:" +  + 
s.error())`



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 111)


A bit confuse why we continue to launch after unmount.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 125)


I think this msg isn't clear. Actually we failed to launch DVD, but this 
message looks like we failed in some general methods like `os::shell`. Maybe we 
could make it more specific.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 131)


Add a line above.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 146)


I suggest to replace `const Owned& externalMount,` to 
`Owned& externalMount,` which make it more matched.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 148)


I think you want to log `externalMount` here. You could 
`jsonify(JSON::Protobuf(*externalMount))` directly.


- haosdent huang


On April 2, 2016, 5:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 2, 2016, 5:56 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-04 Thread Gilbert Song

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




src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 33)


s/DVDClient/DvdClient/g



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 38)


s/dvdCLIPath/dvdcliPath/g



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 54)


`driver_options` is an optional field, right? This may lead to segfault.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 65)


let's make it
```
VLOG(1) << "Mount command '" << cmd << "'";
```



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (lines 67 - 
71)


could we set `dvdcliPath + /dvdcli` as cmd, and append the rest as argv?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 77)


Should we user await()?

```
reutrn await(
s.get().status(),
io::read(s.get().out().get()),
io::read(s.get().err().get()))
  .then(...)
```



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 86)


ditto for umount.


- Gilbert Song


On April 1, 2016, 10:56 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 1, 2016, 10:56 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>



Re: Review Request 45360: Added dvd client for mount and unmount.

2016-04-01 Thread Guangya Liu

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

(Updated 四月 2, 2016, 5:56 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Added dvd client for mount and unmount.


Repository: mesos


Description (updated)
---

Added dvd client for mount and unmount.


Diffs (updated)
-

  src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
PRE-CREATION 

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


Testing (updated)
---

make
make check


Thanks,

Guangya Liu