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

Reply via email to