> On March 13, 2014, 9:55 p.m., Ian Downes wrote: > >
Thank you so much Ian, after addressing you comments, code became much nicer. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.hpp, line 179 > > <https://reviews.apache.org/r/17567/diff/12/?file=515138#file515138line179> > > > > no need for const on pass by value After revisiting our common style, I realized that we don't do that on scalars even if that was an explicit signal about the use of that parameter. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.hpp, line 224 > > <https://reviews.apache.org/r/17567/diff/12/?file=515138#file515138line224> > > > > s/string &/string& / > > can this be const? No, it is a returned value. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.hpp, line 251 > > <https://reviews.apache.org/r/17567/diff/12/?file=515138#file515138line251> > > > > no need for reference It is a returned value. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.hpp, line 257 > > <https://reviews.apache.org/r/17567/diff/12/?file=515138#file515138line257> > > > > no need for reference It is a returned value. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.hpp, line 264 > > <https://reviews.apache.org/r/17567/diff/12/?file=515138#file515138line264> > > > > no need for reference It is a returned value. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.hpp, line 140 > > <https://reviews.apache.org/r/17567/diff/12/?file=515138#file515138line140> > > > > could you please clarify why we need this tuple? why do we need to wait > > on both - could we not .then on one before the other? We could certainly wait on both separately, but I find it semantically clearer and more elegant to simply use await on a tuple than to chain. After all, await-tuple is mostly a convenience function for uses just like this. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.hpp, line 209 > > <https://reviews.apache.org/r/17567/diff/12/?file=515138#file515138line209> > > > > use const references for list<> and Duration. no need for const on > > unsigned int Great catch, that certainly was incorrect. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 267 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line267> > > > > isn't this setting environment variables in the slave's environment!? Yes, very good point. Now fixed based on the environment extension of subprocess. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 337 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line337> > > > > as above, wouldn't it be better to return the continuation rather than > > passing around this promise? Very nice, yes, much better. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > include/mesos/mesos.proto, line 496 > > <https://reviews.apache.org/r/17567/diff/12/?file=515133#file515133line496> > > > > I saw this was raised before, but why not just use External everywhere, > > i.e., ExternalContainerizer has an ExternalTermination, and rename > > files/classes Yes, I like that much better. Reasons where historic as there once was an ExternalContainerizer existing already. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.hpp, line 211 > > <https://reviews.apache.org/r/17567/diff/12/?file=515138#file515138line211> > > > > rename stepCount to something like retries? Removed the escalation to ease the review. Escalation will be a subsequent RR. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 367 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line367> > > > > onAny? Now that I am using subprocess, this can even be entirely removed as the subprocess implementation takes care. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 489 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line489> > > > > can you please move all of the continuations to immediately after each > > method I like that better as well. Initially had it that way but was asked to change it. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 808 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line808> > > > > why do you need to pass the future to commandSupported? why not check > > if the future is ready, then pass in the ResultFutures. Code reduction - I would have to do that in many places. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 946 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line946> > > > > Wouldn't this be cleaner as retryPeriod and numRetries ? Removed escalation for subsequent RR. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 904 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line904> > > > > if this was retries then this branch occurs which retries == 0, and > > it's unsigned. Removed escalation for subsequent RR. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 1103 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line1103> > > > > can this be replaced by the new Subprocess abstraction? Subprocess missed in-child-context-pre-exec-function which is needed for additional "chdir". Added a RR on that. And switched over to using it. Much cleaner now, thanks for reminding me. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 311 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line311> > > > > is this promise needed? could you not do: > > > > return read().onAny(/* check result */); Much better indeed. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 417 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line417> > > > > onAny? Removed entirely as subprocess takes care of that. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 441 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line441> > > > > likewise, can you not do away with this promise and return from the > > await.then? Yes! :) > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 452 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line452> > > > > onAny? Removed entirely as subprocess takes care of that. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 485 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line485> > > > > onAny? Removed entirely as subprocess takes care of that. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 326 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line326> > > > > shouldn't the file descriptor always be closed, i.e., .onAny()? True, thanks. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 390 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line390> > > > > s/Resource */Resource* / Old habits don't go away that easy ;) ... thanks! > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 691 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line691> > > > > can you re-use the code from Posix{Cpu,Memory}Isolator if it was pulled > > out? We should indeed discuss this (see JIRA) and depending on the results of that, implement it in a subsequent RR. I would like to leave it as is for now but add a TODO. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 707 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line707> > > > > add a comment that resources aren't known after recovery unless > > update() is called Copied your comment from mesos-containerizer.cpp. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 777 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line777> > > > > this seems a little draconian, why do you need to terminate the > > container, couldn't you just fail the statistics? The external containerizer delivered an invalid protobuf which is a severe failure on the protocol level. I think your point is valid and should be discussed on JIRA again. Adding a TODO on this and similar lines. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 878 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line878> > > > > what about simplifying it a little > > > > if (!contains) { > > LOG(); > > } > > > > sandboxes.erase(); > > > > Run erase on a non-existing key, aren't we entering UB-land by doing so? Checked this question on std::map that seems to answer the question by undefined behaviour. Checked it on boost::unordered_map (which hashmap derives from) and there things seem to be perfectly ok by doing so. For reasons of upstream flexibility and remaining entirely safe, I would like to keep this as is. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 1085 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line1085> > > > > this has now been added stout/abort.hpp Fixed by using Subprocess > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/pluggable_containerizer.cpp, line 1165 > > <https://reviews.apache.org/r/17567/diff/12/?file=515139#file515139line1165> > > > > refactor to avoid if/else > > > > pid = fork(); > > if (pid == 0) { > > // in child > > } > > > > // continue in parent Fixed by using Subprocess > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/flags.hpp, line 207 > > <https://reviews.apache.org/r/17567/diff/12/?file=515140#file515140line207> > > > > 'fails' is a bad word ;-), what about > > "The default container image to use if not specified by a task." Yep, that is better. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/flags.hpp, line 205 > > <https://reviews.apache.org/r/17567/diff/12/?file=515140#file515140line205> > > > > why not call this default_image or default_container_image The latter is much more explicit, thanks. > On March 13, 2014, 9:55 p.m., Ian Downes wrote: > > src/slave/containerizer/containerizer.cpp, line 184 > > <https://reviews.apache.org/r/17567/diff/12/?file=515137#file515137line184> > > > > ExternalContainerizer? ;-) Aye :) - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17567/#review37069 ----------------------------------------------------------- On March 16, 2014, 10:02 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17567/ > ----------------------------------------------------------- > > (Updated March 16, 2014, 10:02 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Ian Downes, > Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-816 > https://issues.apache.org/jira/browse/MESOS-816 > > > Repository: mesos-git > > > Description > ------- > > This patch adds the so-called external containerizer. This > containerizer delegates all containerizer calls directly to > an external containerizer program (which can be specified on > start-up). Few calls have internal fall-back implementations > such as wait(), destroy() and usage(). > > The protocol for the interactions with the external program > is as follows: > > COMMAND (ADDITIONAL-PARAMETERS) < INPUT-PROTO > RESULT-PROTO > > launch (ContainerID, --mesos-executor, <path>) < TaskInfo > ExternalStatus > update (ContainerID) < ResourceArray > ExternalStatus > usage (ContainerID) > ResourceStatistics > wait (ContainerID) > ExternalTermination > destroy (ContainerID) > ExternalStatus > > When protocol buffers need to be provided, the Mesos side of > the external containerizer implementation will serialize the > protos on stdin and vice-versa for reading protos on stdout as > drafted in the above scheme. > > > Diffs > ----- > > configure.ac 9a6de87 > include/mesos/mesos.proto 37f8a7f > src/Makefile.am 0775a0d > src/examples/python/test-containerizer.in PRE-CREATION > src/examples/python/test_containerizer.py PRE-CREATION > src/slave/containerizer/containerizer.cpp d0a1023 > src/slave/containerizer/external_containerizer.hpp PRE-CREATION > src/slave/containerizer/external_containerizer.cpp PRE-CREATION > src/slave/flags.hpp c9a627b > src/tests/external_containerizer_test.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/17567/diff/ > > > Testing > ------- > > make check and functional testing. > > > Thanks, > > Till Toenshoff > >
