Skip to content

Fix for BGP peer not coming up even after doing config BGP startup all#4547

Merged
lguohan merged 1 commit intosonic-net:masterfrom
abdosi:bgp_peer_fix
May 7, 2020
Merged

Fix for BGP peer not coming up even after doing config BGP startup all#4547
lguohan merged 1 commit intosonic-net:masterfrom
abdosi:bgp_peer_fix

Conversation

@abdosi
Copy link
Contributor

@abdosi abdosi commented May 6, 2020

- Why I did it
Fix for BGP peer not coming up even after doing config BGP startup all. Issue was key not correct to lookup into self.peer. Because of that we always used to go for add_peer and admin status update never happened,

- How I did it

Made key tuple of (vrf,nbr). Updated for both add/del peer case

- How to verify it
Manually Verified on T1-topology setup using show ip bgp summary

@abdosi abdosi requested review from lguohan and pavel-shirshov May 6, 2020 19:23
Copy link
Contributor

@jleveque jleveque May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems odd to redefine key here. Maybe just change the following line to if (vrf, nbr) not in self.peers:. Will this suffice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. It is better to use different name.
key in the param list is a string
key which is defined here is a tuple of two strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavel-shirshov @JLevesque84 updated to use "peer_key"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abdosi; FYI, you tagged somebody that isn't me, but has a similar username :)

Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. It is better to use different name.
key in the param list is a string
key which is defined here is a tuple of two strings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use different name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavel-shirshov updated to use "peer_key"

@wendani
Copy link
Contributor

wendani commented May 6, 2020

We notice bgp sessions not up after issuing 'sudo config reload -y' on 201911. Does it fix this issue as well?

Issue was key not correct to look into self.peer. It need to be tuple of
(vrf,nbr). Updated for both add/del
@lguohan
Copy link
Collaborator

lguohan commented May 6, 2020

@wendani, do you have issue created?

@abdosi abdosi requested a review from pavel-shirshov May 6, 2020 21:26
@wendani
Copy link
Contributor

wendani commented May 6, 2020

Not yet. Need further confirmation on a clean, latest 201911.

#4547 (comment)

@JLevesque84
Copy link

JLevesque84 commented May 6, 2020 via email

@abdosi
Copy link
Contributor Author

abdosi commented May 6, 2020

Lol.. what is this?

Sent from my iPhone
On May 6, 2020, at 3:58 PM, Joe LeVeque @.***> wrote:  @jleveque commented on this pull request. In dockers/docker-fpm-frr/bgpcfgd: > @@ -744,6 +744,7 @@ class BGPPeerMgrBase(Manager): :param data: the data associated with the change """ vrf, nbr = self.split_key(key) + key = (vrf, nbr) @abdosi; FYI, you tagged somebody that isn't me, but has a similar username :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

oops.. sorry. meant for other Joe

@lguohan lguohan merged commit fc28af7 into sonic-net:master May 7, 2020
abdosi added a commit that referenced this pull request May 7, 2020
…up all (#4547)

Issue was key not correct to look into self.peer. It need to be tuple of
(vrf,nbr). Updated for both add/del
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants