output/flush: Correct EVE flushing logic#15107
Conversation
Add flushing logic driven off of the file contexts. This is a simpler solution that removes the need for logger registration changes. Overview: Use the heartbeat-driven thread to periodically flush all registered EVE contexts via a global flush list. The global flush list is a mutex-protected TAILQ of LogFileFlushEntry nodes; each node points to a LogFileCtx. Mutex = log_file_flush_mutex Periodic flushing performed by a thread according to the heartbeat.output-flush-interval [1,60]. LogFileFlushAll() is invoked to initiate flushing of registered LogFileCtx structs; each struct's fp_mutex is obtained while the flush occurs to synchronize with LogFileWrite activity. Interacts with file-rotation via the fp_mutex. Deadlock prevention: the log_file_flush_mutex must be obtained before the fp_mutex. Issue: 8286 (cherry picked from commit a78911f)
Remove packet-based flush logic in favor of simpler solution Issue: 8286 (cherry picked from commit d0ba1c4)
Remove log flush functions and update registration logic as context-based flushing doesn't require it. Issue: 8286 (cherry picked from commit 1923ca1)
Update output flushing description to reflect EVE based approach in documentation and config template. Issue: 8286 (cherry picked from commit e7dc0d8)
jlucovsky
requested review from
a team,
jufajardini and
victorjulien
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main-8.0.x #15107 +/- ##
===========================================
Coverage 83.59% 83.60%
===========================================
Files 1011 1011
Lines 266822 266748 -74
===========================================
- Hits 223048 223011 -37
+ Misses 43774 43737 -37
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Information: QA ran without warnings. Pipeline = 30557 |
|
Should there be an upgrade note ? Seems a big functional change to backport... |
Agree. Also going to give this a bit of time in |
Note that there are no behavioral differences with this implementation so I'm not sure what the upgrade notes would contain. |
| ThreadDeinitFunc ThreadDeinit; | ||
|
|
||
| PacketLogger PacketLogFunc; | ||
| PacketLogger PacketFlushFunc; |
There was a problem hiding this comment.
This seems a behavioral difference : any plugins that define their own modules will need to be updated.
What do you think @jasonish ?
There was a problem hiding this comment.
I think we're OK? As long as users use the registration functions, they are not concerned with changes to the size of this structure. Do you see another concern?
There was a problem hiding this comment.
I pinged you because you knew better :-)
There was a problem hiding this comment.
Yeah. Any data structure change in a backport should get careful review.
|
@jlucovsky Can you take a look at |
|
Continued in #15241 |
5 participants
Backport of changes made for issue 8286 for main-8.0.x backport.
Link to ticket: https://redmine.openinfosecfoundation.org/issues/8400
Describe changes:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=