Add route flow counter related test cases by Junchao-Mellanox · Pull Request #5134 · sonic-net/sonic-mgmt

Junchao-Mellanox · 2022-02-14T02:00:15Z

HLD:
sonic-net/SONiC#908

Change-Id: I4b9b1dd1c5372645c67511ddcc667b99f427d556

Description of PR

Summary:
Added test cases for route flow counter feature

Type of change

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

Back port request

201911

Approach

What is the motivation for this PR?

Add route flow counter related test cases

How did you do it?

Add new test case file test_route_flow_counter.py to covers basic function of this feature
Add check in test_vnet_vxlan.py to cover VNET route
Add check in test_bgp_speaker.py to cover BGP route
Add check in test_static_route.py to cover static route

How did you verify/test it?

Run the test cases

Any platform specific information?

N/A

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

N/A

Documentation

Change-Id: I4b9b1dd1c5372645c67511ddcc667b99f427d556

lgtm-com · 2022-02-14T02:25:26Z

This pull request introduces 1 alert when merging 5469367 into 8b81dc8 - view on LGTM.com

new alerts:

1 for Unused import

Change-Id: I70d4fdd3160e7a31dfca66d829491b19630adc92

Junchao-Mellanox · 2022-02-15T01:00:46Z

The failure is expected because some the feature PRs are still in review.

Conflicts: tests/vxlan/test_vnet_vxlan.py Change-Id: Ia9bc3b3523c6c3f547a745ed1530fb7c143bac45

liat-grozovik · 2022-04-25T10:31:18Z

@roysr-nv could you please review?

roy-sror

Hi Junchao,

Apart from the specific comments, I have some general notes:

Please use allure steps.
I suggest to combine test_update_route_pattern and test_add_remove_route - I don't see any reason to cover it separately.
in the new route flow tests - please make sure that cleanup is done in any case, even if exception is thrown during the test execution.
Can you please share the test runtime.

tests/flow_counter/flow_counter_utils.py

roy-sror · 2022-04-25T15:45:07Z

tests/flow_counter/flow_counter_utils.py

+        support_route_flow_counter = True
+    elif support == 'false':
+        support_route_flow_counter = False
+    return support_route_flow_counter is not None


Can't we simply return the variable?

No. get_route_flow_counter_capability is a callback of wait_until, the return value indicates that whether wait_until need continue running. If we return the support_route_flow_counter, there could be case that: support_route_flow_counter is False and wait_until will continue running until timeout.

how about raising an exception if the value is None? wait_until should return False in that case

The semantic here is to "wait support_route_flow_counter until there is a value", so I support "return support_route_flow_counter is not None" just matches the semantic.

tests/route/test_route_flow_counter.py

tests/bgp/test_bgp_speaker.py

tests/flow_counter/flow_counter_utils.py

roy-sror · 2022-04-25T16:26:50Z

tests/flow_counter/flow_counter_utils.py

+    dut.command('counterpoll flowcnt-route interval {}'.format(interval))
+
+
+def set_route_flow_counter_pattern(dut, route_pattern, max_match_count=30):


To me it seems more readable to get another parameter- VRF, rather than encapsulating it in the route pattern string

Indeed, using VRF and prefix here makes this function more readable. However, I would like to keep this based on two reasons:

Most of case VRF is none(default VRF)

If we use VRF and prefix, the caller has to use a tuple (, ) to represent the route pattern, it increases unneeded complexity of caller side. And a tuple is not so readable.

roy-sror · 2022-04-25T16:30:31Z

tests/flow_counter/flow_counter_utils.py

+    Returns:
+        bool: Match if True.
+    """
+    logger.info('Expected stats: {}'.format(expect_stats))


here I suggest to exit the function only once(at the end), so we can set the rc and message when needed and return it at the end

Prefer to keep this. There is a "for loop in for loop" in this function, it is hard to use rc in the inner loop.

for key, value in expect_stats.items(): if key not in actual_stats: return False, 'Failed to find {} in result'.format(key) for stats_type, expect_value in value.items(): if int(expect_value) != int(actual_stats[key][stats_type].replace(',', '')): return False, 'Expected {} value of {} is {}, but got {}'.format(stats_type, key, expect_value, actual_stats[key][stats_type])

Junchao-Mellanox · 2022-04-26T07:11:46Z

Hi Junchao,

Apart from the specific comments, I have some general notes:

Please use allure steps.

I suggest to combine test_update_route_pattern and test_add_remove_route - I don't see any reason to cover it separately.

in the new route flow tests - please make sure that cleanup is done in any case, even if exception is thrown during the test execution.

Can you please share the test runtime.

I will fix 1~3. For item#2, I will remove test_add_remove_route since it is fully covered by test_update_route_pattern. Will update test runtime after all fixes.

Change-Id: Iad925eaf3c605780e9d41802240ba89b8a4dce2f

Junchao-Mellanox · 2022-04-27T06:15:38Z

Hi @roysr-nv, the test run time is about 180 seconds.

Change-Id: I75d88cdd90244de99f832f154716ca6ad001c1b2

Junchao-Mellanox · 2022-05-10T03:09:39Z

Hi @roysr-nv, could you please review again?

lgtm-com · 2022-05-10T03:18:24Z

This pull request introduces 2 alerts when merging 5d6cc25 into 71e0833 - view on LGTM.com

new alerts:

2 for Unused import

Change-Id: If6e571d803b7ba8702164f000ac7a0a10919f175

lgtm-com · 2022-05-10T03:48:55Z

This pull request introduces 2 alerts when merging 0d7cd5f into 38d6d3a - view on LGTM.com

new alerts:

2 for Unused import

Change-Id: Ia784fc3ad19483fbc84ebd657066c0bce218c273

lgtm-com · 2022-05-12T10:22:42Z

This pull request introduces 2 alerts when merging 6f2fe3e into c453cf4 - view on LGTM.com

new alerts:

2 for Unused import

Change-Id: I89c90deabd38720136ab2dea5b1eb38283f1bcf2

lgtm-com · 2022-05-13T02:00:50Z

This pull request introduces 2 alerts when merging f9278f7 into b2cc638 - view on LGTM.com

new alerts:

2 for Unused import

Change-Id: I7b304d40004ea699c29d61d88b7bd1f64ec63a53

lgtm-com · 2022-05-13T07:22:35Z

This pull request introduces 2 alerts when merging e2fe2d8 into 1193c99 - view on LGTM.com

new alerts:

2 for Unused import

liat-grozovik · 2022-05-14T12:33:13Z

/azp run Azure.sonic-mgmt

azure-pipelines · 2022-05-14T12:33:21Z

Azure Pipelines successfully started running 1 pipeline(s).

Junchao-Mellanox · 2022-05-15T11:20:05Z

It seems it is not using the latest sonic-utilities.

Junchao-Mellanox · 2022-05-15T11:20:16Z

/azpw run Azure.sonic-mgmt

mssonicbld · 2022-05-15T11:20:18Z

/AzurePipelines run Azure.sonic-mgmt

azure-pipelines · 2022-05-15T11:20:27Z

Azure Pipelines successfully started running 1 pipeline(s).

Junchao-Mellanox · 2022-05-17T02:56:27Z

/azpw run Azure.sonic-mgmt

mssonicbld · 2022-05-17T02:56:28Z

/AzurePipelines run Azure.sonic-mgmt

azure-pipelines · 2022-05-17T02:56:36Z

Azure Pipelines successfully started running 1 pipeline(s).

Junchao-Mellanox · 2022-05-18T01:05:20Z

/azpw run Azure.sonic-mgmt

mssonicbld · 2022-05-18T01:05:22Z

/AzurePipelines run Azure.sonic-mgmt

azure-pipelines · 2022-05-18T01:05:31Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2022-05-21T07:47:07Z

/azp run Azure.sonic-mgmt

azure-pipelines · 2022-05-21T07:47:15Z

Azure Pipelines successfully started running 1 pipeline(s).

Junchao-Mellanox · 2022-05-25T08:41:56Z

/azpw run Azure.sonic-mgmt

mssonicbld · 2022-05-25T08:41:58Z

/AzurePipelines run Azure.sonic-mgmt

azure-pipelines · 2022-05-25T08:42:07Z

Azure Pipelines successfully started running 1 pipeline(s).

lgtm-com · 2022-05-26T07:17:18Z

This pull request introduces 2 alerts when merging 8e31615 into 99ef5c3 - view on LGTM.com

new alerts:

2 for Unused import

Junchao-Mellanox · 2022-05-31T02:55:26Z

Closed as this PR always use old SONiC image, new PR created: #5736

Add route flow counter related test cases

5469367

Change-Id: I4b9b1dd1c5372645c67511ddcc667b99f427d556

Junchao-Mellanox requested a review from a team as a code owner February 14, 2022 02:00

keboliu previously approved these changes Feb 14, 2022

View reviewed changes

Fix LGTM warning

dac195d

Change-Id: I70d4fdd3160e7a31dfca66d829491b19630adc92

Junchao-Mellanox dismissed keboliu’s stale review via dac195d February 14, 2022 10:02

liat-grozovik mentioned this pull request Apr 13, 2022

High level design for route flow counter sonic-net/SONiC#908

Merged

Merge remote-tracking branch 'upstream/master' into route-flow-counter

339e6a7

Conflicts: tests/vxlan/test_vnet_vxlan.py Change-Id: Ia9bc3b3523c6c3f547a745ed1530fb7c143bac45

roy-sror reviewed Apr 25, 2022

View reviewed changes

Fix review comment

6238b27

Change-Id: Iad925eaf3c605780e9d41802240ba89b8a4dce2f

Fix review comment

5d6cc25

Change-Id: I75d88cdd90244de99f832f154716ca6ad001c1b2

express invalid LGTM warning

0d7cd5f

Change-Id: If6e571d803b7ba8702164f000ac7a0a10919f175

Fix typo

6f2fe3e

Change-Id: Ia784fc3ad19483fbc84ebd657066c0bce218c273

roy-sror previously approved these changes May 12, 2022

View reviewed changes

Fix fixture issue

f9278f7

Change-Id: I89c90deabd38720136ab2dea5b1eb38283f1bcf2

Junchao-Mellanox dismissed roy-sror’s stale review via f9278f7 May 13, 2022 01:35

Change fixture scope from session to module

e2fe2d8

Change-Id: I7b304d40004ea699c29d61d88b7bd1f64ec63a53

roy-sror previously approved these changes May 19, 2022

View reviewed changes

Merge branch 'master' into route-flow-counter

8e31615

Junchao-Mellanox dismissed roy-sror’s stale review via 8e31615 May 26, 2022 06:55

Junchao-Mellanox closed this May 31, 2022

		dut.command('counterpoll flowcnt-route interval {}'.format(interval))


		def set_route_flow_counter_pattern(dut, route_pattern, max_match_count=30):

Conversation

Junchao-Mellanox commented Feb 14, 2022

Description of PR

Type of change

Back port request

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!

lgtm-com bot commented Feb 14, 2022

Uh oh!

Junchao-Mellanox commented Feb 15, 2022

Uh oh!

liat-grozovik commented Apr 25, 2022

Uh oh!

roy-sror left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Junchao-Mellanox commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Junchao-Mellanox commented Apr 27, 2022

Uh oh!

Junchao-Mellanox commented May 10, 2022

Uh oh!

lgtm-com bot commented May 10, 2022

Uh oh!

lgtm-com bot commented May 10, 2022

Uh oh!

lgtm-com bot commented May 12, 2022

Uh oh!

lgtm-com bot commented May 13, 2022

Uh oh!

lgtm-com bot commented May 13, 2022

Uh oh!

liat-grozovik commented May 14, 2022

Uh oh!

azure-pipelines bot commented May 14, 2022

Uh oh!

Junchao-Mellanox commented May 15, 2022

Uh oh!

Junchao-Mellanox commented May 15, 2022

Uh oh!

mssonicbld commented May 15, 2022

Uh oh!

azure-pipelines bot commented May 15, 2022

Uh oh!

Junchao-Mellanox commented May 17, 2022

Uh oh!

mssonicbld commented May 17, 2022

Uh oh!

azure-pipelines bot commented May 17, 2022

Uh oh!

Junchao-Mellanox commented Apr 26, 2022 •

edited

Loading