[beatreceiver][packetbeat] Delegate shutdown_timeout logic to libbeat - #52005
Conversation
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform) |
TL;DRThe five failed Packetbeat unit-test jobs share a shutdown-drain regression introduced by this PR, not an OS-specific failure. Remediation
Investigation detailsRoot CauseThe PR removes the wait in The new Evidence
VerificationNot run locally; the available evidence is from the five failed Buildkite jobs. What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
a05b6f0 to
92ccf96
Compare
|
There is fundamental difference in what In filebeat, Since this PR immediately stops sending any buffered events to the pipeline on stop signal, we are seeing certain python tests fail. I think Can the @elastic/sec-linux-platform team confirm why we need this config? |
So it sounds like we simply shouldn't set this for packetbeat receiver until we figure out what it should do. Ideally shutdown_timeout should work the same way for packetbeat as it does for other beats and the internal setting should have a different name. |
|
This is a really old parameter. I don't personally know of a reason it couldn't be renamed in packetbeat. But maybe we should see if @ruflin has any context. It looks like the parameter was renamed to unify it with FileBeat as part of #3588 but then when the delay was executed was adjusted to make tests pass here: c0de062 @khushijain21 I don't know that anyone on my team will have the context needed to answer this really well. I see that @fearful-symmetry and @efd6 have both contributed near this so it is probably worth having both of them weigh in on if they remember anything about it. |
|
Hmm, yeah, I don't think I've ever tinkered with this, so I don't have any context to add here. There's a lot of old cruft in Packetbeat. I suspect there wasn't a whole lot of reasoning going into those changes, however. We should probably bias towards "whatever makes it work" |
|
From talking to both Alex and Dan none of us have a lot of context with this but we think the change makes sense. |
|
Tick the box to add this pull request to the merge queue (same as
|
|
@Mergifyio backport 9.5 9.4 9.3 8.19 |
✅ Backports have been createdDetails
Cherry-pick of 663f454 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of 663f454 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of 663f454 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Bring in #52269 (logger backport) required by this change. Adapt #52005 to 8.19 by adding Beat.ShutdownTimeout for the beater assignment and switching the processor flush wait to publish_timeout so CheckConfig no longer blocks on shutdown_timeout. Assisted-By: Composer Co-authored-by: Cursor <cursoragent@cursor.com>
Adapt #52005 to 9.3 by adding Beat.ShutdownTimeout for the beater assignment and switching the processor flush wait to publish_timeout, without pulling main-only logger changes. Assisted-By: Composer Co-authored-by: Cursor <cursoragent@cursor.com>
Adapt #52005 to 9.4 by adding Beat.ShutdownTimeout for the beater assignment and switching the processor flush wait to publish_timeout, without pulling main-only logger changes. Assisted-By: Composer Co-authored-by: Cursor <cursoragent@cursor.com>
Proposed commit message
This PR delegates
shutdown_timeoutlogic to libbeat. It stays consistent with how other beats also handle the same.This is also required so that we do not wait for
shutdown_timeoutperiod every time the config is validated hereChecklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.Disruptive User Impact
None
Will also close #52031