-
Notifications
You must be signed in to change notification settings - Fork 746
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
qos-sai:dwrr:cisco-8000:Handle non-multiasic part as well for the dshell-script change. #16315
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@rraghav-cisco it fails on t1 tesbed. Can you test it locally in your t0/t1 testbed?
|
@rraghav-cisco the dwrr test on this testbed was constantly passing. |
@sdszhang , Understood.. We are able to recreate the problem internally. Pls file a JIRA to track this. Thanks. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@sdszhang : I have updated the PR for single and mAsic conditions. Pls check.
|
tests/saitests/py3/sai_qos_tests.py
Outdated
if err != "" and out == "": | ||
raise RuntimeError("cmd({}) might have failed in the DUT. Error:{}".format(cmd, err)) | ||
if err and out == []: | ||
if "Invalid value for " in " ".join(err): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using fallback mechanism, can you check for the hwsku? you may need to add a function is_hwsku_multi_asic() for this.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
dut.docker_copy_to_all_asics( | ||
container_name=f"syncd{asic}", | ||
container_name=dest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function copy_and_run_set_cir_script_cisco_8000() just upload dshell script to dut, not really run this script. why do we call it copy_and_run__ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is asic value not 'none' on single asic platforms? If the value is none, would it also solve the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is asic value not 'none' on single asic platforms? If the value is none, would it also solve the issue?
@yxieca : Yes, it was "0" for single-asic. That is why the original code failed. If it were "None" the original code would have worked.
"show platform summary | egrep 'ASIC Count' | awk -F: '{print $2}'") | ||
cmd_opt = "-n asic{}".format(self.test_params['dst_asic_index']) | ||
if out[0].strip() == "1": | ||
cmd_opt = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we set scheduler to 5Gpps during the wrr test, but why don't we revert scheduler to default value after test complete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XuChen-MSFT : That was an artifact from earlier logic. Earlier this function used to copy and run, but now only copy. The run is done in the ptf code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XuChen-MSFT : Its not needed, since tx_enable is taking care of that part. It resets the scheduler as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update the function name of "copy_and_run_set_cir_script_cisco_8000()" to reflect its real behaviors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XuChen-MSFT this is updated now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
@rraghav-cisco PR conflicts with 202411 branch |
@mssonicbld : This PR is incorporated into the 202405 PR:#16199 |
Description of PR
Summary:
Fixes # 16314
Type of change
Back port request
Approach
What is the motivation for this PR?
The recent commit for dwrr works for multi-asic only. This PR handles the single asic platforms.
How did you do it?
Making sure to use the correct container to use for single asic tests.
How did you verify/test it?
Not done yet. @kevinskwang , @yejianquan : Can you pls try this out ? I will try it when I get the testbed access.
Any platform specific information?
For cisco-8000 only.