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