Skip to content
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

Code changes to support IPv6 Link local enhancements #1463

Merged
merged 31 commits into from
Aug 5, 2021

Conversation

AkhileshSamineni
Copy link
Contributor

@AkhileshSamineni AkhileshSamineni commented Oct 10, 2020

Code changes :

  1. Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.
  2. Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".
  3. Allow the ipv6 link-local address as neighbors.
  4. Allow adding the IPv6 routes with link-local nexthops.
  5. Changes in fpm to handle ipv6 nexthop with ipv4 prefix (as BGP is going to send ipv6 link-local as the nexthop in case of 5549).
  6. Skip Ipv4LL neighbor add in Orchagent for RFC5549.

As per HLD - sonic-net/SONiC#625

Depends on :
sonic-net/sonic-utilities#1159
sonic-net/sonic-buildimage#5584

Signed-off-by: Akhilesh Samineni akhilesh.samineni@broadcom.com

@AkhileshSamineni
Copy link
Contributor Author

retest vs please

@batmancn
Copy link

batmancn commented Jan 4, 2021

Hi, all

I'm testing this patch, but I got this error:

Jan  4 13:22:32.653510 dut-testbed NOTICE swss#orchagent: :- addIp2MeRoute: Create IP2me route ip:fe80::320d:9eff:fe00:121
Jan  4 13:22:32.661766 dut-testbed NOTICE swss#orchagent: :- doTask: Route 10.0.0.0/31: resize ipv to match alsv, 0 -> 1.
Jan  4 13:22:32.661766 dut-testbed NOTICE swss#orchagent: :- doTask: Route 10.0.0.0/31: set the empty nexthop ip to zero.
Jan  4 13:22:32.661918 dut-testbed NOTICE swss#orchagent: :- doTask: Route fc00::/126: resize ipv to match alsv, 0 -> 1.
Jan  4 13:22:32.661918 dut-testbed NOTICE swss#orchagent: :- doTask: Route fc00::/126: set the empty nexthop ip to zero.
Jan  4 13:22:32.662021 dut-testbed NOTICE swss#orchagent: :- doTask: Route fe80::/64: resize ipv to match alsv, 0 -> 1.
Jan  4 13:22:32.662021 dut-testbed NOTICE swss#orchagent: :- doTask: Route fe80::/64: set the empty nexthop ip to zero.
Jan  4 13:22:32.708112 dut-testbed NOTICE swss#orchagent: :- doTask: Get port state change notification id:1000000000003 status:1
Jan  4 13:22:32.708349 dut-testbed NOTICE swss#orchagent: :- setHostIntfsOperStatus: Set operation status UP to host interface Ethernet2
Jan  4 13:22:32.710027 dut-testbed ERR swss#orchagent: :- meta_sai_validate_route_entry: object key SAI_OBJECT_TYPE_ROUTE_ENTRY:{"dest":"fe80::320d:9eff:fe00:121/128","switch_id":"oid:0x21000000000000","vr":"oid:0x300000000003a"} already exists
Jan  4 13:22:32.710027 dut-testbed ERR swss#orchagent: :- addIp2MeRoute: %%intfsorch_sai: Failed to create IP2me route ip:fe80::320d:9eff:fe00:121, rv:-6
Jan  4 13:22:32.710428 dut-testbed INFO swss#supervisord: orchagent terminate called after throwing an instance of 'std::runtime_error'
Jan  4 13:22:32.710883 dut-testbed INFO swss#supervisord: orchagent   what():  Failed to create IP2me route.

I found this error is because "swss add same route twice or more".
"swss add same route twice or more" is because each INTERFACE has a IPv6 LLA, so swss will add IP2ME route for each INTERFACE's IPv6 LLA.

So how to deal with this?

@AkhileshSamineni AkhileshSamineni force-pushed the ipv6-link-local-changes branch from 8c5b06a to 0baba1c Compare April 5, 2021 17:46
@AkhileshSamineni AkhileshSamineni changed the title Code changes to support IPv6 Link local enchancements Code changes to support IPv6 Link local enhancements Apr 5, 2021
@AkhileshSamineni
Copy link
Contributor Author

@arlakshm Please retest it, as I don't have permission for Azurepipelines run.

@AkhileshSamineni
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 1463 in repo Azure/sonic-swss

@AkhileshSamineni
Copy link
Contributor Author

@arlakshm @prsunny Please retest it, as I don't have permission for Azurepipelines run.

@arlakshm
Copy link
Contributor

/Azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lguohan lguohan requested a review from zhenggen-xu April 14, 2021 21:18
@AkhileshSamineni
Copy link
Contributor Author

The failures are not related to the code changes, Please check it.

@AkhileshSamineni
Copy link
Contributor Author

Please re-run the build, as the failures are not related to the code changes.

@anshuv-mfst
Copy link

@arlakshm - could you please help with re-run.

@lguohan
Copy link
Contributor

lguohan commented May 4, 2021

can you add vstest?

@prsunny
Copy link
Collaborator

prsunny commented May 5, 2021

@AkhileshSamineni , can you do a rebase and push?

@AkhileshSamineni AkhileshSamineni force-pushed the ipv6-link-local-changes branch from 0fb6a09 to 7053e33 Compare June 7, 2021 08:46
@lgtm-com
Copy link

lgtm-com bot commented Jun 7, 2021

This pull request introduces 1 alert when merging a726b09 into 5847af3 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a call

@AkhileshSamineni
Copy link
Contributor Author

can you add vstest?

@lguohan Added VS test cases.

@AkhileshSamineni
Copy link
Contributor Author

@AkhileshSamineni , can you do a rebase and push?

@lguohan @prsunny Done rebase and pushed the changes. Now I see vrf_creation is failing in most of the test cases, which were not related to my changes. Could you please check.

@AkhileshSamineni
Copy link
Contributor Author

/azpw run

@AkhileshSamineni
Copy link
Contributor Author

/AzurePipelines run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 1463 in repo Azure/sonic-swss

@prsunny prsunny self-requested a review July 31, 2021 00:41
cfgmgr/intfmgr.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@venkatmahalingam venkatmahalingam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.

I think, BGP unnumbered use-case is expected to work without this field explicitly configured as ipv6 enabled by default globally on all interfaces i.e LLA should be present., right?
Wondering what's the expectation on the community image for BGP unnumbered functionality to work,
Should we need to disable ipv6 on all interfaces globally first and then enable per interface basis?

Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".

This is no longer valid as we by default have ipv6 enabled on all interfaces.

Allow the ipv6 link-local address as neighbors.

Not sure why do we need LLA as neighbors.

@AkhileshSamineni
Copy link
Contributor Author

Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.

I think, BGP unnumbered use-case is expected to work without this field explicitly configured as ipv6 enabled by default globally on all interfaces i.e LLA should be present., right?
Wondering what's the expectation on the community image for BGP unnumbered functionality to work,
Should we need to disable ipv6 on all interfaces globally first and then enable per interface basis?

Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".

This is no longer valid as we by default have ipv6 enabled on all interfaces.

Allow the ipv6 link-local address as neighbors.

Not sure why do we need LLA as neighbors.

@venkatmahalingam
As per the community new design

  1. Yes, the BGP unnumbered might work without configuring the ipv6 link local on an interface.
  2. Yes, IPv6 LL is enabled by default in the kernel stack and the config command is not enabling/disabling the IPv6 LL in stack. Basically we are not touching that part here.
  3. The global/interface enable config command will create the RIF in the hardware, if there is no ipv6 global address configured on a interface and global/interface disable config command will delete the RIF in the hardware, if there is no ipv6 global address configured on a interface.

@anshuv-mfst
Copy link

hi @prsunny - could you please review and approve asap, this is needed in 202106 release

@prsunny
Copy link
Collaborator

prsunny commented Aug 3, 2021

hi @prsunny - could you please review and approve asap, this is needed in 202106 release

@anshuv-mfst , waiting on one outstanding comment. @AkhileshSamineni , could you please check the unresolved comments and reply?

@AkhileshSamineni
Copy link
Contributor Author

hi @prsunny - could you please review and approve asap, this is needed in 202106 release

@anshuv-mfst , waiting on one outstanding comment. @AkhileshSamineni , could you please check the unresolved comments and reply?

@prsunny Added code changes to skip the ipv6 link-local netlink msg in neighsyncd when link-local-mode config is disabled.
Please review the changes.

@prsunny
Copy link
Collaborator

prsunny commented Aug 5, 2021

@venkatmahalingam , can you please sign off?

@prsunny prsunny merged commit 8f7ea14 into sonic-net:master Aug 5, 2021
judyjoseph pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Aug 7, 2021
As per HLD - sonic-net/SONiC#625

FRR Patches:

0009-Link-local-scope-was-not-set-while-binding-socket-for-bgp-ipv6-link-local-neighbors.patch
Files modified : bgpd_network.c and bgpd/bgp_zebra.c
Fix for : Link local scope was not set while binding socket with local address causing socket errors for bgp ipv6 link local neighbors.

0010-VRF-interface-lookup-was-still-done-in-the-default-vrf.patch
Files modified : staticd/static_zebra.c
Fix for : VRF interface lookup was still done in the default-vrf which was causing the interface lookup to fail. Due to this static-route pointing to link-local was not getting installed.

0011-Changes-to-send-ipv6-link-local-address-as-nexthop-to-fpmsyncd.patch
Files modified : zebra/zebra_fpm_netlink.c
Fix for : Made changes to send ipv6 address as nexthop to fpmsyncd.

Depends on:
sonic-net/sonic-utilities#1159
sonic-net/sonic-swss#1463

Signed-off-by: Akhilesh Samineni akhilesh.samineni@broadcom.com
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
As per HLD - sonic-net/SONiC#625

FRR Patches:

0009-Link-local-scope-was-not-set-while-binding-socket-for-bgp-ipv6-link-local-neighbors.patch
Files modified : bgpd_network.c and bgpd/bgp_zebra.c
Fix for : Link local scope was not set while binding socket with local address causing socket errors for bgp ipv6 link local neighbors.

0010-VRF-interface-lookup-was-still-done-in-the-default-vrf.patch
Files modified : staticd/static_zebra.c
Fix for : VRF interface lookup was still done in the default-vrf which was causing the interface lookup to fail. Due to this static-route pointing to link-local was not getting installed.

0011-Changes-to-send-ipv6-link-local-address-as-nexthop-to-fpmsyncd.patch
Files modified : zebra/zebra_fpm_netlink.c
Fix for : Made changes to send ipv6 address as nexthop to fpmsyncd.

Depends on:
sonic-net/sonic-utilities#1159
sonic-net/sonic-swss#1463

Signed-off-by: Akhilesh Samineni akhilesh.samineni@broadcom.com
judyjoseph pushed a commit that referenced this pull request Aug 10, 2021
* Code changes to support IPv6 Link local enchancements
- Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.
- Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".
- Allow the ipv6 link-local address as neighbors.
- Allow adding the IPv6 routes with link-local nexthops.
- Skip Ipv4LL neighbor add in Orchagent for RFC5549.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
msupuran pushed a commit to Metaswitch/sonic-swss that referenced this pull request Sep 8, 2021
* Code changes to support IPv6 Link local enchancements
- Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.
- Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".
- Allow the ipv6 link-local address as neighbors.
- Allow adding the IPv6 routes with link-local nexthops.
- Skip Ipv4LL neighbor add in Orchagent for RFC5549.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
msupuran pushed a commit to Metaswitch/sonic-swss that referenced this pull request Sep 8, 2021
* Code changes to support IPv6 Link local enchancements
- Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.
- Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".
- Allow the ipv6 link-local address as neighbors.
- Allow adding the IPv6 routes with link-local nexthops.
- Skip Ipv4LL neighbor add in Orchagent for RFC5549.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
msupuran pushed a commit to Metaswitch/sonic-swss that referenced this pull request Sep 10, 2021
* Code changes to support IPv6 Link local enchancements
- Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.
- Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".
- Allow the ipv6 link-local address as neighbors.
- Allow adding the IPv6 routes with link-local nexthops.
- Skip Ipv4LL neighbor add in Orchagent for RFC5549.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021
* Code changes to support IPv6 Link local enchancements
- Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.
- Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".
- Allow the ipv6 link-local address as neighbors.
- Allow adding the IPv6 routes with link-local nexthops.
- Skip Ipv4LL neighbor add in Orchagent for RFC5549.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
The key used is as read from APPL-DB, hence use it as such to get the value,
to help get the interfce name, so it can be used for filtering out.
liuyuefengcn added a commit to liuyuefengcn/sonic-swss that referenced this pull request Jan 15, 2024
…thub.com>

Date:   Thu Aug 5 23:49:44 2021 +0530
    Code changes to support IPv6 Link local enhancements (sonic-net#1463)
liuyuefengcn pushed a commit to liuyuefengcn/sonic-swss that referenced this pull request Feb 28, 2024
* Code changes to support IPv6 Link local enchancements
- Changes to handle the "ipv6_use_link_local_only" option on a interface which is added/deleted based on Global Config or Interface config.
- Adds/Deletes the RIF for an interface in the absence of IPv6 global address on a interface based on "ipv6_use_link_local_only".
- Allow the ipv6 link-local address as neighbors.
- Allow adding the IPv6 routes with link-local nexthops.
- Skip Ipv4LL neighbor add in Orchagent for RFC5549.

Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AkhileshSamineni Could you please share the reason why this part of code was removed? There is a long opened PR #1926 added this code back. Without this changes the BGP unnumbered cannot work properly with VRF.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants