-
Notifications
You must be signed in to change notification settings - Fork 346
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
T6641: Add vyos-network-event-logger Service #4216
base: current
Are you sure you want to change the base?
Conversation
👍 |
As mentioned on Slack, this should be rebased over current and pushed to sync with the changes in A local build of this PR confirms a successful build and smoketests. |
Is there any downside not enabling this service by default? |
at least, you must remember to turn it on to analyze some situations and reproduce these? otherwise logs will not stored. |
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 left a bunch of suggestions for improvement.
op-mode-definitions/include/show-interface-type-event-log.xml.i
Outdated
Show resolved
Hide resolved
<properties> | ||
<help>Show log for route network events</help> | ||
</properties> | ||
<command>journalctl --no-hostname --boot --unit vyos-network-event-logger.service | grep "$(echo "\[$6\]" | tr '[:lower:]' '[:upper:]')" | grep "\b$4\b"</command> |
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.
The journalctl command is duplicated a lot here. Should we make it a script?
I'm also not convinced about the value of the tr
call here.
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 this is too simple a command to make a script, in the case of a script we will also duplicate the script call with approximately the same parameters
op-mode-definitions/include/show-interface-type-event-log.xml.i
Outdated
Show resolved
Hide resolved
<interfaceDefinition> | ||
<node name="service"> | ||
<children> | ||
<node name="monitoring"> |
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.
We should probably make a skeleton node for monitoring and include everything else into it, although it's a topic for a different PR.
logger.setLevel(logging.INFO) | ||
|
||
|
||
# https://github.com/torvalds/linux/blob/adc218676eef25575469234709c2d87185ca223a/include/uapi/linux/neighbour.h#L46 |
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.
Should we move the constants to a separate module? There may be more places in the future where they might be useful for handling netlink events.
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 moved a few constants to the vyos library, but it would probably be better to make a PR for the pyroute2 library in future
op-mode-definitions/include/show-interface-type-event-log.xml.i
Outdated
Show resolved
Hide resolved
Another reason for having this running always in the background: From FRR
|
The service parses and logs network events for improved monitoring and diagnostics. Supported event types include: - `RTM_NEWROUTE`, `RTM_DELROUTE` - `RTM_NEWLINK`, `RTM_DELLINK` - `RTM_NEWADDR`, `RTM_DELADDR` - `RTM_NEWNEIGH`, `RTM_DELNEIGH`, `RTM_GETNEIGH` - `RTM_NEWRULE`, `RTM_DELRULE` Added operational mode commands for filtered log retrieval: - `show log network-event <event-type> <interface>`: Retrieve logs filtered by event type and interface. - `show interfaces <type> <name> event-log <event-type>`: Display interface-specific logs filtered by event type.
CI integration ❌ failed! Details
|
@@ -0,0 +1,11 @@ | |||
<!-- included start from isis-common.xml.i --> |
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.
How does it relate to isis-common
?
# rtnl.RTM_NEWADDRLABEL: {'parser': AddrlabelFormatter, 'event': 'addrlabel'}, | ||
# rtnl.RTM_DELADDRLABEL: {'parser': AddrlabelFormatter, 'event': 'addrlabel'}, |
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.
Do we need this?
# rtnl.RTM_NEWNETCONF: {'parser': NetconfFormatter, 'event': 'netconf'}, | ||
# rtnl.RTM_DELNETCONF: {'parser': NetconfFormatter, 'event': 'netconf'}, |
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.
Do we need this?
The service parses and logs network events for improved monitoring and diagnostics. Supported event types include:
RTM_NEWROUTE
,RTM_DELROUTE
RTM_NEWLINK
,RTM_DELLINK
RTM_NEWADDR
,RTM_DELADDR
RTM_NEWNEIGH
,RTM_DELNEIGH
,RTM_GETNEIGH
RTM_NEWRULE
,RTM_DELRULE
Added operational mode commands for filtered log retrieval:
show log network-event <event-type> <interface>
: Retrieve logs filtered by event type and interface.show interfaces <type> <name> event-log <event-type>
: Display interface-specific logs filtered by event type.Change Summary
Types of changes
Related Task(s)
Related PR(s)
Component(s) name
Proposed changes
log examples:
How to test
To check the correct parsing you can use logs from
ip monitor label
configure network-event logger to listen events:
you can use IP utils to manipulate the interface and check logs or use some
set interface *
commands e.g.Smoketest result
Checklist: