bgpd: Send "Peer De-configured" notification before closing the socket #5273
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🛑 Basic BGPD CI results: FAILUREResults table
For details, please contact louberger |
|
Here's the crash: |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotest tests on Ubuntu 16.04 i386: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-9587/test Topology Tests failed for Topotest tests on Ubuntu 16.04 i386: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9587/artifact/TOPOI386/ErrorLog/log_topotests.txt Topology tests on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-9587/test Topology Tests failed for Topology tests on Ubuntu 16.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9587/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topology tests on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-9587/test Topology Tests failed for Topology tests on Ubuntu 18.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9587/artifact/TOPOU1804/ErrorLog/log_topotests.txt Successful on other platforms
Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9587/artifact/TOPOI386/MemoryLeaks/
|
|
Instead of postponing the send notification at the end of bgp socket closure. Is it not fine to trigger bgp_notify_send(peer, BGP_NOTIFY_CEASE,BGP_NOTIFY_CEASE_PEER_UNCONFIG); in the config path where we handle the "no neighbour x.x.x.x" ? |
|
How are you sure it’s the latest “passenger” before closing the doors? In other words I explained this behavior in the commit. We must be sure it’s happening after timers are turned off. |
|
From the comments in code, bgp_stop() can get called multiple times for the same peer. When this happens, the peer ibuf and obuf gets freed. And while sending bgp_notify_send() reference to obuf happens which could be NULL and will cause seg-fault. This where the crash also indicated ? |
🛑 Basic BGPD CI results: FAILUREResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopology tests on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-9595/test Topology Tests failed for Topology tests on Ubuntu 18.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9595/artifact/TOPOU1804/ErrorLog/log_topotests.txt Topology tests on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-9595/test Topology Tests failed for Topology tests on Ubuntu 16.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9595/artifact/TOPOU1604/ErrorLog/log_topotests.txt Topotest tests on Ubuntu 16.04 i386: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-9595/test Topology Tests failed for Topotest tests on Ubuntu 16.04 i386: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9595/artifact/TOPOI386/ErrorLog/log_topotests.txt Successful on other platforms
Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9595/artifact/TOPOU1804/MemoryLeaks/
|
rfc4486 states: If a BGP speaker decides to de-configure a peer, then the speaker SHOULD send a NOTIFICATION message with the Error Code Cease and the Error Subcode "Peer De-configured". Before the patch: ~# vtysh -c 'show ip bgp neighbors 192.168.0.2 json' | \ jq '."192.168.0.2".lastNotificationReason' null ~# vtysh -c 'show ip bgp neighbors 192.168.0.2 json' | \ jq '."192.168.0.2".lastResetDueTo' "Peer closed the session" After the patch: ~# vtysh -c 'show ip bgp neighbors 192.168.0.2 json' | \ jq '."192.168.0.2".lastNotificationReason' "Cease/Peer Unconfigured" Closes #5262 Signed-off-by: Donatas Abraitis <donatas.abraitis@gmail.com>
🛑 Basic BGPD CI results: FAILUREResults table
For details, please contact louberger |
|
figured out some issues, I will open an new PR with a fix. |
ton31337
deleted the
fix/send_BGP_NOTIFY_CEASE_PEER_UNCONFIG_after_no_neighbor
branch
bisdhdh
suggested changes
Nov 5, 2019
| * when you send a notification, FSM Connect fires before | ||
| * closing the socket and you still see `Peer closed the session`. | ||
| */ | ||
| if (BGP_IS_VALID_STATE_FOR_NOTIF(peer->status)) |
There was a problem hiding this comment.
Sending notification inside bgp_stop() would lead to generating a "BGP_Stop" event inside the function bgp_write_notify(), there by calling bgp_stop() again ?
BGP_XYZ_State --> Idle ----> Idle
This can be solved by moving the bgp stop event generation from bgp_write_notify() (--> BGP_EVENT_ADD(peer, BGP_Stop) )to the respective calling function.
| @@ -1181,6 +1181,19 @@ int bgp_stop(struct peer *peer) | |||
| } | |||
| } | |||
|
|
|||
| /* If a BGP speaker decides to de-configure a peer, then | |||
There was a problem hiding this comment.
bgp_stop() would be called when "BGP_Stop" even is raised. And "BGP_Stop" event can be raised at different states of bgp and for various reasons, not only for "Peer De-configured".
Eg clear bgp cmd
so we would end up
- sending "Peer De-configured" notification wrongly even when the peer is not delete.
- We might end up sending more than one notification message (pls see the below comment for the reason)
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopology tests on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1804-9598/test Topology Tests failed for Topology tests on Ubuntu 18.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9598/artifact/TOPOU1804/ErrorLog/log_topotests.txt Topotest tests on Ubuntu 16.04 i386: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-9598/test Topology Tests failed for Topotest tests on Ubuntu 16.04 i386: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9598/artifact/TOPOI386/ErrorLog/log_topotests.txt Topology tests on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-9598/test Topology Tests failed for Topology tests on Ubuntu 16.04 amd64: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9598/artifact/TOPOU1604/ErrorLog/log_topotests.txt Successful on other platforms
Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-9598/artifact/TOPOU1804/MemoryLeaks/
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rfc4486 states:
If a BGP speaker decides to de-configure a peer, then the speaker
SHOULD send a NOTIFICATION message with the Error Code Cease and
the Error Subcode "Peer De-configured".
Before the patch:
~# vtysh -c 'show ip bgp neighbors 192.168.0.2 json' |
jq '."192.168.0.2".lastNotificationReason'
null
~# vtysh -c 'show ip bgp neighbors 192.168.0.2 json' |
jq '."192.168.0.2".lastResetDueTo'
"Peer closed the session"
After the patch:
~# vtysh -c 'show ip bgp neighbors 192.168.0.2 json' |
jq '."192.168.0.2".lastNotificationReason'
"Cease/Peer Unconfigured"
Closes #5262
Signed-off-by: Donatas Abraitis donatas.abraitis@gmail.com