Add performance test for set/delete routes by shi-su · Pull Request #2344 · sonic-net/sonic-mgmt

shi-su · 2020-10-14T03:55:15Z

Description of PR

Summary: Add a performance test for setting and deleting routes
Fixes # (issue)

Type of change

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

Approach

What is the motivation for this PR?

The goal of this PR is to add a test case to quantify the performance of set/delete routes and make sure the operation could finish within the time limit.

How did you do it?

Set or delete multiple (10k level) routes with swssconfig and record the time for the routes to show up in ASIC_DB.

How did you verify/test it?

Verified by running the test locally with a virtual switch.

Any platform specific information?

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

Any.

Documentation

qiluo-msft · 2020-10-15T01:56:43Z

tests/route/test_route_perf.py

10.1.0.37 [](start = 37, length = 9)

The neigh IP is too similar to the interface IPs. Do you want to use a totally different IP? #Closed

Updated to a different IP.

@qiluo-msft , @shi-su , the neighbor is expected to be in the same subnet as interface IP. With the subnet of /31, it can have only one neighbor, ie. 10.1.0.55. Suggest to change to a /24 subnet and have few neighbors so as to cover ECMP as well.

@prsunny . Thanks for this suggestion. I updated the test to fix it.

qiluo-msft · 2020-10-15T01:58:36Z

tests/route/test_route_perf.py

ROUTE_JSON [](start = 0, length = 10)

Use ansible tempfile module to generate a safe temp filename? #Closed

Using mktemp in DUT to generate this temp file.

qiluo-msft · 2020-10-15T02:04:49Z

tests/route/test_route_perf.py

ROUTE_JSON [](start = 62, length = 10)

Actually you don't need to copy a file into container. You can call docker exec -i swss swssconfig /dev/stdin < filename #Closed

Fixed as prescribed.

qiluo-msft · 2020-10-15T02:06:14Z

tests/route/test_route_perf.py

duthost.shell [](start = 22, length = 13)

duthost.shell [](start = 22, length = 13)

Is there already a function for this? #Closed

I did not find a function that does this job. I did find a few other places that use similar sonic-db-cli and redis-cli to get keys.

I am worried about the performance. I found a better solution https://stackoverflow.com/a/18247870/2514803

In reply to: 505121217 [](ancestors = 505121217)

qiluo-msft · 2020-10-15T02:08:02Z

tests/route/test_route_perf.py

10000 [](start = 17, length = 5)

I thought you change this number much to get a performance diagram. If this is the right use case, could you make a testcase parameter with a default value? #Closed

Added a testcase parameter with default value for the number of routes.

qiluo-msft

As comments

wangxin · 2020-10-15T09:44:33Z

tests/conftest.py

The --num_routes option has been defined in tests/vxlan/conftest.py. There is conflicting. PR testing failed because of this conflicting. Suggest to move definition of this --num_routes specific to route testing to tests/route/conftest.py.

Thanks for this suggestion! I did move it to tests/route/conftest.py but the PR tests still fail. And I could not reproduce this problem locally. Wondering if you have any idea about this?

It turns out that the vxlan tests define the option in init.py and it can be seen outside. Moved the part to a separate conftest.py file resolves the problem.

prsunny

I think we should have two more cases to consider:

Add IPv6 routes
Add ECMP routes, by creating ~8 neighbors or so and add route pointing to those 8 neighbors.

prsunny · 2020-10-15T19:25:25Z

tests/route/test_route_perf.py

Not every platforms have Ethernet72. Is this intended to run only on some platform?

Updated the interface used for the test. Also added IPv6 and multiple neighbors.

shi-su · 2020-10-20T18:01:31Z

retest this please

qiluo-msft

LGTM, please also resolve others' review comments.

prsunny · 2020-10-21T00:31:05Z

tests/route/test_route_perf.py

+            'interface' : 'Ethernet%d' % (idx_neigh * 4 + 4),
+            'ip' : '10.%d.0.1/24' % (idx_neigh + 1),
+            'neighbor' : '10.%d.0.2' % (idx_neigh + 1),
+            'mac' : '55:54:00:ad:48:%0.2x' % idx_neigh


55 as the first byte would cause issue in some platforms as they treat this as a multicast mac. For e,g 01:00:53 is the standard multicast mac but many platforms checks only the first byte 01. So please change it to 54 or 00

Thanks for the suggestion! Fixed.

prsunny · 2020-10-21T00:31:39Z

tests/route/test_route_perf.py

+            'interface' : 'Ethernet%d' % (idx_neigh * 4),
+            'ip' : '%x::1/64' % (0x2000 + idx_neigh),
+            'neighbor' : '%x::2' % (0x2000 + idx_neigh),
+            'mac' : '55:54:00:ad:48:%0.2x' % idx_neigh


Same as above

prsunny · 2020-10-21T00:33:33Z

tests/vxlan/__init__.py


 logger = logging.getLogger(__name__)

-def pytest_addoption(parser):


Why do we have vxlan change in this PR? Suggest to separate if not part of this feature. @theasianpianist for visibility

This change is to resolve the --num-routes option conflicting issue. Please refer to #2344 (comment)

The --num_routes option is used for route tests and vxlan tests. Currently, vxlan test put pytest_addoption in init.py. This will be seen by pytest during initialization and cause conflict if any other tests try to add --num_routes option. I moved pytest_addoption to conftest.py to resolve conflict.

@prsunny I originally put these in __init__.py because we had a bug that caused CLI options defined in test specific conftest.py files to not be recognized. I believe @wangxin has since fixed the issue, so this should be fine.

shi-su · 2020-10-21T00:52:13Z

retest this please

This reverts commit 814f702.

shi-su requested a review from qiluo-msft October 14, 2020 03:55

qiluo-msft requested a review from prsunny October 15, 2020 01:55

qiluo-msft reviewed Oct 15, 2020

View reviewed changes

wangxin reviewed Oct 15, 2020

View reviewed changes

prsunny reviewed Oct 15, 2020

View reviewed changes

shi-su added 12 commits October 20, 2020 16:54

Add performance test for set/delete routes

300c9ae

fix a typo

86adb3d

Improve test code

97001aa

remove unnecessary imports and add pytest mark

60fa45e

move _check_num_routes()

cd2bdf9

update pull interval

b7bcad4

Address review comments

3ca6363

move option to tests/route/conftest.py

8e003ab

restore line spacing in tests/conftest.py

a7e444f

Add ipv6 and ECMP

d4e47d6

using eval function to get keys

e5adfe4

Move addoption to conftest.py in vxlan to avoid addoption conflict

2cd203d

shi-su force-pushed the perf_test branch from a8ccb65 to 2cd203d Compare October 20, 2020 17:23

qiluo-msft approved these changes Oct 20, 2020

View reviewed changes

shi-su requested review from prsunny and wangxin October 20, 2020 21:57

prsunny reviewed Oct 21, 2020

View reviewed changes

prsunny requested a review from theasianpianist October 21, 2020 00:34

update mac address

b5fdf1b

prsunny approved these changes Oct 21, 2020

View reviewed changes

shi-su merged commit 814f702 into sonic-net:master Oct 21, 2020

wangxin mentioned this pull request Oct 21, 2020

Fix conflicting options of vxlan and route tests #2385

Merged

3 tasks

shi-su added a commit that referenced this pull request Oct 21, 2020

Revert "Add performance test for set/delete routes (#2344)"

0c2fb37

This reverts commit 814f702.

shi-su mentioned this pull request Oct 21, 2020

Revert "Add performance test for set/delete routes" #2386

Closed


		logger = logging.getLogger(__name__)

		def pytest_addoption(parser):

Conversation

shi-su commented Oct 14, 2020

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

Uh oh!

qiluo-msft Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

qiluo-msft Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

qiluo-msft Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

wangxin Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

prsunny left a comment

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!

shi-su commented Oct 20, 2020

Uh oh!

qiluo-msft left a comment

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

qiluo-msft Oct 15, 2020 •

edited

Loading

qiluo-msft Oct 15, 2020 •

edited

Loading

qiluo-msft Oct 15, 2020 •

edited

Loading

qiluo-msft Oct 15, 2020 •

edited

Loading

qiluo-msft Oct 15, 2020 •

edited

Loading

wangxin Oct 15, 2020 •

edited

Loading