[fdborch] support mac update by shine4chen · Pull Request #877 · sonic-net/sonic-swss

shine4chen · 2019-05-05T06:38:19Z

What I did
support mac update on fdborch

Why I did it
Some apps such as MCLAG need update mac address

How I verified it
test it on nephos lab

Details if related
If the FDB entry is already exist when the event is ADD, remove the entry first, then add the entry.

pull from origin

Signed-off-by: shine.chen <[email protected]>

prsunny · 2019-05-06T20:35:13Z

@shine4chen , this is a mac-move case. Can we handle it by setting the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID if there is a change? The observers are able to handle port change, so modifications are only required in fdborch.cpp. We don't want #if 0 in code.

prsunny

As comments

lguohan · 2019-05-07T06:55:09Z

please add vs test cases here.

jianjundong · 2019-05-07T08:30:04Z

@shine4chen , this is a mac-move case. Can we handle it by setting the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID if there is a change? The observers are able to handle port change, so modifications are only required in fdborch.cpp. We don't want #if 0 in code.

If we set the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID when the port is a change, this will be a mac-move case. In function FdbOrch::update(), the event SAI_FDB_EVENT_MOVE is equal to SAI_FDB_EVENT_AGED, this will cause MAC deleting.

prsunny · 2019-05-08T00:03:10Z

@jianjundong , so when were remove and add, we also get SAI_FDB_EVENT_AGED and SAI_FDB_EVENT_LEARNED as well? If so, then i'm thinking we may have some issue since you are now removing and adding and SAI events can come later.

jianjundong · 2019-05-08T07:10:26Z

@jianjundong , so when were remove and add, we also get SAI_FDB_EVENT_AGED and SAI_FDB_EVENT_LEARNED as well? If so, then i'm thinking we may have some issue since you are now removing and adding and SAI events can come later.

We don't find the result of test is not correctly. Although SAI events may be come later, SAI_FDB_EVENT_LEARNED is the last event, in theory it will add the MAC into ASIC_DB by syncd.

prsunny · 2019-05-08T20:19:49Z

@jianjundong , so when were remove and add, we also get SAI_FDB_EVENT_AGED and SAI_FDB_EVENT_LEARNED as well? If so, then i'm thinking we may have some issue since you are now removing and adding and SAI events can come later.

We don't find the result of test is not correctly. Although SAI events may be come later, SAI_FDB_EVENT_LEARNED is the last event, in theory it will add the MAC into ASIC_DB by syncd.

Ok, Can you address the #if 0 part and also the VS test asked by Guohan.

jianjundong · 2019-05-09T00:55:21Z

Ok, Can you address the #if 0 part and also the VS test asked by Guohan.

OK, we will do this ASAP. thanks.

Signed-off-by: leo.li <[email protected]>

Signed-off-by: shine.chen <[email protected]>

…to fdborch

stcheng · 2019-06-04T23:22:58Z

+                     ("SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID", iface_2_bridge_port_id["Ethernet4"]),
+                     ('SAI_FDB_ENTRY_ATTR_PACKET_ACTION', 'SAI_PACKET_ACTION_FORWARD')]
+    )
+    assert ok, str(extra)


could you tear down all the configurations after the test is finished?

yes, we have add it. Pls review

Signed-off-by: leo.li <[email protected]>

leoli-nps · 2019-06-14T07:35:27Z

The vstest has been updated, please review. Thanks!

shine4chen · 2019-06-18T06:36:27Z

@jianjundong , so when were remove and add, we also get SAI_FDB_EVENT_AGED and SAI_FDB_EVENT_LEARNED as well? If so, then i'm thinking we may have some issue since you are now removing and adding and SAI events can come later.
We don't find the result of test is not correctly. Although SAI events may be come later, SAI_FDB_EVENT_LEARNED is the last event, in theory it will add the MAC into ASIC_DB by syncd.

Ok, Can you address the #if 0 part and also the VS test asked by Guohan.
yes, we have fixed '#if 0' part and add vs test cases. Pls help to review it. Thanks

prsunny · 2019-06-18T22:09:12Z

@yxieca to review. I'm not very convinced on the sequencing impact it can have. For e.g, you add entry, insert to cache m_entries, get AGE event due to the new removeFdbEntry which will remove from cache and then LEARN event which will re-add to cache. Please test for any impact with a larger scale and verify the cache/show mac to see if its as expected. Approving from my side.

jipanyang · 2019-06-19T00:10:43Z

Could you add one VS test case that a dynamic FDB entry exists, then perform update on it from appDB.

jianjundong · 2019-06-19T06:38:58Z

@shine4chen , this is a mac-move case. Can we handle it by setting the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID if there is a change? The observers are able to handle port change, so modifications are only required in fdborch.cpp. We don't want #if 0 in code.

@prsunny , If we handle it by setting the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID if there is a change, this will result in an event SAI_FDB_EVENT_MOVE. In current codes, SAI_FDB_EVENT_MOVE is the same as SAI_FDB_EVENT_MOVE. The entry may be not exist because of the time difference between ASIC_DB and ASIC.

jianjundong · 2019-06-19T06:51:39Z

Could you add one VS test case that a dynamic FDB entry exists, then perform update on it from appDB.

@jipanyang , The FDB entry in this VS test case is already 'dynamic'.

jipanyang · 2019-06-19T07:09:15Z

Could you add one VS test case that a dynamic FDB entry exists, then perform update on it from appDB.

@jipanyang , The FDB entry in this VS test case is already 'dynamic'.

@jianjundong I mean the dynamic FDB which comes from syncd, not the one faked in appDB. You may find example in this link: https://github.com/Azure/sonic-swss/blob/master/tests/test_fdb_warm.py#L134

FDB processing currently has caveat of race condition, it may result in SAI failure and orchagent to exit. Would like to check this case.

Signed-off-by: leo.li <[email protected]>

leoli-nps · 2019-06-24T10:06:19Z

Could you add one VS test case that a dynamic FDB entry exists, then perform update on it from appDB.

@jipanyang , The corresponding VS test case has been added, please review.

stcheng · 2019-07-08T23:39:44Z


+    if (m_entries.count(entry) != 0) // we already have such entries
+    {
+        removeFdbEntry(entry);


I would suggest we still keep the original code location to remove the FDB entry at the entrance of this function. Other than this, the PR looks good to me.

prsunny · 2019-07-10T22:45:26Z

@shine4chen , this is a mac-move case. Can we handle it by setting the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID if there is a change? The observers are able to handle port change, so modifications are only required in fdborch.cpp. We don't want #if 0 in code.

If we set the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID when the port is a change, this will be a mac-move case. In function FdbOrch::update(), the event SAI_FDB_EVENT_MOVE is equal to SAI_FDB_EVENT_AGED, this will cause MAC deleting.

This would undergo a change based on this PR. Does that mean, this change needs to be revisited?

jianjundong · 2019-07-11T02:25:58Z

@shine4chen , this is a mac-move case. Can we handle it by setting the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID if there is a change? The observers are able to handle port change, so modifications are only required in fdborch.cpp. We don't want #if 0 in code.
If we set the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID when the port is a change, this will be a mac-move case. In function FdbOrch::update(), the event SAI_FDB_EVENT_MOVE is equal to SAI_FDB_EVENT_AGED, this will cause MAC deleting.

This would undergo a change based on this PR. Does that mean, this change needs to be revisited?

I think PR can fix the bug of MAC move. There is no conflict between PR and this modification. If we set the attribute SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID when the port is a change, this MAC item may be not exist in SAI and ASIC, such as MAC is aged, and this will cause fail to update.

shine4chen · 2019-07-11T02:56:29Z

I think in PR. mac update logic should be same as this PR. Since the dynamic mac addresses may be aged out anytime in asic/sdk. When you use SAI-mac-set API to update port , the mac address in asic chip may disappear and set operation would fail. So using the deletion and add combination is more safe operation.

lguohan · 2019-07-31T17:17:26Z

retest this please

* [Phase 1] Multi ASIC config command changes, db_mgrator.py script updates for handing namespace. * Fixes and comment updates * Comments addressed + added support for user to input the config files per namespace also. * Updates per comments + based on the updated SonicV2Connector/ConfigDBConnector class design * Review comments update. * Help string updated for config save/reload/load

…lure. (sonic-net#877) Python package requests' latest release 2.32.0 has issues with docker package. Use old stable version 2.31.0 instead.

shine4chen and others added 2 commits February 13, 2019 17:59

Merge pull request #1 from Azure/master

9326978

pull from origin

suppoort mac-update on Fdborch

2e2d278

Signed-off-by: shine.chen <[email protected]>

shine4chen changed the title ~~support macFdborch~~ [fdborch]support mac update May 5, 2019

shine4chen changed the title ~~[fdborch]support mac update~~ [fdborch] support mac update May 5, 2019

prsunny requested changes May 6, 2019

View reviewed changes

leoli-nps and others added 3 commits June 2, 2019 08:11

add the corresponding vs-test

b9400fb

Signed-off-by: leo.li <[email protected]>

remove some obsolate code

41578f5

Signed-off-by: shine.chen <[email protected]>

Merge branch 'fdborch' of https://github.com/shine4chen/sonic-swss in…

7515c70

…to fdborch

stcheng suggested changes Jun 4, 2019

View reviewed changes

[vstest] update the corresponding vstest for pr#887

e54b532

Signed-off-by: leo.li <[email protected]>

prsunny approved these changes Jun 18, 2019

View reviewed changes

yxieca reviewed Jun 19, 2019

View reviewed changes

Comment thread orchagent/fdborch.cpp

[vstest] update the corresponding vstest for pr#887

5ee5e31

Signed-off-by: leo.li <[email protected]>

stcheng approved these changes Jul 8, 2019

View reviewed changes

stcheng added the Enhancement ➕ label Jul 8, 2019

jipanyang approved these changes Jul 9, 2019

View reviewed changes

Merge branch 'master' into fdborch

57d7db5

lguohan merged commit 9143018 into sonic-net:master Aug 1, 2019

shine4chen deleted the fdborch branch August 1, 2019 07:55

shine4chen mentioned this pull request Aug 30, 2019

MCLAG feature for SONIC sonic-net/sonic-buildimage#2514

Merged

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023

Install sonic-buildimage libnl3 for AZP (sonic-net#877)

cf1af5f

Conversation

shine4chen commented May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prsunny commented May 6, 2019

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

lguohan commented May 7, 2019

Uh oh!

jianjundong commented May 7, 2019

Uh oh!

prsunny commented May 8, 2019

Uh oh!

jianjundong commented May 8, 2019

Uh oh!

prsunny commented May 8, 2019

Uh oh!

jianjundong commented May 9, 2019

Uh oh!

stcheng Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

shine4chen Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

leoli-nps commented Jun 14, 2019

Uh oh!

shine4chen commented Jun 18, 2019

Uh oh!

prsunny commented Jun 18, 2019

Uh oh!

jipanyang commented Jun 19, 2019

Uh oh!

Uh oh!

jianjundong commented Jun 19, 2019

Uh oh!

jianjundong commented Jun 19, 2019

Uh oh!

jipanyang commented Jun 19, 2019

Uh oh!

leoli-nps commented Jun 24, 2019

Uh oh!

stcheng Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

prsunny commented Jul 10, 2019

Uh oh!

jianjundong commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shine4chen commented Jul 11, 2019

Uh oh!

lguohan commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

shine4chen commented May 5, 2019 •

edited

Loading

jianjundong commented Jul 11, 2019 •

edited

Loading