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

[fast-reboot] Fix dump script to support PortChannels in a VLAN group #1393

Merged
merged 7 commits into from
Apr 2, 2021
Merged

[fast-reboot] Fix dump script to support PortChannels in a VLAN group #1393

merged 7 commits into from
Apr 2, 2021

Conversation

shlomibitton
Copy link
Contributor

@shlomibitton shlomibitton commented Jan 31, 2021

- What I did
Add PortChannels to the list of interfaces (port_id_2_iface) to support FDB dump for PortChannel in a VLAN group.
Fixes sonic-net/sonic-buildimage#4793

- How I did it

  • Get LAG ID from the DB.
  • Find the LAG name from APP DB.
  • Add it to the list of 'port_id_2_iface' to be used.

- How to verify it
Reproduce the issue mentioned on this PR and try to run fast-reboot with this fix.

- Previous command output (if the output of a command-line utility has changed)
Traceback:
src_ifs = {map_mac_ip_per_vlan[vlan_name][dst_mac] for vlan_name, dst_mac, _ in arp_entries}
KeyError: 'b8:59:9f:a8:e2:00'

- New command output (if the output of a command-line utility has changed)
Success .

Fix of IP parsing.

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

this is a new feature. please add unit test support.

@shlomibitton
Copy link
Contributor Author

shlomibitton commented Feb 9, 2021

this is a new feature. please add unit test support.

@lguohan there is no unit-test for fast-reboot or the fast-reboot-dump, I will add a test for it

Adapt the fast-reboot-dump script for unit testing
@shlomibitton
Copy link
Contributor Author

@lguohan added a unit-test, please review
thanks

@shlomibitton
Copy link
Contributor Author

retest this please

@liat-grozovik
Copy link
Collaborator

@tahmed-dev @stepanblyschak can you please review?

liat-grozovik
liat-grozovik previously approved these changes Mar 2, 2021
@liat-grozovik
Copy link
Collaborator

liat-grozovik commented Mar 2, 2021

@tahmed-dev could u please review?
@lguohan any further comments or we can move forward and merge?

stepanblyschak
stepanblyschak previously approved these changes Mar 2, 2021
@tahmed-dev
Copy link
Contributor

@tahmed-dev could u please review?
@lguohan any further comments or we can move forward and merge?

@lguohan , @liat-grozovik ACK.

Copy link
Contributor

@tahmed-dev tahmed-dev left a comment

Choose a reason for hiding this comment

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

Can you please add Why I did it to the PR description?

@dprital
Copy link
Collaborator

dprital commented Mar 10, 2021

@tahmed-dev - It done in order to fix the following issue:
sonic-net/sonic-buildimage#4793
And it is mentioned on "Why I did it" above.
Can you please approve and merge (also to 202012) ?

@liat-grozovik
Copy link
Collaborator

@tahmed-dev comments were addressed, can you please review?

@@ -161,6 +161,7 @@
'swsssdk>=2.0.1',
'tabulate==0.8.2',
'xmltodict==0.12.0',
'deepdiff==5.2.3',
Copy link
Contributor

Choose a reason for hiding this comment

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

deepdiff [](start = 9, length = 8)

This is tests_require?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it is

@lguohan lguohan merged commit e9cfb32 into sonic-net:master Apr 2, 2021
@@ -158,29 +190,37 @@ def get_fdb(db, vlan_name, vlan_id, bridge_id_2_iface):
return fdb_entries, available_macs, map_mac_ip

def generate_fdb_entries(filename):
fdb_entries = []
asic_db = swsssdk.SonicV2Connector(host='127.0.0.1')
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 3, 2021

Choose a reason for hiding this comment

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

swsssdk [](start = 14, length = 7)

swsssdk is deprecated and the import change is alreay merged.

I tested the master latest image plus master latest sonic-utilities

admin@str-msn2700-20:~$ show platform summary
Platform: x86_64-mlnx_msn2700-r0
HwSKU: Mellanox-SN2700-D40C8S8
ASIC: mellanox
ASIC Count: 1
admin@str-msn2700-20:~$ sudo fast-reboot
Failed to run fast-reboot-dump.py. Exit code: 2
admin@str-msn2700-20:~$ sudo fast-reboot-dump.py
admin@str-msn2700-20:~$ echo $?
2

Apr  3 21:59:03.838231 str-msn2700-20 ERR fast-reboot-dump: Got an exception name 'swsssdk' is not defined: Traceback: Traceback (most recent call last):#012  File "/usr/local/bin/fast-reboot-dump.py", line 341, in <module>#012    res = main()#012  File "/usr/local/bin/fast-reboot-dump.py", line 331, in main#012    all_available_macs, map_mac_ip_per_vlan = generate_fdb_entries(root_dir + '/fdb.json')#012  File "/usr/local/bin/fast-reboot-dump.py", line 193, in generate_fdb_entries#012    asic_db = swsssdk.SonicV2Connector(host='127.0.0.1')#012NameError: name 'swsssdk' is not defined
Apr  3 21:59:03.850487 str-msn2700-20 NOTICE admin: fast-reboot failure (12) cleanup ...

Copy link
Contributor

Choose a reason for hiding this comment

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

how can we avoid this in the future? for example, can we remove swsssdk as a build or test dependency so that the unit test will automatically fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

The test case does not cover the problematic code, so the exception is not triggered.

lguohan added a commit that referenced this pull request Apr 4, 2021
bug introduced in #1393

Signed-off-by: Guohan Lu <lguohan@gmail.com>
qiluo-msft pushed a commit that referenced this pull request Apr 5, 2021
#### What I did
This PR #1393 merged after PR: #1392
This caused the first PR to be not aligned with SonicV2Connector change.
This PR motivation is to fix it.
qiluo-msft pushed a commit that referenced this pull request Apr 6, 2021
… [201911] (#1547)

**- What I did**
Add PortChannels to the list of interfaces (port_id_2_iface) to support FDB dump for PortChannel in a VLAN group.
Fixes sonic-net/sonic-buildimage#4793
This PR is the 201911 version of the original PR: #1393

**- How I did it**
* Get LAG ID from the DB.
* Find the LAG name from APP DB.
* Add it to the list of 'port_id_2_iface' to be used.

**- How to verify it**
Reproduce the issue mentioned on this PR and try to run fast-reboot with this fix.

**- Previous command output (if the output of a command-line utility has changed)**
Traceback:
src_ifs = {map_mac_ip_per_vlan[vlan_name][dst_mac] for vlan_name, dst_mac, _ in arp_entries}
KeyError: 'b8:59:9f:a8:e2:00'

**- New command output (if the output of a command-line utility has changed)**
Success .
yxieca pushed a commit that referenced this pull request Apr 8, 2021
bug introduced in #1393

Signed-off-by: Guohan Lu <lguohan@gmail.com>
yxieca pushed a commit that referenced this pull request Apr 8, 2021
…#1393)

Add PortChannels to the list of interfaces (port_id_2_iface) to support FDB dump for PortChannel in a VLAN group.

Fix sonic-net/sonic-buildimage#4793

- How I did it

- Get LAG ID from the DB.
- Find the LAG name from APP DB.
- Add it to the list of 'port_id_2_iface' to be used.

- How to verify it
Reproduce the issue mentioned on this PR and try to run fast-reboot with this fix.

- Previous command output (if the output of a command-line utility has changed)
Traceback:
src_ifs = {map_mac_ip_per_vlan[vlan_name][dst_mac] for vlan_name, dst_mac, _ in arp_entries}
KeyError: 'b8:59:9f:a8:e2:00'

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
yxieca pushed a commit that referenced this pull request Apr 8, 2021
#### What I did
This PR #1393 merged after PR: #1392
This caused the first PR to be not aligned with SonicV2Connector change.
This PR motivation is to fix it.
gitsabari pushed a commit to gitsabari/sonic-utilities that referenced this pull request Jun 15, 2021
…1546)

#### What I did
This PR sonic-net#1393 merged after PR: sonic-net#1392
This caused the first PR to be not aligned with SonicV2Connector change.
This PR motivation is to fix it.
stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Apr 28, 2022
 [sonic-swsss] Fix the issue of field "next_hop_ip" not getting updated
 in state DB in ERSPAN Mirror (sonic-net#1375)
[vlanmgr] Support Jumbo Frame By Default (sonic-net#1393)
[fec] added logic that put port down before applying fec
onfiguration (sonic-net#1399)
 [fec] Get FEC mode when port is already admin down (sonic-net#1403)
 Refine getDbId() calling to fix build after swss-common change
 (sonic-net#1245)
malletvapid23 added a commit to malletvapid23/Sonic-Utility that referenced this pull request Aug 3, 2023
#### What I did
This PR sonic-net/sonic-utilities#1393 merged after PR: sonic-net/sonic-utilities#1392
This caused the first PR to be not aligned with SonicV2Connector change.
This PR motivation is to fix it.
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.

fast-reboot fails when LAG has VLAN membership
9 participants