frr icon indicating copy to clipboard operation
frr copied to clipboard

Published 20 hours ago • verified publisher icon FRRouting

zebra: remove ZEBRA_INTERFACE_VRF_UPDATE

Open anlancs opened this issue 3 years ago • 6 comments

Currently when one interface changes its VRF, zebra will send these messages to
all daemons in order:
1) ZEBRA_INTERFACE_DELETE ( notify them delete from old VRF )
2) ZEBRA_INTERFACE_VRF_UPDATE ( notify them move from old to new VRF )
3) ZEBRA_INTERFACE_ADD ( notify them added into new VRF )

When daemons deal with VRF_UPDATE, they use
zebra_interface_vrf_update_read()->if_lookup_by_name()
to check the interface exist or not in old VRF. This check will always return
NULL because DELETE ( deleted from old VRF ) is already done, so can't
find this interface in old VRF.

Send VRF_UPDATE is redundant and unuseful. DELETE and ADD are enough,
they will deal with RB tree, so don't send this VRF_UPDATE message.

anlancs avatar Mar 04 '22 15:03 anlancs

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

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.

Warnings Generated during build:

Checkout code: Successful with additional warnings 
Report for redistribute.c | 2 issues
===============================================
< WARNING: C99 // comments do not match recommendation
< #619: FILE: /tmp/f1-4765/redistribute.c:619:

NetDEF-CI avatar Mar 04 '22 17:03 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-3933/

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 amd64 part 9: Failed (click for details)

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

Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-3933/artifact/TOPO9U18AMD64/ErrorLog/log_topotests.txt

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

NetDEF-CI avatar Mar 05 '22 08:03 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-3936/

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 Mar 05 '22 13:03 NetDEF-CI

IMO shouldn't the operation just be a simple VRF_UPDATE? instead of doing the delete than update then add?

donaldsharp avatar Mar 13 '22 13:03 donaldsharp

Yes, use VRF_UPDATE can shorten the message from zebra to daemons. But since ADD and DEL is already enough for most cases, so i think converting VRF_UPDATE to ADD and DEL is ok.

anlancs avatar Mar 15 '22 13:03 anlancs

@donaldsharp Any suggestion? Always many errors during changing vrf: INTERFACE_VRF_UPDATE: Cannot find IF %s in VRF %d

anlancs avatar May 17 '22 06:05 anlancs

This PR is stale because it has been open 180 days with no activity. Comment or remove the autoclose label in order to avoid having this PR closed.

github-actions[bot] avatar Feb 14 '23 02:02 github-actions[bot]

If removed this VRF_UPDATE, a few code relating to this process need to be cleaned up.

anlancs avatar Apr 29 '23 10:04 anlancs

Hmm, I still wonder about this: it looks like isis and ospf6 don't even handle this message, but I see bgp doing quite a lot of work when this arrives. has someone knowledgeable carefully examined the bgp code and confirmed that nothing would be lost if that code never ran again? Also, if we're going to stop sending the zapi message, really we should remove it, and remove all the handlers for it (daemon and lib/zclient I guess.) Leaving the stale code around is just asking for future pain?

mjstapp avatar Sep 26 '23 14:09 mjstapp

Thanks for checking this PR, i have re-pushed it.

anlancs avatar Oct 04 '23 11:10 anlancs

I don't see an answer to my question from earlier. There is significant code in the bgp handler for this message: has someone knowledgeable examined that code and verified that bgp won't lose anything if that handler function is removed?

mjstapp avatar Oct 04 '23 12:10 mjstapp

Currently bgpd uses bgp_ifp_down() and bgp_ifp_up() to deal with vrf changes, it works well. @mjstapp

anlancs avatar Oct 05 '23 05:10 anlancs

I'm ok with this; the (scrupulous) change to the docs did raise a question for me about that dev doc file.

mjstapp avatar Oct 05 '23 12:10 mjstapp

I'm ok with this; the (scrupulous) change to the docs did raise a question for me about that dev doc file.

Okay, add one commit for it.

anlancs avatar Oct 07 '23 02:10 anlancs

ci:rerun

anlancs avatar Oct 07 '23 07:10 anlancs