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

2016-04-15 Thread Guangya Liu 


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

2016-04-15 Thread Guangya Liu 


> On 四月 14, 2016, 9:27 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp, 
> > lines 60-61
> > 
> >
> > 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.

2016-04-14 Thread Jie Yu 

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




src/CMakeLists.txt (line 145)


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)


Remove this.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
37)


s/VolumeClient/DockerVolumeDriverClient/



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
46)


Why virtual? Let's not make them virtual yet.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
52)


```
// Returns the path of the mount point.
Future mount(
const string& driver,
const string& name,
const hashmap& options);
```



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
57)


```
Future 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.

2016-04-14 Thread Gilbert Song 

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


How about using:

`const stirng&`



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
58)


ditto.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 
64 - 81)


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=", ...}) into a 
`vector argv;`.

and in subprocess, if you grep the examples `Try`, 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 s = subprocess(
  dvdcliPath,
  argv,
  Subprocess::PATH("/dev/null"),
  Subprocess::PIPE(),
  Subprocess::PIPE());
```



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
87)


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)


ditto.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 
128 - 150)


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.

2016-04-13 Thread Guangya Liu 

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

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, 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.

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