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

[FDBSYNCD] Added support for FDBSYNCD for EVPN as described in the PR Azure/SONiC#437 #1276

Merged
merged 27 commits into from
Dec 11, 2020

Conversation

kishorekunal01
Copy link
Contributor

[FDBSYNCD] Added support for FDBSYNCD for EVPN as described in the PR sonic-net/SONiC#437

This Change set is to support EVPN MAC/IMET routes from Kernel to DB and DB to Kernel.

What I did

  1. New demon is added to sync MAC from Kernel to DB and vise versa.
  2. Add remote IMET route learned via EVPN to EVPN_REMOTE_VNI_TABLE in APP_DB.
  3. Add remote MAC route learned via EVPN to VXLAN_FDB_TABLE.
  4. Add State DB FDB_TABLE for local learned MAC to Kernel

Why I did it
Part of overall EVPN-VXLAN Implementation

How I verified it
Not yet compiled or verified

Details if related
HLD Location : https://github.com/Azure/SONiC/pull/437/commits

Signed-off-by: Kishore Kunal kishore.kunal@broadcom.com

@lguohan
Copy link
Contributor

lguohan commented Apr 29, 2020

need vs test to cover this feature.

fdbsyncd/Makefile.am Outdated Show resolved Hide resolved
fdbsyncd/fdbsync.cpp Outdated Show resolved Hide resolved
@msftclas
Copy link

msftclas commented May 14, 2020

CLA assistant check
All CLA requirements met.


FdbSync::FdbSync(RedisPipeline *pipelineAppDB, DBConnector *stateDb) :
m_fdbTable(pipelineAppDB, APP_VXLAN_FDB_TABLE_NAME),
m_imetTable(pipelineAppDB, APP_EVPN_REMOTE_VNI_TABLE_NAME),
Copy link
Contributor

Choose a reason for hiding this comment

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

According to sonic-swss-common PR339, should this constant be renamed to APP_VXLAN_REMOTE_VNI_TABLE_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. Updated the same

if (m_AppRestartAssist)
{
m_AppRestartAssist->registerAppTable(APP_VXLAN_FDB_TABLE_NAME, &m_fdbTable);
m_AppRestartAssist->registerAppTable(APP_EVPN_REMOTE_VNI_TABLE_NAME, &m_imetTable);
Copy link
Contributor

Choose a reason for hiding this comment

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

According to sonic-swss-common PR339, should this constant be renamed to APP_VXLAN_REMOTE_VNI_TABLE_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. Updated the same

// If warmstart is in progress, we take all netlink changes into the cache map
if (m_AppRestartAssist->isWarmStartInProgress())
{
m_AppRestartAssist->insertToMap(APP_EVPN_REMOTE_VNI_TABLE_NAME, key, fvVector, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

According to sonic-swss-common PR339, should this constant be renamed to APP_VXLAN_REMOTE_VNI_TABLE_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. Updated the same

// If warmstart is in progress, we take all netlink changes into the cache map
if (m_AppRestartAssist->isWarmStartInProgress())
{
m_AppRestartAssist->insertToMap(APP_EVPN_REMOTE_VNI_TABLE_NAME, key, fvVector, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

According to sonic-swss-common PR339, should this constant be renamed to APP_VXLAN_REMOTE_VNI_TABLE_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. Updated the same

fdbmac.erase(key);
}

if (fdb_type == FDB_TYPE_DYNAMIC)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a question about the bridge fdb command, whether the remote MAC need to have extern_learn field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review, Yes extern_learn flag need to be set but currently exec command doesn't has extern_learn support, Hence it is not set.

fdbsyncd/fdbsync.h Show resolved Hide resolved
fdbsyncd/fdbsyncd.cpp Outdated Show resolved Hide resolved
fdbsyncd/fdbsyncd.cpp Outdated Show resolved Hide resolved
fdbsyncd/fdbsync.h Outdated Show resolved Hide resolved
fdbsyncd/fdbsync.h Outdated Show resolved Hide resolved
fdbsyncd/fdbsync.cpp Outdated Show resolved Hide resolved
fdbsyncd/fdbsync.cpp Outdated Show resolved Hide resolved
fdbsyncd/fdbsync.cpp Outdated Show resolved Hide resolved
fdbsyncd/fdbsync.cpp Outdated Show resolved Hide resolved
fdbsyncd/fdbsync.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@kishorekunal01 kishorekunal01 left a comment

Choose a reason for hiding this comment

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

All review comment address. WIll raise 2 more PR for the sonic-buildimage to start fdbsyncd process, and adding a table for warm reboot store in swss-common


if (info->op_type == FDB_OPER_ADD)
{
macAddSrcDB(info);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't move this to caller since we use the DB data to send del to the kernel and we want to keep add/del from DB at one place.

}
else
{
/* If this is for vnet bridge vxlan interface, then return */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prsunny If the interface is Vxlan and is Vnet interface then return from here. Am I missing something

return;
}
SWSS_LOG_INFO("Op:%d VxLAN dev %s index:%d vni:%d", nlmsg_type, ifname? ifname: nil, ifindex, vni);
if (nlmsg_type == RTM_NEWLINK)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are only creating interface for vxlan interface so that we can extract VNI information for MAC installation, When interface is delete, each MAC del will be received in the kernel. Hence we don't need to handle interface delete here.

@prsunny
Copy link
Collaborator

prsunny commented Dec 4, 2020

retest this please

@prsunny prsunny merged commit 14d8c5f into sonic-net:master Dec 11, 2020
arlakshm pushed a commit to arlakshm/sonic-swss that referenced this pull request Dec 15, 2020
…NiC#437 (sonic-net#1276)

* [FPMSYNCD] EVPN Type5 prefix handling in FPMSYNCD.

This Change set is to support EVPN MAC/IMET routes from Kernel to DB and DB to Kernel:

New demon is added to sync MAC from Kernel to DB and vise versa.
Add remote IMET route learned via EVPN to EVPN_REMOTE_VNI_TABLE in APP_DB.
Add remote MAC route learned via EVPN to VXLAN_FDB_TABLE.
Add State DB FDB_TABLE for local learned MAC to Kernel

Signed-off-by: Kishore Kunal kishore.kunal@broadcom.com
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…onic-net#1276)

**- What I did**
Fix that it cannot add loopback interface by CLI, like the following:
```
root@as7816-64x:~# config loopback add Loopback1
Usage: config [OPTIONS] COMMAND [ARGS]...
Try "config --help" for help.

Error: No such command "L".
```
**- How I did it**
Remove `@config.group()` before the function `is_loopback_name_valid(loopback_name)`, which will be called by add/del command

Signed-off-by: MuLin <mulin_huang@edge-core.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants