-
Notifications
You must be signed in to change notification settings - Fork 529
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
[DPB/VLAN] Add test VS cases and fix FDB flush issues #1242
[DPB/VLAN] Add test VS cases and fix FDB flush issues #1242
Conversation
9dbd861
to
47793d4
Compare
retest this please |
please use the new test assert api. check with @daall 's recent pr |
Could you please provide me the PR# and if possible elaborate on what API you are talking abiut |
47793d4
to
90141af
Compare
This pull request introduces 10 alerts when merging 90141af into bfaadcd - view on LGTM.com new alerts:
|
fe5eeb0
to
1c656d9
Compare
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments. I mostly focused on the testing code. @prsunny can you help verify the FDB changes?
tests/test_port_dpb_vlan.py
Outdated
assert(p.exists_in_asic_db() == True) | ||
|
||
self.dvs_vlan.remove_vlan_member(vlan, p.get_name()) | ||
time.sleep(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding the flow correctly then I think this sleep can be replaced by waiting for the vlan member to be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sleep was NOT there to wait for VLAN member to be deleted. Instead, when we remove port from VLAN member, port dependency gets removed and the ports gets deleted. Then we can verify that port is deleted from ASIC DB. This was correct. But anyways, I have introduced "not_exists_in_asic_db()" method in port_dpb.py class which uses the wai_for_deleted_entry() function to verify that port is deleted.
Since we are here, I have a questions for you. I saw that you have introduced field, value match method (wait_for_field_match()). Can we introduce one for just field. Why I am asking is:
Suppose I want to verify that port is added to to PORT_NAME_MAP key in counter DB, I can use it. Similary wait_for_field_delete_entry().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work! I'm wondering if it might be cleaner to support "don't cares" or even wild carding rather than adding another method.
7cd3814
to
f4023f6
Compare
This pull request introduces 1 alert when merging f4023f6 into 727a518 - view on LGTM.com new alerts:
|
f4023f6
to
ed7a3a5
Compare
|
||
SWSS_LOG_INFO("Flushing FDB bridge_port_oid: 0x%" PRIx64 ", and bvid_oid:0x%" PRIx64 ".", bridge_port_oid, vlan_oid); | ||
|
||
rv = sai_fdb_api->flush_fdb_entries(gSwitchId, (uint32_t)attrs.size(), attrs.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need CRM counters update. Can you check?
orchagent/portsorch.cpp
Outdated
@@ -3983,3 +3997,22 @@ void PortsOrch::getPortSerdesVal(const std::string& val_str, | |||
lane_values.push_back(lane_val); | |||
} | |||
} | |||
|
|||
void PortsOrch::flushFDBEntries(sai_object_id_t bridge_port_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is flushFDBEntries
in portsorch
? IMO, this should be handled in fdborch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I cherrypicked these changes. Is it ok if I handle it in next PR or you insist on doing it in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to handle it in this PR itself. The functionality doesn't fit correctly in portsorch
orchagent/portsorch.cpp
Outdated
attr.id = SAI_FDB_FLUSH_ATTR_BRIDGE_PORT_ID; | ||
attr.value.oid = bridge_port_id; | ||
attrs.push_back(attr); | ||
rv = sai_fdb_api->flush_fdb_entries(gSwitchId, (uint32_t)attrs.size(), attrs.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing CRM counter update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are updated on receiving flush notification in function storeFdbEntryState()
4bed552
to
e9be327
Compare
Could you fix the build? It is blocking vs and also LGTM checker. |
Sure, will do it tonight.
…On Fri, May 29, 2020 at 12:59 PM Qi Luo ***@***.***> wrote:
Could you fix the build? It is blocking vs and also LGTM checker.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1242 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIWEL5UTX3HCBJGG27OJF3TRUAHYLANCNFSM4MCUM3MQ>
.
|
fb3994a
to
63fc891
Compare
orchagent/portsorch.cpp
Outdated
|
||
gNeighOrch->ifChangeInformNextHop(alias, | ||
(operation_status == "down") ? | ||
false : true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the same as operation_status != "down"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
orchagent/portsorch.cpp
Outdated
{ | ||
gNeighOrch->ifChangeInformNextHop(alias, true); | ||
operation_status_changed = (string_oper_status.at(operation_status) != lag.m_oper_status) ? | ||
true : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true : false; [](start = 50, length = 14)
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I dint get what to remove. Could you please elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove ? true : false
. The original bool statement is good enough.
In reply to: 433554653 [](ancestors = 433554653)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As comments
* Flush fdb entries when port/LAG is operational down Signed-off-by: Zhenggen Xu <zxu@linkedin.com> * Added vs-test cases, fixed one bug and addressed review feedback Signed-off-by: Zhenggen Xu <zxu@linkedin.com>
* FIX bugs and add VLAN scale test * Code-review comments addressed * Addressed code-review comments Co-authored-by: Vasant <vapatil@linkedin.com>
63fc891
to
8e4ab1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please also check with other reviewers.
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nitpick, rest LGTM.
tests/dvslib/dvs_vlan.py
Outdated
def create_port_channel(self, lag_id, admin_status="up", mtu="1500"): | ||
lag = "PortChannel{}".format(lag_id) | ||
lag_entry = {"admin_status": admin_status, "mtu": mtu} | ||
self.config_db.create_entry("PORTCHANNEL", lag, lag_entry) | ||
|
||
def remove_port_channel(self, lag_id): | ||
lag = "PortChannel{}".format(lag_id) | ||
self.config_db.delete_entry("PORTCHANNEL", lag) | ||
|
||
def create_port_channel_member(self, lag_id, interface): | ||
member = "PortChannel{}|{}".format(lag_id, interface) | ||
member_entry = {"NULL": "NULL"} | ||
self.config_db.create_entry("PORTCHANNEL_MEMBER", member, member_entry) | ||
|
||
def remove_port_channel_member(self, lag_id, interface): | ||
member = "PortChannel{}|{}".format(lag_id, interface) | ||
self.config_db.delete_entry("PORTCHANNEL_MEMBER", member) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were moved to dvs_lag so can we delete them from here?
retest this please |
Thank you all!
…On Wed, Jun 10, 2020 at 3:50 PM Prince Sunny ***@***.***> wrote:
Merged #1242 <#1242> into master.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1242 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIWEL5Q35O6KB374OJO26WDRWAE3ZANCNFSM4MCUM3MQ>
.
|
…mponent fw updates (sonic-net#1242) - What I did Add the support for the automatic platform component fw updates - How I did it Add fwutil auto_update interfaces - How to verify it Work with dell to verify the auto_update interfaces with ssd firmware update. - Previous command output (if the output of a command-line utility has changed) - New command output (if the output of a command-line utility has changed) New added command outputs are available in the following HLD: sonic-net/SONiC#648
What I did
Added DPB-VLAN dependency test cases
Fixed FDB flush issues encountered during test
Why I did it
When port is a member of VLAN and we do dynamic port breakout, we identify the dependency and remove the port from VLAN and then delete the port.
But, when we remove the port from VLAN, couple of things happen or should happen
This will enable/allow us to delete the port.
While testing we hit the following issue:
Bug: Even though we remove the port from all VLANS, we were unable to delete the port, as associated bridge port OID was still present
Root cause: The bridge port OID associated with port was NOT deleted because, associated FDB entries were NOT flushed.
Solution:
We are going to clear FDB entries in two cases:
Firstly, when port is shutdown, we are going to call SAI FDB flush API by passing bridge port OID. Note that we will do this only if the port has valid (NON NULL) bridge port OID. Thats because if you call SAI FDB flush without any valid bridge port OID or BVID, it will flush all FDB entries.
Secondly, whenever we remove the port from a VLAN, we are going to call SAI FDB flush API with valid <bridge port OID, BVID>
With both these fixes in place, irrespective of the order we will clean the FDB entries, delete bridge port OID and then delete the port. The order I am talking here is, whether the port is shutdown first or its removed from all VLANs.
How I verified it
Ran VS test cases
Details if related
Test case results after refactoring code based on newly introduced dvslib.