Repository: mesos Updated Branches: refs/heads/master bca600cf5 -> 02013593c
Added a way to set logrotate settings per executor. The provided `LogrotateContainerLogger` did not have enough granularity when setting log rotation settings. This patch adds a way for each executor to set its own log rotation settings, using the global values as defaults. The executor settings are provided via environment variables in the `ExecutorInfo`. Review: https://reviews.apache.org/r/51565/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/02013593 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/02013593 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/02013593 Branch: refs/heads/master Commit: 02013593cf7589070ced2f5e2524ae4c4537c300 Parents: bca600c Author: Joseph Wu <jos...@mesosphere.io> Authored: Thu Sep 8 14:15:33 2016 -0700 Committer: Joseph Wu <josep...@apache.org> Committed: Thu Sep 8 14:15:33 2016 -0700 ---------------------------------------------------------------------- docs/logging.md | 23 +++++ src/slave/container_loggers/lib_logrotate.cpp | 51 +++++++++- src/slave/container_loggers/lib_logrotate.hpp | 67 +++++++++---- src/tests/container_logger_tests.cpp | 108 +++++++++++++++++++++ 4 files changed, 225 insertions(+), 24 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/02013593/docs/logging.md ---------------------------------------------------------------------- diff --git a/docs/logging.md b/docs/logging.md index 3b36870..695a883 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -130,6 +130,29 @@ setting the `--container_logger` Agent flag to <tr> <td> + <code>environment_variable_prefix</code> + </td> + <td> + Prefix for environment variables meant to modify the behavior of + the logrotate logger for the specific executor being launched. + The logger will look for four prefixed environment variables in the + <code>ExecutorInfo</code>'s <code>CommandInfo</code>'s + <code>Environment</code>: + <ul> + <li><code>MAX_STDOUT_SIZE</code></li> + <li><code>LOGROTATE_STDOUT_OPTIONS</code></li> + <li><code>MAX_STDERR_SIZE</code></li> + <li><code>LOGROTATE_STDERR_OPTIONS</code></li> + </ul> + If present, these variables will overwrite the global values set + via module parameters. + + Defaults to <code>MESOS_LOGROTATE_LOGGER_</code>. + </td> + </tr> + + <tr> + <td> <code>launcher_dir</code> </td> <td> http://git-wip-us.apache.org/repos/asf/mesos/blob/02013593/src/slave/container_loggers/lib_logrotate.cpp ---------------------------------------------------------------------- diff --git a/src/slave/container_loggers/lib_logrotate.cpp b/src/slave/container_loggers/lib_logrotate.cpp index 0155275..1c170e5 100644 --- a/src/slave/container_loggers/lib_logrotate.cpp +++ b/src/slave/container_loggers/lib_logrotate.cpp @@ -36,6 +36,7 @@ #include <stout/none.hpp> #include <stout/nothing.hpp> #include <stout/path.hpp> +#include <stout/strings.hpp> #include <stout/os/environment.hpp> #include <stout/os/fcntl.hpp> @@ -97,6 +98,48 @@ public: environment["LIBPROCESS_NUM_WORKER_THREADS"] = stringify(flags.libprocess_num_worker_threads); + // Copy the global rotation flags. + // These will act as the defaults in case the executor environment + // overrides a subset of them. + LoggerFlags overridenFlags; + overridenFlags.max_stdout_size = flags.max_stdout_size; + overridenFlags.logrotate_stdout_options = flags.logrotate_stdout_options; + overridenFlags.max_stderr_size = flags.max_stderr_size; + overridenFlags.logrotate_stderr_options = flags.logrotate_stderr_options; + + // Check for overrides of the rotation settings in the + // `ExecutorInfo`s environment variables. + if (executorInfo.has_command() && + executorInfo.command().has_environment()) { + // Search the environment for prefixed environment variables. + // We un-prefix those variables before parsing the flag values. + std::map<std::string, std::string> executorEnvironment; + foreach (const Environment::Variable variable, + executorInfo.command().environment().variables()) { + if (strings::startsWith( + variable.name(), flags.environment_variable_prefix)) { + std::string unprefixed = strings::lower(strings::remove( + variable.name(), + flags.environment_variable_prefix, + strings::PREFIX)); + executorEnvironment[unprefixed] = variable.value(); + } + } + + // We will error out if there are unknown flags with the same prefix. + Try<flags::Warnings> load = overridenFlags.load(executorEnvironment); + + if (load.isError()) { + return Failure( + "Failed to load executor logger settings: " + load.error()); + } + + // Log any flag warnings. + foreach (const flags::Warning& warning, load->warnings) { + LOG(WARNING) << warning.message; + } + } + // NOTE: We manually construct a pipe here instead of using // `Subprocess::PIPE` so that the ownership of the FDs is properly // represented. The `Subprocess` spawned below owns the read-end @@ -124,8 +167,8 @@ public: // Spawn a process to handle stdout. mesos::internal::logger::rotate::Flags outFlags; - outFlags.max_size = flags.max_stdout_size; - outFlags.logrotate_options = flags.logrotate_stdout_options; + outFlags.max_size = overridenFlags.max_stdout_size; + outFlags.logrotate_options = overridenFlags.logrotate_stdout_options; outFlags.log_filename = path::join(sandboxDirectory, "stdout"); outFlags.logrotate_path = flags.logrotate_path; @@ -182,8 +225,8 @@ public: // Spawn a process to handle stderr. mesos::internal::logger::rotate::Flags errFlags; - errFlags.max_size = flags.max_stderr_size; - errFlags.logrotate_options = flags.logrotate_stderr_options; + errFlags.max_size = overridenFlags.max_stderr_size; + errFlags.logrotate_options = overridenFlags.logrotate_stderr_options; errFlags.log_filename = path::join(sandboxDirectory, "stderr"); errFlags.logrotate_path = flags.logrotate_path; http://git-wip-us.apache.org/repos/asf/mesos/blob/02013593/src/slave/container_loggers/lib_logrotate.hpp ---------------------------------------------------------------------- diff --git a/src/slave/container_loggers/lib_logrotate.hpp b/src/slave/container_loggers/lib_logrotate.hpp index f216548..9f8ec2f 100644 --- a/src/slave/container_loggers/lib_logrotate.hpp +++ b/src/slave/container_loggers/lib_logrotate.hpp @@ -39,16 +39,20 @@ namespace logger { class LogrotateContainerLoggerProcess; -struct Flags : public virtual flags::FlagsBase +// These flags are loaded twice: once when the `ContainerLogger` module +// is created and each time before launching executors. The flags loaded +// at module creation act as global default values, whereas flags loaded +// prior to executors can override the global values. +struct LoggerFlags : public virtual flags::FlagsBase { - Flags() + LoggerFlags() { add(&max_stdout_size, "max_stdout_size", "Maximum size, in bytes, of a single stdout log file.\n" "Defaults to 10 MB. Must be at least 1 (memory) page.", Megabytes(10), - &Flags::validateSize); + &LoggerFlags::validateSize); add(&logrotate_stdout_options, "logrotate_stdout_options", @@ -66,7 +70,7 @@ struct Flags : public virtual flags::FlagsBase "Maximum size, in bytes, of a single stderr log file.\n" "Defaults to 10 MB. Must be at least 1 (memory) page.", Megabytes(10), - &Flags::validateSize); + &LoggerFlags::validateSize); add(&logrotate_stderr_options, "logrotate_stderr_options", @@ -78,6 +82,44 @@ struct Flags : public virtual flags::FlagsBase " size <max_stderr_size>\n" " }\n" "NOTE: The 'size' option will be overriden by this module."); + } + + static Option<Error> validateSize(const Bytes& value) + { + if (value.bytes() < static_cast<uint64_t>(os::pagesize())) { + return Error( + "Expected --max_stdout_size and --max_stderr_size of " + "at least " + stringify(os::pagesize()) + " bytes"); + } + + return None(); + } + + Bytes max_stdout_size; + Option<std::string> logrotate_stdout_options; + + Bytes max_stderr_size; + Option<std::string> logrotate_stderr_options; +}; + + +struct Flags : public LoggerFlags +{ + Flags() + { + add(&environment_variable_prefix, + "environment_variable_prefix", + "Prefix for environment variables meant to modify the behavior of\n" + "the logrotate logger for the specific executor being launched.\n" + "The logger will look for four prefixed environment variables in the\n" + "'ExecutorInfo's 'CommandInfo's 'Environment':\n" + " * MAX_STDOUT_SIZE\n" + " * LOGROTATE_STDOUT_OPTIONS\n" + " * MAX_STDERR_SIZE\n" + " * LOGROTATE_STDERR_OPTIONS\n" + "If present, these variables will overwrite the global values set\n" + "via module parameters.", + "CONTAINER_LOGGER_"); add(&launcher_dir, "launcher_dir", @@ -130,22 +172,7 @@ struct Flags : public virtual flags::FlagsBase }); } - static Option<Error> validateSize(const Bytes& value) - { - if (value.bytes() < static_cast<uint64_t>(os::pagesize())) { - return Error( - "Expected --max_stdout_size and --max_stderr_size of " - "at least " + stringify(os::pagesize()) + " bytes"); - } - - return None(); - } - - Bytes max_stdout_size; - Option<std::string> logrotate_stdout_options; - - Bytes max_stderr_size; - Option<std::string> logrotate_stderr_options; + std::string environment_variable_prefix; std::string launcher_dir; std::string logrotate_path; http://git-wip-us.apache.org/repos/asf/mesos/blob/02013593/src/tests/container_logger_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/container_logger_tests.cpp b/src/tests/container_logger_tests.cpp index e8f9341..a2dc126 100644 --- a/src/tests/container_logger_tests.cpp +++ b/src/tests/container_logger_tests.cpp @@ -36,6 +36,7 @@ #include <stout/os/killtree.hpp> #include <stout/os/mkdir.hpp> #include <stout/os/pstree.hpp> +#include <stout/os/read.hpp> #include <stout/os/stat.hpp> #include "master/master.hpp" @@ -522,6 +523,113 @@ TEST_F(ContainerLoggerTest, LOGROTATE_RotateInSandbox) } +// Tests that the packaged logrotate container logger will find and use +// overrides inside the Executor's environment. +TEST_F(ContainerLoggerTest, LOGROTATE_CustomRotateOptions) +{ + // Create a master, agent, and framework. + Try<Owned<cluster::Master>> master = StartMaster(); + ASSERT_SOME(master); + + Future<SlaveRegisteredMessage> slaveRegisteredMessage = + FUTURE_PROTOBUF(SlaveRegisteredMessage(), _, _); + + // We'll need access to these flags later. + slave::Flags flags = CreateSlaveFlags(); + + // Use the non-default container logger that rotates logs. + flags.container_logger = LOGROTATE_CONTAINER_LOGGER_NAME; + + Fetcher fetcher; + + // We use an actual containerizer + executor since we want something to run. + Try<MesosContainerizer*> _containerizer = + MesosContainerizer::create(flags, false, &fetcher); + + CHECK_SOME(_containerizer); + Owned<MesosContainerizer> containerizer(_containerizer.get()); + + Owned<MasterDetector> detector = master.get()->createDetector(); + + Try<Owned<cluster::Slave>> slave = + StartSlave(detector.get(), containerizer.get(), flags); + ASSERT_SOME(slave); + + AWAIT_READY(slaveRegisteredMessage); + SlaveID slaveId = slaveRegisteredMessage.get().slave_id(); + + MockScheduler sched; + MesosSchedulerDriver driver( + &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL); + + Future<FrameworkID> frameworkId; + EXPECT_CALL(sched, registered(&driver, _, _)) + .WillOnce(FutureArg<1>(&frameworkId)); + + // Wait for an offer, and start a task. + Future<vector<Offer>> offers; + EXPECT_CALL(sched, resourceOffers(&driver, _)) + .WillOnce(FutureArg<1>(&offers)) + .WillRepeatedly(Return()); // Ignore subsequent offers. + + driver.start(); + AWAIT_READY(frameworkId); + + AWAIT_READY(offers); + EXPECT_NE(0u, offers.get().size()); + + const std::string customConfig = "some-custom-logrotate-option"; + + TaskInfo task = createTask(offers.get()[0], "exit 0"); + + // Add an override for the logger's stdout stream. + // We will check this by inspecting the generated configuration file. + Environment::Variable* variable = + task.mutable_command()->mutable_environment()->add_variables(); + variable->set_name("CONTAINER_LOGGER_LOGROTATE_STDOUT_OPTIONS"); + variable->set_value(customConfig); + + Future<TaskStatus> statusRunning; + Future<TaskStatus> statusFinished; + EXPECT_CALL(sched, statusUpdate(&driver, _)) + .WillOnce(FutureArg<1>(&statusRunning)) + .WillOnce(FutureArg<1>(&statusFinished)) + .WillRepeatedly(Return()); // Ignore subsequent updates. + + driver.launchTasks(offers.get()[0].id(), {task}); + + AWAIT_READY(statusRunning); + EXPECT_EQ(TASK_RUNNING, statusRunning.get().state()); + + AWAIT_READY(statusFinished); + EXPECT_EQ(TASK_FINISHED, statusFinished.get().state()); + + driver.stop(); + driver.join(); + + // Check for the expected logger files. + string sandboxDirectory = path::join( + slave::paths::getExecutorPath( + flags.work_dir, + slaveId, + frameworkId.get(), + statusRunning->executor_id()), + "runs", + "latest"); + + ASSERT_TRUE(os::exists(sandboxDirectory)); + + // Check to see if our custom string is sitting in the configuration. + string stdoutPath = path::join(sandboxDirectory, "stdout.logrotate.conf"); + ASSERT_TRUE(os::exists(stdoutPath)); + + Try<std::string> stdoutConfig = os::read(stdoutPath); + ASSERT_SOME(stdoutConfig); + + ASSERT_TRUE(strings::contains(stdoutConfig.get(), customConfig)); +} + + // Tests that the logrotate container logger only closes FDs when it // is supposed to and does not interfere with other FDs on the agent. TEST_F(ContainerLoggerTest, LOGROTATE_ModuleFDOwnership)
