> On Jan. 27, 2016, 4:02 p.m., Benjamin Hindman wrote: > > src/slave/container_loggers/lib_logrotate.cpp, line 149 > > <https://reviews.apache.org/r/42865/diff/2/?file=1224268#file1224268line149> > > > > Is this supposed to be `outfds` or `errfds`? Do we need the `cloexec` > > at all? If just the post-fork function can we simply put a comment there > > that explains it? Using `cloexec` is better though because the fork > > solution won't work on Windows.
Looks like only the cloexec is needed. (Removed the setup functions). - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42865/#review116657 ----------------------------------------------------------- On Jan. 27, 2016, 4:11 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42865/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2016, 4:11 p.m.) > > > Review request for mesos, Benjamin Hindman and Artem Harutyunyan. > > > Bugs: MESOS-4535 > https://issues.apache.org/jira/browse/MESOS-4535 > > > Repository: mesos > > > Description > ------- > > Changes the logrotate container logger to manually construct and deal with > pipes. Specifically, both read and write ends of the pipe must end up in the > child processes (read -> logger executables, write -> container). > > If ownership is not transferred, the pipe's FDs may be closed (again) when > `Subprocess` is destructed, which may unexpectedly close random FDs belonging > to other threads. > > > Diffs > ----- > > src/slave/container_loggers/lib_logrotate.cpp > bfc7cade2eed98f21fc1c364c104ad5583648c63 > > Diff: https://reviews.apache.org/r/42865/diff/ > > > Testing > ------- > > make check (OSX) > > --- > > NOTE: I'll add an integration to codify the manual testing below. > This discarded review has the gist of the needed test: > https://reviews.apache.org/r/42864/ > > --- > > Manual testing: > > ``` > ./mesos-master.sh --work_dir=/tmp/master --ip=127.0.0.1 > ./mesos-slave.sh --master=127.0.0.1:5050 > --modules=file:///path/to/modules.json > --container_logger="org_apache_mesos_LogrotateContainerLogger" > --work_dir=/tmp/agent > > # Repeatedly trigger executor terminations. > ./mesos-execute --master=127.0.0.1:5050 --name="Repeater" --command="while > true; do /Users/josephwu/mesos/build/src/mesos-execute > --master=127.0.0.1:5050 --name=Repeat --command=\"echo Wheee; sleep 0.4\"; > done" > ``` > > "modules.json" follows the template: > ``` > { > "libraries": [ > { > "file": "/path/to/liblogrotate_container_logger.la", > "modules": [ > { > "name": "org_apache_mesos_LogrotateContainerLogger", > "parameters": [ > { > "key": "launcher_dir", > "value": "/path/to/mesos/build/src/" > }, { > "key": "logrotate_stdout_options", > "value": "rotate 2" > }, { > "key": "logrotate_stderr_options", > "value": "rotate 2" > } > ] > } > ] > } > ] > } > ``` > > > Thanks, > > Joseph Wu > >
