[QoS] Support dynamic headroom calculation for Barefoot platforms#6151
[QoS] Support dynamic headroom calculation for Barefoot platforms#6151MariuszStachura wants to merge 20 commits intosonic-net:masterfrom
Conversation
|
|
Signed-off-by: Mariusz Stachura <[email protected]>
|
This pull request introduces 2 alerts when merging 8381a27 into 2e40f50 - view on LGTM.com new alerts:
|
|
/easycla |
Do not use PG 5, but stick with 6 as it was enabled. Also fix check_pool_size for barefoot case.
MariuszStachura
requested review from
stephenxs
and removed request for
neethajohn
|
This pull request introduces 2 alerts when merging 6a37963 into 00b9f3e - view on LGTM.com new alerts:
|
tests/qos/test_buffer.py
Outdated
| original_cable_length = duthost.shell('redis-cli -n 4 hget "CABLE_LENGTH|AZURE" {}'.format(port_to_test))['stdout'] | ||
| original_pool_size = duthost.shell('redis-cli hget BUFFER_POOL_TABLE:ingress_lossless_pool size')['stdout'] | ||
| if DEFAULT_OVER_SUBSCRIBE_RATIO: | ||
| if duthost.facts['asic_type'] == 'barefoot': |
There was a problem hiding this comment.
Maybe we can introduce a method that returns whether the shared headroom pool is enabled. By doing so the logic can be simplified.
def is_shared_headroom_pool_enabled(duthost):
return DEFAULT_OVER_SUBSCRIBE_RATIO or duthost.facts['asic_type'] in ['barefoot']
There was a problem hiding this comment.
I removed this condition as it is not needed anymore. Thanks!
Co-authored-by: Stephen Sun <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
MariuszStachura
left a comment
MariuszStachura
left a comment
There was a problem hiding this comment.
Do not disable loganalyzer
It was failing for me in this test, but probably it should not be disabled.
|
Sorry @stephenxs, I did not dismissed your review, I was just testing github.dev and edit this file with that and this caused this automatic dismissal. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
no problem. this is the way it works. just let me know once you finish all the changes. |
|
@MariuszStachura, there are a lot of changes in this PR touching multiple things. please split this into 2 PRs - one for qos sai test alone and other for dynamic headroom calculation |
Ok, this one is dynamic headroom calculation only, pasting the result:
This PR relates on: Thank you! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
| from tests.common.utilities import check_qos_db_fv_reference_with_table | ||
| from tests.common.utilities import skip_release | ||
| from tests.common.dualtor.dual_tor_utils import is_tunnel_qos_remap_enabled, dualtor_ports # lgtm[py/unused-import] | ||
| from tests.common.dualtor.dual_tor_utils import is_tunnel_qos_remap_enabled |
There was a problem hiding this comment.
add back the function 'dualtor_ports'
There was a problem hiding this comment.
Hi, this was to fix:
tests/qos/test_buffer.py:23:1: F401 'tests.common.dualtor.dual_tor_utils.dualtor_ports' imported but unused
dualtor_ports is used inside:
def test_buffer_deployment(duthosts, rand_one_dut_hostname, conn_graph_facts, tbinfo, dualtor_ports):
and there it is passed as an argument.
| pytest.skip('Speed, MTU and cable length matches the default value, nothing to test, skip') | ||
| expected_profile = make_expected_profile_name(speed_to_test, cable_len_to_test) | ||
| if duthost.shell('redis-cli hget BUFFER_PROFILE_TABLE:{}'.format(expected_profile))['stdout']: | ||
| if duthost.shell('redis-cli keys BUFFER_PROFILE_TABLE:{}'.format(expected_profile))['stdout'] != '': |
There was a problem hiding this comment.
why is this changed to 'keys'?
There was a problem hiding this comment.
It was failing for me and I thought it must have been a mistake:
admin@sonic:~$ redis-cli keys BUFFER_PROFILE_TABLE:expected_profile
(empty array)
admin@sonic:~$ redis-cli hget BUFFER_PROFILE_TABLE:expected_profile
(error) ERR wrong number of arguments for 'hget' command
hget always failed
tests/qos/test_buffer.py
Outdated
| buffer_table_up[KEY_2_LOSSLESS_QUEUE][1] = ('BUFFER_QUEUE_TABLE', '0-2', '[BUFFER_PROFILE_TABLE:]') | ||
| buffer_table_up[KEY_2_LOSSLESS_QUEUE][3] = ('BUFFER_QUEUE_TABLE', '5-6', '[BUFFER_PROFILE_TABLE:]') |
There was a problem hiding this comment.
The profile name is missing here
There was a problem hiding this comment.
I think this was intentional for BUFFER_QUEUE_TABLE in our case, thank you for thorough review, I'll try to fix pre-commit issues locally first to not trigger jobs again and again.
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
|
This pull request fixes 4 alerts when merging b3a6b2a into 79ffe4c - view on LGTM.com fixed alerts:
|
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
|
@neethajohn I did what I could regarding the pre-commit issues. Only issues left are about lines being too long, but I couldn't fix them in a nice way. |
|
The pre-commit check detected issues in the files touched by this pull request. For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
|
This pull request fixes 9 alerts when merging fb31e77 into 79ffe4c - view on LGTM.com fixed alerts:
|
|
Hi @StormLiangMS here are the actual changes: |
|
Hello @XuChen-MSFT, could we get these tests merged? Many thanks |
6 participants
Signed-off-by: Mariusz Stachura [email protected]
What I did
Adding the dynamic headroom calculation support for Barefoot platforms.
Why I did it
Enabling dynamic mode for barefoot case.
How I verified it
The community tests are adjusted and pass.
Details if related