Skip to content

Add ECN WRED configuration test#384

Merged
yxieca merged 1 commit intosonic-net:masterfrom
andriymoroz-mlnx:wred_config_test
Jan 31, 2018
Merged

Add ECN WRED configuration test#384
yxieca merged 1 commit intosonic-net:masterfrom
andriymoroz-mlnx:wred_config_test

Conversation

@andriymoroz-mlnx
Copy link
Contributor

@andriymoroz-mlnx andriymoroz-mlnx commented Dec 11, 2017

Signed-off-by: Andriy Moroz [email protected]

Description of PR

Tests for the QoS/ECN configuration and ecnconfig utility

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Approach

How did you verify/test it?
ansible-playbook test_sonic.yml -i inventory --limit {DUT_NAME} --become --tags ecn_wred

Any platform specific information?
Platform independent. Requires SAI support set for ECN/WRED parameters

Supported testbed topology if it's a new test case?
Topology independent

Documentation

Test plan:
QoS configuration in Config DB. ECN WRED configuration utility test plan

@andriymoroz-mlnx
Copy link
Contributor Author

Requires sonic-net/sonic-buildimage#1226 to work properly on Mellanox switch

Copy link
Collaborator

Choose a reason for hiding this comment

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

s/ojgects/objects ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add an access point of this test case in test by test name infrastructure?

I think you need to change following file:

Thanks,
Ying

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if this is a big deal or not. But we do have some devices doesn't yet have qos config.

Failing test on a dut which doesn't have qos config seems to be too harsh?

Is there a way to early exit instead of failing the test?

Please don't spend too much time on this question. It is a very minor issue. I am ok with it not fixed.

Copy link
Collaborator

@yxieca yxieca left a comment

Choose a reason for hiding this comment

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

The test looks pretty good to me.

A few minor issues has been noted. Please take a look.

@andriymoroz-mlnx andriymoroz-mlnx changed the title Add QoS configuration and ECN WRED get/set tests Add ECN WRED configuration test Jan 31, 2018
@yxieca yxieca merged commit 2e63fcd into sonic-net:master Jan 31, 2018
sdszhang pushed a commit to sdszhang/sonic-mgmt that referenced this pull request Jun 14, 2025
…os (sonic-net#384)

<!--
Please make sure you've read and understood our contributing guidelines;
https://github.com/sonic-net/SONiC/blob/gh-pages/CONTRIBUTING.md

Please provide following information to help code review process a bit easier:
-->
### Description of PR
<!--
- Please include a summary of the change and which issue is fixed.
- Please also include relevant motivation and context. Where should reviewer start? background context?
- List any dependencies that are required for this change.
-->

Summary:
Fixes # (issue)

### Type of change

<!--
- Fill x for your type of change.
- e.g.
- [x] Bug fix
-->

- [ ] Bug fix
- [ ] Testbed and Framework(new/improvement)
- [ ] New Test case
 - [ ] Skipped for non-supported platforms
- [ ] Test case improvement

### Back port request
- [ ] 202205
- [ ] 202305
- [ ] 202311
- [ ] 202405
- [ ] 202411
- [ ] 202505

### Approach
#### What is the motivation for this PR?
The ansible-playbook checks container's reachability on the wrong host. If the number of neighbors is large, it can waste a lot of time.

#### How did you do it?
Fix the wrong host from ansible executing host to container host.

#### How did you verify/test it?
Check it locally.

#### Any platform specific information?

#### Supported testbed topology if it's a new test case?

### Documentation
<!--
(If it's a new feature, new test case)
Did you update documentation/Wiki relevant to your implementation?
Link to the wiki page?
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants