[sanity check]: Add mux simulator sanity check #2862

theasianpianist · 2021-01-26T02:34:03Z

Signed-off-by: Lawrence Lee [email protected]

Description of PR

Summary:
Closes #2753

Type of change

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

Approach

What is the motivation for this PR?

Ensure that the mux cable simulator is fully functional prior to each test run

How did you do it?

Send packets from the ToRs to the PTF, check that the active ToR's packet is received and the standby ToR's is not
- I leveraged the fact that the ping command automatically sends ARP requests when it cannot find the target address, and check for the receipt of these ARP requests on the PTF. The ping command itself is expected to fail.
Send packets from the PTF to the ToRs, check that both ToR's received the packet
- Since there are no packet testing utils installed on the ToRs, I sent GARP packets to the ToRs and checked their neighbor tables to verify receipt.

Note that due to the new sanity check structure, check_simulator_read_side had to be renamed to get_simulator_read_side to prevent pytest from looking for a non-existent fixture named check_simulator_read_side (this is only necessary because checks.py iemports this method).

How did you verify/test it?

Run a sanity check on a dual ToR testbed, ensure that it is able to pass and is also able to detect failures
Run a sanity check on a single ToR testbed, ensure the mux simulator check does not run
Run a sanity check on a T1 testbed, ensure the mux simulator check does not run

Any platform specific information?

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

Documentation

lgtm-com · 2021-01-26T03:06:46Z

This pull request introduces 1 alert when merging fca2ac3d0b0a0fc2d63cefff9e7e366cdca61a9c into 2f0b561 - view on LGTM.com

new alerts:

1 for Unused import

tests/common/plugins/sanity_check/checks.py

lgtm-com · 2021-01-26T19:54:07Z

This pull request introduces 1 alert when merging e63cdaf852a5505baa3e3db3d23f7b8832b64c15 into 15793fb - view on LGTM.com

new alerts:

1 for Unused import

Signed-off-by: Lawrence Lee <[email protected]>

* Make compatible with other topologies Signed-off-by: Lawrence Lee <[email protected]>

lgtm-com · 2021-02-18T02:27:28Z

This pull request introduces 1 alert when merging 5729745 into e893f73 - view on LGTM.com

new alerts:

1 for Module is imported more than once

wangxin · 2021-02-19T07:34:26Z

tests/common/plugins/sanity_check/checks.py

+            return results
+
+        try:
+            pytest_assert(ptf_arp_tgt_ip in lower_tor_arp_table)


ptf_arp_tgt_ip in lower_tor_arp_table --> ptf_arp_tgt_ip not in lower_tor_arp_table?

See #2862 (comment)

wangxin · 2021-02-19T07:35:10Z

tests/common/plugins/sanity_check/checks.py

+            pytest_assert(ptf_arp_tgt_ip in lower_tor_arp_table)
+        except AssertionError:
+            results['failed'] = True
+            results['failed_reason'] = 'Packet from PTF not received on standby lower ToR'


'Packet from PTF not received on standby lower ToR' --> 'Packet from PTF received on standby lower ToR'?

We are actually expecting the packet from PTF to reach both ToRs here, since upstream traffic from the server is broadcast to both ToRs. I'll fix my comments to be accurate, sorry for the confusion!

wangxin · 2021-02-19T07:39:39Z

tests/common/plugins/sanity_check/checks.py

+            results['failed_reason'] = 'Unable to switch active link to upper ToR'
+            return results
+
+        # Ping from both ToRs, expect both messages to reach PTF


# Ping from both ToRs, expect both messages to reach PTF -> # Ping from both ToRs, expect only message from upper_tor to reach PTF

Ack, thanks!

wangxin · 2021-02-19T07:42:27Z

tests/common/plugins/sanity_check/checks.py

+        upper_tor_arp_table = upper_tor_host.switch_arptable()['ansible_facts']['arptable']['v4']
+        lower_tor_arp_table = lower_tor_host.switch_arptable()['ansible_facts']['arptable']['v4']
+        try:
+            pytest_assert(ptf_arp_tgt_ip in upper_tor_arp_table)


ptf_arp_tgt_ip in upper_tor_arp_table --> ptf_arp_tgt_ip not in upper_tor_arp_table?

See comment #2862 (comment)

wangxin · 2021-02-19T07:42:47Z

tests/common/plugins/sanity_check/checks.py

+            pytest_assert(ptf_arp_tgt_ip in upper_tor_arp_table)
+        except AssertionError:
+            results['failed'] = True
+            results['failed_reason'] = 'Packet from PTF not received on standby upper ToR'


'Packet from PTF not received on standby upper ToR' -> 'Packet from PTF received on standby upper ToR'

See #2862 (comment)

wangxin · 2021-02-19T07:45:42Z

tests/common/plugins/sanity_check/checks.py

+            results['failed_reason'] = 'Packet from PTF not received on active lower ToR'
+            return results
+
+        logger.info(json.dumps(results, indent=4, sort_keys=True))


Suggest not to dump the check result here. All the check results will be dumped at INFO level after all checks are done. It would be better just to log an INFO message indicating mux simulator sanity check is done.

Signed-off-by: Lawrence Lee <[email protected]>

lgtm-com · 2021-02-19T19:34:04Z

This pull request introduces 1 alert when merging bea9715 into a1a713e - view on LGTM.com

new alerts:

1 for Module is imported more than once

Signed-off-by: Lawrence Lee <[email protected]>

lgtm-com · 2021-02-24T23:33:57Z

This pull request introduces 1 alert when merging 0b6d219 into 7e92609 - view on LGTM.com

new alerts:

1 for Explicit export is not defined

Signed-off-by: Lawrence Lee <[email protected]>

bingwang-ms · 2021-02-25T06:22:52Z

tests/common/plugins/sanity_check/checks.py

+            results['failed_reason'] = 'Unable to switch active link to upper ToR'
+            return results
+
+        # Ping from both ToRs, expect only message from upper ToR to reach PTF


Do we need to clear ARP on DUT before start ping?

* Avoid errors when using fixtures in other fixtures that are module scoped (e.g. sanity_check) Signed-off-by: Lawrence Lee <[email protected]>

* Only run check items that are actually defined in checks.py Signed-off-by: Lawrence Lee <[email protected]>

Signed-off-by: Lawrence Lee <[email protected]>

theasianpianist requested review from wangxin, vaibhavhd, lolyu, bingwang-ms and a team January 26, 2021 02:34

wangxin reviewed Jan 26, 2021

View reviewed changes

tests/common/plugins/sanity_check/checks.py Outdated Show resolved Hide resolved

tests/common/plugins/sanity_check/checks.py Outdated Show resolved Hide resolved

theasianpianist marked this pull request as draft January 26, 2021 22:34

theasianpianist added 2 commits February 17, 2021 22:58

[sanity_check]: Add mux simulator sanity check

4eda4ba

Signed-off-by: Lawrence Lee <[email protected]>

[sanity check]: Improve sanity check

5729745

* Make compatible with other topologies Signed-off-by: Lawrence Lee <[email protected]>

theasianpianist force-pushed the mux-sim-sanity-check branch from 3d3c643 to 5729745 Compare February 18, 2021 01:59

theasianpianist requested a review from wangxin February 18, 2021 02:01

theasianpianist marked this pull request as ready for review February 18, 2021 18:39

theasianpianist requested a review from a team February 18, 2021 18:39

wangxin reviewed Feb 19, 2021

View reviewed changes

[sanity_check]: Update mux simulator comments

bea9715

Signed-off-by: Lawrence Lee <[email protected]>

[sanity_check]: Remove duplicate import

52810a2

Signed-off-by: Lawrence Lee <[email protected]>

theasianpianist requested a review from wangxin February 20, 2021 03:19

theasianpianist added 2 commits February 22, 2021 08:13

[sanity_check]: Remove check results from log message

701b897

Signed-off-by: Lawrence Lee <[email protected]>

Merge branch 'master' into mux-sim-sanity-check

0b6d219

[dualtor]: Fix lgtm

140b4a0

Signed-off-by: Lawrence Lee <[email protected]>

bingwang-ms reviewed Feb 25, 2021

View reviewed changes

theasianpianist added 3 commits February 25, 2021 19:52

[mux_simulator_control]: Make all fixtures module scoped

bb6c8ee

* Avoid errors when using fixtures in other fixtures that are module scoped (e.g. sanity_check) Signed-off-by: Lawrence Lee <[email protected]>

[sanity_check]: Improve check item filter

1d48fa4

* Only run check items that are actually defined in checks.py Signed-off-by: Lawrence Lee <[email protected]>

[sanity_check]: Update mux sim check for new mux_simulator_control.py

b658048

Signed-off-by: Lawrence Lee <[email protected]>

wangxin approved these changes Mar 2, 2021

View reviewed changes

theasianpianist merged commit 7383082 into sonic-net:master Mar 2, 2021

theasianpianist deleted the mux-sim-sanity-check branch March 2, 2021 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sanity check]: Add mux simulator sanity check #2862

[sanity check]: Add mux simulator sanity check #2862

theasianpianist commented Jan 26, 2021 •

edited

Loading

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Feb 18, 2021

wangxin Feb 19, 2021

theasianpianist Feb 22, 2021

wangxin Feb 19, 2021

theasianpianist Feb 19, 2021

wangxin Feb 19, 2021

theasianpianist Feb 19, 2021

wangxin Feb 19, 2021

theasianpianist Feb 19, 2021

wangxin Feb 19, 2021

theasianpianist Feb 19, 2021

wangxin Feb 19, 2021

lgtm-com bot commented Feb 19, 2021

lgtm-com bot commented Feb 24, 2021

bingwang-ms Feb 25, 2021

[sanity check]: Add mux simulator sanity check #2862

[sanity check]: Add mux simulator sanity check #2862

Conversation

theasianpianist commented Jan 26, 2021 • edited Loading

Description of PR

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

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

Documentation

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Feb 19, 2021

lgtm-com bot commented Feb 24, 2021

Choose a reason for hiding this comment

theasianpianist commented Jan 26, 2021 •

edited

Loading