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