frr icon indicating copy to clipboard operation
frr copied to clipboard

Published 20 hours ago • verified publisher icon FRRouting

ospf6d: add missing ABR task on interface start and change default task delay to 5

Open punith-shivakumar opened this issue 2 years ago • 25 comments

Routes are not synced by ABR. Assume the topology:

  • R1---R2---R3
  • Clients C1 connected to R1 and C2 connected to R3.

Area 0 configured on R1 and R2

  • On R1: All routes of area 0 are learnt successfully
  • On R2: All routes of area 0 are learnt successfully

Area 1 configured on R2 and R3

  • On R2: All routes are learnt from R3
  • On R3: Already learnt routes of C1 on ABR R2 does not get forward to R3

Root cause: On interface start, ABR schedule task is missing.

Fix:

  • Handle ABR schedule during interface start event
  • The fix is causing ospf6_gr_topo1/test_ospf6_gr_topo1.py topotest to fail.
  • On analysing the topotest, failure is caused by timing issue

Test script need to be adapted or decrease the default ABR schedule timer to less 5 sec

fixes #10637

punith-shivakumar avatar May 29 '22 20:05 punith-shivakumar

Continuous Integration Result: FAILED

Continuous Integration Result: FAILED

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

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 3: Failed (click for details) Topotests debian 10 amd64 part 3: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-5671/artifact/TOPO3DEB10AMD64/ErrorLog/ Topotests debian 10 amd64 part 3: No useful log found
Successful on other platforms/tests
  • Topotests Ubuntu 18.04 arm8 part 5
  • CentOS 7 rpm pkg check
  • Addresssanitizer topotests part 6
  • Topotests Ubuntu 18.04 i386 part 9
  • Topotests debian 10 amd64 part 1
  • Static analyzer (clang)
  • Topotests debian 10 amd64 part 7
  • Topotests Ubuntu 18.04 arm8 part 0
  • Topotests Ubuntu 18.04 i386 part 5
  • Topotests Ubuntu 18.04 i386 part 0
  • Topotests Ubuntu 18.04 amd64 part 8
  • Topotests debian 10 amd64 part 2
  • Addresssanitizer topotests part 0
  • Topotests Ubuntu 18.04 amd64 part 6
  • Topotests Ubuntu 18.04 arm8 part 6
  • Topotests Ubuntu 18.04 amd64 part 1
  • Ubuntu 18.04 deb pkg check
  • Addresssanitizer topotests part 1
  • Topotests Ubuntu 18.04 i386 part 4
  • Topotests debian 10 amd64 part 6
  • Topotests Ubuntu 18.04 arm8 part 1
  • Addresssanitizer topotests part 4
  • Debian 9 deb pkg check
  • Addresssanitizer topotests part 8
  • Topotests debian 10 amd64 part 5
  • Topotests Ubuntu 18.04 i386 part 7
  • Addresssanitizer topotests part 7
  • Topotests Ubuntu 18.04 arm8 part 8
  • Topotests debian 10 amd64 part 4
  • Topotests Ubuntu 18.04 arm8 part 3
  • Topotests Ubuntu 18.04 i386 part 2
  • Topotests Ubuntu 18.04 amd64 part 3
  • Topotests debian 10 amd64 part 9
  • Addresssanitizer topotests part 5
  • IPv4 ldp protocol on Ubuntu 18.04
  • Ubuntu 16.04 deb pkg check
  • Topotests Ubuntu 18.04 amd64 part 2
  • IPv6 protocols on Ubuntu 18.04
  • Topotests Ubuntu 18.04 i386 part 1
  • Topotests debian 10 amd64 part 8
  • Debian 10 deb pkg check
  • Topotests Ubuntu 18.04 i386 part 6
  • Topotests Ubuntu 18.04 arm8 part 9
  • Topotests Ubuntu 18.04 arm8 part 4
  • Topotests Ubuntu 18.04 amd64 part 4
  • Addresssanitizer topotests part 2
  • Topotests Ubuntu 18.04 amd64 part 0
  • Topotests Ubuntu 18.04 amd64 part 5
  • Ubuntu 20.04 deb pkg check
  • Topotests Ubuntu 18.04 arm8 part 7
  • Addresssanitizer topotests part 3
  • Fedora 29 rpm pkg check
  • IPv4 protocols on Ubuntu 18.04
  • Addresssanitizer topotests part 9
  • Topotests Ubuntu 18.04 amd64 part 7
  • Topotests debian 10 amd64 part 0
  • Topotests Ubuntu 18.04 amd64 part 9
  • Topotests Ubuntu 18.04 i386 part 3
  • Topotests Ubuntu 18.04 i386 part 8
  • Topotests Ubuntu 18.04 arm8 part 2

NetDEF-CI avatar May 29 '22 22:05 NetDEF-CI

Continuous Integration Result: FAILED

Continuous Integration Result: FAILED

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

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 Ubuntu 18.04 i386 part 8: Failed (click for details)

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

Topology Tests failed for Topotests Ubuntu 18.04 i386 part 8 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-5670/artifact/TOPO8U18I386/ErrorLog/log_topotests.txt

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

NetDEF-CI avatar May 29 '22 22:05 NetDEF-CI

ci:rerun

punith-shivakumar avatar May 30 '22 04:05 punith-shivakumar

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-5675/

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 avatar May 30 '22 06:05 NetDEF-CI

fixes #10637

punith-shivakumar avatar May 30 '22 18:05 punith-shivakumar

@punith-shivakumar -> @ton31337 is correct, can you update the commit message so that someone coming after you can understand your thought process a bit. Just putting fixes X on this gives no clue to the next developer behind you on what you were thinking and why. This is of tremendous value for the next person down the line. As an example why the change from 7 seconds to 5 seconds?

donaldsharp avatar May 31 '22 11:05 donaldsharp

With the fix, topo test failure seen in below mentioned step of execution ospf6_gr_topo1/test_ospf6_gr_topo1.py::test_gr_rt3 - AssertionError: "rt1" JSON output mismatches the expected result.

As experimental

  • Increasing the sleep interval(21), cleared the above step and failed in the step FAILED ospf6_gr_topo1/test_ospf6_gr_topo1.py::test_gr_rt5 - AssertionError: "rt2" JSON output mismatches the expected result

  • Tuning retry parameter in check_routers randomly started passing.

  • So, decreasing the default parameter to less than 5 always passed in all runs.

I do not have expertise in topotests area, if there are any concerns about changing default value, need some help from any of the maintainers to fix the script.

punith-shivakumar avatar Jun 01 '22 22:06 punith-shivakumar

ci:rerun

donaldsharp avatar Jun 02 '22 23:06 donaldsharp

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-5755/

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 avatar Jun 03 '22 01:06 NetDEF-CI

ci:rerun

punith-shivakumar avatar Jun 04 '22 05:06 punith-shivakumar

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-5762/

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 avatar Jun 04 '22 07:06 NetDEF-CI

@ton31337 @donaldsharp Do you have any further comments?

punith-shivakumar avatar Jun 06 '22 13:06 punith-shivakumar

Yes please address my review comments, by modifying the commit itself and force pushing. No-one is going to remember to look here. People are going to look at the code and the code commits

donaldsharp avatar Jun 24 '22 11:06 donaldsharp

Yes please address my review comments, by modifying the commit itself and force pushing. No-one is going to remember to look here. People are going to look at the code and the code commits

Updated the commit message with details

punith-shivakumar avatar Jul 07 '22 15:07 punith-shivakumar

ci:rerun

punith-shivakumar avatar Jul 07 '22 15:07 punith-shivakumar

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-6320/

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 avatar Jul 07 '22 20:07 NetDEF-CI

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-6321/

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 avatar Jul 07 '22 21:07 NetDEF-CI

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-6322/

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 avatar Jul 07 '22 21:07 NetDEF-CI

Still, can you split change default task delay to 5 into a separate commit?

ton31337 avatar Jul 12 '22 16:07 ton31337

Still, can you split change default task delay to 5 into a separate commit?

Sure

punith-shivakumar avatar Jul 13 '22 03:07 punith-shivakumar

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-6444/

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 avatar Jul 13 '22 06:07 NetDEF-CI

ci:rerun

punith-shivakumar avatar Aug 07 '22 17:08 punith-shivakumar

@donaldsharp Can we merge the changes if no other open points?

punith-shivakumar avatar Aug 07 '22 17:08 punith-shivakumar

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-6819/

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 avatar Aug 07 '22 19:08 NetDEF-CI

For OSPFv2, we have different macro OSPF_ABR_TASK_DELAY for schedule timer It does not have impact on timer, we can still continue to have 7 seconds

punith-shivakumar avatar Sep 19 '22 11:09 punith-shivakumar

@ton31337 Do you still have open question about OSPFv2 timer?

punith-shivakumar avatar Sep 29 '22 12:09 punith-shivakumar

Yes, I have still, it looks inconsistent to me. What do others think?

ton31337 avatar Oct 06 '22 07:10 ton31337

Yes, I have still, it looks inconsistent to me. What do others think?

Changed OSPFv2 timer also to 5 second

punith-shivakumar avatar Oct 12 '22 09:10 punith-shivakumar

Continuous Integration Result: FAILED

Continuous Integration Result: FAILED

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

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: Incomplete

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

NetDEF-CI avatar Oct 12 '22 12:10 NetDEF-CI

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-7854/

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 avatar Oct 12 '22 15:10 NetDEF-CI