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.

Sign up for GitHub

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

Jump to bottom

[VxlanMgr] changes for EVPN VXLAN #1266

Merged
merged 7 commits into from
Dec 14, 2020
Merged

[VxlanMgr] changes for EVPN VXLAN #1266

merged 7 commits into from
Dec 14, 2020

Conversation

srj102
Copy link
Contributor

@srj102 srj102 commented Apr 21, 2020 •
edited
Loading

  1. Add support for EVPN NVO config table.
  2. VxlanTunnel and TunnelMap handlers are enhanced to also cache the data so that
    this can be used during the deletion.
  3. ARP suppression changes
    Create state db for vxlanmgr and vlanmgr to sync the tunnel association to vlan
  4. Changes to support warm reboot.

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

Signed-off-by: Rajesh Sankaran rajesh.sankaran@broadcom.com

What I did
Please refer to details provided above.

Why I did it
EVPN VXLAN Implementation

How I verified it
Tested along with PR 1264. Used test script in PR 1318.
Also ran test_vnet.py from master branch to verify there were no failures.

Details if related

@prsunny prsunny self-assigned this Apr 22, 2020
@lguohan
Copy link
Contributor

lguohan commented Apr 29, 2020

need vs test for this feature.

gord1306
gord1306 reviewed May 15, 2020
View reviewed changes
cfgmgr/vxlanmgr.h Outdated Show resolved Hide resolved
gord1306
gord1306 reviewed May 15, 2020
View reviewed changes
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
gord1306
gord1306 reviewed May 15, 2020
View reviewed changes
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
gord1306
gord1306 reviewed May 15, 2020
View reviewed changes
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
@srj102 srj102 marked this pull request as ready for review June 10, 2020 10:06
@srj102 srj102 mentioned this pull request Jun 10, 2020
Merged
@prsunny
Copy link
Collaborator

prsunny commented Sep 10, 2020

retest this please

prsunny
prsunny reviewed Oct 7, 2020
View reviewed changes
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/vxlanmgrd.cpp Outdated Show resolved Hide resolved
cfgmgr/vxlanmgr.h Outdated Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp Outdated
SWSS_LOG_NOTICE("Creating VxlanNetDevice %s", vxlan_dev_name.c_str());
}

// Remove kernel device if it exists
Copy link
Collaborator

Choose a reason for hiding this comment

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

When would this happen? All Vxlan netdevices are cleared as part of constructor init right?

Copy link
Contributor Author

@srj102 srj102 Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes this is not required. Will remove.

cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp
bridge_untagged_add_cmd = std::string("") + BRIDGE_CMD + " vlan add vid " +
std::string(vlan_id) + " untagged pvid dev " + vxlan_dev_name;

bridge_del_vid_cmd = std::string("") + BRIDGE_CMD + " vlan del vid 1 dev " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not mentioned in HLD section. What is the purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After the command ip link set <vxlan_dev_name> master DOT1Q_BRIDGE_NAME the vxlan dev automatically
gets added to vid 1.. We wanted to avoid flooding vid-1 packets to vxlan device unless explicitly added.

ip link add vtep1-22 type vxlan id 100 local 1.1.1.1 dstport 4789
ip link set vtep1-22 master Bridge

root@sonic:/home/admin# bridge vlan show
port vlan ids
docker0 1 PVID Egress Untagged

Bridge None
dummy 1 PVID Egress Untagged

vtep1-22 1 PVID Egress Untagged

cfgmgr/vxlanmgr.cpp Outdated
size_t found = vxlanTunnelMapName.find(delimiter);
const auto vxlanTunnelName = vxlanTunnelMapName.substr(0, found);

createVxlanNetdevice(vxlanTunnelName, vni_id, src_ip, dst_ip, vlan_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

in this, it wouldn't be creating the netdevice right, since m_in_reconcile will be true? Just to clarify, will discuss in the review meeting

Copy link

Choose a reason for hiding this comment

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

Yes, this is correct.
During the initialization, vxlanmgrd reads the kernel and creates a cache of all the existing new devices.
When the configuration is restored, if the cache contains the corresponding kernel device, it will skip to create the kernel netdevice creation.
Typically for the swss docker restart, the cache will contain all the kernel netdevice and hence will not try to recreate it.
However for system warmboot, the kernel cache will be empty and hence while restoring the configuration, the corresponding kernel netdevices are created.

The m_in_reconcile is set to true (beginReconcile) before the configuration is restored.

kperumalbfn
kperumalbfn reviewed Oct 20, 2020
View reviewed changes
cfgmgr/vxlanmgr.cpp Outdated

tuncache.fvt = kfvFieldsValues(t);
tuncache.vlan_vni_refcnt = 0;
tuncache.m_sourceIp = fvValue(tuncache.fvt.back());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to get the source ip from the tuple fvt using SOURCE_IP??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

cfgmgr/vxlanmgr.cpp
tuncache.vlan_vni_refcnt = 0;
tuncache.m_sourceIp = fvValue(tuncache.fvt.back());

m_appVxlanTunnelTable.set(vxlanTunnelName, kfvFieldsValues(t));
Copy link
Contributor

Choose a reason for hiding this comment

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

check for existing tunnel before updating the tunnel table map??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using Click there is a check for existence of a VTEP object and rejected. Otherwise the code is the same as what exists today.

@srj102
Copy link
Contributor Author

srj102 commented Oct 30, 2020

retest vs please

kperumalbfn
kperumalbfn reviewed Dec 4, 2020
View reviewed changes
cfgmgr/vxlanmgr.cpp
dst_ip = dstIp->second;
}
else
{
Copy link
Contributor

Choose a reason for hiding this comment

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

On what scenario, dst_ip can be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dst_ip is that of the VXLAN_TUNNEL object. For EVPN it is expected to be NULL. Since the non EVPN cases had the support for destination IP in the VXLAN_TUNNEL object, this check was added.

cfgmgr/vxlanmgr.cpp
{
vni_id = value;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For a tunnel without vlan-vni mapper, we could bypass all these checks and return. I believe vlan <--> VNI mapper is required only for L3 EVPN asymmetric IRB and L2 EVPN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function handles the VXLAN_TUNNEL_MAP entry creation and will have VLAN-VNI mapping.

cfgmgr/vxlanmgr.cpp
return true;
}

if (m_vniMapCache.find(vni_id) != m_vniMapCache.end())
Copy link
Contributor

Choose a reason for hiding this comment

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

m_vniMapCache maintains only vlan <--> VNI map cache. Should we check for VRF <--> VNI mapper as well? There is no cache maintained as of now for that mapper in vxlanmgr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VNI-VRF Mapping is being handled in the VrfMgr.

cfgmgr/vxlanmgr.cpp Outdated
}

createAppDBTunnelMapTable(t);
createVxlanNetdevice(vxlanTunnelName, vni_id, src_ip, dst_ip, vlan_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Check for error state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@prsunny
Copy link
Collaborator

prsunny commented Dec 4, 2020

retest this please

@srj102
VxlanMgr changes for EVPN VXLAN
c0b1274 
1. Add support for EVPN NVO config table.
2. Changes to VxlanTunnel create handler to support KLISH and openconfig.
   The VXLAN_TUNNEL table in the config db gets populated as follows.
   NULL,NULL - interface vxlan create
   NULL,NULL,src_ip,<a.b.c.d> - when SIP is set.
   NULL,NULL - when SIP is unset.
   src_ip,<a.b.c.d> - This format is when click is used.
3. VxlanTunnel and TunnelMap handlers are enhanced to also cache the data so that
   this can be used during the deletion.
4. ARP suppression changes
   Create state db for vxlanmgr and vlanmgr to sync the tunnel association to vlan
5. Changes to support warm reboot.

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

Signed-off-by: Rajesh Sankaran <rajesh.sankaran@broadcom.com>
@srj102
Compiled with the latest changes to swss-common.
1d0defd
@srj102
Tested vxlanmgr changes with corresponding orchagent changes and evpn…
00ecfdb 
… pytest.
@srj102
Incorporated Review comments.
2d9d371
kperumalbfn
kperumalbfn previously approved these changes Dec 9, 2020
View reviewed changes
prsunny
prsunny reviewed Dec 10, 2020
View reviewed changes
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp
//Inform the Vlan Mgr to update the tunnel flags if Arp/Nd Suppression is set.
std::string key = "Vlan" + std::string(vlan_id);
vector<FieldValueTuple> fvVector;
FieldValueTuple s("netdev", vxlan_dev_name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this newly added? I don't remember seeing this in the previous review, May be missed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No was part of the first commit itself.

cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
cfgmgr/vxlanmgr.cpp Outdated Show resolved Hide resolved
@srj102
Incorporated review comments
c6be754 
1. Changes to logging from NOTICE to INFO.
2. Return value for createVxlanNEtDevice handled.
   changed the createvxlannetdevice to be similar to vnet to call swss:exec
   instead of EXEC_WITH_THROW macro.
3. Changes to portsorch for lowering the severity levels of couple of logs.
@srj102
Copy link
Contributor Author

srj102 commented Dec 10, 2020

retest vs please

@prsunny prsunny merged commit 0028f4f into sonic-net:master Dec 14, 2020
arlakshm pushed a commit to arlakshm/sonic-swss that referenced this pull request Dec 15, 2020
@srj102 @arlakshm
[VxlanMgr] changes for EVPN VXLAN (sonic-net#1266)
d7ec92f 
* VxlanMgr changes for EVPN VXLAN

1. Add support for EVPN NVO config table.
2. VxlanTunnel and TunnelMap handlers are enhanced to also cache the data so that this can be used during the deletion.
3. ARP suppression changes
4. Create state db for vxlanmgr and vlanmgr to sync the tunnel association to vlan
5. Changes to support warm reboot.

Signed-off-by: Rajesh Sankaran <rajesh.sankaran@broadcom.com>
Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
@stephenxs stephenxs mentioned this pull request Dec 16, 2020
Closed
3 tasks
@prsunny prsunny mentioned this pull request Feb 12, 2021
Closed
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
@smaheshm
[pytest][qos][config] Added pytests for "config qos reload" commands (s…
6202a81 
…onic-net#1266)

 Added pytests for "config qos reload" commands.
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
@lguohan
Revert "[pytest][qos][config] Added pytests for "config qos reload" c…
de14939 
…ommands (sonic-net#1266)"

This reverts commit 6202a81.
@msosyak msosyak mentioned this pull request Mar 15, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gord1306 gord1306 gord1306 left review comments

@rpjain-nw rpjain-nw rpjain-nw left review comments

@prsunny prsunny prsunny approved these changes

@kperumalbfn kperumalbfn kperumalbfn approved these changes

Assignees

@prsunny prsunny

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@srj102 @lguohan @prsunny @gord1306 @kperumalbfn @rpjain-nw