You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2016/01/27 21:45:06 UTC
Review Request 42865: Fixed LogrotateContainerLogger's FD ownership.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42865/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Bugs: MESOS-4535
https://issues.apache.org/jira/browse/MESOS-4535
Repository: mesos
Description
-------
Releases ownership of pipe FDs from `Subprocess` before passing them into `Subprocess::FD(___, IO::OWNED)`. If ownership is not transferred, the pipe's FDs may be closed (again), 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
-------
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
./mesos-execute --master=127.0.0.1:5050 --name="Snore" --command="while true; do echo 'ZZZzzz...'; sleep 0.4; done" &
./mesos-execute --master=127.0.0.1:5050 --name="Snorelax" --command="while true; do echo 'SNORE'; sleep 1; done" &
./mesos-execute --master=127.0.0.1:5050 --name="Sleepy" --command="while true; do echo 'Yawn :D'; sleep 0.6; done" &
./mesos-execute --master=127.0.0.1:5050 --name="Tired" --command="while true; do echo 'Blegh X_X'; sleep 0.5; done" &
./mesos-execute --master=127.0.0.1:5050 --name="Sleep Zombie" --command="while true; do echo 'Pillowz...'; sleep 1.2; done" &
./mesos-execute --master=127.0.0.1:5050 --name="ZZZ" --command="while true; do echo '...'; sleep 0.8; done" &
# Trigger a bunch of executor terminations. The agent may fail an assert when it re-closes a socket.
killall mesos-execute
```
"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
Re: Review Request 42865: Fixed LogrotateContainerLogger's FD
ownership.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42865/#review116663
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Jan. 28, 2016, 12:11 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42865/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2016, 12:11 a.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
>
>
Re: Review Request 42865: Fixed LogrotateContainerLogger's FD
ownership.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Addressed cloexec-ing vs setup functions. (Fixed typo in code)
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 (updated)
-----
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
Re: Review Request 42865: Fixed LogrotateContainerLogger's FD
ownership.
Posted by Joseph Wu <jo...@mesosphere.io>.
> 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
>
>
Re: Review Request 42865: Fixed LogrotateContainerLogger's FD
ownership.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42865/#review116657
-----------------------------------------------------------
src/slave/container_loggers/lib_logrotate.cpp (line 107)
<https://reviews.apache.org/r/42865/#comment177726>
Can we get a comment on why we need to `cloexec` please?
src/slave/container_loggers/lib_logrotate.cpp (line 149)
<https://reviews.apache.org/r/42865/#comment177728>
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.
- Benjamin Hindman
On Jan. 27, 2016, 11:20 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, 11:20 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
>
>
Re: Review Request 42865: Fixed LogrotateContainerLogger's FD
ownership.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42865/
-----------------------------------------------------------
(Updated Jan. 27, 2016, 3:20 p.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Changed to a different approach, putting all the code change into the module.
Bugs: MESOS-4535
https://issues.apache.org/jira/browse/MESOS-4535
Repository: mesos
Description (updated)
-------
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 (updated)
-----
src/slave/container_loggers/lib_logrotate.cpp bfc7cade2eed98f21fc1c364c104ad5583648c63
Diff: https://reviews.apache.org/r/42865/diff/
Testing (updated)
-------
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