----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review91014 -----------------------------------------------------------
can you please set the dependency for this review correctly? it's hard to understand which reviews introduced the code that this review depends on. include/mesos/slave/isolator.hpp (lines 70 - 71) <https://reviews.apache.org/r/34137/#comment144221> We don't align arguments for constructors this way IIRC. ExecutorRunState(arg1, arg2, ....); src/slave/containerizer/mesos/containerizer.cpp (line 400) <https://reviews.apache.org/r/34137/#comment144231> kill new line. src/slave/containerizer/mesos/containerizer.cpp (line 418) <https://reviews.apache.org/r/34137/#comment144232> Is it possible for rootfs to not exist when we are here? If not, there should be a CHECK and a comment on what guarantees its presence (the fact that there is a forked pid?). src/slave/containerizer/mesos/containerizer.cpp (lines 612 - 614) <https://reviews.apache.org/r/34137/#comment144234> Why do you want to consider that? Don't we want to support both type of containers on the same host? src/slave/containerizer/mesos/containerizer.cpp (lines 622 - 623) <https://reviews.apache.org/r/34137/#comment144240> shouldn't there be a check to see if 'image' even exists in the protobuf? src/slave/containerizer/mesos/containerizer.cpp (line 632) <https://reviews.apache.org/r/34137/#comment144242> If you've extracted 'image' into a temp variable instead of 'type' above in #622, you could fit 'provision()' in one line. src/slave/containerizer/mesos/containerizer.cpp (line 651) <https://reviews.apache.org/r/34137/#comment144244> So what happens if the provisioner provisions the file system but the slave restarts before checkpointing 'rootfs'? Is that safe? We don't leak anything? src/slave/containerizer/mesos/containerizer.cpp (line 790) <https://reviews.apache.org/r/34137/#comment144245> when was flags.sandbox_directory introduced? which review? src/slave/containerizer/mesos/containerizer.cpp (lines 815 - 816) <https://reviews.apache.org/r/34137/#comment144247> +1 src/slave/containerizer/mesos/containerizer.cpp (line 817) <https://reviews.apache.org/r/34137/#comment144251> where is the update to launchFlags to add 'rootfs' ? which review? src/slave/containerizer/mesos/containerizer.cpp (line 1248) <https://reviews.apache.org/r/34137/#comment144248> why include the containerid? doesn't the caller of this method already know that? src/slave/containerizer/mesos/containerizer.cpp (line 1279) <https://reviews.apache.org/r/34137/#comment144249> ditto. why include the container id? src/slave/containerizer/provisioner.hpp (lines 59 - 60) <https://reviews.apache.org/r/34137/#comment144228> What does this return? src/slave/containerizer/provisioner.cpp (line 37) <https://reviews.apache.org/r/34137/#comment144229> if (flags.provisioners.isNone()) { } src/slave/flags.cpp (line 59) <https://reviews.apache.org/r/34137/#comment144222> s/./(e.g., appc, docker)./ - Vinod Kone On July 7, 2015, 7:42 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34137/ > ----------------------------------------------------------- > > (Updated July 7, 2015, 7:42 p.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > The MesosContainerizer can optionally provision a root filesystem for the > containers. > > > Diffs > ----- > > include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/mesos/containerizer.hpp > 3ac2387eabded61c897a5016655aae10cd1bca91 > src/slave/containerizer/mesos/containerizer.cpp > 8c102fb7d1f79ee768cb06de3a976ea12f958712 > src/slave/containerizer/provisioner.hpp PRE-CREATION > src/slave/containerizer/provisioner.cpp PRE-CREATION > src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b > src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 > src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f > src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 > src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 > src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec > src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e > > Diff: https://reviews.apache.org/r/34137/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
