[crm] Add support for snat, dnat and ipmc crm resources by PrabhuSreenivasan · Pull Request #1511 · sonic-net/sonic-swss

PrabhuSreenivasan · 2020-11-19T17:07:25Z

Signed-off-by: Prabhu Sreenivasan [email protected]

What I did
Added support for snat, dnat and ipmc resources under CRM module.

Why I did it
New feature NAT adds new resources snat_enty and dnat_entry that needs to be monitored. ipmc_entry tracks IP multicast resources used by switch.

Associated PRs
sonic-net/sonic-utilities/pull/1258
sonic-net/sonic-buildimage#6012

How I verified it

root@sonic:/home/admin# crm show resources snat

Resource Name      Used Count    Available Count
---------------  ------------  -----------------
snat_entry                  0               1024

root@sonic:/home/admin# crm show resources dnat

Resource Name      Used Count    Available Count
---------------  ------------  -----------------
dnat_entry                  0               1024

root@sonic:/home/admin# crm show resources ipmc

Resource Name      Used Count    Available Count
---------------  ------------  -----------------
ipmc_entry                  0              24576

root@sonic:/home/admin# crm show resources all

Resource Name           Used Count    Available Count
--------------------  ------------  -----------------
ipv4_route                      54              32698
ipv6_route                       2              10238
ipv4_nexthop                     0              65460
ipv6_nexthop                     0              65460
ipv4_neighbor                    0              24576
ipv6_neighbor                    0              12288
nexthop_group_member             0              32768
nexthop_group                    0                256
fdb_entry                        0              65535
ipmc_entry                       0              24576
snat_entry                       0               1024
dnat_entry                       0               1024


Stage    Bind Point    Resource Name      Used Count    Available Count
-------  ------------  ---------------  ------------  -----------------
INGRESS  PORT          acl_group                   0                256
INGRESS  PORT          acl_table                   0                  3
INGRESS  LAG           acl_group                   0                256
INGRESS  LAG           acl_table                   0                  3
INGRESS  VLAN          acl_group                   0                256
INGRESS  VLAN          acl_table                   0                  9
INGRESS  RIF           acl_group                   0                256
INGRESS  RIF           acl_table                   0                  9
INGRESS  SWITCH        acl_group                   0                256
INGRESS  SWITCH        acl_table                   0                  9
EGRESS   PORT          acl_group                   0                256
EGRESS   PORT          acl_table                   0                  2
EGRESS   LAG           acl_group                   0                256
EGRESS   LAG           acl_table                   0                  2
EGRESS   VLAN          acl_group                   0                256
EGRESS   VLAN          acl_table                   0                  2
EGRESS   RIF           acl_group                   0                256
EGRESS   RIF           acl_table                   0                  2
EGRESS   SWITCH        acl_group                   0                256
EGRESS   SWITCH        acl_table                   0                  2


Table ID    Resource Name    Used Count    Available Count
----------  ---------------  ------------  -----------------

Details if related

PrabhuSreenivasan · 2020-11-24T17:31:46Z

Retest this please

PrabhuSreenivasan · 2020-11-25T04:37:55Z

The tests depend on sonic-net/sonic-utilities/pull/1258 for the new commands and hence failing. Shall I remove the tests till the CLI commands gets merged?

lguohan · 2020-11-25T11:46:57Z

orchagent/crmorch.cpp

-                    SWSS_LOG_ERROR("Failed to get switch attribute %u , rv:%d", attr.id, status);
+                    if(status != SAI_STATUS_NOT_SUPPORTED)
+                    {
+                        SWSS_LOG_ERROR("Failed to get switch attribute %u , rv:%d", attr.id, status);


if it is not supported, i think it is better to remove query. otherwise, in the syslog, we are going to continously having this error log for this unsupported attribute. it will cause confusion.

you mean to remove it from m_resourcesMap on the first failure with SAI_STATUS_NOT_SUPPORTED?

please note that the default poll interval is 5 minutes and the logs will be observed once every 5 minutes (poll interval)

@PrabhuSreenivasan , we change the polling interval to 1 sec while running tests and this can come pretty often in that case causing test failures.

@lguohan, @prsunny, I have updated the code to remove any unavailable resource from the map.

prsunny · 2020-12-03T02:42:22Z

orchagent/crmorch.cpp

-        for (const auto &cnt : m_resourcesMap.at(i.second).countersMap)
+        try
+        {
+            for (const auto &cnt : m_resourcesMap.at(i.second).countersMap)


Since you are erasing the unsupported resource, do we hit this case?

The code iterates over the const map crmUsedCntsTableMap which includes the unsupported resources as well. The out_of_range exception will be hit when it tries to access m_resourcesMap.at(i.second).countersMap for any of the unsupported resources.

Signed-off-by: Prabhu Sreenivasan <[email protected]>

PrabhuSreenivasan · 2020-12-05T05:29:29Z

retest this please

prsunny · 2020-12-09T03:54:21Z

@PrabhuSreenivasan , could you please look into the VS test errors for:

test_crm.TestCrm.test_Configure_snat
test_crm.TestCrm.test_Configure_dnat
test_crm.TestCrm.test_Configure_ipmc

https://sonic-jenkins.westus2.cloudapp.azure.com/job/vs/job/sonic-swss-build-pr/2518/

PrabhuSreenivasan · 2020-12-09T17:55:37Z

@PrabhuSreenivasan , could you please look into the VS test errors for:

test_crm.TestCrm.test_Configure_snat
test_crm.TestCrm.test_Configure_dnat
test_crm.TestCrm.test_Configure_ipmc

https://sonic-jenkins.westus2.cloudapp.azure.com/job/vs/job/sonic-swss-build-pr/2518/

@prsunny, The test relies on crm CLI commands for snat, dnat, ipmc and those commands will be available when the associated PR sonic-net/sonic-utilities/pull/1258 gets merged.

ben-gale · 2020-12-15T23:17:38Z

All, @arlakshm - this has been quiet for 6 days - can we get some review input so we can move forwards please? The clock is ticking ...

ben-gale · 2020-12-22T21:38:21Z

@arlakshm , @lguohan, @prsunny - What's the next step here? This has been inactive for 6 days now - how do we move towards merge pls?

Signed-off-by: Prabhu Sreenivasan <[email protected]>

lgtm-com · 2020-12-24T12:04:30Z

This pull request introduces 3 alerts when merging 053fe31 into c39a4b1 - view on LGTM.com

new alerts:

3 for Unused local variable

Signed-off-by: Prabhu Sreenivasan <[email protected]>

arlakshm · 2020-12-24T16:35:52Z

@PrabhuSreenivasan, the unit tests are failing. Please take a look.

lgtm-com · 2020-12-25T03:56:58Z

This pull request introduces 1 alert and fixes 7 when merging 59b19a7 into 9ed3026 - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

4 for Unused import
1 for Redundant comparison
1 for Unused local variable
1 for Variable defined multiple times

Signed-off-by: Prabhu Sreenivasan <[email protected]>

arlakshm · 2020-12-28T19:24:08Z

@PrabhuSreenivasan, majority of the Unit tests are commented out. Please re-enable them so that this PR can be merged.
This change is required to fix the https://github.com/Azure/sonic-buildimage/issues/6302

Signed-off-by: Prabhu Sreenivasan <[email protected]>

PrabhuSreenivasan · 2020-12-29T07:41:41Z

Looks like it picked up 532 which did not have CRM changes. The latest one https://sonic-jenkins.westus2.cloudapp.azure.com/job/vs/job/buildimage-vs-all/533/ has the changes. will trigger test again.

PrabhuSreenivasan · 2020-12-29T07:41:54Z

retest vs please

PrabhuSreenivasan · 2020-12-29T11:21:53Z

@PrabhuSreenivasan, majority of the Unit tests are commented out. Please re-enable them so that this PR can be merged.
This change is required to fix the Azure/sonic-buildimage#6302

Done.

arlakshm

lgtm

* [config][vxlan]fix 'vxlan evpn_nvo add' command error Remove extra 'CONFIG_DB' argument from db.cfgdb.get_keys() Signed-off-by: shikenghua <[email protected]>

What I did Added support for snat, dnat and ipmc resources under CRM module. Why I did it New feature NAT adds new resources snat_enty and dnat_entry that needs to be monitored. ipmc_entry tracks IP multicast resources used by switch. Associated PRs sonic-net/sonic-utilities/pull/1258 sonic-net/sonic-buildimage#6012 Signed-off-by: Prabhu Sreenivasan <[email protected]>

This was referenced Nov 23, 2020

Add support for snat, dnat and ipmc crm resources sonic-net/sonic-utilities#1258

Merged

Add support for snat, dnat and ipmc crm resources sonic-net/sonic-buildimage#6012

Merged

PrabhuSreenivasan force-pushed the crm_ipmc_nat branch from b60d12f to 46e902e Compare November 24, 2020 17:30

lguohan reviewed Nov 25, 2020

View reviewed changes

PrabhuSreenivasan force-pushed the crm_ipmc_nat branch from 46e902e to be8bfaa Compare December 2, 2020 06:26

prsunny reviewed Dec 3, 2020

View reviewed changes

[crm] Add support for snat, dnat and ipmc crm resources

12e5b1e

Signed-off-by: Prabhu Sreenivasan <[email protected]>

PrabhuSreenivasan force-pushed the crm_ipmc_nat branch from be8bfaa to 12e5b1e Compare December 4, 2020 06:15

rlhui requested a review from arlakshm December 9, 2020 20:30

ben-gale added the brcm-upstream label Dec 11, 2020

prsunny previously approved these changes Dec 16, 2020

View reviewed changes

arlakshm previously approved these changes Dec 23, 2020

View reviewed changes

[crm] Add support for snat, dnat and ipmc crm resources

053fe31

Signed-off-by: Prabhu Sreenivasan <[email protected]>

PrabhuSreenivasan dismissed stale reviews from arlakshm and prsunny via 053fe31 December 24, 2020 11:18

fix LGTM errors

f4d439b

Signed-off-by: Prabhu Sreenivasan <[email protected]>

comment ipmc test case

04ab199

Signed-off-by: Prabhu Sreenivasan <[email protected]>

PrabhuSreenivasan force-pushed the crm_ipmc_nat branch from 59b19a7 to 04ab199 Compare December 25, 2020 10:30

PrabhuSreenivasan mentioned this pull request Dec 25, 2020

Add support for SAI_SWITCH_ATTR_AVAILABLE_IPMC_ENTRY needed by CRM sonic-net/sonic-sairedis#756

Merged

enable CRM CLI based tests

1d02c5e

Signed-off-by: Prabhu Sreenivasan <[email protected]>

arlakshm approved these changes Dec 29, 2020

View reviewed changes

arlakshm merged commit a43f6be into sonic-net:master Dec 29, 2020

rlhui linked an issue Dec 30, 2020 that may be closed by this pull request

orchagent reporting error 'Unknown attribute snat_entry_threshold_type' #1574

Closed

PrabhuSreenivasan deleted the crm_ipmc_nat branch January 4, 2021 06:28

PrabhuSreenivasan mentioned this pull request Jan 22, 2021

orchagent reporting error 'Unknown attribute snat_entry_threshold_type' #1574

Closed

Junchao-Mellanox mentioned this pull request Jan 26, 2021

Not supported attribute SAI_SWITCH_ATTR_AVAILABLE_IPMC_ENTRY sonic-net/sonic-buildimage#6563

Closed

Conversation

PrabhuSreenivasan commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PrabhuSreenivasan commented Nov 24, 2020

Uh oh!

PrabhuSreenivasan commented Nov 25, 2020

Uh oh!

lguohan Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

PrabhuSreenivasan Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

lguohan Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

PrabhuSreenivasan Nov 26, 2020

Choose a reason for hiding this comment

Uh oh!

prsunny Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

PrabhuSreenivasan Dec 2, 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!

prsunny Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

PrabhuSreenivasan Dec 3, 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!

PrabhuSreenivasan commented Dec 5, 2020

Uh oh!

prsunny commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PrabhuSreenivasan commented Dec 9, 2020

Uh oh!

ben-gale commented Dec 15, 2020

Uh oh!

ben-gale commented Dec 22, 2020

Uh oh!

lgtm-com bot commented Dec 24, 2020

Uh oh!

arlakshm commented Dec 24, 2020

Uh oh!

lgtm-com bot commented Dec 25, 2020

Uh oh!

arlakshm commented Dec 28, 2020

Uh oh!

PrabhuSreenivasan commented Dec 29, 2020

Uh oh!

PrabhuSreenivasan commented Dec 29, 2020

Uh oh!

PrabhuSreenivasan commented Dec 29, 2020

Uh oh!

arlakshm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PrabhuSreenivasan commented Nov 19, 2020 •

edited

Loading

PrabhuSreenivasan Dec 2, 2020 •

edited

Loading

PrabhuSreenivasan Dec 3, 2020 •

edited

Loading

prsunny commented Dec 9, 2020 •

edited

Loading