----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26517/#review56293 -----------------------------------------------------------
This review motivated me to do the refactoring we had discussed in DockerContainerizerProcess to consolidate the redundant code paths when launching an container for a task versus for an executor. There is a flattened version of that refactoring that you can rebase off of until we finialize each of the independeent patches that the flattened review was based on. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment96607> I want us to associate this with the instance of the slave that is running. Otherwise, when you're running two slaves on the same machine they'll use the same directory and an operator will have a hard time knowing when things go wrong which directories are still valid/in-use, etc. Any reason why we can't add this directory into the slave work directory itself? Then we won't need a flag, just a constant that represents the relative path. Furthermore, when the slave gets cleaned up this directory will also properly get cleaned up rather than having to have the operator know that they need to delete two directories! src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment96616> Now that I've introduced more functionality in the Container struct I think properly creating the symlink when the Container gets created would be really clean. See comment re: deleting the symlink below too. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment96604> Rather than introducing this why not let the Container destructor do the os::rm of the symlink? This is more localized to the Container. src/slave/containerizer/docker.cpp <https://reviews.apache.org/r/26517/#comment96602> Let's give our users a better error message so that they know that we were trying to create a directory to put symlinks for using with Docker and that failed. It would be nice to output the directory we tried to create too! - Benjamin Hindman On Oct. 10, 2014, 6:35 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26517/ > ----------------------------------------------------------- > > (Updated Oct. 10, 2014, 6:35 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1833 > https://issues.apache.org/jira/browse/MESOS-1833 > > > Repository: mesos-git > > > Description > ------- > > Review: https://reviews.apache.org/r/26517 > > > Diffs > ----- > > src/slave/containerizer/docker.hpp fbbd45d77e5f2f74ca893552f85eb893b3dd948f > src/slave/containerizer/docker.cpp 9a2948951f57f3ab16291df51cd9f33e5e96add4 > src/tests/docker_containerizer_tests.cpp > 67d60a885d65edbcbbf702bce83a54d1a5c0411f > > Diff: https://reviews.apache.org/r/26517/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
