Unify shutdown timeout #4107
Unify shutdown timeout #4107
Conversation
|
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
1 similar comment
|
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
ruflin
added
Filebeat
|
@ruflin can you review this pull request? Sorry for all those commits, I got some trouble running tests on my computer. |
There was a problem hiding this comment.
Thanks for all the work on this. Big +1 on unifying -waitstop and shutdown_timeout. Perhaps in a first step we should just add packetbeat.shutdown_timeout to packetbeat instead of making it a global option. The problem with the global option is that it must work across all beats which complicates things. See also comment inside. Having it per beat will also allow us to document it separately.
packetbeat/beater/packetbeat.go
Outdated
| waitShutdown := pb.cmdLineArgs.waitShutdown | ||
| if waitShutdown != nil && *waitShutdown > 0 { | ||
| time.Sleep(time.Duration(*waitShutdown) * time.Second) | ||
| timeout := b.Config.ShutdownTimeout |
There was a problem hiding this comment.
The problem is that packetbeat does not have the same logic here as filebeat. On the filebeat side it waits for all events to be published and then stops. If all events are sent before shutdown_timeout is reached, it will directly shut down. Because this tracking if all events are sent does not exist in packetbeat, packetbeat just "sleeps" for the time.
The reason I worry about this it because there are two different behaviours with 1 config option. As this problem can't be covered in this PR I think we should solve it through documentation.
Also this code should probably not be in packetbeat anymore, but on the beats side instead. Otherwise each beat has to implement it.
There was a problem hiding this comment.
You right, I move it back to packetbeat. This will make it more clearer as it's not handled in libbeat yet.
| @@ -832,6 +832,22 @@ periodically afterwards, it scans the process table for | |||
| processes that match the values specified for this option. The match is done against the | |||
| process' command line as read from `/proc/<pid>/cmdline`. | |||
|
|
|||
|
|
|||
| [[shutdown-timeout]] | |||
| ===== shutdown_timeout | |||
There was a problem hiding this comment.
Seems like this should be now documented in libbeat and not packetbeat.
There was a problem hiding this comment.
LGTM. Left one minor comment for the docs. There is a conflict on the CHANGELOG.
@monicasarbu Could you also have a look at this one?
| ===== shutdown_timeout | ||
|
|
||
| How long Packetbeat waits on shutdown. By default, this option is disabled, and Packetbeat | ||
| does not wait for the publisher to finish sending events before shutting down |
There was a problem hiding this comment.
We should add here that packetbeat is just going to wait for shutdown_timeout and the closes and will not track if all events were sent previously.
There was a problem hiding this comment.
I rephrased the documentation as you commented it :)
ruflin
added
review
and removed
in progress
|
@glefloch I think you have to rebase again on master as there are now lots of changes in this PR which shouldn't be. Could you also squash the commits into 1 commit? |
glefloch
force-pushed
the
fix/3588
branch
2 times, most recently
from
6a80f1b to
4d81ac5
Compare
|
@ruflin Sorry for all that mess. I rebase master in my branch |
| ===== shutdown_timeout | ||
|
|
||
| How long Packetbeat waits on shutdown. By default, this option is disabled. Packetbeat | ||
| will wait for `shutdown_timeout` and then close. It will not track if all events were send |
CHANGELOG.asciidoc
Outdated
| @@ -179,6 +179,7 @@ https://github.com/elastic/beats/compare/v5.4.0...v6.0.0-alpha1[View commits] | |||
|
|
|||
| - Add `fields` and `fields_under_root` to Packetbeat protocols configurations. {pull}3518[3518] | |||
| - Add list style Packetbeat protocols configurations. This change supports specifying multiple configurations of the same protocol analyzer. {pull}3518[3518] | |||
| - Replace `waitstop` command line argument by `shutdown_timeout` in configuration file. {pull}3588[3588] | |||
There was a problem hiding this comment.
This should go under breaking changes.
|
@glefloch Thanks a lot for the contribution. |
This pull request aims to unify shutdown timeout.
For now, this is must be implemented in each beat
Runmethod due to specific code infilebeat.Close #3588