Skip to content

[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

Merged

Conversation

vganesan-nokia
Copy link
Contributor

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

  • Configure BGP_VOQ_CHASSIS_NEIGHBOR using minigraph. The minigraph will generate these neighbors with keepalive and holdtime timer values of 0 & 0 respectively.
  • Reboot the chassis with this configuration
  • After chassis is up and operational, check the bgp summary and observe voq chassis bgp sessions are up.
  • Check the bgp confiuration for the internal neighbors in FRR (using show running-configuration, for example) and observe 3 & 10 are configured for keepalive and holdtime timers for voq chassis bgp neighbors. Also observe that connection retry timer is configured to 10.

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)

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]>
@anshuv-mfst anshuv-mfst requested review from a team and arlakshm September 8, 2021 16:21
@anshuv-mfst
Copy link

Hi @ysmanman - could you please assign Arista reviewers to the PR, thanks.

@ysmanman
Copy link
Contributor

Add @jmmikkel who was reviewing the change.

@jmmikkel
Copy link
Contributor

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?

@vganesan-nokia
Copy link
Contributor Author

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.

@jmmikkel
Copy link
Contributor

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.

@rlhui rlhui added the Chassis 🤖 Modular chassis support label Oct 31, 2021
@arlakshm
Copy link
Contributor

/Azp run

@azure-pipelines
Copy link

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.

@vganesan-nokia
Copy link
Contributor Author

@abdosi, @arlakshm, would you please check if this PR can be merged? It seems the pipeline triggered by @arlakshm did not start the tests. Should rebase be done again?

@arlakshm arlakshm merged commit 78de107 into sonic-net:master Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chassis 🤖 Modular chassis support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants