----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50125/#review146823 -----------------------------------------------------------
src/docker/executor.hpp (lines 80 - 83) <https://reviews.apache.org/r/50125/#comment213536> This comment needs rewording as its hard to understand. Maybe something like: ``` A comma separated list of devices to expose to the docker container. The format of this flag mimics that of the `--devices` flag to docker run, i.e: `host_path:container_path:permissions`. For example, `/dev/tty:/dev/tty:mrw'. ``` src/docker/executor.cpp (line 89) <https://reviews.apache.org/r/50125/#comment213542> By the time the devices are passed in here they should be of type `vector<Device>`, not just a string. This is similar to how `task_environment` comes in as a simple string via `Flags`, but is converted to a `map<string, string>` by the time it is passed into `DockerExecutorProcess`. src/docker/executor.cpp (lines 159 - 174) <https://reviews.apache.org/r/50125/#comment213543> With the change suggessted above, this entire code block disappears. src/docker/executor.cpp (line 189) <https://reviews.apache.org/r/50125/#comment213544> With the change suggested above, this becomes: ``` devices, ``` src/docker/executor.cpp (line 594) <https://reviews.apache.org/r/50125/#comment213545> `vector<Device> devices` src/docker/executor.cpp (line 618) <https://reviews.apache.org/r/50125/#comment213547> `const vector<Device>& devices` src/docker/executor.cpp (lines 819 - 826) <https://reviews.apache.org/r/50125/#comment213546> Here is where you should convert the input string to a `vector<Device>`, i.e.: ``` vector<Device> devices; if (flags.devices.isSome()) { const vector<string> deviceList = strings::split(devices, ","); foreach (const string& device, deviceList) { Try<Device> parsed = Device::parse(device); if(parse.isError()) { cerr << flags.usage("Failed to parse --devices: " + parse.error()) << endl; return EXIT_FAILURE; } devices->push_back(parsed); } } ``` src/docker/executor.cpp (line 822) <https://reviews.apache.org/r/50125/#comment213548> I don't think we want to `cout` anything here since this is an optional flag. - Kevin Klues On Aug. 15, 2016, 7:29 a.m., Yubo Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50125/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2016, 7:29 a.m.) > > > Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull. > > > Bugs: MESOS-5795 > https://issues.apache.org/jira/browse/MESOS-5795 > > > Repository: mesos > > > Description > ------- > > Added a new flag '--devices' to mesos-docker-executor, and gave its > feature to control devices exposition, isolation, and access permission. > > > Diffs > ----- > > src/docker/executor.hpp 7b63d784d6b8685912598b77fb38cf6e70646ae3 > src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac > src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 > > Diff: https://reviews.apache.org/r/50125/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Yubo Li > >
