Skip to content

net/frr: watchfrr service handling #4712

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AndyX90
Copy link
Contributor

@AndyX90 AndyX90 commented May 17, 2025

This commit fixes CARP event handler integration with FRR service startup.
It resolves issues with triggering the CARP event handler during service startup, particularly in setups where route costs depend on CARP status.

  • Removes the "start_postcmd" override, as routing deamon lifecycle management is now handled by watchfrr.
  • Introduces two service wrapper scripts to hook into watchfrr's startup and restart procedures.

More information on actual startup behavior:
https://cgit.freebsd.org/ports/tree/net/frr8/files/frr.in#n15
https://cgit.freebsd.org/ports/tree/net/frr8/files/watchfrr.in#n25

Tested Scenarios (correct costs applied):
✓ Stopping / Starting FRR
✓ Killing ospfd process
✓ Reboot
✓ Service reload through frr-reload.py

Resolves #4702

@AdSchellevis AdSchellevis self-assigned this May 18, 2025
@AndyX90 AndyX90 marked this pull request as draft May 19, 2025 08:22
@AndyX90 AndyX90 marked this pull request as ready for review May 20, 2025 12:36
Copy link
Member

@AdSchellevis AdSchellevis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at a first glance this looks fine, but I rather have a single wrapper script in stead of two.

@AndyX90
Copy link
Contributor Author

AndyX90 commented May 23, 2025

@AdSchellevis This one works!
Note: It also removes the old 50-frr start syshook. It is not needed anymore and produces errors on startup:
>>> Error in start script '50-frr'

EDIT: Another option instead of removal could've been to change it to something like the carp syshook:
... shell_exec('/usr/local/opnsense/scripts/frr/carp_event_handler'); ...

@@ -1,4 +0,0 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's ok to try and ditch this, but it's also hard to predict that this isn't needed anymore because it handles a startup error edge case. Functionally this shouldn't matter. If we are worried about the error being shown a || true would do the trick, or checking if already running before starting. But only commenting here for future us. ;)

@AndyX90
Copy link
Contributor Author

AndyX90 commented May 23, 2025

Changes:

  • the wrapper now catches the exit code of the service and returns it to watchfrr.
  • actions_quagga.conf changed accordingly
  • added copyright header

I would try to omit the start syshook, because watchfrr is now enabled by default and should handle such a case.
If I'm not mistaken this was because of (sub)process startup issues.

Copy link
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@AndyX90 AndyX90 force-pushed the frr-fixes branch 2 times, most recently from ed0e2bd to 2bcd16d Compare May 23, 2025 08:22
@AndyX90
Copy link
Contributor Author

AndyX90 commented May 23, 2025

Pushed the wrong one 😅
So i think this is the safest version.
It uses the startup syshook to invoke the carp_event_handler on successful startup through rc and internally watchfrr start/restarts are handled.
If you are okay with this @fichtner and @AdSchellevis then this is the one.

Note: The startup now is taken from the carp hook. This is not explicitly required, but i noticed sometimes some weird timing issues on reboot, when the service starts and watchfrr starts the all the services. It safeguards the setting of needed costs.

@AndyX90
Copy link
Contributor Author

AndyX90 commented May 24, 2025

Changes:

  • added syslog filter definition for frr_wrapper

@Monviech Monviech mentioned this pull request Jun 4, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

net/frr: OSPF CARP interface costs don't survive a service restart
3 participants