Skip to content
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

babel: Clean babel related config on daemon stop #17715

Merged
merged 1 commit into from
Jan 4, 2025

Conversation

ykholod
Copy link

@ykholod ykholod commented Dec 23, 2024

When deactivating babel no router babel and later re-enabling it router babel the previous configuration is still in place.

Steps to reproduce:

Enable babel
Configure babel
Disable babel with "no router babel"
Verify config

Expected correct behavior: No config present

@ykholod
Copy link
Author

ykholod commented Dec 27, 2024

When disable and enable babel router network configurations are still present. This is an issue, cause it disrupts normal babel router usage.
Please have a look at this PR

babeld/babeld.c Outdated
@@ -310,6 +314,17 @@ void babel_clean_routing_process(void)
resend_delay = BABEL_DEFAULT_RESEND_DELAY;
change_smoothing_half_life(BABEL_DEFAULT_SMOOTHING_HALF_LIFE);

/* Clean babel network interfaces */
FOR_ALL_INTERFACES(vrf, ifp) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we do this inside babel_interface_close_all()?

Copy link
Author

Choose a reason for hiding this comment

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

Done
Makes sense

interface_reset(ifp);
}
/* Disable babel redistribution */
for (type = 0; type < ZEBRA_ROUTE_MAX; type++) {
zclient_redistribute (ZEBRA_REDISTRIBUTE_DELETE, zclient, family2afi(AF_INET), type, 0, VRF_DEFAULT);
Copy link
Member

Choose a reason for hiding this comment

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

Why not directly AFI_IP?

Copy link
Author

Choose a reason for hiding this comment

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

Makes no difference to me. Fixed

/* Disable babel redistribution */
for (type = 0; type < ZEBRA_ROUTE_MAX; type++) {
zclient_redistribute (ZEBRA_REDISTRIBUTE_DELETE, zclient, family2afi(AF_INET), type, 0, VRF_DEFAULT);
zclient_redistribute (ZEBRA_REDISTRIBUTE_DELETE, zclient, family2afi(AF_INET6), type, 0, VRF_DEFAULT);
Copy link
Member

Choose a reason for hiding this comment

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

Why not directly AFI_IP6?

Copy link
Author

Choose a reason for hiding this comment

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

Makes no difference to me. Fixed

@@ -719,6 +719,7 @@ babel_interface_close_all(void)
{
struct vrf *vrf = vrf_lookup_by_id(VRF_DEFAULT);
struct interface *ifp = NULL;
int type = ZEBRA_ROUTE_ALL;
Copy link
Member

Choose a reason for hiding this comment

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

We don't need initialization here at all, because we do it inside the loop.

Copy link
Author

Choose a reason for hiding this comment

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

Cause I am afraid of uninitialized variables, regardless compiler settings
Fixed

When deactivating babel no router babel and later re-enabling it router babel the previous configuration is still in place.

Steps to reproduce:

    Enable babel
    Configure babel
    Disable babel with "no router babel"
    Verify config

Expected correct behavior: No config present

Signed-off-by: Yaroslav Kholod <y.kholod@vyos.io>
@ykholod ykholod requested a review from ton31337 January 2, 2025 11:17
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

LGTM

@ton31337 ton31337 merged commit c5fb0a9 into FRRouting:master Jan 4, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants