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

[fdborch] support mac update #877

Merged
merged 8 commits into from
Aug 1, 2019
Merged

Conversation

shine4chen
Copy link
Contributor

@shine4chen shine4chen commented May 5, 2019

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.

shine4chen and others added 2 commits February 13, 2019 17:59
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
@shine4chen shine4chen changed the title support macFdborch [fdborch]support mac update May 5, 2019
@shine4chen shine4chen changed the title [fdborch]support mac update [fdborch] support mac update May 5, 2019
@prsunny
Copy link
Collaborator

prsunny commented May 6, 2019

@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.

Copy link
Collaborator

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

As comments

@lguohan
Copy link
Contributor

lguohan commented May 7, 2019

please add vs test cases here.

@jianjundong
Copy link

@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
Copy link
Collaborator

prsunny commented May 8, 2019

@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
Copy link

@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
Copy link
Collaborator

prsunny commented May 8, 2019

@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
Copy link

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

OK, we will do this ASAP. thanks.

leoli-nps and others added 3 commits June 2, 2019 08:11
Signed-off-by: leo.li <leo.li@nephosinc.com>
Signed-off-by: shine.chen <shine.chen@nephosinc.com>
("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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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, we have add it. Pls review

Signed-off-by: leo.li <leo.li@nephosinc.com>
@leoli-nps
Copy link
Contributor

The vstest has been updated, please review. Thanks!

@shine4chen
Copy link
Contributor Author

@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
Copy link
Collaborator

prsunny commented Jun 18, 2019

@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
Copy link
Contributor

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

@jianjundong
Copy link

@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
Copy link

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
Copy link
Contributor

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 <leo.li@nephosinc.com>
@leoli-nps
Copy link
Contributor

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.

@@ -483,6 +475,11 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const
attr.value.s32 = SAI_PACKET_ACTION_FORWARD;
attrs.push_back(attr);

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

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

prsunny commented Jul 10, 2019

@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
Copy link

jianjundong commented Jul 11, 2019

@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
Copy link
Contributor Author

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
Copy link
Contributor

lguohan commented Jul 31, 2019

retest this please

@lguohan lguohan merged commit 9143018 into sonic-net:master Aug 1, 2019
@shine4chen shine4chen deleted the fdborch branch August 1, 2019 07:55
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
* [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
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
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.

8 participants