Fix sFlow sampling-rate and admin-state by venkatmahalingam · Pull Request #1728 · sonic-net/sonic-swss

venkatmahalingam · 2021-04-27T00:01:59Z

Fixed incorrect interface admin-status when 'all' interface is disabled but there are local sampling-rate configurations.
The original changes are present in the PR

Signed-off-by: Venkatesan Mahalingam venkatesan_mahalinga@dell.com

What I did
Fixed a bug where the interface sFlow admin-status is not correct if the user configures a custom sFlow sampling-rate. (UT performed here)

As a side-effect of this fix, users can now restore the default sFlow sampling-rate with the default option using: config sflow interface sampling-rate Ethernet4 default
This is not tested here but will be tested when the config script update PR is submitted to sonic-utilities. The sFlow corresponding HLD and command-line reference will also be updated.

Why I did it
If the user ONLY configures a custom (local) sFlow sampling-rate for a interface, the interface's admin-status should still adhere to the global interface admin-status.

How I verified it

UT cases (added the sonic-vs test case)
Manual testing - see below


root@L6:/home/admin# show sflow

sFlow Global Information:
  sFlow Admin State:          up
  sFlow Polling Interval:     default
  sFlow AgentID:              default

  0 Collectors configured:
root@L6:/home/admin# show sflow interface

sFlow interface configurations
+-------------+---------------+-----------------+
| Interface   | Admin State   |   Sampling Rate |
+=============+===============+=================+
| Ethernet0   | up            |             256 |
+-------------+---------------+-----------------+
| Ethernet4   | up            |             512 |
+-------------+---------------+-----------------+
| Ethernet8   | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet12  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet16  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet20  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet24  | up            |          100000 |
+-------------+---------------+-----------------+
............
...........
root@L6:/home/admin# config sflow interface disable all
root@L6:/home/admin# show sflow interface

sFlow interface configurations
+-------------+---------------+-----------------+
| Interface   | Admin State   | Sampling Rate   |
+=============+===============+=================+
+-------------+---------------+-----------------+
root@L6:/home/admin#
root@L6:/home/admin#
root@L6:/home/admin# config sflow interface enable all
root@L6:/home/admin# show sflow interface

sFlow interface configurations
+-------------+---------------+-----------------+
| Interface   | Admin State   |   Sampling Rate |
+=============+===============+=================+
| Ethernet0   | up            |             256 |
+-------------+---------------+-----------------+
| Ethernet4   | up            |             512 |
+-------------+---------------+-----------------+
| Ethernet8   | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet12  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet16  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet20  | up            |          100000 |

.............
............
root@L6:/home/admin# config sflow interface sample-rate Ethernet0 default
root@L6:/home/admin# show sflow interface

sFlow interface configurations
+-------------+---------------+-----------------+
| Interface   | Admin State   |   Sampling Rate |
+=============+===============+=================+
| Ethernet0   | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet4   | up            |             512 |
+-------------+---------------+-----------------+
| Ethernet8   | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet12  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet16  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet20  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet24  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet28  | up            |          100000 |
+-------------+---------------+-----------------+
| Ethernet32  | up            |          100000 |
+-------------+---------------+-----------------+

...................

…ed but there are local sampling-rate configurations. Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

venkatmahalingam · 2021-04-27T05:54:44Z

@prsunny @dgsudharsan Please review this PR.

liat-grozovik · 2021-04-27T06:22:21Z

@venkatmahalingam can you please verify if this can be taken to 202012?
The idea is to have sflow fully qualified as part of 202012 so if this is a bug fix we should consider it to be included.
But this can be done only if you can confirm it is cleanly cherry picked and working as expected. 10x

venkatmahalingam · 2021-04-27T16:52:39Z

@venkatmahalingam can you please verify if this can be taken to 202012?
The idea is to have sflow fully qualified as part of 202012 so if this is a bug fix we should consider it to be included.
But this can be done only if you can confirm it is cleanly cherry picked and working as expected. 10x

@liat-grozovik Cherry-pick to 202012 works fine if we do it in the PR merge order i.e first Vivek's commit and then this PR commit.

build:~/202012/sonic-swss$ git cherry-pick d9f28b6
[202012 f6d4dfc] [SflowMgr] SamplingRate Update by Speed Change Added (#1721)
 Author: Vivek Reddy <vivekreddykarri98@gmail.com>
 Date: Sun Apr 25 00:43:15 2021 -0400
 2 files changed, 57 insertions(+), 4 deletions(-)
build:~/202012/sonic-swss$
build:~/202012/sonic-swss$
build:~/202012/sonic-swss$ git cherry-pick 77f6c59
[202012 ecce87d] Fixed incorrect interface admin-status when 'all' interface is disabled but there are local sampling-rate configurations.
 Date: Mon Apr 26 23:57:29 2021 +0000
 3 files changed, 67 insertions(+), 11 deletions(-)
build:~/202012/sonic-swss$ git cherry-pick bd9ce69
[202012 878f194] UT done
 Date: Tue Apr 27 05:50:57 2021 +0000
 1 file changed, 14 insertions(+), 12 deletions(-)
build:~/202012/sonic-swss$ git cherry-pick 2efe415
[202012 f3c49ea] Added new testcase.
 Date: Tue Apr 27 17:38:48 2021 +0000
 1 file changed, 36 insertions(+), 1 deletion(-)
build:~/202012/sonic-swss$

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

lgtm-com · 2021-04-27T18:34:29Z

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

new alerts:

1 for Unused local variable

prsunny · 2021-04-27T23:35:36Z

@dgsudharsan, please review

prsunny · 2021-04-27T23:36:01Z

The PR is failing on VS test

AssertionError: Did not expect field/values to match, but they did: provided={'sample_rate': '256', 'admin_state': 'up'}, received={'sample_rate': '256', 'admin_state': 'up'}, key="Ethernet0", table="SFLOW_SESSION_TABLE"

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

venkatmahalingam · 2021-04-27T23:52:01Z

The PR is failing on VS test

AssertionError: Did not expect field/values to match, but they did: provided={'sample_rate': '256', 'admin_state': 'up'}, received={'sample_rate': '256', 'admin_state': 'up'}, key="Ethernet0", table="SFLOW_SESSION_TABLE"

Yes, looking into the failures.

vivekrnv · 2021-04-28T00:17:18Z

cfgmgr/sflowmgr.cpp

                // If the Local Conf is already present, dont't override it even though the speed is changed
-                if (new_port || (speed_change && !m_sflowPortConfMap[key].local_conf))
+                if (new_port || (speed_change && 
+                   !(m_sflowPortConfMap[key].local_rate_cfg || m_sflowPortConfMap[key].local_admin_cfg)))


This can be removed "|| m_sflowPortConfMap[key].local_admin_cfg" as the rate update based on speed change is only blocked if there is any local_rate_cfg. It doesn't depend on local_admin_cfg.

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

venkatmahalingam · 2021-04-30T03:50:59Z

@prsunny @dgsudharsan @vivekreddynv This PR is ready to merge, please review and approve the same.

Also, the below PRs for SONiC CLI & HLD changes.
https://github.com/Azure/sonic-utilities/pull/845/files
sonic-net/SONiC#755

dgsudharsan · 2021-08-23T23:53:48Z

@prsunny @venkatmahalingam Request for cherry-pick to 202012

* Fixed incorrect interface admin-status when 'all' interface is disabled but there are local sampling-rate configurations. Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

In order to include the following commits: d29a49a 2021-08-25 [ACL] Match TCP protocol while matching TCP_FLAG (sonic-net/sonic-swss#1854) 2569ad9 2021-08-25 Fix sFlow sampling-rate and admin-state (sonic-net/sonic-swss#1728) 8908a8f 2021-08-19 Change rif_rates.lua and port_rates.lua scripts to calculate rates correct (sonic-net/sonic-swss#1848) b42c2fb 2021-08-19 [VS Test] Skip flaky tests (sonic-net/sonic-swss#1875)

* Fixed incorrect interface admin-status when 'all' interface is disabled but there are local sampling-rate configurations. Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

Fixed incorrect interface admin-status when 'all' interface is disabl…

77f6c59

…ed but there are local sampling-rate configurations. Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

venkatmahalingam mentioned this pull request Apr 27, 2021

swss: Fix sFlow sampling-rate and admin-state #1224

Closed

UT done

bd9ce69

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

liat-grozovik changed the title ~~swss: Fix sFlow sampling-rate and admin-state~~ Fix sFlow sampling-rate and admin-state Apr 27, 2021

liat-grozovik added the Bug 🐛 label Apr 27, 2021

Added new testcase.

2efe415

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

Updated the test case.

b848450

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

vivekrnv reviewed Apr 28, 2021

View reviewed changes

Comment addressed.

54d8e3d

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

dgsudharsan previously approved these changes Apr 28, 2021

View reviewed changes

Test case updated.

40370c9

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

venkatmahalingam dismissed dgsudharsan’s stale review via 40370c9 April 28, 2021 19:56

venkatmahalingam added 3 commits April 28, 2021 23:36

Test case updated.

7997f3f

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

Fixed the test case failure.

b2229b9

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

Updated.

6a1fcf9

Signed-off-by: Venkatesan Mahalingam <venkatesan_mahalinga@dell.com>

dgsudharsan approved these changes Apr 30, 2021

View reviewed changes

prsunny approved these changes Apr 30, 2021

View reviewed changes

prsunny merged commit 91068fb into sonic-net:master Apr 30, 2021

venkatmahalingam mentioned this pull request May 10, 2021

Add 'default' option for sFlow. sonic-net/sonic-utilities#1606

Merged

prsunny added the Request for 202012 Branch label Aug 24, 2021

qiluo-msft added the Included in 202012 Branch label Aug 25, 2021

This was referenced Aug 25, 2021

[202012] [submodule] sonic-swss submodule update sonic-net/sonic-buildimage#8582

Merged

Update sonic-swss hash to d29a49a noaOrMlnx/sonic-buildimage#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sFlow sampling-rate and admin-state#1728

Fix sFlow sampling-rate and admin-state#1728
prsunny merged 9 commits intosonic-net:masterfrom
venkatmahalingam:sflow-fix

venkatmahalingam commented Apr 27, 2021 •

edited by liat-grozovik

Loading

Uh oh!

venkatmahalingam commented Apr 27, 2021

Uh oh!

liat-grozovik commented Apr 27, 2021

Uh oh!

venkatmahalingam commented Apr 27, 2021 •

edited

Loading

Uh oh!

lgtm-com bot commented Apr 27, 2021

Uh oh!

prsunny commented Apr 27, 2021

Uh oh!

prsunny commented Apr 27, 2021

Uh oh!

venkatmahalingam commented Apr 27, 2021

Uh oh!

vivekrnv Apr 28, 2021

Uh oh!

venkatmahalingam Apr 28, 2021

Uh oh!

venkatmahalingam commented Apr 30, 2021 •

edited

Loading

Uh oh!

dgsudharsan commented Aug 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

venkatmahalingam commented Apr 27, 2021 • edited by liat-grozovik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

venkatmahalingam commented Apr 27, 2021

Uh oh!

liat-grozovik commented Apr 27, 2021

Uh oh!

venkatmahalingam commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Apr 27, 2021

Uh oh!

prsunny commented Apr 27, 2021

Uh oh!

prsunny commented Apr 27, 2021

Uh oh!

venkatmahalingam commented Apr 27, 2021

Uh oh!

vivekrnv Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

venkatmahalingam Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

venkatmahalingam commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgsudharsan commented Aug 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

venkatmahalingam commented Apr 27, 2021 •

edited by liat-grozovik

Loading

venkatmahalingam commented Apr 27, 2021 •

edited

Loading

venkatmahalingam commented Apr 30, 2021 •

edited

Loading