----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52310/#review151274 -----------------------------------------------------------
src/slave/container_loggers/logrotate.cpp (lines 39 - 41) <https://reviews.apache.org/r/52310/#comment219624> `#include <stout/os/su.hpp>` should go in alphabetical order (middle). src/slave/container_loggers/logrotate.cpp <https://reviews.apache.org/r/52310/#comment219625> We tend to put two newlines between certain sections of the code. A non-exhaustive list is: * include-headers * after `using ...;` statements * between function implementations (but not declarations) src/slave/container_loggers/logrotate.cpp (lines 248 - 252) <https://reviews.apache.org/r/52310/#comment219626> My bad for not saying this clearly ( https://reviews.apache.org/r/52310/#comment218796 ). I actually meant the user should be switched one line further up. There's no need to split this logical block in half: ``` // Asynchronously control the flow and size of logs. LogrotateLoggerProcess process(flags); spawn(&process); ``` src/slave/container_loggers/logrotate.cpp (lines 248 - 249) <https://reviews.apache.org/r/52310/#comment219628> Typo: `logroate` src/slave/container_loggers/logrotate.cpp (line 251) <https://reviews.apache.org/r/52310/#comment219627> You should catch the return value here `Try<Nothing>` and see if it actually succeeded. In the case of the Logrotate logger, we're mostly concerned with the logger being run as root, when the user's container is non-root. It turns out that `logrotate` will handle this situation safely (but it might refuse to rotate any logs). We can handle this in two ways: 1) Exit the logger immediately. The task won't be able to log anything, but an error message will most likely end up in the agent's logs (unless the agent crashes exactly before this). 2) Log something (i.e. `LOG(WARNING)`) and continue. Because this is right near the beginning of the logger, I'd recommend (1). - Joseph Wu On Sept. 30, 2016, 6:34 p.m., Sivaram Kannan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52310/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2016, 6:34 p.m.) > > > Review request for mesos and Joseph Wu. > > > Bugs: MESOS-5856 > https://issues.apache.org/jira/browse/MESOS-5856 > > > Repository: mesos > > > Description > ------- > > Switch the uid of the binary if a user is passed from the lib_logrotate. > > > Diffs > ----- > > src/slave/container_loggers/logrotate.cpp > 431bc3cbb54e94359078e4dae0b32ad301393640 > > Diff: https://reviews.apache.org/r/52310/diff/ > > > Testing > ------- > > 1. Run the mesos-logrotate-logger with un-priviledged user and verify whether > the logs are getting rotated. > 2. Run the mesos-logrotate-logger as root user and verify whether the logs > are getting rotated. > > > Thanks, > > Sivaram Kannan > >
