----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51257/#review148063 -----------------------------------------------------------
src/slave/container_loggers/lib_externallogger.hpp (lines 20 - 38) <https://reviews.apache.org/r/51257/#comment215460> You don't use the following includes: * #include <map> * #include <vector> * #include <stout/base64.hpp> * #include <stout/os/shell.hpp> src/slave/container_loggers/lib_externallogger.hpp (lines 52 - 61) <https://reviews.apache.org/r/51257/#comment215450> This flag should be required. You can use `flags.add` like: ``` add(&external_logger_cmd, "external_logger_cmd", None(), "Path to the external command which will read STDIN for logs", [](const std::string& executablePath) -> Option<Error> { if (!os::exists(executablePath)) { return Error("Cannot find: " + executablePath); } return None(); }); ``` The `None()` as the third argument is an alias. We need this to disambiguate the various overloads of `flags.add`. --- Also, add a newline after this block. src/slave/container_loggers/lib_externallogger.hpp (line 86) <https://reviews.apache.org/r/51257/#comment215454> Remove this copied comment. src/slave/container_loggers/lib_externallogger.hpp (lines 89 - 90) <https://reviews.apache.org/r/51257/#comment215451> The comment `piped to the files "stdout" and "stderr"...` is not accurate. This will pipe to whatever the operator has configured the ExternalContainerLogger to do :) src/slave/container_loggers/lib_externallogger.hpp (line 98) <https://reviews.apache.org/r/51257/#comment215452> Oops, more `journald` :) src/slave/container_loggers/lib_externallogger.hpp (line 107) <https://reviews.apache.org/r/51257/#comment215453> `journald` here too. src/slave/container_loggers/lib_externallogger.cpp (lines 87 - 89) <https://reviews.apache.org/r/51257/#comment215461> You can move this underscore into the flag. i.e. change the default from `MESOS_LOG` to `MESOS_LOG_`. Maybe someone doesn't want an underscore after the prefix. src/slave/container_loggers/lib_externallogger.cpp (line 130) <https://reviews.apache.org/r/51257/#comment215465> You don't need these flags. In fact, if your external command parses commandline arguments, it will see these as unknown flags and bail out. The Logrotate module had a dedicated binary, with its own set of defined flags. For this module, everything is passed in via environment variables. src/slave/container_loggers/lib_externallogger.cpp (line 152) <https://reviews.apache.org/r/51257/#comment215467> You should `SETSID` here. Otherwise, you will lose logging (the external command will die) when the agent dies. src/slave/container_loggers/lib_externallogger.cpp (line 188) <https://reviews.apache.org/r/51257/#comment215466> Ditto with these flags. src/slave/container_loggers/lib_externallogger.cpp (line 199) <https://reviews.apache.org/r/51257/#comment215468> Ditto with `SETSID`. - Joseph Wu On Sept. 2, 2016, 4:26 a.m., Will Rouesnel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51257/ > ----------------------------------------------------------- > > (Updated Sept. 2, 2016, 4:26 a.m.) > > > Review request for mesos and Joseph Wu. > > > Bugs: MESOS-6003 > https://issues.apache.org/jira/browse/MESOS-6003 > > > Repository: mesos > > > Description > ------- > > Adds the external process container logger. This functions like the > logrotate container logger, but instead calls a custom host binary > (or script) and passes the executorInfo as JSON via environment > variables. This makes it very easy for users to configure custom > logging solutions, without needing to write and maintain logging > modules. > > Tests passing and complete. > > > Diffs > ----- > > src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 > src/slave/container_loggers/lib_externallogger.hpp PRE-CREATION > src/slave/container_loggers/lib_externallogger.cpp PRE-CREATION > src/tests/container_logger_external.sh PRE-CREATION > src/tests/container_logger_tests.cpp > e8f934106510fe02b8b92be19c918a1e5c0b78fd > src/tests/module.hpp e661d95fa44fc1aedfe83c564c826d5b7d32c85b > src/tests/module.cpp 5b83fd6358ddea4c9d849b8992e1a6040ef74505 > > Diff: https://reviews.apache.org/r/51257/diff/ > > > Testing > ------- > > Adds ContainerLoggerTest.EXTERNAL_RecieveEnvironment which tests all major > parameters of the change. > > A synthetic external container logger is provided by the script > tests/container_logger_external.sh which is setup to fail if any important > output is unavailable to the logging process. > > The other basic checks are duplicated from the Logrotate container logger, > from where this change inherits a lot of its code. > > > Thanks, > > Will Rouesnel > >
