zebra: When registering a nexthop, we do not always need to re-eval #2897
Conversation
|
@mwinter-osr @rwestphal any help here on what has gone wrong? |
|
@donaldsharp ldpd doesn't do nexthop tracking so this failure does't make sense to me. I'll rerun the failed tests to rule out the possibility of this being a false warning. If it turns out to be a real problem, I'll take a closer look. |
| @@ -1064,7 +1065,7 @@ static void zread_rnh_register(ZAPI_HANDLER_ARGS) | |||
| p.family); | |||
| return; | |||
| } | |||
| rnh = zebra_add_rnh(&p, zvrf_id(zvrf), type); | |||
| rnh = zebra_add_rnh(&p, zvrf_id(zvrf), type, &exist); | |||
There was a problem hiding this comment.
nit: zebra_add_rnh() might return NULL in the case of an error and we're not checking this possibility here (this is a pre-existing problem). CS might complain that the exists variable might be used uninitialized in this case. Checking if rnh is NULL or not would solve both problems. Other than that the changes look good to me.
There was a problem hiding this comment.
yeah it is a bit of a pre-existing problem( but only going to happen when someone moves around initialization ) and the code would quickly crash too due to rnh deref. I'll make it behave a bit better though.
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedAddresssanitizer topotest: Successful IPv4 ldp protocol on Ubuntu 16.04: FailedRFC Compliance Test ANVL-LDP-1.24 failing: RFC Compliance Test ANVL-LDP-26.8 failing: RFC Compliance Test ANVL-LDP-1.24 failing: RFC Compliance Test ANVL-LDP-26.8 failing: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base4 Static Analyzer issues remaining.See details at |
|
@rwestphal I already reran! Hence me attempting to poke multiple people yesterday/today |
donaldsharp
force-pushed
the
zebra_rnh_fixup
branch
3 times, most recently
from
332591e
to
b7ee96c
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
The code prior to this change, was allowing clients to register for nexthop tracking. Then zebra would look up the rnh and send to that particular client any known data. Additionally zebra was blindly re-evaluating the rnh for every registration. This leads to interesting behavior in that all people registered for that nexthop will get callbacks even if nothing changes. Modify the code to know if we have evaluated the rnh or not and if so limit the re-evaluation to when absolutely necessary This is of particular importance to do because of nht callbacks for protocols cause those protocols to do not insignificant work and as more protocols are registering for nht callbacks we will cause more work than is necessary. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
When we add / remove a nexthop that we need to track, keep track of the number of times we have done this for each nexthop. Consequently keep track of the number of available nexthops, so that we can just install new routes when we get one that uses a pre-existing nexthop. Deletion of nexthops is done on refcount going to 0. Removal of routes is handled elsewhere for removal. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5039/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base4 Static Analyzer issues remaining.See details at |
donaldsharp
force-pushed
the
zebra_rnh_fixup
branch
from
b7ee96c
to
74f0a94
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5040/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base4 Static Analyzer issues remaining.See details at |
| nhtd->refcount++; | ||
|
|
||
| if (nhtd->refcount > 1) { | ||
| static_nht_update(nhtd->nh, nhtd->nh_num, |
There was a problem hiding this comment.
Just curious about this added clause: is there a condition where registering interest in a tracked nexthop would change the status of the nexthop?
There was a problem hiding this comment.
no, they should be independent. Registering a nexthop tells zebra to just let you know when something changes that is it.
The code prior to this change, was allowing clients to register
for nexthop tracking. Then zebra would look up the rnh and
send to that particular client any known data. Additionally
zebra was blindly re-evaluating the rnh for every registration.
This leads to interesting behavior in that all people registered
for that nexthop will get callbacks even if nothing changes.
Modify the code to know if we have evaluated the rnh or not
and if so limit the re-evaluation to when absolutely necessary
This is of particular importance to do because of nht callbacks
for protocols cause those protocols to do not insignificant
work and as more protocols are registering for nht callbacks
we will cause more work than is necessary.
Signed-off-by: Donald Sharp sharpd@cumulusnetworks.com