-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: Handle Graceful Restart capability using dynamic capabilities #14285
bgpd: Handle Graceful Restart capability using dynamic capabilities #14285
Conversation
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedUbuntu 22.04 amd64 build: Failed (click for details)Ubuntu 22.04 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/U22AMD64BUILD/config.log/config.log.gzMake failed for Ubuntu 22.04 amd64 build:
Ubuntu 22.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/U22AMD64BUILD/config.status/config.status Redhat 9 amd64 build: Failed (click for details)Redhat 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/RH9BUILD/config.status/config.statusMake failed for Redhat 9 amd64 build:
Redhat 9 amd64 build: Unknown Log <config.log.gz> FreeBSD 11 amd64 build: Failed (click for details)FreeBSD 11 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/CI009BUILD/config.log/config.log.gz FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/CI009BUILD/config.status/config.statusMake failed for FreeBSD 11 amd64 build:
Debian 11 amd64 build: Failed (click for details)Debian 11 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/DEB11AMD64/config.log/config.log.gzMake failed for Debian 11 amd64 build:
Debian 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/DEB11AMD64/config.status/config.status OpenBSD 7 amd64 build: Failed (click for details)OpenBSD 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/CI011BUILD/config.log/config.log.gz OpenBSD 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13851/artifact/CI011BUILD/config.status/config.statusMake failed for OpenBSD 7 amd64 build:
Successful on other platforms/tests
|
There was a problem hiding this 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 ci and such
compile filed on ubuntu 22, trying again |
38b3218
to
efadb08
Compare
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedUbuntu 22.04 amd64 build: Failed (click for details)Ubuntu 22.04 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/U22AMD64BUILD/config.log/config.log.gzMake failed for Ubuntu 22.04 amd64 build:
Ubuntu 22.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/U22AMD64BUILD/config.status/config.status FreeBSD 11 amd64 build: Failed (click for details)FreeBSD 11 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/CI009BUILD/config.log/config.log.gz FreeBSD 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/CI009BUILD/config.status/config.statusMake failed for FreeBSD 11 amd64 build:
Redhat 9 amd64 build: Failed (click for details)Redhat 9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/RH9BUILD/config.status/config.statusMake failed for Redhat 9 amd64 build:
Redhat 9 amd64 build: Unknown Log <config.log.gz> Debian 11 amd64 build: Failed (click for details)Debian 11 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/DEB11AMD64/config.log/config.log.gzMake failed for Debian 11 amd64 build:
Debian 11 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/DEB11AMD64/config.status/config.status OpenBSD 7 amd64 build: Failed (click for details)OpenBSD 7 amd64 build: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/CI011BUILD/config.log/config.log.gz OpenBSD 7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-PULLREQ2-13854/artifact/CI011BUILD/config.status/config.statusMake failed for OpenBSD 7 amd64 build:
Successful on other platforms/tests
|
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteTopotests Ubuntu 18.04 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO9U18AMD64-13857/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 Addresssanitizer topotests part 4: Incomplete(check logs for details)Topotests Ubuntu 18.04 arm8 part 9: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13857/artifact/TOPO9U18ARM8/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 9: No useful log foundSuccessful on other platforms/tests
|
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13857/ This is a comment from an automated CI system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like you to take another look at the decoding of the packet. I am concerned about a cou8ple of the spots
return BGP_Stop; | ||
} | ||
|
||
bgp_dynamic_capability_graceful_restart(pnt, action, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to pass in length? What happens if the length specified is a multiple of 4 but the buffer size is not congruent with the length?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU, the length is already checked above in bgp_capability_msg_parse()
:
hdr = (struct capability_header *)(pnt + 1);
We pass hdr
to bgp_dynamic_capability_graceful_restart()
, which checks against hrd->length
(the actual size of the capability).
Graceful-Restart restart time is exchanged using OPEN messages. In order to reduce restart time before doing an actual graceful restart, it might be useful to increase the time, but this is not possible without resetting the session. With this change, it's possible to send dynamic capability with a new value, and GR will respect a new reset time value. Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
… cap Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
restart-time and/or notification support. Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
…r GR cap Just a safety check to avoid out of bound reading. Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Just to be consistent with other zlog_ stuff for dynamic capabilities. Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
efadb08
to
14e3452
Compare
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteAddresssanitizer topotests part 4: Incomplete(check logs for details)Topotests Ubuntu 18.04 amd64 part 8: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO8U18ARM64-13887/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 8 Successful on other platforms/tests
|
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDTest incomplete. See below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: IncompleteAddresssanitizer topotests part 4: Incomplete(check logs for details)Successful on other platforms/tests
|
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-13887/ This is a comment from an automated CI system. |
No description provided.