Skip to content
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

bgpd: Do not send Deconfig/Shutdown message when restarting #12034

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

ton31337
Copy link
Member

Fixes #12030

@frrbot frrbot bot added bgp tests Topotests, make check, etc labels Sep 27, 2022
@NetDEF-CI
Copy link
Collaborator

NetDEF-CI commented Sep 27, 2022

Continuous Integration Result: FAILED

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7607/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests debian 10 amd64 part 9: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9DEB10AMD64-7607/test

Topology Tests failed for Topotests debian 10 amd64 part 9
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-7607/artifact/TOPO9DEB10AMD64/ErrorLog/log_topotests.txt

Successful on other platforms/tests
  • Topotests debian 10 amd64 part 1
  • Addresssanitizer topotests part 3
  • Topotests debian 10 amd64 part 6
  • Topotests Ubuntu 18.04 arm8 part 1
  • Fedora 29 rpm pkg check
  • Topotests Ubuntu 18.04 amd64 part 7
  • Topotests Ubuntu 18.04 i386 part 5
  • Topotests Ubuntu 18.04 i386 part 0
  • Topotests Ubuntu 18.04 amd64 part 4
  • Addresssanitizer topotests part 2
  • CentOS 7 rpm pkg check
  • Topotests Ubuntu 18.04 amd64 part 0
  • Topotests Ubuntu 18.04 i386 part 9
  • Topotests Ubuntu 18.04 i386 part 4
  • Topotests debian 10 amd64 part 7
  • Debian 9 deb pkg check
  • Addresssanitizer topotests part 8
  • Topotests debian 10 amd64 part 5
  • Topotests Ubuntu 18.04 i386 part 7
  • Topotests Ubuntu 18.04 amd64 part 6
  • Topotests Ubuntu 18.04 arm8 part 8
  • Topotests Ubuntu 18.04 arm8 part 6
  • Topotests Ubuntu 18.04 amd64 part 1
  • Topotests Ubuntu 18.04 i386 part 2
  • Addresssanitizer topotests part 6
  • Ubuntu 18.04 deb pkg check
  • Addresssanitizer topotests part 5
  • Addresssanitizer topotests part 4
  • Topotests Ubuntu 18.04 i386 part 3
  • Topotests Ubuntu 18.04 i386 part 8
  • Topotests Ubuntu 18.04 amd64 part 2
  • Addresssanitizer topotests part 0
  • Topotests debian 10 amd64 part 8
  • Topotests debian 10 amd64 part 4
  • Topotests debian 10 amd64 part 3
  • Topotests Ubuntu 18.04 arm8 part 4
  • Topotests Ubuntu 18.04 arm8 part 9
  • Topotests Ubuntu 18.04 arm8 part 3
  • Topotests Ubuntu 18.04 amd64 part 3
  • Addresssanitizer topotests part 1
  • Topotests Ubuntu 18.04 arm8 part 7
  • Static analyzer (clang)
  • Addresssanitizer topotests part 9
  • Topotests debian 10 amd64 part 0
  • Topotests Ubuntu 18.04 arm8 part 0
  • Ubuntu 16.04 deb pkg check
  • Topotests Ubuntu 18.04 amd64 part 9
  • Topotests Ubuntu 18.04 amd64 part 8
  • Topotests debian 10 amd64 part 2
  • Topotests Ubuntu 18.04 arm8 part 2
  • Topotests Ubuntu 18.04 i386 part 1
  • Debian 10 deb pkg check
  • Topotests Ubuntu 18.04 i386 part 6
  • Addresssanitizer topotests part 7
  • Topotests Ubuntu 18.04 arm8 part 5
  • Topotests Ubuntu 18.04 amd64 part 5
  • Ubuntu 20.04 deb pkg check

@ton31337
Copy link
Member Author

ci:rerun failed test_ospf_asbr_summary_topo1

@NetDEF-CI
Copy link
Collaborator

NetDEF-CI commented Sep 27, 2022

Continuous Integration Result: SUCCESSFUL

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7614/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

@ton31337 ton31337 marked this pull request as draft September 29, 2022 18:36
@ton31337 ton31337 force-pushed the fix/gr_hard_notification branch from 8dcfe0a to 1ba2320 Compare September 30, 2022 06:02
@ton31337 ton31337 added the freeze Show the warning before merging as failed label Sep 30, 2022
We might disable sending unconfig/shutdown notifications when
Graceful-Restart is enabled and negotiated.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Not only in BGP table.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
@ton31337 ton31337 force-pushed the fix/gr_hard_notification branch from 2e9ddac to 18e427b Compare September 30, 2022 06:51
@NetDEF-CI
Copy link
Collaborator

NetDEF-CI commented Sep 30, 2022

Continuous Integration Result: SUCCESSFUL

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7644/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

@NetDEF-CI
Copy link
Collaborator

NetDEF-CI commented Sep 30, 2022

Continuous Integration Result: SUCCESSFUL

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7647/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7648/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

@cgoncalves
Copy link
Contributor

I originally reported this bug in #8425 and @mruprich followed up with #12030.

I tested this PR on a test build provided by @mruprich (FRR 7.5.1 based, CentOS Stream 8). I can confirm I can not longer reproduce the issue. The BGP router is withdrawn after 120 seconds (graceful restart timer).

2022/09/30 09:07:17 BGP: 10.20.30.41 [Event] BGP connection closed fd 28
2022/09/30 09:07:17 BGP: 10.20.30.41 [FSM] TCP_connection_closed (Established->Clearing), fd 28
2022/09/30 09:07:17 BGP: %ADJCHANGE: neighbor 10.20.30.41(centos8.localdomain) in vrf default Down NSF peer closed the session
2022/09/30 09:07:17 BGP: 10.20.30.41 graceful restart timer started for 120 sec
2022/09/30 09:07:17 BGP: 10.20.30.41 graceful restart stalepath timer started for 360 sec
2022/09/30 09:07:17 BGP: bgp_fsm_change_status : vrf default(0), Status: Clearing established_peers 0
2022/09/30 09:07:17 BGP: RID change : vrf VRF default(0), RTR ID 192.168.121.139
2022/09/30 09:07:17 BGP: 10.20.30.41 went from Established to Clearing
2022/09/30 09:07:17 BGP: 10.20.30.41 [FSM] Clearing_Completed (Clearing->Idle), fd -1
2022/09/30 09:07:17 BGP: bgp_fsm_change_status : vrf default(0), Status: Idle established_peers 0
2022/09/30 09:07:17 BGP: 10.20.30.41 went from Clearing to Idle
2022/09/30 09:07:18 BGP: 10.20.30.41 [FSM] Timer (start timer expire).
2022/09/30 09:07:18 BGP: 10.20.30.41 [FSM] BGP_Start (Idle->Connect), fd -1
2022/09/30 09:07:18 BGP: 10.20.30.41 [Event] Connect start to 10.20.30.41 fd 28
2022/09/30 09:07:18 BGP: 10.20.30.41 [FSM] Non blocking connect waiting result, fd 28
2022/09/30 09:07:18 BGP: bgp_fsm_change_status : vrf default(0), Status: Connect established_peers 0
2022/09/30 09:07:18 BGP: 10.20.30.41 went from Idle to Connect
2022/09/30 09:07:18 BGP: 10.20.30.41 [Event] Connect failed 111(Connection refused)
2022/09/30 09:07:18 BGP: 10.20.30.41 [FSM] TCP_connection_open_failed (Connect->Active), fd 28
2022/09/30 09:07:18 BGP: bgp_fsm_change_status : vrf default(0), Status: Active established_peers 0
2022/09/30 09:07:18 BGP: 10.20.30.41 went from Connect to Active
2022/09/30 09:09:17 BGP: 10.20.30.41 graceful restart timer expired
2022/09/30 09:09:17 BGP: 10.20.30.41 graceful restart stalepath timer stopped
2022/09/30 09:09:17 BGP: Tx route delete VRF 0 10.20.50.98/32
[root@centos8 frr]# ip r
default via 192.168.121.1 dev eth0 proto dhcp metric 100 
10.20.30.0/24 dev eth1 proto kernel scope link src 10.20.30.42 metric 101 
192.168.121.0/24 dev eth0 proto kernel scope link src 192.168.121.139 metric 100

@ton31337 ton31337 marked this pull request as ready for review September 30, 2022 09:23
@mruprich
Copy link
Contributor

I agree with Carlos, this looks good. Thank you @ton31337 !!

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

looks good; waiting on freeze

@riw777 riw777 merged commit a00621d into FRRouting:master Oct 6, 2022
@ton31337 ton31337 deleted the fix/gr_hard_notification branch October 6, 2022 14:10
@klausenbusk
Copy link

Any chance this could be backported to 8.3 or the upcoming 8.4?

@ton31337
Copy link
Member Author

ton31337 commented Nov 6, 2022

It will be in 8.4 next week.

@klausenbusk
Copy link

klausenbusk commented Nov 6, 2022 via email

@ton31337
Copy link
Member Author

ton31337 commented Nov 6, 2022

Sorry, forgot. It will be in 8.4.1.

@ton31337
Copy link
Member Author

ton31337 commented Nov 7, 2022

@Mergifyio backport stable/8.4

@mergify
Copy link

mergify bot commented Nov 7, 2022

backport stable/8.4

✅ Backports have been created

ton31337 added a commit that referenced this pull request Nov 8, 2022
bgpd: Do not send Deconfig/Shutdown message when restarting (backport #12034)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bgp freeze Show the warning before merging as failed master tests Topotests, make check, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BGP sends hard reset to the neighbor, even if graceful restart capability is enabled
6 participants