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

zebra,bgpd: add match tracker option to bgp route-map #12372

Closed
wants to merge 17 commits into from

Conversation

louis-6wind
Copy link
Contributor

@louis-6wind louis-6wind commented Nov 22, 2022

We have a cluster of two routers running the same daemons to deal with the traffic (IPSec, firewall...). Some daemons must not process traffic simultaneously on both cluster members. We use Keepalived to decide which of them should deal with the traffic. Basically, when Keepalived elects the router as the MASTER, the router must process the traffic, else it must not process the traffic but must router it to the MASTER.

To do this, we would like to advertise some prefixes from BGP only when the router is in MASTER state.

Since Keepalived is able to write its state on a file, I suggest to the community a feature to:

  • track the state of a file
  • add a route-map option "match tracker NAME" to advertise the prefixes when the tracker is up
  • re process the route-map to remove or add prefix advertisement without delay and without changing the user configuration

An event driven "match" option is present in Cisco IOS-XR route-policy.

Other community suggestions:

  • using a custom LUA script. Writing a custom script is more complex for the end user than using a native solution. Not sure we can reprocess the route-map in case an external file changes.
  • use an external script to modify the conf and add an option to process route-map immediately. We do not want the user configuration to change on an event. I prefer a native solution than writting a custom external script.

The current pull request is a draft. We need to agree on the design before continuing

@louis-6wind louis-6wind marked this pull request as draft November 22, 2022 15:57
@sworleys
Copy link
Member

This is what I was saying may be of interest to you:

http://docs.frrouting.org/projects/dev-guide/en/latest/scripting.html#examples

Basically you can use lua scripts to track external object state in a route-map

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

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.

Documentation is missing as well.


DEFUNSH(VTYSH_ZEBRA, no_tracker_file, vtysh_no_trackerfile_cmd,
"no tracker NAME [file]",
"Tracker configuration\n"
Copy link
Member

Choose a reason for hiding this comment

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

NO_STR is missing,

"Event trackers";
key name;
leaf name {
type string {
Copy link
Member

Choose a reason for hiding this comment

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

string?


for (ALL_LIST_ELEMENTS_RO(zebra_tracker_file_master, node,
tracker_file)) {
if (strncmp(name, tracker_file->name,
Copy link
Member

Choose a reason for hiding this comment

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

nit: strmatch().

tracker_file =
XCALLOC(MTYPE_TRACKER_FILE, sizeof(struct zebra_tracker_file));

snprintf(tracker_file->name, sizeof(tracker_file->name), "%s", name);
Copy link
Member

Choose a reason for hiding this comment

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

name is already char, can't we use strlcpy(), here?

}

DEFPY_YANG_NOSH(no_tracker_file, no_trackerfile_cmd,
"no tracker NAME$name [file]",
Copy link
Member

Choose a reason for hiding this comment

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

NO_STR is missing.

}

DEFPY_YANG(tracker_file_path, tracker_file_path_cmd, "path PATH",
"Absolute file path\n"
Copy link
Member

Choose a reason for hiding this comment

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

Can it be more descriptive in this place?

}


DEFUN(tracker_file_show, tracker_file_show_cmd, "show tracker file",
Copy link
Member

Choose a reason for hiding this comment

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

DEFPY

} else if (line > 1) {
if (IS_ZEBRA_DEBUG_TRACKER)
zlog_debug("%s: %s file has more than one line",
__func__, tracker_file->path);
Copy link
Member

Choose a reason for hiding this comment

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

No __func__ everywhere, please.

break;
}

if (event->mask & IN_CLOSE_WRITE) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: CHECK_FLAG()

event = (struct inotify_event *)((char *)event + sizeof(*event) +
event->len)) {

if (!(event->mask & (IN_DELETE_SELF | IN_CLOSE_WRITE)))
Copy link
Member

Choose a reason for hiding this comment

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

nit: CHECK_FLAG()

@pguibert6WIND
Copy link
Member

The advantage of this framework is that a change in a file is immediately notified to the route-map.
Without this, a change may be propagated in tens of seconds.

This is what I was saying may be of interest to you:

http://docs.frrouting.org/projects/dev-guide/en/latest/scripting.html#examples

Basically you can use lua scripts to track external object state in a route-map

@donaldsharp
Copy link
Member

I agree w/ @sworleys -> this is something that should be done with the lua event hooks system already in place. We need agreement in general that this is the approach we want to take or not. I also fail to see how any of this can't be done within the lua framework

@ton31337
Copy link
Member

Extending Lua subsystem would be a better choice. Not only in terms of this case but overall, improving an existing, which is a bit abandoned right now.

@pguibert6WIND
Copy link
Member

Extending Lua subsystem would be a better choice. Not only in terms of this case but overall, improving an existing, which is a bit abandoned right now.

There is a brick missing in what is proposed.
If I understand right, lua is called periodically (as per the route-map period of 20 seconds if I remember well). you say, lets use lua, but how can we speed up the polling without flooding the bgp CPU ?

I talk about a kind of trigger.
For instance, I was thinking of zlog_rotate mechanism. Today it is reopening the file, if a given interrupt is received. what about using that mechanism ?

@sworleys
Copy link
Member

sworleys commented Dec 13, 2022

There is a brick missing in what is proposed. If I understand right, lua is called periodically (as per the route-map period of 20 seconds if I remember well). you say, lets use lua, but how can we speed up the polling without flooding the bgp CPU ?

For what you need, I would first look at adding a hook point to our thread_event system such that at X specified poll intervals when we hit that event we call into a lua script to check whatever object you want to. In this case, a file. And then return from the script and handle any resulting change in C code based on file state.

@sworleys
Copy link
Member

The thread_event system operates on a loop so you could make it poll pretty fast I am sure.

@sworleys
Copy link
Member

using a custom LUA script. Writing a custom script is more complex for the end user than using a native solution

we can always ship a short lua script along with FRR that already does it so the user doesn't have to write anything

@github-actions
Copy link

This PR is stale because it has been open 180 days with no activity. Comment or remove the autoclose label in order to avoid having this PR closed.

Add yang schema to support zebra file trackers. The schema is ready for
future types of tracker.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add the file tracker CLI. The commands does nothing more than displaying
the configuration in show run and allowing the configuration to be
loaded from a file at startup.

> conf
> tracker TRACKER-KEEPALIVED file
>  path /var/run/keepalived
>  condition pattern MASTER
> exit
> no tracker TRACKER-KEEPALIVED

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add the file tracker backend, a struct zebra_tracker_file and make zebra
manages the list of file tracker in memory.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add the show tracker file command to display running tracker files.

> r1# show tracker file
> List of file trackers
>
> Name                Status  Path                    Condition
> ------------------------------------------------------------------------
> TRACKER-KEEPALIVED  down    /var/keepalived/notify  pattern MASTER exact
> TEST                down    /tmp/test               exist

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add the "debug zebra tracker" command to display debug info about
trackers.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add the mechanism to fetch a tracker status and to update it as soon as
the file is updated. If the file is not yet created, a check happens
every seconds to check that the file has not been created and when it is
created, its updates are triggered by inotify (deletion and content
update events are watched).

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add some ZAPI messages to send tracker notifications from zebra to
daemons. ZEBRA_TRACKER_NOTIFY notifies about new trackers and their
status change. ZEBRA_TRACKER_DEL notifies about tracker deletions.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
This set of commits is about adding a conditional advertising of prefix
from BGP feature by the means of trackers and a route-map "match
tracker" option.

The tracker ZAPI messages are flooded to all daemons.

Only send tracker ZAPI messages to bgpd.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Send tracker ZAPI notifications to daemons when a new tracker is added,
its status is changed and it is deleted.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add a library to decode tracker ZAPI messages in daemons.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Make bgpd manage a list of trackers from the tracker ZAPI messages
information.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
If a routing daemon starts after zebra and zebra has finished to update
the tracker status, the routing daemon will not be informed of the
tracker status. It will consider that no tracker exists.

Send notify ZAPI messages about all trackers when a new daemon registers
to zebra.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add the following "match tracker" command:

> r1(config)# route-map RMAP-OUT permit 1
> r1(config-route-map)# match tracker TRACKER-VRRP up

The CLI does not check that TRACKER-VRRP exists in zebra configuration.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add the backend logic to deal with the route-map match tracker option.
The match tracker takes into account the status of the tracker at the
route-map creation. Next commits are going to deal with the update of
the tracker status.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Reapply the route-maps that use the tracker that is updated.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
A route-map is re-applied after its configuration changes with a delay.
"bgp route-map delay-timer" allows configuring this timer.

Re-apply the route-map immediately without waiting for the delay timer
to expire when a tracker status changes.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Add a bgp_route_map_tracker topotest to test the zebra tracker file
and the BGP route-map match tracker feature.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Failed

OpenBSD 7 amd64 build: Failed (click for details) OpenBSD 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI011BUILD/config.log/config.log.gz OpenBSD 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI011BUILD/config.status/config.status

Make failed for OpenBSD 7 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI011BUILD/ErrorLog/log_make.txt)

/home/ci/cibuild.14278/frr-source/doc/user/zebra.rst:830: WARNING: duplicate clicmd description of segment-routing, other instance in pathd
/home/ci/cibuild.14278/frr-source/doc/user/zebra.rst:843: WARNING: duplicate clicmd description of locator NAME, other instance in isisd
zebra/zebra_tracker_notify.c:23:10: fatal error: 'sys/inotify.h' file not found
#include <sys/inotify.h>
1 error generated.
gmake[1]: *** [Makefile:10627: zebra/zebra_tracker_notify.o] Error 1
gmake[1]: Leaving directory '/home/ci/cibuild.14278/frr-source'
gmake[1]: Target 'all-am' not remade because of errors.
gmake: *** [Makefile:6483: all] Error 2
FreeBSD 11 amd64 build: Failed (click for details) FreeBSD 11 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI009BUILD/config.log/config.log.gz FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI009BUILD/config.status/config.status

Make failed for FreeBSD 11 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI009BUILD/ErrorLog/log_make.txt)

/usr/home/ci/cibuild.14278/frr-source/doc/user/zebra.rst:830: WARNING: duplicate clicmd description of segment-routing, other instance in pathd
/usr/home/ci/cibuild.14278/frr-source/doc/user/zebra.rst:843: WARNING: duplicate clicmd description of locator NAME, other instance in isisd
zebra/zebra_tracker_notify.c:23:10: fatal error: sys/inotify.h: No such file or directory
compilation terminated.
gmake[1]: *** [Makefile:10627: zebra/zebra_tracker_notify.o] Error 1
gmake[1]: Leaving directory '/usr/home/ci/cibuild.14278/frr-source'
gmake[1]: Target 'all-am' not remade because of errors.
gmake: *** [Makefile:6483: all] Error 2
FreeBSD 12 amd64 build: Failed (click for details) FreeBSD 12 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/FBSD12AMD64/config.log/config.log.gz FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/FBSD12AMD64/config.status/config.status

Make failed for FreeBSD 12 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/FBSD12AMD64/ErrorLog/log_make.txt)

/usr/home/ci/cibuild.14278/frr-source/doc/user/ospfd.rst:642: WARNING: Cannot analyze code. No Pygments lexer found for "frr".
/usr/home/ci/cibuild.14278/frr-source/doc/user/pbr.rst:50: WARNING: duplicate label nexthop-groups, other instance in /usr/home/ci/cibuild.14278/frr-source/doc/user/nexthop_groups.rst
zebra/zebra_tracker_notify.c:23:10: fatal error: sys/inotify.h: No such file or directory
 #include <sys/inotify.h>
compilation terminated.
gmake[1]: *** [Makefile:10624: zebra/zebra_tracker_notify.o] Error 1
gmake[1]: Target 'all-am' not remade because of errors.
gmake[1]: Leaving directory '/usr/home/ci/cibuild.14278/frr-source'
gmake: *** [Makefile:6480: all] Error 2
Successful on other platforms/tests
  • Redhat 8 amd64 build
  • Debian 10 amd64 build
  • Ubuntu 20.04 amd64 build
  • Ubuntu 22.04 amd64 build
  • CentOS 7 amd64 build
  • Debian 9 amd64 build
  • Ubuntu 18.04 amd64 build
  • Redhat 9 amd64 build
  • Ubuntu 18.04 ppc64le build
  • Ubuntu 18.04 i386 build
  • Ubuntu 18.04 arm7 build
  • Ubuntu 18.04 arm8 build
  • Debian 11 amd64 build

Warnings Generated during build:

Checkout code: Successful with additional warnings
OpenBSD 7 amd64 build: Failed (click for details) OpenBSD 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI011BUILD/config.log/config.log.gz OpenBSD 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI011BUILD/config.status/config.status

Make failed for OpenBSD 7 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI011BUILD/ErrorLog/log_make.txt)

/home/ci/cibuild.14278/frr-source/doc/user/zebra.rst:830: WARNING: duplicate clicmd description of segment-routing, other instance in pathd
/home/ci/cibuild.14278/frr-source/doc/user/zebra.rst:843: WARNING: duplicate clicmd description of locator NAME, other instance in isisd
zebra/zebra_tracker_notify.c:23:10: fatal error: 'sys/inotify.h' file not found
#include <sys/inotify.h>
1 error generated.
gmake[1]: *** [Makefile:10627: zebra/zebra_tracker_notify.o] Error 1
gmake[1]: Leaving directory '/home/ci/cibuild.14278/frr-source'
gmake[1]: Target 'all-am' not remade because of errors.
gmake: *** [Makefile:6483: all] Error 2
FreeBSD 11 amd64 build: Failed (click for details) FreeBSD 11 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI009BUILD/config.log/config.log.gz FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI009BUILD/config.status/config.status

Make failed for FreeBSD 11 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/CI009BUILD/ErrorLog/log_make.txt)

/usr/home/ci/cibuild.14278/frr-source/doc/user/zebra.rst:830: WARNING: duplicate clicmd description of segment-routing, other instance in pathd
/usr/home/ci/cibuild.14278/frr-source/doc/user/zebra.rst:843: WARNING: duplicate clicmd description of locator NAME, other instance in isisd
zebra/zebra_tracker_notify.c:23:10: fatal error: sys/inotify.h: No such file or directory
compilation terminated.
gmake[1]: *** [Makefile:10627: zebra/zebra_tracker_notify.o] Error 1
gmake[1]: Leaving directory '/usr/home/ci/cibuild.14278/frr-source'
gmake[1]: Target 'all-am' not remade because of errors.
gmake: *** [Makefile:6483: all] Error 2
FreeBSD 12 amd64 build: Failed (click for details) FreeBSD 12 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/FBSD12AMD64/config.log/config.log.gz FreeBSD 12 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/FBSD12AMD64/config.status/config.status

Make failed for FreeBSD 12 amd64 build:
(see full Make log at https://ci1.netdef.org/browse/FRR-PULLREQ2-14278/artifact/FBSD12AMD64/ErrorLog/log_make.txt)

/usr/home/ci/cibuild.14278/frr-source/doc/user/ospfd.rst:642: WARNING: Cannot analyze code. No Pygments lexer found for "frr".
/usr/home/ci/cibuild.14278/frr-source/doc/user/pbr.rst:50: WARNING: duplicate label nexthop-groups, other instance in /usr/home/ci/cibuild.14278/frr-source/doc/user/nexthop_groups.rst
zebra/zebra_tracker_notify.c:23:10: fatal error: sys/inotify.h: No such file or directory
 #include <sys/inotify.h>
compilation terminated.
gmake[1]: *** [Makefile:10624: zebra/zebra_tracker_notify.o] Error 1
gmake[1]: Target 'all-am' not remade because of errors.
gmake[1]: Leaving directory '/usr/home/ci/cibuild.14278/frr-source'
gmake: *** [Makefile:6480: all] Error 2
Report for bgp_tracker.c | 12 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/bgp_tracker.c:1:
+/* BGP Tracker

ERROR: Bad function definition - void bgp_tracker_terminate() should probably be void bgp_tracker_terminate(void)
#103: FILE: /tmp/f1-1223375/bgp_tracker.c:103:
+void bgp_tracker_terminate()

ERROR: Bad function definition - void bgp_tracker_init() should probably be void bgp_tracker_init(void)
#119: FILE: /tmp/f1-1223375/bgp_tracker.c:119:
+void bgp_tracker_init()
Report for bgp_tracker.h | 4 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/bgp_tracker.h:1:
+/* BGP Tracker
Report for tracker.h | 4 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/tracker.h:1:
+/*
Report for zclient.c | 2 issues
===============================================
< WARNING: externs should be avoided in .c files
< #1479: FILE: /tmp/f1-1223375/zclient.c:1479:
Report for zebra_tracker.c | 13 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/zebra_tracker.c:1:
+/*

ERROR: do not initialise statics to NULL
#35: FILE: /tmp/f1-1223375/zebra_tracker.c:35:
+static struct list *zebra_tracker_file_master = NULL;

WARNING: break is not useful after a return
#199: FILE: /tmp/f1-1223375/zebra_tracker.c:199:
+		return "init";
+		break;
Report for zebra_tracker.h | 4 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/zebra_tracker.h:1:
+/*
Report for zebra_tracker_nb.c | 4 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/zebra_tracker_nb.c:1:
+/*
Report for zebra_tracker_nb_config.c | 4 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/zebra_tracker_nb_config.c:1:
+/*
Report for zebra_tracker_nb.h | 4 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/zebra_tracker_nb.h:1:
+/*
Report for zebra_tracker_notify.c | 18 issues
===============================================
WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#1: FILE: /tmp/f1-1223375/zebra_tracker_notify.c:1:
+/*

ERROR: do not use assignment in if condition
#113: FILE: /tmp/f1-1223375/zebra_tracker_notify.c:113:
+	if ((file = fopen(tracker_file->path, "r"))) {

WARNING: void function return statements are not generally useful
#127: FILE: /tmp/f1-1223375/zebra_tracker_notify.c:127:
+	return;
+}

WARNING: void function return statements are not generally useful
#185: FILE: /tmp/f1-1223375/zebra_tracker_notify.c:185:
+	return;
+}

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@louis-6wind
Copy link
Contributor Author

louis-6wind commented Jun 10, 2024

not accepted by the community

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.

None yet

6 participants