-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[voq][bgpcfg] BGP_VOQ_CHASSIS_NEIGHBORS timers config #8455
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
[voq][bgpcfg] BGP_VOQ_CHASSIS_NEIGHBORS timers config #8455
Conversation
The BGP_VOQ_CHASSIS_NEIGHBOR keepalive and holdtime timers are configured similar to general neighbors. Changes are done to configure BGP_VOQ_CHASSIS_NEIGHBOR timers similar to BGP_INTENAL_NEIGBOR since voq chassis bgp neighbors are similar to bgp internal neighbors in multi-asic. As it is done for bgp internal neighbors, the keepalive and holdtime timers are set to 3 and 10 seconds respectively. Also similar to bgp internal neighbors, connection retry timer is also configured for voq chassis bgp neighbors. Signed-off-by: vedganes <[email protected]>
Hi @ysmanman - could you please assign Arista reviewers to the PR, thanks. |
Add @jmmikkel who was reviewing the change. |
The code change is trivial but I don't understand the motivation for it. If the timers should't be set to 0, why are they 0 in the minigraph? Shouldn't whatever's generating the minigraph be what's aware of the differences between peers in the different tables? Because by ignoring the config, you also remove the ability for someone to set the timers themselves. Maybe that's not considered important? |
It seems yes. When this issue was brought up in one of the sonic chassis subroup meetings, all agreed that voq chassis neighbors are similar to multi-asic internal neighbors and these timers should be hard coded similar to bgp_internal neighbors. |
VOQ neighbors are indeed similar to multi-asic, but they are not the same. The fact you need this change demonstrates that treating them the same elsewhere is wrong, so why not change the assumption elsewhere? However, if it has already been determined that this explicit ignoring of config is "right" I am not sure why my review is required. |
/Azp run |
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
Why I did it
Currently, voq chassis bgp neighbor timer configuration is done similar to general neighbors. i.e., keepalive and holdtime timers are set to configured values, if configured,, otherwise set to default of 60 and 180. This is causing a problem of configuring timer values to 0 & 0 since minigraph configures 0 & 0 for voq chassis neighbors. Voq chassis neighbors are equivalent to multi-asic internal neighbors but we need to configure timers to values other than 0 & 0 since the voq chassis bgp neighbors may go down due to line card failures, for example. This PR fixes this problem of having 0 and 0 timer confiuration for voq chassis neighbors.
How I did it
Since the voq chassis bgp neighbors are similar to multi-asic internal neighbors, the voq chassis bgp timers are configured similar to multi-asic internal bgp neighbors. i.e, 3 & 10 are configured (hardcoded) for keepalive and holdtime respectively and the configurations given via config db are ignored. Also, similar to bgp internal neighbors, connection retry timer is also configured for voq chassis bgp neighbors.
How to verify it
Which release branch to backport (provide reason below if selected)
N/A
Description for the changelog
Keepalive, Holddown and Connection retry timers configuration for BGP_VOQ_CHASSIS_NEIGHBORs
A picture of a cute animal (not mandatory but encouraged)