Incremental QOS config updater test update #5028
Conversation
isabelmsft
requested review from
qiluo-msft,
wen587,
wangxin,
neethajohn and
ghooo
|
This pull request introduces 3 alerts and fixes 4 when merging 662248c into d07e1d9 - view on LGTM.com new alerts:
fixed alerts:
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| """ | ||
| config_db_json = json.loads(duthost.shell("show runningconfig all")["stdout"]) | ||
| device_neighbor_metadata = config_db_json["DEVICE_NEIGHBOR_METADATA"] |
There was a problem hiding this comment.
You may reuse duthost.config_facts
ref: https://github.com/Azure/sonic-mgmt/blob/3a9720789b4cd4527865b0053795e98c14ccfd2c/tests/test_nbr_health.py#L81 #Closed
|
|
||
| expected_profile = 'pg_lossless_{}_{}_profile'.format(port_speed, cable_length) | ||
|
|
||
| xoff = int(duthost.shell('redis-cli hget "BUFFER_PROFILE_TABLE:{}" xoff'.format(expected_profile))['stdout']) |
| buffer_pool = "ingress_lossless_pool" | ||
| elif "egress_lossy_pool" in configdb_field: | ||
| buffer_pool = "egress_lossy_pool" | ||
| oid = duthost.shell('redis-cli -n 2 HGET COUNTERS_BUFFER_POOL_NAME_MAP {}'.format(buffer_pool))["stdout"] |
|
|
||
| pytestmark = [ | ||
| pytest.mark.topology('t0'), | ||
| pytest.mark.topology('t0', 't1'), | ||
| pytest.mark.asic('mellanox') |
There was a problem hiding this comment.
These formula are only applicable to 2700
There was a problem hiding this comment.
@wangxin Is there a marker we can add for 2700?
There was a problem hiding this comment.
Do you only want this test to run on 2700? If so, you can add a pytest_requires condition in your test code entrance to only allow 2700. But why?
There was a problem hiding this comment.
Yes, the Mellanox SPC1 buffer calculations only apply to 2700 devices. @neethajohn may confirm/correct me
And thanks, I added pytest_require to check for 2700 in hwsku
|
Could you fix valid LGTM alerts in the build checkers? |
|
This pull request introduces 1 alert and fixes 4 when merging f5f9744 into e4fe46f - view on LGTM.com new alerts:
fixed alerts:
|
|
|
||
| xoff = int(duthost.shell('redis-cli hget "BUFFER_PROFILE_TABLE:{}" xoff'.format(expected_profile))['stdout']) | ||
| xon = int(duthost.shell('redis-cli hget "BUFFER_PROFILE_TABLE:{}" xon'.format(expected_profile))['stdout']) | ||
| pg_headroom = (xoff + xon) / 1024 |
There was a problem hiding this comment.
LGTM reports a warning here.
The main concern is that the behavior of / is different in python3 vs python2. And we can improve the code to make it behave the same way.
You may find some solutions https://stackoverflow.com/a/15633893/2514803. I prefer from __future__ import division with a int() conversion. #Closed
|
This pull request fixes 4 alerts when merging 0bb712b into e4fe46f - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 729e4f7 into ef91856 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 3f25af979ec5b5a209b7c85d2a1ffbdfa7ffffca into c3bcea8 - view on LGTM.com fixed alerts:
|
|
This pull request introduces 17 alerts and fixes 8 when merging d631e0818f665d1ccce93da2ce1a961cfb22d0b8 into fbafa94 - view on LGTM.com new alerts:
fixed alerts:
|
|
The PR includes so many files due to recently merge. Is it expected? |
isabelmsft
force-pushed
the
qos_update_config_test
branch
2 times, most recently
from
9650672 to
729e4f7
Compare
|
This pull request fixes 4 alerts when merging 0277347 into 377a9e5 - view on LGTM.com fixed alerts:
|
Description of PR
Summary:
Fixes # (issue)
Type of change
Back port request
Approach
What is the motivation for this PR?
End to End test support for Generic Updater apply-patch
Update qos config values to update based on Mellanox SPC1 buffer calculations
How did you do it?
Calculate proper buffer field values based, integrate into config updater JSON patch
How did you verify/test it?
Tested on msn2700 devices, all tests pass
Any platform specific information?
For Mellanox msn2700
Supported testbed topology if it's a new test case?
t0, t1
Documentation