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

[Orchagent]: FdbOrch changes for EVPN VXLAN #1275

Merged
merged 27 commits into from
Jan 8, 2021

Conversation

jainp1979
Copy link
Contributor

@jainp1979 jainp1979 commented Apr 28, 2020

This Change set is to support EVPN MAC routes addition/deletion in ASIC.

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

Signed-off-by: Pankaj Jain [email protected]

What I did
Support for EVPN advertised mac routes

Why I did it
Part of overall EVPN-VXLAN Implementation

How I verified it
compiled and Unit tested

Details if related
Depends on the following PRs:
#1264

@msftclas
Copy link

msftclas commented Apr 28, 2020

CLA assistant check
All CLA requirements met.

@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2020

This pull request introduces 7 alerts when merging acb5c7e into 3829053 - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 1 for Unused local variable
  • 1 for Variable defined multiple times

@jainp1979
Copy link
Contributor Author

jainp1979 commented Apr 29, 2020

Dependent on the following PRs:
#1264

return false;
}

if (!m_portsOrch->getPortByBridgePortId(attr.value.oid, port))
if (!m_portsOrch->getPortByBridgePortId(bridge_port_id, port))
{
SWSS_LOG_ERROR("Failed to get port by bridge port ID 0x%" PRIx64, attr.value.oid);
Copy link
Contributor

Choose a reason for hiding this comment

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

attr.value.oid should change to bridge_port_id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes definitely; I will correct it.
Thanks for spotting the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rectified in latest commit.

}

sai_status_t status = sai_fdb_api->create_fdb_entry(&fdb_entry, (uint32_t)attrs.size(), attrs.data());
if (status != SAI_STATUS_SUCCESS)
string key = "Vlan" + to_string(vlan.m_vlan_info.vlan_id) + ":" + entry.mac.to_string();
Copy link
Contributor

Choose a reason for hiding this comment

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

this string is unused variable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; I will remove that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rectified in latest commit

attr.value.s32 = (type == "dynamic") ? SAI_FDB_ENTRY_TYPE_DYNAMIC : SAI_FDB_ENTRY_TYPE_STATIC;
if (origin == FDB_ORIGIN_VXLAN_ADVERTIZED)
{
attr.value.s32 = (type == "dynamic") ? SAI_FDB_ENTRY_TYPE_STATIC_MACMOVE : SAI_FDB_ENTRY_TYPE_STATIC;
Copy link
Contributor

@gord1306 gord1306 May 21, 2020

Choose a reason for hiding this comment

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

Which PR contains this define SAI_FDB_ENTRY_TYPE_STATIC_MACMOVE. The SAI PR1024 doesn't include this constant

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 is how it was proposed initially;
later as part of PR filing it was changed; Once the PR1024 gets merged; I will update the code;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code is updated as per SAI PR1024

@jainp1979 jainp1979 marked this pull request as ready for review June 24, 2020 08:44
1. Review comments incorporated
2. Dependency for PR sonic-net#885 is resolved
3. Added test_evpn_fdb.py pytest script to verify EVPN-VXLAN-FDB
4. compiled and verified the FDB functionality with EVPN
5. Changes to handle the static mac and aging from CLI
entry.mac.to_string().c_str(), entry.bv_id);

if (!inserted.second)
bool mac_move = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to interlace add and move? Can we seperate them out as the caller anyways know if its a move or add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, Caller knows it; but since storeFdbEntryState takes FdbUpdate as a parameter which only has add/del indication;
Kept the function signature same and added the logic;
Also only CRM-Counter is handled differently in MOVE case so it did not seem complicated either.


case SAI_FDB_EVENT_FLUSHED:
if (!m_portsOrch->getPortByBridgePortId(bridge_port_id, update.port))
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know bridge_port_id is valid? If it is, we do NOT need to call this function as update.port is already populated at the start of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; you are right
It will be removed in next update.

bridge port ID: 0x%" PRIx64 ".",
update.entry.mac.to_string().c_str(), entry->bv_id,
bridge_port_id);
if (existing_entry->second.bridge_port_id != bridge_port_id)
Copy link
Contributor

@vasant17 vasant17 Oct 8, 2020

Choose a reason for hiding this comment

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

Assuming existing_entry->second.bridge_port_id is always valid, I think we should do this comparison only if bridge_port_id is also valid, if it is NOT, I think we should go ahead and delete it. Of course we still need to get the port by using the existing_entry->bridge_port_id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

validity of bridge-port is already checked in the beginning.
As you indicated in previous comment, that check is not required.
This comparison is fine and fdb entry is deleted irrespective of the comparison result.

{
SWSS_LOG_NOTICE("FdbOrch AGE notification: Failed to get port by bridge port ID 0x%lx", existing_entry->second.bridge_port_id);
}
// dont return, let it delete just to bring SONiC and SAI in sync
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this commented out code? I wont mind keeping it as it helps the reader!

Copy link
Contributor Author

@jainp1979 jainp1979 Oct 15, 2020

Choose a reason for hiding this comment

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

Lets keep it;

{
port_attr[1].id = SAI_FDB_FLUSH_ATTR_ENTRY_TYPE;
port_attr[1].value.s32 = SAI_FDB_FLUSH_ENTRY_TYPE_DYNAMIC;
status = sai_fdb_api->flush_fdb_entries(gSwitchId, 2, port_attr);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call this only once and use attr_count variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment applies to below two functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

struct SavedFdbEntry
{
FdbEntry entry;
MacAddress mac;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we remove FdbEntry and added mac and vlanID explicitly. Can we keep FdbEntry and use its overloaded comparision operator to compare savedFdbEntry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SaveFdbEntry is used for storing an FDB which has valid vlan (vlan is created) but is waiting for the port to become the associated vlan's member.
now if vlan gets deleted, this entry is stuck in the saveFdbEntry;
later when user deletes the FDB, there is no way to compare it with bv_id (because that is no more there) hence changed to vlan-id so that delete request can be compared against FDBs in savedFdbEntry.

struct SavedFdbEntry
{
FdbEntry entry;
MacAddress mac;
unsigned short vlanId;
string type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove all the variable below this variable and add fdbData as a variable?

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

bool add;
};

struct FdbData
Copy link
Contributor

@vasant17 vasant17 Oct 8, 2020

Choose a reason for hiding this comment

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

This structure will contain all the information related to an FDB entry, so, it would be neat keep VxLAN related info in a separate structure and add it here as a variable, but I will leave it up to you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping it the same for now.

bool removeFdbEntry(const FdbEntry&);
void flushFDBEntries(sai_object_id_t bridge_port_oid,
sai_object_id_t vlan_oid);
bool addFdbEntry(const FdbEntry&, const string&, const string&, FdbOrigin origin, const string& remote_ip="", unsigned int vni=0, const string& esi="");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use FDBData structure here, instead of passing 6 extra arguments?

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.

std::string res;
EXEC_WITH_ERROR_THROW(cmds.str(), res);
if (isVlanMemberStateOk(key)) {
ostringstream cmds, inner;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could define this outside to avoid duplicate declaration

BRIDGE_CMD " vlan add vid " + std::to_string(vlan_id) + " dev " << shellquote(port_alias) << " " + tagging_cmd;
cmds << BASH_CMD " -c " << shellquote(inner.str());

const std::string key = std::string("") + "Vlan1|" + port_alias;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why Vlan1?

Copy link
Contributor

Choose a reason for hiding this comment

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

This has been included in #885, will remove from here.

@@ -241,6 +264,137 @@ bool VlanMgr::isVlanMacOk()
return !!gMacAddress;
}

void VlanMgr::doSwitchTask(Consumer &consumer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure a CLI/config is introduced for this. Lets discuss this in WG call to understand more

Copy link
Contributor

Choose a reason for hiding this comment

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

This has been included in #885, will remove from here.

@@ -103,7 +111,7 @@ bool VlanMgr::addHostVlan(int vlan_id)
+ BASH_CMD + " -c \""
+ BRIDGE_CMD + " vlan add vid " + std::to_string(vlan_id) + " dev " + DOT1Q_BRIDGE_NAME + " self && "
+ IP_CMD + " link add link " + DOT1Q_BRIDGE_NAME
+ " up"
+ " down"
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 a behavior change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has been included in #885, will remove from here.

fvVector.push_back(i);
}
/* Set vlan mtu */
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like tabspaces got added. Please fix

@@ -3709,8 +3831,10 @@ bool PortsOrch::removeLag(Port lag)
return false;
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove extra line

{
updateDbVlanOperStatus(Vlan, "up");
}
SWSS_LOG_NOTICE("Vlan %s Port %s state %s m_up_member_count %d",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to INFO

{
return ;
}
SWSS_LOG_NOTICE("Port %s oper state set from %s to %s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to INFO

{
updateDbVlanOperStatus(Vlan, "up");
}
SWSS_LOG_NOTICE("Vlan %s Port %s state %s m_up_member_count %d",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to INFO

orchagent/port.h Outdated
@@ -24,6 +24,7 @@ namespace swss {

struct VlanMemberEntry
{
std::string alias;
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 the Vlan name?

@@ -3411,13 +3475,14 @@ bool PortsOrch::removeBridgePort(Port &port)
SWSS_LOG_INFO("Flush FDB entries for port %s", port.m_alias.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this flushFDBEntries operation be called before if (port.m_fdb_count != 0)

// try to insert an FDB entry. If the FDB entry is not ready to be inserted yet,
// it would be added back to the saved_fdb_entries structure by addFDBEntry()
(void)addFdbEntry(fdb.entry, fdb.type);
for (const auto& fdb: fdb_list)

Choose a reason for hiding this comment

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

The additional inner loop looks like a mistake. Has it ever compiled? I got an error on gcc 8.3. Probably you want a single-level loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes; it should be single loop;

Choose a reason for hiding this comment

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

@jainp1979 Do you plan to maintain this pull request further to integrate it into master branch? There are some changes requested and some new merges introduced conflicts with your code.

@lgtm-com
Copy link

lgtm-com bot commented Dec 24, 2020

This pull request fixes 1 alert when merging 41a4a18 into c39a4b1 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@@ -1448,8 +1247,7 @@ void FdbOrch::notifyTunnelOrch(Port& port)
{
VxlanTunnelOrch* tunnel_orch = gDirectory.get<VxlanTunnelOrch*>();

if((port.m_type != Port::TUNNEL) ||
(port.m_fdb_count != 0))
if(port.m_type != Port::TUNNEL)
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change, Tunnel Bridge port will now be deleted even though the number of EVPN MACs over a Tunnel is non zero. This will fail and hence the m_fdb_count variable needs to be updated and deleteTunnelPort should be called only when the number goes down to 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

@srj102 @prsunny The m_fdb_count logic is part of generic L2 enhancements and it is used to check for fdb count before deleting a bridgeport, which in turn prevents deleting lag or vlan when fdb entries are present. This change is already part of #885. We can bring in that change back just for tunnel, but this issue exists for all the non tunnel cases as well. let me know if you want the change to be done here only for tunnel or in general.
BTW, if the m_fdb_count is not 0, is there a trigger to delete all the pending tunnel bridgeports when the m_fdb_count becomes 0? How is it handled if the tunnel bridgeport is created again before fdb count becomes 0?

Copy link
Contributor

@srj102 srj102 Dec 24, 2020

Choose a reason for hiding this comment

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

The change in notifyTunnelOrch to check for fdb count could be left as was earlier. PR 885 will not have the changes pertaining to notifyTunnelOrch.
If it is clear that PR 885 will not be merged for Dec release then we should bring back the changes for all types of Port objects - Phy port, LAG, Vlan, Tunnel.

notifyTunnelOrch is called from different places in fdborch whenever the fdb count becomes zero.
on becoming zero the deleteTunnelPort is called which checks for the presence of IMR/IP routes over the tunnel. If there are IMR/IP routes then the tunnel bridge port and tunnel is not deleted.
Similarly when the IMR/IP route count becomes zero the m_fdb_count is checked in vxlanorch before deleting the tunnel port/bridgeport.

Copy link
Contributor

Choose a reason for hiding this comment

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

added the reference count logic for tunnels

@lgtm-com
Copy link

lgtm-com bot commented Dec 28, 2020

This pull request fixes 1 alert when merging f17b20e into 6eb36d9 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@@ -169,7 +171,7 @@ class PortsOrch : public Orch, public Subject
Port m_cpuPort;
// TODO: Add Bridge/Vlan class
sai_object_id_t m_default1QBridge;
sai_object_id_t m_defaultVlan;
sai_object_id_t m_defaultVlan;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this extra space

@@ -198,6 +200,7 @@ class PortsOrch : public Orch, public Subject
map<set<int>, tuple<string, uint32_t, int, string, int>> m_lanesAliasSpeedMap;
map<string, Port> m_portList;
unordered_map<sai_object_id_t, int> m_portOidToIndex;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this extra line?

@@ -9,6 +9,7 @@
#include "observer.h"
#include "macaddress.h"
#include "producertable.h"
#include "notificationproducer.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like not relevant in this PR. Please remove

m_portOidToIndex[id] = index;
m_port_ref_count[alias] = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

* but we should not delete this mac here since now
* mac in orchagent represents locally learnt
*/
SWSS_LOG_NOTICE("FdbOrch RemoveFDBEntry: mac=%s fdb origin is different; found_origin:%d delete_origin:%d",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to INFO

entry.mac.to_string().c_str(), entry.bv_id);
return true; //FIXME: it should be based on status. Some could be retried. some not
}

SWSS_LOG_NOTICE("Removed mac=%s bv_id=0x%lx port:%s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to INFO

{
if (iter->fdbData.origin == origin)
{
SWSS_LOG_NOTICE("FDB entry found in saved fdb. deleting..."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change the NOTICE logs to INFO in the regular flow of FDB add/remove

updated as per latest review comments
@anilkpan
Copy link
Contributor

anilkpan commented Dec 29, 2020 via email

@lgtm-com
Copy link

lgtm-com bot commented Dec 29, 2020

This pull request fixes 1 alert when merging 3ceb819 into e32b9d0 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@@ -1882,7 +1881,6 @@ bool PortsOrch::initPort(const string &alias, const int index, const set<int> &l

/* Add port to port list */
m_portList[alias] = p;
m_port_ref_count[alias] = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think you accidently remove this line. Can you put it back?

Copy link
Contributor

Choose a reason for hiding this comment

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

this line was somehow showing as a new addition as seen in your pervious comment. I just re-added this line.

@lgtm-com
Copy link

lgtm-com bot commented Dec 29, 2020

This pull request fixes 1 alert when merging df6b9e7 into e32b9d0 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@prsunny
Copy link
Collaborator

prsunny commented Dec 30, 2020

retest this please

1 similar comment
@prsunny
Copy link
Collaborator

prsunny commented Dec 30, 2020

retest this please

prsunny
prsunny previously approved these changes Dec 31, 2020
@prsunny
Copy link
Collaborator

prsunny commented Dec 31, 2020

retest this please

@lgtm-com
Copy link

lgtm-com bot commented Dec 31, 2020

This pull request fixes 1 alert when merging 798d5fe into 7ba4e43 - view on LGTM.com

fixed alerts:

  • 1 for FIXME comment

@prsunny
Copy link
Collaborator

prsunny commented Jan 4, 2021

retest this please

1 similar comment
@prsunny
Copy link
Collaborator

prsunny commented Jan 8, 2021

retest this please

@liushilongbuaa
Copy link
Contributor

retest vs please

@prsunny prsunny merged commit a960e2e into sonic-net:master Jan 8, 2021
DavidZagury pushed a commit to DavidZagury/sonic-swss that referenced this pull request Mar 4, 2021
* [Orchagent]: FdbOrch changes for EVPN VXLAN
* Support for EVPN advertised mac routes
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
…ic-net#1275)

* [config/console][consutil] Support enable/disable console switch
* Changed the key to aligned with feature table style

Signed-off-by: Jing Kan [email protected]
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.

10 participants