Avoid refreshing redundant OSPFv2 router and AS-external LSAs. #4718
Conversation
Router and AS-external LSAs can be refreshed without LSA information actually changing, for example AS-external LSAs are refreshed when a neighbor transitions to Full (see nsm_change_state() in ospfd/ospf_nsm.c). This patch checks if any LSA content has changed in router and AS-external LSAs and avoids flooding redundant LSAs that would not otherwise be refreshed due to age. Signed-off-by: Tom Goff <tgoff0@gmail.com>
💚 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-8415/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
|
ospf_lsa_refresh() refreshes all types of LSAs including router, network, summary, and external LSAs. The refresh mechanism is symmetric for all LSAs. Why only router and external LSAs refresh seen redundant. |
| @@ -2339,6 +2371,23 @@ struct ospf_lsa *ospf_external_lsa_refresh(struct ospf *ospf, | |||
|
|
|||
| new->data->ls_seqnum = lsa_seqnum_increment(lsa); | |||
|
|
|||
| /* Check if any content actually changed */ | |||
| if (LS_AGE(lsa) < OSPF_LS_REFRESH_TIME - (2 * OSPF_LS_REFRESH_JITTER) && | |||
| ospf_lsa_same(lsa, new)) { | |||
There was a problem hiding this comment.
OSPF external LSA refreshed when new route added or changed values from zebra. ospf_redistribute_check() checks if LSA has changed or not along with routermap filter. There is force flag passed, i guess in that case it is forced to do refresh. Have you checked force flag value LSA_REFRESH_FORCE or LSA_REFRESH_IF_CHANGED can be employed from the caller? Have you exhust the cases where FORCE flag is passed which meant to do force refresh?
Have you done a UT where existing external route added or modified triggers a refresh even tough AGE within refresh window?
There was a problem hiding this comment.
LSA_REFRESH_FORCE probably needs to be handled here -- just add a check that if that flag is set, refresh even though ospf_lsa_same returns true, I think
|
@tomgoff there was some code in the NSM that caused refreshes whenever a neighbor came up. Is it possible that this is what triggered your unneeded refreshes? I'm removing that in this commit: 853f542 |
| @@ -2339,6 +2371,23 @@ struct ospf_lsa *ospf_external_lsa_refresh(struct ospf *ospf, | |||
|
|
|||
| new->data->ls_seqnum = lsa_seqnum_increment(lsa); | |||
|
|
|||
| /* Check if any content actually changed */ | |||
| if (LS_AGE(lsa) < OSPF_LS_REFRESH_TIME - (2 * OSPF_LS_REFRESH_JITTER) && | |||
| ospf_lsa_same(lsa, new)) { | |||
There was a problem hiding this comment.
LSA_REFRESH_FORCE probably needs to be handled here -- just add a check that if that flag is set, refresh even though ospf_lsa_same returns true, I think
There was a problem hiding this comment.
I'm not OK with accepting this PR without at least 1 described case where this removes redundant flooding. Ideally this would be a test case, but I'll settle for steps to reproduce. Just want to see that it actually fixes something after 853f542.
|
Where are we with this PR? |
|
Closing PR in one week if we do not start seeing action on this one. It's been hanging around forever waiting for discussion/changes |
In some cases
ospfdwill refresh router and AS-external LSAs unnecessarily.This patch checks if LSA content has actually changed and avoids redundant LSA flooding. LSAs are still refreshed due to age when appropriate.