Skip to content
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

Add verification for interface counters on sanity check #1165

Merged
merged 2 commits into from
Mar 5, 2020

Conversation

yvolynets-mlnx
Copy link
Contributor

@yvolynets-mlnx yvolynets-mlnx commented Oct 16, 2019

Description of PR

Do not merge until sonic-net/sonic-buildimage#2738 will be fixed.

Summary:
Extended pre and post test sanity check with:

  • Wait for counters to become enabled
  • Get output of "show interfaces counters" command and verify that "RX_BPS", "TX_BPS", "RX_UTIL", "TX_UTIL" rows does not contain "N/A". If any row contains "N/A", test will fail and this information will be displayed in the fail message.

Type of change

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

Approach

How did you do it?

Developed new common purpose ansible library - counter_facts.py. Its purpose is to gather interface counters. This library can be extendent to gather any kind of counters.
Developed "interface_counters.yml" script which calls "counter_facts" module and perform verification that "N/A" are absent.
Updated "interface.yml" to call "interface_counters.yml".

How did you verify/test it?

Tested on the local setup.

Any platform specific information?

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

Documentation

@liat-grozovik
Copy link
Collaborator

@wangxin can you please review as well?

@@ -56,3 +56,6 @@
- name: Verify VLAN interfaces are up correctly
assert: { that: "'{{ ansible_interface_facts[item]['active'] }}' == 'True'" }
with_items: "{{ minigraph_vlans.keys() }}"

- name: Verify interfaces counters
include: interface_counters.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should this new test as pytest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"I wonder if we should this new test as pytest?" - in my opinion we should add this in pytest and other features which are executed during run ansible test cases.

Here in ansible approach, this specific verification is a part of sanity check which performs before and after each test case. Currently in pytest approach there is no such functionality.

I think first we should understand and agree:
what steps should be done before and after each test case.
If current ansible logic is enough and its not redundant then it can be ported to the pytest.

Then this verification and more others will be executed per test case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lguohan is this closed and we can merge?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lguohan I agree with @yvolynets-mlnx here, I think we should add this check and port it back to pytest with the rest of the interface tests.

@liat-grozovik liat-grozovik requested a review from daall February 11, 2020 07:35
@lguohan lguohan merged commit 6a79b7d into sonic-net:master Mar 5, 2020
@mykolaf
Copy link
Contributor

mykolaf commented Mar 6, 2020

@lguohan Sorry, this was not meant to be merged until sonic-net/sonic-buildimage#2738 is resolved.
Could you please revert?

lguohan pushed a commit that referenced this pull request Mar 19, 2020
Summary: temporally disable interface counters check to avoid test case failures until:

Issue sonic-net/sonic-buildimage#2738 will be fixed
PR #1165 will be merged

Signed-off-by: Yuriy Volynets <[email protected]>
@lguohan
Copy link
Contributor

lguohan commented Mar 24, 2020

looks like this pr cannot be merged automatically, can you generate a pr to manually revert it?

@wangxin
Copy link
Collaborator

wangxin commented Mar 25, 2020

We have created a PR to temporarily disable interface counter sanity check by commenting out a few lines in ansible/roles/test/tasks/interface.yml: #1467
No need to revert this one now. When it's ready, just un-comment those lines should be fine.

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.

6 participants