multi-asic support for copp testcase by abdosi · Pull Request #3060 · sonic-net/sonic-mgmt

abdosi · 2021-02-28T02:06:17Z

What/Why I did:

This is PR continuation of #2627 where infra changes were done to support copp on multi-asic platforms. In this PR test is enhanced to work on multi-asic platforms. Following are major changes:

Multi-asic as of now will support installing of nanomsg and ptf_nn_agent in syncd directly mode. This way test case will randomly select any front-asic syncd for verification. To support this needed to add iptable rules for HTTP and PTF Traffic forwarding from host to asic namespace. swap_syncd mode is not supported as of now.
All the docker name are translated to particular namespace/asic that is under test. (Added utility api for same).
Remove disable_lldp_for_testing fixture as noted below since test case is not dependent on that.

How I Verify:

Verify test_copp.py on both single and multi-asic platforms.

Signed-off-by: Abhishek Dosi <[email protected]>

abdosi · 2021-03-04T17:11:38Z

@daall can you please review this.

daall · 2021-03-04T18:03:35Z

tests/copp/test_copp.py

    duthost = duthosts[rand_one_dut_hostname]

-    logging.info("Disabling LLDP for the COPP tests")
+    logging.info("Disabling LLDP Container auto-restart for the COPP tests")


IIRC the reason we put this in a separate fixture was so that even if there was some issue during setup/teardown then this code should still execute and ensure that LLDP is brought back. Would it be possible to move the startup/shutdown of LLDP and BGP into this fixture and handle the multi-asic parts here as well?

sure, ok I will disable bgp and lldp feature itself (which make sure all instance will be down in multi-asic)

@daall Looks like there is issue with LLDP where even if we stop the lldp the config reload will restart it . Looks like we don't need to stop LLDP then ?

@daall I have confirmed fixture disable_lldp_for_testing where we disable lldp gets restarted as part of config reload/swap_syncd. So I am planning to remove this fixture as test cases works fine and passes without needing to disable lldp.

@daall Updated based on my above comment.

makes sense to me, thanks for checking!

Signed-off-by: Abhishek Dosi <[email protected]>

abdosi · 2021-03-11T18:28:32Z

Hi @daall can you please review again.

lgtm-com · 2021-03-11T18:54:32Z

This pull request introduces 1 alert when merging 0b0d88a into 11c7344 - view on LGTM.com

new alerts:

1 for Unused import

Signed-off-by: Abhishek Dosi <[email protected]>

daall · 2021-03-11T19:46:49Z

tests/copp/test_copp.py

+       logging.info("Adding iptables rules and enabling eth0 port forwarding")
+       http_proxy, https_proxy = copp_utils._get_http_and_https_proxy_ip(creds)
+       # IP Table rule for http and ptf nn_agent traffic.
+       dut.command("sudo sysctl net.ipv4.conf.eth0.forwarding=1")
+       mgmt_ip = dut.host.options["inventory_manager"].get_host(dut.hostname).vars["ansible_host"]
+       # Rule to communicate to http/s proxy from namespace
+       dut.command("sudo iptables -t nat -A POSTROUTING -p tcp --dport 8080 -j SNAT --to-source {}".format(mgmt_ip))
+       dut.command("sudo ip -n {} rule add from all to {} pref 1 lookup default".format(test_params.nn_target_namespace, http_proxy))
+       if http_proxy != https_proxy:
+           dut.command("sudo ip -n {} rule add from all to {} pref 2 lookup default".format(test_params.nn_target_namespace, https_proxy))
+       # Rule to communicate to ptf nn agent client from namespace
+       ns_ip = dut.shell("sudo ip -n {} -4 -o addr show eth0".format(test_params.nn_target_namespace) + " | awk '{print $4}' | cut -d'/' -f1")["stdout"]
+       dut.command("sudo iptables -t nat -A PREROUTING -p tcp --dport 10900 -j DNAT --to-destination {}".format(ns_ip))
+       dut.command("sudo ip -n {} rule add from {} to {} pref 3 lookup default".format(test_params.nn_target_namespace, ns_ip, tbinfo["ptf_ip"]))


For readability it would be good if this was a separate helper method like "setup_multi_asic_proxy" or something similar?

@daall Updated.

daall · 2021-03-11T19:47:06Z

tests/copp/test_copp.py

+       dut.command("sudo sysctl net.ipv4.conf.eth0.forwarding=0")
+
+       mgmt_ip = dut.host.options["inventory_manager"].get_host(dut.hostname).vars["ansible_host"]
+       dut.command("sudo iptables -t nat -D POSTROUTING -p tcp --dport 8080 -j SNAT --to-source {}".format(mgmt_ip))
+       dut.command("sudo ip -n {} rule delete from all to {} pref 1 lookup default".format(test_params.nn_target_namespace, http_proxy))
+       if http_proxy != https_proxy:
+           dut.command("sudo ip -n {} rule delete from all to {} pref 2 lookup default".format(test_params.nn_target_namespace, https_proxy))
+
+       ns_ip = dut.shell("sudo ip -n {} -4 -o addr show eth0".format(test_params.nn_target_namespace) + " | awk '{print $4}' | cut -d'/' -f1")["stdout"]
+       dut.command("sudo iptables -t nat -D PREROUTING -p tcp --dport 10900 -j DNAT --to-destination {}".format(ns_ip))
+       dut.command("sudo ip -n {} rule delete from {} to {} pref 3 lookup default".format(test_params.nn_target_namespace, ns_ip, tbinfo["ptf_ip"]))


Same comment as above

@daall Updated

Signed-off-by: Abhishek Dosi <[email protected]>

daall · 2021-03-11T21:36:15Z

tests/copp/test_copp.py

-
-    enable_container_autorestart(duthost, testcase="test_copp", feature_list=feature_list)
+def _setup_multi_asic_proxy(dut, creds, test_params, tbinfo):
+


no need for this extra line (same applies to below method)

daall · 2021-03-11T21:36:24Z

tests/copp/test_copp.py

        Tears down the testbed, returning it to its initial state.
    """
-
+  


extra space

Signed-off-by: Abhishek Dosi <[email protected]>

abdosi added 5 commits February 5, 2021 17:00

Added test case to verify ebtables rules.

febcacd

Signed-off-by: Abhishek Dosi <[email protected]>

Merge remote-tracking branch 'upstream/master'

9582616

Merge remote-tracking branch 'upstream/master'

940ebe4

Copp test for multi-asic platfroms

5f9379c

Signed-off-by: Abhishek Dosi <[email protected]>

Fix

9b262fc

Signed-off-by: Abhishek Dosi <[email protected]>

abdosi requested a review from daall February 28, 2021 02:06

abdosi requested a review from a team as a code owner February 28, 2021 02:06

abdosi added 2 commits March 1, 2021 22:28

Fix the API when running on host namespace in multi-asic platforms.

b1046e5

Signed-off-by: Abhishek Dosi <[email protected]>

Merge remote-tracking branch 'upstream/master' into copp

fbfd4e2

daall reviewed Mar 4, 2021

View reviewed changes

abdosi added 5 commits March 9, 2021 18:24

Merge remote-tracking branch 'upstream/master' into copp

db07f34

Fix the testcase more.

a14c0ac

Signed-off-by: Abhishek Dosi <[email protected]>

Fix

c30fc90

Signed-off-by: Abhishek Dosi <[email protected]>

Merge remote-tracking branch 'upstream/master' into copp

a090893

Fixes

0b0d88a

Signed-off-by: Abhishek Dosi <[email protected]>

Fix

735b28f

Signed-off-by: Abhishek Dosi <[email protected]>

daall reviewed Mar 11, 2021

View reviewed changes

Review Comments

2fb5065

Signed-off-by: Abhishek Dosi <[email protected]>

daall reviewed Mar 11, 2021

View reviewed changes

tests/copp/test_copp.py Outdated

Tears down the testbed, returning it to its initial state.

"""

Copy link
Copy Markdown

Contributor

daall Mar 11, 2021

Choose a reason for hiding this comment

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

extra space

Fix

9069734

Signed-off-by: Abhishek Dosi <[email protected]>

daall approved these changes Mar 11, 2021

View reviewed changes

abdosi merged commit 0db62d7 into sonic-net:master Mar 12, 2021

abdosi deleted the copp branch March 12, 2021 18:58


		enable_container_autorestart(duthost, testcase="test_copp", feature_list=feature_list)
		def _setup_multi_asic_proxy(dut, creds, test_params, tbinfo):

		Tears down the testbed, returning it to its initial state.
		"""

Conversation

abdosi commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abdosi commented Mar 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abdosi commented Mar 11, 2021

Uh oh!

lgtm-com bot commented Mar 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abdosi commented Feb 28, 2021 •

edited

Loading