Renato Westphal [Thu, 10 Oct 2019 01:20:26 +0000 (22:20 -0300)]
lib: reduce memory allocation when processing large config transactions
Remove the xpath field from the nb_config_cb structure in order
to reduce its size. This allows the northbound to spend less time
allocating memory during the processing of large configuration
transactions.
To make this work, use yang_dnode_get_path() to obtain the xpath
from the dnode field whenever necessary.
Renato Westphal [Thu, 10 Oct 2019 02:24:04 +0000 (23:24 -0300)]
lib: fix processing of the 'apply_finish' callbacks
Commit 6b5d6e2dbc88 changed how we order configuration callbacks
and introduced a regression in the processing of the 'apply_finish'
callbacks. Fix this.
Renato Westphal [Thu, 10 Oct 2019 01:17:08 +0000 (22:17 -0300)]
lib: optimize loading of the startup configuration
Load the startup configuration directly into the CLI shared candidate
configuration instead of loading it into a private candidate
configuration. This way we don't need to initialize the shared
candidate separately later as a copy of the running configuration,
which is a potentially expensive operation.
Also, make the northbound process SIGHUP correctly even when --tcli
is not used.
Renato Westphal [Sun, 6 Oct 2019 04:16:47 +0000 (01:16 -0300)]
lib: avoid expensive operations when editing a candidate config
nb_candidate_edit() was calling both the lyd_schema_sort() and
lyd_validate() functions whenever a new node was added to the
candidate configuration. This was done to ensure the candidate
is always ready to be displayed correctly (libyang only creates
default child nodes during the validation process, and data nodes
aren't guaranteed to be ordered by default).
The problem is that the two aforementioned functions are too
expensive to be called in the northbound hot path. Instead, it makes
more sense to call them only before displaying the configuration
(in which case a recursive sort needs to be done). Introduce the
nb_cli_show_config_prepare() to achieve that purpose.
Renato Westphal [Sun, 6 Oct 2019 00:20:28 +0000 (21:20 -0300)]
lib: remove expensive error handling in the northbound CLI client
The nb_cli_apply_changes() function was creating a full copy of
the candidate configuration before editing it. This excerpt from
the northbond documentation explains why this was being done:
"NOTE: the nb_cli_cfg_change() function clones the candidate
configuration before actually editing it. This way, if any error
happens during the editing, the original candidate is restored to
avoid inconsistencies. Either all changes from the configuration
command are performed successfully or none are. It's like a
mini-transaction but happening on the candidate configuration
(thus the northbound callbacks are not involved)".
The problem is that this kind of error handling is just too
expensive. A command should never fail to edit the candidate
configuration unless there's a bug in the code (e.g. when the
CLI wrapper command passes an integer value that YANG rejects due
to a "range" statement). In such cases, a command might fail to
be applied or applied only partially if it edits multiple YANG
nodes. When that happens, just log an error to make the operator
aware of the problem, but otherwise ignore it instead of rejecting
the command and restoring the candidate to its previous state. We
shouldn't add an extreme overhead to the northbound CLI client only
to handle errors that should never happen in practice.
Renato Westphal [Sat, 5 Oct 2019 23:52:36 +0000 (20:52 -0300)]
lib: optimize VTY_CHECK_XPATH
There's no need to check for removed configuration objects in the
following two cases:
* A private candidate configuration is being edited;
* The startup configuration is being read.
zebra: entries can be pushed in tables, under vrf netns backend
initially, vrf backend if vrf-lite, and a specific table identifier is
associated to a vrf. here, with netns vrf backend, there is no specific
table assigned to except default routing table. use the passed table_id
parameter in zapi api, and apply it to the entry to be pushed in, or to
be removed.
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
bgpd: withdraw fib entry on appropriate table identifier
There are cases where the table identifier is set on a bgp entry, mainly
due to route-map, and associate fib entry needs to be removed.
This change encompasses also the route-map reconfiguration that leads to
removing the previous entry, whereas bgp update had been triggered (
this happens when software inbound reconfiguration is handled).
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
Philippe Guibert [Mon, 29 Apr 2019 13:26:01 +0000 (15:26 +0200)]
bgpd: ability to export prefixes entries to a kernel table identifier
this table identifier can be used for policy routing. incoming entries
are locally exported to that local table identifier.
note that so that the user applies the new table identifier to all
entries, the user should flush local tables first.
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
Effectively prd or vnc.import.rd are `struct prefix_rd` which
are being typecast to a `struct prefix`. Not a big deal except commit 1315d74de97be2944d7b005b2f9a50e9ae5eff4d modified the prefix_cmp
function to allow for a sorted prefix_cmp. In prefix_cmp
we were looking at the offset and shift. In the case
of vnc we were passing a prefix length of 64 which is the exact length of
the remaining data structure for struct prefix_rd. So we calculated
a offset of 8 and a shift of 0. The data structures for the prefix
portion happened to be equal to 64 bits of data. So we checked that
with the memcmp got a 0 and promptly read off the end of the data
structure for the numcmp. The fix is if shift is 0 that means thei
the memcmp has checked everything and there is nothing to do.
Please note: We will still crash if we set the prefixlen > then
~312 bits currently( ie if the prefixlen specifies a bit length
longer than the prefix length ). I do not think there is
anything to do here( nor am I sure how to correct this either )
as that we are going to have some severe problems when we muck
up the prefixlen.
Fixes: #5025 Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Donald Sharp [Thu, 10 Oct 2019 00:19:56 +0000 (20:19 -0400)]
bgpd: When creating extra from stack ensure it is zero'ed out
BGP code assumes that the extra data is zero'ed out. Ensure that we
are not leaving any situation that the data on the stack is actually all
0's when we pass it around as a pointer later.
Please note in issue #5025, Lou reported a different valgrind
issue, which is not the same issue:
==7313== Conditional jump or move depends on uninitialised value(s)
==7313== at 0x181F9F: subgroup_announce_check (bgp_route.c:1555)
==7313== by 0x1A112B: subgroup_announce_table (bgp_updgrp_adv.c:641)
==7313== by 0x1A1340: subgroup_announce_route (bgp_updgrp_adv.c:704)
==7313== by 0x1A13E3: subgroup_coalesce_timer (bgp_updgrp_adv.c:331)
==7313== by 0x4EBA615: thread_call (thread.c:1531)
==7313== by 0x4E8AC37: frr_run (libfrr.c:1052)
==7313== by 0x1429E0: main (bgp_main.c:486)
==7313==
==7313== Conditional jump or move depends on uninitialised value(s)
==7313== at 0x201C0E: rfapi_vty_out_vncinfo (rfapi_vty.c:429)
==7313== by 0x18D0D6: route_vty_out (bgp_route.c:7481)
==7313== by 0x18DD76: bgp_show_table (bgp_route.c:9365)
==7313== by 0x1930C4: bgp_show_table_rd (bgp_route.c:9471)
==7313== by 0x1932A3: bgp_show (bgp_route.c:9510)
==7313== by 0x193E68: show_ip_bgp_json (bgp_route.c:10284)
==7313== by 0x4E6D024: cmd_execute_command_real.isra.2 (command.c:1072)
==7313== by 0x4E6F51E: cmd_execute_command (command.c:1131)
==7313== by 0x4E6F686: cmd_execute (command.c:1285)
==7313== by 0x4EBF9C4: vty_command (vty.c:516)
==7313== by 0x4EBFB9F: vty_execute (vty.c:1285)
==7313== by 0x4EC250F: vtysh_read (vty.c:2119)
==7313==
that is causing the actual crash.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Donald Sharp [Tue, 8 Oct 2019 14:36:02 +0000 (10:36 -0400)]
pimd: Fix zlog_warn when we mean debug and vice versa
There are several places in the pim where we are mixing up
zlog_warn w/ zlog_debug and vice versa. If we are protecting
a zlog_warn w/ a debug is it really a warn? If we have an actual
error situation we should also warn about it.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Stephen Worley [Mon, 7 Oct 2019 22:01:21 +0000 (18:01 -0400)]
pbrd: Don't track ipv6 link locals
Don't bother tracking ipv6 link locals to determine if a map
should be installed. Every interface has a route of `fe80::/64`
so its just going to return the arbitrarily first one it finds
when it resolves it and hands it back to us.
Instead, just track the interface we specify along with it.
Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>
Quentin Young [Mon, 7 Oct 2019 15:36:39 +0000 (15:36 +0000)]
lib: reject leading 0 in ipv4 decimal quad
inet_pton() is used to parse ipv4 addresses internally, therefore FRR
does not support octal notation for quads. The ipv4 cli token validator
should make sure that str2prefix() can parse tokens it allows, and
str2prefix uses inet_pton, so we have to disallow leading zeros in ipv4
quads.
In short, 1.1.1.01 is no longer valid and must be expressed as 1.1.1.1.
Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
Donald Sharp [Thu, 3 Oct 2019 13:26:46 +0000 (09:26 -0400)]
doc: Add some additional warnings around Turning off bgp capabilities
FRR supports the ability to turn off the negotation of bgp capabilities.
Provide a few bread crumbs to the operator that it might not be as
useful as they would hope.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
broke the usage of ZEBRA_ROUTE_ALL as a valid redistribution
command. This commit puts it back in. LDP uses ZEBRA_ROUTE_ALL
as an option to say it is interested in all REDISTRIBUTION events.
Fixes: #5072 Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Donald Sharp [Wed, 2 Oct 2019 13:27:04 +0000 (09:27 -0400)]
sharpd: Start infrastructure to allow for redistribution testing
Start the work in sharpd to allow the testing of redistribution
of routes. Namely telling zebra to tell us about redistribution events
via the callback.
Future work here will allow sharpd to specify the redistribution
events it wants and to allow us to track that via counters.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Donald Sharp [Mon, 30 Sep 2019 12:49:40 +0000 (08:49 -0400)]
tests: Add a topology that shows broken ibgp behavior
In a leaf/spine topology with only IBGP connections, where
the same network is being redistributed at multiple points
in the network ( say a redistribute connected at both leaf and spines )
we end up in a state where zebra gets very confused.
eva# show ip route
Codes: K - kernel route, C - connected, S - static, R - RIP,
O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
T - Table, v - VNC, V - VNC-Direct, A - Babel, D - SHARP,
F - PBR, f - OpenFabric,
> - selected route, * - FIB route, q - queued route, r - rejected route
C>* 192.168.1.0/24 is directly connected, tor1-eth0, 00:00:30
C>* 192.168.2.0/24 is directly connected, tor1-eth1, 00:00:30
B 192.168.3.0/24 [200/0] via 192.168.4.2 inactive, 00:00:25
via 192.168.6.2 inactive, 00:00:25
B>* 192.168.4.0/24 [200/0] via 192.168.2.3, tor1-eth1, 00:00:25
* via 192.168.6.2 inactive, 00:00:25
C>* 192.168.5.0/24 is directly connected, tor1-eth2, 00:00:30
B>* 192.168.6.0/24 [200/0] via 192.168.4.2 inactive, 00:00:25
* via 192.168.5.4, tor1-eth2, 00:00:25
Effectively we have ibgp routes recursing through ibgp routes
and there is no metric to discern whom to listen to.
This draft:
https://tools.ietf.org/html/draft-ietf-idr-bgp-optimal-route-reflection-19
appears to address this issue. From looking at both cisco and arista
deployments they are handling this issue by having the route reflector
prefer the localy learned routes over from their clients.
Add this topology, in a broken state, so that when we do fix this issue
it is a simple matter of touching this topology up and re-adding it
to the normal daily builds. I also wanted to add this topology
since it is in a state of `doneness` and I wanted to move onto
my normal day job without having to remember about this test.
This topology is not configured to be run as part of the normal
topotests.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Christian Franke [Mon, 30 Sep 2019 23:53:44 +0000 (01:53 +0200)]
isisd: Fix handling of neighbor circuit id in three way handshake
RFC 5303 states:
If the system ID and Extended Local Circuit ID of the neighboring
system are known (in adjacency three-way state Initializing or
Up), the neighbor's system ID SHALL be reported in the Neighbor
System ID field, and the neighbor's Extended Local Circuit ID
SHALL be reported in the Neighbor Extended Local Circuit ID field.
There is nothing written about only setting the Extended circuit ID of the
adjacency only when we bring the three-way adjacency up.
In fact, we should always update it, to avoid the problem described in #4783.
Fixes: #4783 Signed-off-by: Christian Franke <chris@opensourcerouting.org>
Rafael Zalamena [Tue, 1 Oct 2019 00:15:15 +0000 (21:15 -0300)]
topotests: skip tests when any assert fails
When an `assert` fails we should skip all other tests on the file. Once
a failure is detected we can't rely on the setup anymore, since most of
the tests assume the previous worked.
Signed-off-by: Rafael Zalamena <rzalamena@opensourcerouting.org>
Mark Stapp [Fri, 27 Sep 2019 16:15:34 +0000 (12:15 -0400)]
zebra: during shutdown processing, drop dplane results
Don't process dataplane results in zebra during shutdown (after
sigint has been seen). The dplane continues to run in order to
clean up, but zebra main just drops results.
donna.cumulusnetworks.com# conf
donna.cumulusnetworks.com(config)# router bgp
donna.cumulusnetworks.com(config-router)# aggregate-address 30.0.5.0 255.255.255.0 summary-only
donna.cumulusnetworks.com(config-router)# do show run
Building configuration...
Don Slice [Tue, 24 Sep 2019 12:02:02 +0000 (05:02 -0700)]
bgpd: stop sending nexthop set by "route-map in" to eBGP peers
Problem reported that when a "neighbor x.x.x.x route-map FOO in"
set a next-hop value, that modified next-hop value was also sent
to eBGP peers. This is incorrect since bgp is expected to set
next-hop to self when sending to eBGP peers unless third party
next-hop on a shared segment is true. This fix modifies the
behavior to stop sending the modified next-hop to eBGP peers
if the route-map was applied inbound on another peer.
Ticket: CM-26025 Signed-off-by: Don Slice <dslice@cumulusnetworks.com>
projects / mirror_frr.git / log