You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Guangya Liu <gy...@gmail.com> on 2016/04/01 07:53:42 UTC
Review Request 45360: WIP: Added dvd client for mount.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
Review request for mesos, Gilbert Song and Jie Yu.
Repository: mesos
Description
-------
Added dvd client for mount.
Diffs
-----
src/CMakeLists.txt 366a7ae8f6ef1d55202699df0502a30f15a35e1f
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
-------
Thanks,
Guangya Liu
Re: Review Request 45360: WIP: Added dvd client for mount.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review126567
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45214, 45217, 45265, 45270, 45360]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 1, 2016, 5:53 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated April 1, 2016, 5:53 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added dvd client for mount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 366a7ae8f6ef1d55202699df0502a30f15a35e1f
> 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
> -------
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added dvd client for mount and unmount.
Posted by Guangya Liu <gy...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line59>
> >
> > 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
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line111>
> >
> > 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
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line125>
> >
> > 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
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line146>
> >
> > I suggest to replace `const Owned<ExternalMount>& externalMount,` to `Owned<ExternalMount>& 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
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line58>
> >
> > 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.
Posted by haosdent huang <ha...@gmail.com>.
> 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 volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 5, 2016, 4:55 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 59
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line59>
> >
> > Do we need check `dvdcli` version or something first?
>
> Guangya Liu wrote:
> 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
I will add version check in `isolator::create`
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review127121
-----------------------------------------------------------
On 四月 15, 2016, 2:43 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated 四月 15, 2016, 2:43 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.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.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/45360/#comment190254>
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)
<https://reviews.apache.org/r/45360/#comment190255>
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)
<https://reviews.apache.org/r/45360/#comment190262>
s/how does dvdcli works/how dvdcli works/g.
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 58)
<https://reviews.apache.org/r/45360/#comment190257>
I think we could use `strings::format` here?
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 59)
<https://reviews.apache.org/r/45360/#comment190263>
Do we need check `dvdcli` version or something first?
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 65)
<https://reviews.apache.org/r/45360/#comment190258>
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)
<https://reviews.apache.org/r/45360/#comment190264>
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)
<https://reviews.apache.org/r/45360/#comment190266>
A bit confuse why we continue to launch after unmount.
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 125)
<https://reviews.apache.org/r/45360/#comment190269>
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)
<https://reviews.apache.org/r/45360/#comment190268>
Add a line above.
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 146)
<https://reviews.apache.org/r/45360/#comment190271>
I suggest to replace `const Owned<ExternalMount>& externalMount,` to `Owned<ExternalMount>& externalMount,` which make it more matched.
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 148)
<https://reviews.apache.org/r/45360/#comment190270>
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.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 4, 2016, 11:37 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 54
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line54>
> >
> > `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.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 4, 2016, 11:37 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, lines 67-71
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line67>
> >
> > 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
> > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line77>
> >
> > 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.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/45360/#comment190092>
s/DVDClient/DvdClient/g
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 38)
<https://reviews.apache.org/r/45360/#comment190094>
s/dvdCLIPath/dvdcliPath/g
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 54)
<https://reviews.apache.org/r/45360/#comment190097>
`driver_options` is an optional field, right? This may lead to segfault.
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 65)
<https://reviews.apache.org/r/45360/#comment190101>
let's make it
```
VLOG(1) << "Mount command '" << cmd << "'";
```
src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (lines 67 - 71)
<https://reviews.apache.org/r/45360/#comment190106>
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)
<https://reviews.apache.org/r/45360/#comment190105>
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)
<https://reviews.apache.org/r/45360/#comment190107>
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 volume driver client for mount and
unmount.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129812
-----------------------------------------------------------
Ship it!
Guangya, I committed for you. I did a few tweaking on the code. I put the client into slave::docker::volume namespace. Please rebase the following patches accordingly.
- Jie Yu
On April 20, 2016, 5:44 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated April 20, 2016, 5:44 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt e4923c4d4a7a9eefcab9267fe5def7f7535e49ed
> src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
(Updated 四月 20, 2016, 5:44 p.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Repository: mesos
Description
-------
Added volume driver client for mount and unmount.
Diffs (updated)
-----
src/CMakeLists.txt e4923c4d4a7a9eefcab9267fe5def7f7535e49ed
src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/45360/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
(Updated 四月 20, 2016, 11:07 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Repository: mesos
Description
-------
Added volume driver client for mount and unmount.
Diffs (updated)
-----
src/CMakeLists.txt e4923c4d4a7a9eefcab9267fe5def7f7535e49ed
src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/45360/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
(Updated 四月 20, 2016, 9:25 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Repository: mesos
Description
-------
Added volume driver client for mount and unmount.
Diffs (updated)
-----
src/CMakeLists.txt e4923c4d4a7a9eefcab9267fe5def7f7535e49ed
src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/45360/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129712
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46245, 45373, 45326, 45270, 45360]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 15, 2016, 2:43 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 2:43 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 68
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line68>
> >
> > Why 'static'? If it's static, can that be a file local helper in driver.cpp?
>
> Guangya Liu wrote:
> I do not have strong intention to use `static` here but just following same coding style as docker: https://github.com/apache/mesos/blob/master/src/docker/docker.hpp#L184-L189 , I'm OK to remove it for now.
This method is now removed.
> On 四月 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, lines 72-75
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line72>
> >
> > Ditto.
This method is now removed.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129573
-----------------------------------------------------------
On 四月 15, 2016, 2:43 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated 四月 15, 2016, 2:43 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Jie Yu <yu...@gmail.com>.
> On April 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 41
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line41>
> >
> > Do you want to use `const Option<string>& dvdcliPath` here? The idea is that if not specified, the client will just use 'dvdcli' in $PATH. You want to use os::which to do a check in 'create' to make sure it's there if dvdcliPath is not specified.
>
> Guangya Liu wrote:
> So you mean that we still need an agent flag to speicify the dvdcli path as an option?
>
> Guangya Liu wrote:
> Can we stick to `const std::string& dvdcliPath`? My thinking is that when `DockerVolumeIsolatorProcess::create`, I can first get `dvdcliPath` from agent flag, if flag is not specified, get it form `$PATH`, if no `dvdcliPath`, just return `Error` in `DockerVolumeIsolatorProcess::create`.
OK, makes sense.
> On April 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 58
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line58>
> >
> > Ditto on removing 'const'.
>
> Guangya Liu wrote:
> Why remove the `const` here? I think that we do not expect the `name` be changed when call `unmount`?
the tailing 'const' has nothing to do with function parameters. It's about object fields.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129573
-----------------------------------------------------------
On April 20, 2016, 5:44 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated April 20, 2016, 5:44 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt e4923c4d4a7a9eefcab9267fe5def7f7535e49ed
> src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 41
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line41>
> >
> > Do you want to use `const Option<string>& dvdcliPath` here? The idea is that if not specified, the client will just use 'dvdcli' in $PATH. You want to use os::which to do a check in 'create' to make sure it's there if dvdcliPath is not specified.
>
> Guangya Liu wrote:
> So you mean that we still need an agent flag to speicify the dvdcli path as an option?
Can we stick to `const std::string& dvdcliPath`? My thinking is that when `DockerVolumeIsolatorProcess::create`, I can first get `dvdcliPath` from agent flag, if flag is not specified, get it form `$PATH`, if no `dvdcliPath`, just return `Error` in `DockerVolumeIsolatorProcess::create`.
> On 四月 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 43
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line43>
> >
> > Are you using virtual because of testing?
> >
> > If not, let's try not to use virtual for now. Ditto for 'mount' and 'unmount'. Let's just use concrete class with concrete implementation for now.
> >
> > If it's for testing, then i think that makse sense as you want to use a MockDockerVolumeDriverClient.
Yes, it is for testing.
> On 四月 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 51
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line51>
> >
> > Is the idea here that we use 'None' to express the intention that we don't want to call 'Create' implicitly? If yes, please document this in the comments.
> >
> > Also, please remove 'const' for now as it's not clear it'll be const or not.
The current `dvdcli` is setting `explicitcreate` as a new parameter but not an option, so seems we need either add a new flag in agent or add a new field in `DockerVolume` protobuf. The agent flag will be coarse grained, and the protobuf will be fine grained, and my thinking is that the fine grained management may be better: Add a new field for protobuf of `DockerVolume`.
```
root@mesos002:~/src/mesos/m2/mesos/build# dvdcli --volumedriver=convoy --volumename=test111123 mount --explicitcreate=true
ERRO[0000] VolumeDriver.Get: Handler not found: POST /VolumeDriver.Get volumeName=test111123
root@mesos002:~/src/mesos/m2/mesos/build# echo $?
1
root@mesos002:~/src/mesos/m2/mesos/build# dvdcli --volumedriver=convoy --volumename=test111123 mount
INFO[0000] /var/lib/convoy/devicemapper/mounts/7176a77c-993f-4145-b601-a0a81be2d1a7
/var/lib/convoy/devicemapper/mounts/7176a77c-993f-4145-b601-a0a81be2d1a7
root@mesos002:~/src/mesos/m2/mesos/build# echo $?
0
```
Define as `const` here is because I do not expect this value to be updated after `isolator` transfer the volume mount options, default as `None()` is because the `options` is not required. What do you think? Thanks.
> On 四月 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 58
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line58>
> >
> > Ditto on removing 'const'.
Why remove the `const` here? I think that we do not expect the `name` be changed when call `unmount`?
> On 四月 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 68
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line68>
> >
> > Why 'static'? If it's static, can that be a file local helper in driver.cpp?
I do not have strong intention to use `static` here but just following same coding style as docker: https://github.com/apache/mesos/blob/master/src/docker/docker.hpp#L184-L189 , I'm OK to remove it for now.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129573
-----------------------------------------------------------
On 四月 15, 2016, 2:43 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated 四月 15, 2016, 2:43 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 19, 2016, 10:37 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp, line 41
> > <https://reviews.apache.org/r/45360/diff/8/?file=1346544#file1346544line41>
> >
> > Do you want to use `const Option<string>& dvdcliPath` here? The idea is that if not specified, the client will just use 'dvdcli' in $PATH. You want to use os::which to do a check in 'create' to make sure it's there if dvdcliPath is not specified.
So you mean that we still need an agent flag to speicify the dvdcli path as an option?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129573
-----------------------------------------------------------
On 四月 15, 2016, 2:43 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated 四月 15, 2016, 2:43 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129573
-----------------------------------------------------------
src/Makefile.am (line 687)
<https://reviews.apache.org/r/45360/#comment193004>
Please align the tailing ``
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 41)
<https://reviews.apache.org/r/45360/#comment193079>
Do you want to use `const Option<string>& dvdcliPath` here? The idea is that if not specified, the client will just use 'dvdcli' in $PATH. You want to use os::which to do a check in 'create' to make sure it's there if dvdcliPath is not specified.
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 43)
<https://reviews.apache.org/r/45360/#comment193044>
Are you using virtual because of testing?
If not, let's try not to use virtual for now. Ditto for 'mount' and 'unmount'. Let's just use concrete class with concrete implementation for now.
If it's for testing, then i think that makse sense as you want to use a MockDockerVolumeDriverClient.
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 51)
<https://reviews.apache.org/r/45360/#comment193006>
Is the idea here that we use 'None' to express the intention that we don't want to call 'Create' implicitly? If yes, please document this in the comments.
Also, please remove 'const' for now as it's not clear it'll be const or not.
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 58)
<https://reviews.apache.org/r/45360/#comment193047>
Ditto on removing 'const'.
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 60)
<https://reviews.apache.org/r/45360/#comment193048>
Let's make it 'private' for now.
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 68)
<https://reviews.apache.org/r/45360/#comment193082>
Why 'static'? If it's static, can that be a file local helper in driver.cpp?
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (lines 72 - 75)
<https://reviews.apache.org/r/45360/#comment193085>
Ditto.
src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp (lines 58 - 63)
<https://reviews.apache.org/r/45360/#comment193102>
Can you use the argv version of 'subprocess' to avoid the need for escaping special chars.
```
vector<string> argv = {
dvdcli,
"mount",
"--volumedriver=" + driver,
"--volumename=" + name,
};
...
```
src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp (lines 83 - 87)
<https://reviews.apache.org/r/45360/#comment193115>
You need to call io::read() before the process terminates. Otherwise, if the pipe is full, the child process will block.
```
return await(
s->status(),
s->out().get(),
s->err().get())
.then([](const tuple<
Future<Option<int>>,
Future<string>,
Future<string>>& t) -> Future<string> {
Future<Option<int>> status = std::get<0>(t);
if (!status.isReady()) {
...
}
...
});
```
Please follow the similar logic here:
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L807
- Jie Yu
On April 15, 2016, 2:43 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 2:43 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
(Updated 四月 15, 2016, 2:43 p.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Summary (updated)
-----------------
Added volume driver client for mount and unmount.
Repository: mesos
Description (updated)
-------
Added volume driver client for mount and unmount.
Diffs (updated)
-----
src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/45360/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 14, 2016, 6:12 p.m., Gilbert Song wrote:
> >
We can talk more for your comments when sync up. ;-)
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review128931
-----------------------------------------------------------
On 四月 14, 2016, 6:05 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated 四月 14, 2016, 6:05 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
> src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review128931
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 57)
<https://reviews.apache.org/r/45360/#comment192375>
How about using:
`const stirng&`
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 58)
<https://reviews.apache.org/r/45360/#comment192376>
ditto.
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 64 - 81)
<https://reviews.apache.org/r/45360/#comment192379>
It is still the issue I mentioned above:
Since we have to specify `--volumedriver` and `--volumename`, and possibly have a bunch of `--volumeopt` appended. I would suggest collecting all this arguments (e.g., {dvdcliPath, "mount", "--volumedriver=XXXX", ...}) into a `vector<string> argv;`.
and in subprocess, if you grep the examples `Try<subprocess>`, your can see for most cases we have cmd and argv separated. Considering the cmd may not be short. Let's make it more clear:
```
Try<Subprocess> s = subprocess(
dvdcliPath,
argv,
Subprocess::PATH("/dev/null"),
Subprocess::PIPE(),
Subprocess::PIPE());
```
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 87)
<https://reviews.apache.org/r/45360/#comment192385>
Let's following the pattern in CNI isolator by using await here and passing the `tuple` to `launch()` instead of passing a subprocess.
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 104)
<https://reviews.apache.org/r/45360/#comment192382>
ditto.
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 128 - 150)
<https://reviews.apache.org/r/45360/#comment192389>
Understand that you want to simplify the logic for mount and unmount here. Let us discuss this comparing the logic in CNI isolator `attach()` and `detach()` V.S. passing the reference of subprocess to launch.
- Gilbert Song
On April 13, 2016, 11:05 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 11:05 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
> src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 14, 2016, 9:27 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp, lines 60-61
> > <https://reviews.apache.org/r/45360/diff/7/?file=1343858#file1343858line60>
> >
> > Let's use `https://github.com/emccode/rexray`, which is under maintainance.
I think you mean https://github.com/emccode/dvdcli ;-)
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review128997
-----------------------------------------------------------
On 四月 14, 2016, 6:05 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated 四月 14, 2016, 6:05 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
> src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review128997
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 60 - 61)
<https://reviews.apache.org/r/45360/#comment192451>
Let's use `https://github.com/emccode/rexray`, which is under maintainance.
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 102)
<https://reviews.apache.org/r/45360/#comment192452>
ditto.
- Gilbert Song
On April 13, 2016, 11:05 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 11:05 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
> src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume driver client for mount and
unmount.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 14, 2016, 10:41 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp, line 46
> > <https://reviews.apache.org/r/45360/diff/7/?file=1343857#file1343857line46>
> >
> > Why virtual? Let's not make them virtual yet.
I was following the docker way here: https://github.com/apache/mesos/blob/master/src/docker/docker.hpp#L53
Also we need to be virtual as `deleting object of polymorphic class type 'mesos::internal::slave::DockerVolumeDriverClient' which has non-virtual destructor might cause undefined behaviour` (Got from compiler)
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129016
-----------------------------------------------------------
On 四月 15, 2016, 2:43 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated 四月 15, 2016, 2:43 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume driver client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review129016
-----------------------------------------------------------
src/CMakeLists.txt (line 145)
<https://reviews.apache.org/r/45360/#comment192479>
s/volume_client/driver.cpp
No need for the 'volume' since it's already in the directory.
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 31)
<https://reviews.apache.org/r/45360/#comment192480>
Remove this.
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 37)
<https://reviews.apache.org/r/45360/#comment192481>
s/VolumeClient/DockerVolumeDriverClient/
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 46)
<https://reviews.apache.org/r/45360/#comment192483>
Why virtual? Let's not make them virtual yet.
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 52)
<https://reviews.apache.org/r/45360/#comment192482>
```
// Returns the path of the mount point.
Future<string> mount(
const string& driver,
const string& name,
const hashmap<string, string>& options);
```
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 57)
<https://reviews.apache.org/r/45360/#comment192484>
```
Future<Nothing> unmount(
const string& driver,
const string& name);
```
- Jie Yu
On April 14, 2016, 6:05 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
>
> (Updated April 14, 2016, 6:05 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added volume client for mount and unmount.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
> src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45360/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
(Updated 四月 14, 2016, 6:05 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Repository: mesos
Description
-------
Added volume client for mount and unmount.
Diffs (updated)
-----
src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/45360/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
(Updated 四月 14, 2016, 4:45 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Repository: mesos
Description
-------
Added volume client for mount and unmount.
Diffs (updated)
-----
src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/45360/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
(Updated 四月 13, 2016, 11:12 p.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Repository: mesos
Description
-------
Added volume client for mount and unmount.
Diffs (updated)
-----
src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/45360/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45360: Added volume client for mount and unmount.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/
-----------------------------------------------------------
(Updated 四月 13, 2016, 11:01 p.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Changes
-------
Moved to docker/volume folder and renamed to volume_client.hpp|cpp
Summary (updated)
-----------------
Added volume client for mount and unmount.
Repository: mesos
Description (updated)
-------
Added volume client for mount and unmount.
Diffs (updated)
-----
src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041
src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/volume_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.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/45360/#comment192205>
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.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
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