zebra: remove ZEBRA_INTERFACE_VRF_UPDATE #10733

anlancs · 2022-03-04T15:02:35Z

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.

NetDEF-CI · 2022-03-04T17:04:18Z

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 · 2022-03-05T08:50:28Z

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 · 2022-03-05T13:13:12Z

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.

donaldsharp · 2022-03-13T13:53:47Z

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

anlancs · 2022-03-15T13:06:15Z

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.

patrasar · 2022-03-17T11:49:04Z

zebra/redistribute.c

@@ -616,7 +616,6 @@ void zebra_interface_vrf_update_del(struct interface *ifp, vrf_id_t new_vrf_id)
 		zsend_interface_update(ZEBRA_INTERFACE_DOWN, client, ifp);
 		client->ifdel_cnt++;
 		zsend_interface_delete(client, ifp);
-		zsend_interface_vrf_update(client, ifp, new_vrf_id);


How it will be taken care at the client side.
Example pimd client:
In the function, pim_zebra_interface_vrf_update() it will update the interface to new VRF.
If zebra is not sending the vrf_update msg then how it will be taken care.

As i said, it is redundant and no use.
pim_zebra_interface_vrf_update() will never call if_update_to_new_vrf(), it will immediately exit with NULL ifp, which is returned by zebra_interface_vrf_update_read().

anlancs · 2022-05-17T06:51:36Z

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

github-actions · 2023-02-14T02:01:34Z

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.

anlancs · 2023-04-29T10:07:40Z

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

rzalamena

Change look good.

When zsend_interface_delete is sent the protocol daemons call the library function zclient_interface_delete which removes the interface from the daemon, so when the zsend_interface_vrf_update message is sent no interface will be found.

Unless there is a daemon which does not use the standard zebra_interface_vrf_update_read, this ZEBRA_INTERFACE_VRF_UPDATE will be useless because the interface will never be there causing the following log message: https://github.com/FRRouting/frr/blob/master/lib/zclient.c#L3068

I checked all daemons and they all use zebra_interface_vrf_update_read with ZEBRA_INTERFACE_VRF_UPDATE (maybe we could even move this repeated code to lib and split the specialized protocol code).

mjstapp · 2023-09-26T14:53:21Z

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?

rzalamena

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?

Agreed, looks like bgpd was the only real user of this. I looked at the history of that function and the most meaningful commit was from 7 years ago ( bfcd43b ).

I think we should remove all uses of this message since its the only place its being used and it seems unnecessary now.

anlancs · 2023-10-04T11:18:39Z

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

mjstapp · 2023-10-04T12:04:14Z

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?

anlancs · 2023-10-05T05:58:59Z

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

mjstapp · 2023-10-05T12:37:52Z

doc/developer/zebra.rst

@@ -234,221 +234,219 @@ Zebra Protocol Commands
 +-----------------------------------------+-------+
 | ZEBRA_VRF_LABEL                         | 35    |
 +-----------------------------------------+-------+
-| ZEBRA_INTERFACE_VRF_UPDATE              | 36    |


oof - yeah, I'd forgotten about this table. I think it's not worth documenting these values, and maintaining this table. Let's just refer to the header file?

mjstapp · 2023-10-05T12:39:01Z

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

rzalamena

Changes looks good. ZEBRA_INTERFACE_VRF_UPDATE was introduced 8 years ago ( c8e264b ) and at that time zebra clients had to ask for VRF interest to get updates. When there was no interest in updates the ZEBRA_INTERFACE_VRF_UPDATE was useful:

      if (!vrf_bitmap_check (client->ifinfo, new_vrf_id))
        {
          /* Need to delete if the client is not interested in the new VRF. */
          zsend_interface_update (ZEBRA_INTERFACE_DOWN, client, ifp);
          client->ifdel_cnt++;
          zsend_interface_delete (client, ifp);
        }
      else if (vrf_bitmap_check (client->ifinfo, ifp->vrf_id))
        {
          /* Client is interested in both VRFs, inform about the change. */
          zsend_interface_vrf_update (client, ifp, new_vrf_id);
        }

Later ( 4fe5171 ) the code was simplified, but the VRF_UPDATE message was kept.

I don't see how this message makes sense anymore since the work for """update""" the VRF is the same as delete all interface information and then add back (which is what is already happening).

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 when vrf changes. Since all daemons have good mechanism to deal with changing vrf, and don't use this `VRF_UPDATE` mechanism. So, it is safe to completely remove all the code with `VRF_UPDATE`. Signed-off-by: anlan_cs <[email protected]>

Signed-off-by: anlan_cs <[email protected]>

anlancs · 2023-10-07T02:18:38Z

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 · 2023-10-07T07:20:28Z

ci:rerun

frrbot bot added the zebra label Mar 4, 2022

anlancs marked this pull request as draft March 4, 2022 15:48

anlancs force-pushed the zebra-remove-update branch from 3b720aa to f616c32 Compare March 5, 2022 06:50

anlancs force-pushed the zebra-remove-update branch from f616c32 to a9cdf18 Compare March 5, 2022 11:13

anlancs marked this pull request as ready for review March 5, 2022 11:14

patrasar reviewed Mar 17, 2022

View reviewed changes

anlancs requested a review from patrasar August 17, 2022 22:42

github-actions bot added the autoclose label Feb 14, 2023

anlancs mentioned this pull request Apr 29, 2023

zebra: Fix missing VRF flag #13403

Merged

github-actions bot removed the autoclose label Jul 28, 2023

rzalamena approved these changes Sep 26, 2023

View reviewed changes

rzalamena requested changes Sep 29, 2023

View reviewed changes

anlancs force-pushed the zebra-remove-update branch from a9cdf18 to 690681a Compare October 4, 2023 10:34

github-actions bot added the size/L label Oct 4, 2023

anlancs requested a review from rzalamena October 4, 2023 11:19

mjstapp reviewed Oct 5, 2023

View reviewed changes

rzalamena approved these changes Oct 6, 2023

View reviewed changes

anlancs and others added 2 commits October 7, 2023 10:06

doc: replace commands list with header file

56f141e

Signed-off-by: anlan_cs <[email protected]>

anlancs force-pushed the zebra-remove-update branch from 690681a to 56f141e Compare October 7, 2023 02:08

frrbot bot added the documentation label Oct 7, 2023

github-actions bot added size/XL and removed size/L labels Oct 7, 2023

anlancs requested a review from mjstapp October 8, 2023 00:31

rzalamena merged commit 4a60045 into FRRouting:master Oct 8, 2023
80 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zebra: remove ZEBRA_INTERFACE_VRF_UPDATE #10733

zebra: remove ZEBRA_INTERFACE_VRF_UPDATE #10733

anlancs commented Mar 4, 2022 •

edited

Loading

NetDEF-CI commented Mar 4, 2022 •

edited

Loading

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

NetDEF-CI commented Mar 5, 2022 •

edited

Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

NetDEF-CI commented Mar 5, 2022

donaldsharp commented Mar 13, 2022

anlancs commented Mar 15, 2022

patrasar Mar 17, 2022

anlancs Mar 17, 2022

anlancs commented May 17, 2022 •

edited

Loading

github-actions bot commented Feb 14, 2023

anlancs commented Apr 29, 2023

rzalamena left a comment

mjstapp commented Sep 26, 2023

rzalamena left a comment

anlancs commented Oct 4, 2023

mjstapp commented Oct 4, 2023

anlancs commented Oct 5, 2023

mjstapp Oct 5, 2023

mjstapp commented Oct 5, 2023

rzalamena left a comment

anlancs commented Oct 7, 2023

anlancs commented Oct 7, 2023

zebra: remove ZEBRA_INTERFACE_VRF_UPDATE #10733

zebra: remove ZEBRA_INTERFACE_VRF_UPDATE #10733

Conversation

anlancs commented Mar 4, 2022 • edited Loading

NetDEF-CI commented Mar 4, 2022 • edited Loading

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

NetDEF-CI commented Mar 5, 2022 • edited Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

NetDEF-CI commented Mar 5, 2022

Continuous Integration Result: SUCCESSFUL

donaldsharp commented Mar 13, 2022

anlancs commented Mar 15, 2022

patrasar Mar 17, 2022

Choose a reason for hiding this comment

anlancs Mar 17, 2022

Choose a reason for hiding this comment

anlancs commented May 17, 2022 • edited Loading

github-actions bot commented Feb 14, 2023

anlancs commented Apr 29, 2023

rzalamena left a comment

Choose a reason for hiding this comment

mjstapp commented Sep 26, 2023

rzalamena left a comment

Choose a reason for hiding this comment

anlancs commented Oct 4, 2023

mjstapp commented Oct 4, 2023

anlancs commented Oct 5, 2023

mjstapp Oct 5, 2023

Choose a reason for hiding this comment

mjstapp commented Oct 5, 2023

rzalamena left a comment

Choose a reason for hiding this comment

anlancs commented Oct 7, 2023

anlancs commented Oct 7, 2023

anlancs commented Mar 4, 2022 •

edited

Loading

NetDEF-CI commented Mar 4, 2022 •

edited

Loading

NetDEF-CI commented Mar 5, 2022 •

edited

Loading

anlancs commented May 17, 2022 •

edited

Loading