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

Implement rfc4363 FdbUpdater for lag inside vlan #203

Merged
merged 9 commits into from
Mar 15, 2021

Conversation

qiluo-msft
Copy link
Contributor

- What I did
Currently if there are lags in vlan, and FDB entries with lag will not visible in FdbUpdater.

- How I did it
Implement rfc4363 FdbUpdater for lag inside vlan

- How to verify it
Test in DUT, and add unit test.

@qiluo-msft qiluo-msft marked this pull request as ready for review March 15, 2021 01:57
db_conn.connect(COUNTERS_DB)
lag_sai_map = db_conn.get_all(COUNTERS_DB, "COUNTERS_LAG_NAME_MAP")
for name, sai_id in lag_sai_map.items():
sai_id_key = get_sai_id_key(db_conn.namespace, sai_id.lstrip("oid:0x"))
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi Mar 15, 2021

Choose a reason for hiding this comment

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

Can we use if_lag_name_map from port_util.if_lag_name_map() ? #Resolved

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 function get_interface_oid_map returns both port and lag. So not fit well.


In reply to: 594597081 [](ancestors = 594597081)

@@ -31,7 +32,7 @@ def fdb_vlanmac(self, fdb):
self.bvid_vlan_map[fdb["bvid"]] = vlan_id
else:
return None
if not isinstance(vlan_id, str):
if not isinstance(vlan_id, bytes) and not isinstance(vlan_id, str):
Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi Mar 15, 2021

Choose a reason for hiding this comment

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

why is this change required? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it in above line, and this is not required anymore.


In reply to: 594597739 [](ancestors = 594597739)

Copy link
Contributor

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

@@ -56,6 +61,20 @@ def test_getnextpdu(self):
self.assertEqual(value0.type_, ValueType.INTEGER)
self.assertEqual(value0.data, 113)

def test_getnextpdu_lag(self):
Copy link
Contributor

@abdosi abdosi Mar 15, 2021

Choose a reason for hiding this comment

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

can we add same case for multi-asic also . tests/namespace/test_fdb.py

Copy link
Contributor

Choose a reason for hiding this comment

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

we currently skip the tests/namespace/test_fdb.py as fdb MIB is not modified to support for mulit-asic as it is a T0 topology feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok ideally we should remove test itself then.

abdosi
abdosi previously approved these changes Mar 15, 2021
Copy link
Contributor

@abdosi abdosi left a comment

Choose a reason for hiding this comment

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

LGTM

@qiluo-msft qiluo-msft merged commit c20bf60 into sonic-net:master Mar 15, 2021
@qiluo-msft qiluo-msft deleted the qiluo/laginvlan branch March 15, 2021 21:28
qiluo-msft added a commit that referenced this pull request Mar 15, 2021
- What I did
Same as #203
But target on 201911 branch.
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.

3 participants