-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
T6362: Create conntrack logger daemon #3804
Conversation
👍 |
👍 |
src/services/vyos-conntrack-logger
Outdated
try: | ||
while True: | ||
logger.debug('Enter main loop...') | ||
if shutdown: | ||
break | ||
for msg in ct.get(): | ||
parsed_event = parse_conntrack_event(msg, conf_proto) | ||
|
||
if parsed_event: | ||
message = format_event_message(parsed_event) | ||
logger.info(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to use queues with threads here to keep control over the daemon in high loads and utilize all available CPU cores for processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
One thread receives socket messages and puts them into the queue, several processes (one per CPU) get messages from the queue and parse them, write log.
NOTE: In the current implementation, the order of messages in the log may be disrupted because messages are parsed asynchronously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: In the current implementation, the order of messages in the log may be disrupted because messages are parsed asynchronously
This is a problem. A common use case for this feature is to detect a connection's start and end times.
10e584b
to
6a7e455
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems mostly good, I left some comments about possible improvements and clarifications.
|
||
while not shutdown_event.is_set(): | ||
if not ct.pthread.is_alive(): | ||
if ct.buffer_queue.qsize()/ct.async_qsize < 0.9: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this part, to be fair. Could you clarify?
- Why is the queue fullness check needed here?
- How was the threshold of 90% chosen?
- Why the listener thread should not be restarted if it's dead and the queue is >90% full?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restarting the thread right away seems like a bad idea to me, because it will probably crash again almost immediately because the queue will still be nearly full or full.
To avoid wasting resources on restarting a thread that is obviously not working, it is better to wait until the queue is a little unloaded.
The threshold of 90% is taken as one of the options for waiting for free space in the queue, so as not to lose too many messages and not to spam with a new thread too often, we can set any threshold or even just empty the queue upon restart
CI integration ❌ failed! Details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get it in, then we can refine it.
Change Summary
Created conntrack logger daemon based on pyroute2
The current format log is similar to the command
contract -E
, also added cta_id and ability to add flow-based timestamp extensionlog example:
Types of changes
Related Task(s)
Related PR(s)
Component(s) name
Proposed changes
here
timestamp
- turns on flow-based timestamp extensionhttps://patchwork.ozlabs.org/project/netdev/patch/[email protected]/
queue-size
- allows to configure of internal queue message size to avoid queue overload in high load.event
- configure events available for logging, by default all protocols in selected events will be loggedalso each event type can be filtered by protocols e.g.:
How to test
Smoketest result
Checklist: