Skip to content

Skip all admin down ports for non-Mellanox device #6433

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.

Sign up for GitHub

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

Merged
Blueve merged 2 commits into from
Oct 2, 2022
Merged

Skip all admin down ports for non-Mellanox device #6433

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c36013a
remove asic type and speed check, skip all admin down ports on broadc…
lipxu Sep 28, 2022
e8ee2b5
update
lipxu Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions tests/qos/test_buffer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2481,15 +2481,8 @@ def _check_port_buffer_info_and_return(duthost, table, ids, port, expected_profi
else:
if is_mellanox_device(duthost):
buffer_items_to_check = buffer_items_to_check_dict["down"][key_name]
elif is_broadcom_device(duthost) and (asic_type in ['td2', 'td3'] or speed <= '10000'):
Copy link
Copy Markdown
Collaborator

@Blueve Blueve Sep 30, 2022

Choose a reason for hiding this comment

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

Why remove is_broadcom_device condition? We also have other platform such as marvell , seems like this change will also cause skipping happening on other platforms, not just broadcom.

Copy link
Copy Markdown
Contributor

@neethajohn neethajohn Sep 30, 2022

Choose a reason for hiding this comment

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

The special handling is needed only for mellanox. buffermgr will not attach any profiles for admin down ports on other platforms

Copy link
Copy Markdown
Collaborator

@Blueve Blueve Oct 1, 2022

Choose a reason for hiding this comment

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

@lipxu could you update your PR title to Skip all admin down ports for non-Mellanox device?

Copy link
Copy Markdown
Contributor Author

@lipxu lipxu Oct 1, 2022

Choose a reason for hiding this comment

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

Sure, @Blueve , have updated the title.
Updated the commit last time, but forgot to update the title, sorry for the confusion.

buffer_items_to_check = [(None, None, None)]
else:
if key_name == KEY_2_LOSSLESS_QUEUE:
buffer_items_to_check = [('BUFFER_PG_TABLE', '3-4', profile_wrapper.format(expected_profile))]
else:
buffer_items_to_check.extend(
[('BUFFER_PG_TABLE', '2-4', profile_wrapper.format(expected_profile)),
('BUFFER_PG_TABLE', '6', profile_wrapper.format(expected_profile))])
buffer_items_to_check = [(None, None, None)]

for table, ids, expected_profile in buffer_items_to_check:
logging.info("Checking buffer item {}:{}:{}".format(table, port, ids))
Expand Down