Orchagent changes in sonic-swss submodule to support NAT feature. #1125

AkhileshSamineni · 2019-11-11T07:24:36Z

Added orchagent and zone related changes.

Link to NAT HLD : https://github.com/Azure/SONiC/blob/master/doc/nat/nat_design_spec.md

Depends on:
sonic-swss : #1059 and #1126
sonic-swss-common : sonic-net/sonic-swss-common#304
sonic-linux-kernel : sonic-net/sonic-linux-kernel#100
sonic-sairedis : sonic-net/sonic-sairedis#519 and sonic-net/sonic-sairedis#546

Signed-off-by: Akhilesh Samineni [email protected]

… as part of PRs sonic-net#1125 and sonic-net#1126

Signed-off-by: Akhilesh Samineni <[email protected]>

arlakshm · 2019-11-15T19:02:50Z

orchagent/natorch.cpp

+    sai_attribute_t  attr;
+    memset(&attr, 0, sizeof(attr));
+    attr.id = SAI_SWITCH_ATTR_AVAILABLE_SNAT_ENTRY;
+    maxAllowedNatEntries = 0;


please rename this variable as we are using this to check the number SNAT entries

renamed this variable to maxAllowedSNatEntries.

arlakshm · 2019-11-15T19:03:38Z

orchagent/natorch.cpp

+    attr.id = SAI_SWITCH_ATTR_AVAILABLE_SNAT_ENTRY;
+    maxAllowedNatEntries = 0;
+
+    status = sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr);


please move this SAI call to when NAT is enabled from CLI

Get operation on a unimplemented SAI attribute doesn't result in crash, so retaining in current changes as discussed.

arlakshm · 2019-11-15T19:05:03Z

orchagent/natorch.cpp

+    if (m_natEntries.find(ip_address) != m_natEntries.end())
+    {
+        SWSS_LOG_INFO("Duplicate %s %s NAT entry with ip %s and it's translated ip %s, do nothing",
+                      entry.entry_type.c_str(), entry.nat_type.c_str(), ip_address.to_stri


May be can do this check at the beginning of the function

NAT entries have to be cached irrespective of NAT is enabled or not.

arlakshm · 2019-11-15T23:21:57Z

orchagent/natorch.cpp

+    if (m_natEntries.find(ip_address) != m_natEntries.end())
+    {
+        SWSS_LOG_INFO("Duplicate %s %s NAT entry with ip %s and it's translated ip %s, do nothing",
+                      entry.entry_type.c_str(), entry.nat_type.c_str(), ip_address.to_stri


Why are counters being updated here ?

The existing dynamic entry is replaced by static entry as Static entry has higher priority, so decremented the dynamic and incremented the static counters.

arlakshm · 2019-11-16T00:33:46Z

orchagent/natorch.cpp

+    if (m_natEntries.find(ip_address) != m_natEntries.end())
+    {
+        SWSS_LOG_INFO("Duplicate %s %s NAT entry with ip %s and it's translated ip %s, do nothing",
+                      entry.entry_type.c_str(), entry.nat_type.c_str(), ip_address.to_stri


do we need to clean up the entries which are added to HW as well ?

Yes, it will also delete the entries from hardware.

arlakshm · 2019-11-16T01:25:00Z

orchagent/natorch.cpp

+    if (m_natEntries.find(ip_address) != m_natEntries.end())
+    {
+        SWSS_LOG_INFO("Duplicate %s %s NAT entry with ip %s and it's translated ip %s, do nothing",
+                      entry.entry_type.c_str(), entry.nat_type.c_str(), ip_address.to_stri


As I understand, this notification comes when a L3 interface is added to NAT ZONE, so every time a interface is added to a NAT_ZONE. I have 2 questions:

since all the conntrack entries are flushed, the entries in asic will also be removed right ? What will happen to the traffic, they will be dropped ?

When we need to flush of conntrack entries from natorch can this be done from natmgr instead ?

This notification comes when do "sonic-clear nat translations" is executed. Yes all the entries in the asic are removed. Current traffic flows will see a drop and relearned again and added to asic.

OA is the component that tries to keep the Kernel conntrack entries and the hardware entries in sync. So, it is doing the flush entries.

arlakshm · 2019-11-16T01:31:31Z

orchagent/natorch.cpp

+    if (m_natEntries.find(ip_address) != m_natEntries.end())
+    {
+        SWSS_LOG_INFO("Duplicate %s %s NAT entry with ip %s and it's translated ip %s, do nothing",
+                      entry.entry_type.c_str(), entry.nat_type.c_str(), ip_address.to_stri


In the current SONiC architecture, the kernel entries are not updated/added/deleted from orchagent. Can you move this functionality to natmgr.

OA is responsible for checking the hardware activity/hitbit of each of the entry in its cache. Since the cache is in OA code, it is the appropriate place to update the timeouts for the corresponding entries in the kernel.

I understand OA is checking the hitbit, but in the current SONiC architecture the kernel entries are no update from OA. The mgr modules normally updates the kernel entries

Agree with Arvind.

The natmgr has no idea about the hardware activity of nat entries, so it cannot update the entries timeouts in kernel, if natmgr has to update the timeouts, OA has to inform the natmgr through separate communication channel which is an overhead and has it issues with scaling.

The NAT entries are not updated real time, polling for hit-bit happens every 30 seconds, so adding another hop will not have a big impact on the performance.

As discussed offline, lets keep the current code for now. Let's open a issue to track this change. We can make the code changes in the next release.

arlakshm · 2019-11-16T01:38:44Z

orchagent/copporch.cpp

@@ -73,6 +73,8 @@ static map<string, sai_hostif_trap_type_t> trap_id_map = {
    {"udld", SAI_HOSTIF_TRAP_TYPE_UDLD},
    {"bfd", SAI_HOSTIF_TRAP_TYPE_BFD},
    {"bfdv6", SAI_HOSTIF_TRAP_TYPE_BFDV6}
+    {"src_nat_miss", SAI_HOSTIF_TRAP_TYPE_SNAT_MISS},


Can we make sure that these trap ids are created only on platform which support NAT

Done based on the feature check.

prsunny · 2019-11-22T01:52:35Z

orchagent/Makefile.am

            chassisorch.cpp \
            debugcounterorch.cpp

 orchagent_SOURCES += flex_counter/flex_counter_manager.cpp flex_counter/flex_counter_stat_manager.cpp
 orchagent_SOURCES += debug_counter/debug_counter.cpp debug_counter/drop_counter.cpp

+


Please remove this extra line

prsunny · 2019-11-22T01:53:47Z

orchagent/copporch.h

@@ -50,6 +50,7 @@ class CoppOrch : public Orch
 protected:
    object_map m_trap_group_map;
    bool       enable_sflow_trap;
+    bool       isNatSupported;


Please follow the same style for variables as in file

Removed this variable and using 'gIsNatSupported' global variable.

prsunny · 2019-11-22T01:58:13Z

orchagent/intfsorch.h

    std::set<IpPrefix> getSubnetRoutes();
+    std::map<string, uint32_t> m_nat_zone;
+    bool isNatSupported;


please use same style

Removed this variable and using 'gIsNatSupported' global variable.

prsunny · 2019-11-22T02:01:34Z

orchagent/intfsorch.cpp

@@ -1015,3 +1087,30 @@ void IntfsOrch::doTask(SelectableTimer &timer)
        }
    }
 }
+
+void IntfsOrch::getNatSupportedInfo()


I see that you are repeating this same function in multiple places. Can you do this query one time, like in switchorch, and use a single API to check if NAT is supported.

Ok. Moved this functionality to main.cpp file and updating the global variable 'gIsNatSupported'.

prsunny · 2019-11-22T02:12:14Z

orchagent/intfsorch.cpp

+                        m_nat_zone[alias] = nat_zone_id;
+                        if (isNatSupported)
+                        {
+                            setRouterIntfsNatZoneId(port, nat_zone_id);


I don't understand this call. The router interface is created only in setIntf. So this API is always going to return true after its first check. You are actually setting the zone in as part of Line 808. Can you please check?

User configures the new 'nat_zone' even after rif is created, this call is used to set the new 'nat_zone'.

Got it but the set of RIF attribute is currently handled in setIntf. It is done for SUB_PORT currently but I think can be modified to set NAT zone for any RIF type. Another follow up is, why do we need m_nat_zone map seperate? Can this nat zone be part of the port structure, like m_rif_id?

Setting NAT zone for Physical, Vlan and Lag interfaces only, so not handled in setIntf.
Yes, nat zone can be part of port structure, made changes to address it.

prsunny · 2019-11-22T02:12:54Z

orchagent/intfsorch.cpp

@@ -739,6 +796,21 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port)
    attr.value.u32 = port.m_mtu;
    attrs.push_back(attr);

+    if (isNatSupported)


This is not required.

Set operation on a unimplemented SAI attribute (SAI_ROUTER_INTERFACE_ATTR_NAT_ZONE_ID as of SAI 1.5) results in crash, to avoid it added this check here.

prsunny · 2019-11-22T02:13:17Z

orchagent/intfsorch.cpp

+        attr.id = SAI_ROUTER_INTERFACE_ATTR_NAT_ZONE_ID;
+        if (m_nat_zone.find(port.m_alias) == m_nat_zone.end())
+        {
+            attr.value.u32 = DEFAULT_NAT_ZONE_ID;


This is not required, its the SAI default

Yes, removed it.

prsunny · 2019-11-22T02:14:13Z

orchagent/intfsorch.cpp

+        else
+        {
+            attr.value.u32 = m_nat_zone[port.m_alias];
+        }


You can revisit the check as if natzone is present in configuration, set the attribute.

yes, modified the code to set the attribute if it is present in cache.

prsunny · 2019-11-22T02:15:26Z

orchagent/saihelper.cpp

 sai_samplepacket_api_t*     sai_samplepacket_api;
 sai_debug_counter_api_t*    sai_debug_counter_api;

+


Please remove this extra line

kirankella · 2019-11-24T04:54:53Z

The current nexthop tracking in OA works on different SAI implementations (those which track and those which do not track the nexthop changes) though it will be sub-optimal on the SAI that tracks too. We can proceed as-is with the current OA changes till the SAI implementations converge.

AkhileshSamineni · 2019-11-24T16:04:16Z

Retest this please.

… as part of PRs sonic-net#1125 and sonic-net#1126

Signed-off-by: Akhilesh Samineni <[email protected]>

- Added OA and zone related changes. Signed-off-by: Akhilesh Samineni <[email protected]>

Signed-off-by: Akhilesh Samineni <[email protected]>

arlakshm · 2019-12-20T19:10:25Z

@AkhileshSamineni, @kirankella,
There seems to conflicts with the with the PR. Please resolve the conflicts.

AkhileshSamineni · 2019-12-21T06:45:19Z

@AkhileshSamineni, @kirankella,
There seems to conflicts with the with the PR. Please resolve the conflicts.

@arlakshm Resolved merge conflicts.

AkhileshSamineni · 2020-01-10T09:25:06Z

retest this please.

AkhileshSamineni · 2020-01-12T18:03:39Z

Retest this please.

AkhileshSamineni · 2020-01-13T08:15:39Z

Retest this please.

AkhileshSamineni · 2020-01-16T05:54:24Z

Retest this please.

kirankella · 2020-01-17T08:21:11Z

retest vs please

kirankella · 2020-01-19T07:09:46Z

retest vs please

) Signed-off-by: Akhilesh Samineni [email protected]

Add more dependencies to setup.py. Now that we're building as a wheel, we can add all dependent packages to make building and installation more robust, as there will no longer be a need to install them explicitly. Move sonic-config-engine from `install_requires` to `tests_require`, as it is only needed by the unit tests. Remove code to install fastentrypoints package if not installed. When building a Python 3 wheel using python3 setup.py bdist_wheel, we would receive a permissions error when calling easy_install.main(['fastentrypoints']), as with Python 3 it appears elevated permissions are required to install the package. This appears to be a behavior change between the Python 2 and Python 3 versions of setuptools. This needs to be explicitly installed before building the package. We are already installing in the SONiC slave container. Will update the sonic-utilities README to specify, also.

AkhileshSamineni added a commit to AkhileshSamineni/sonic-swss that referenced this pull request Nov 11, 2019

Removed the Orchagent and Natsyncd related files as those are covered…

df5863b

… as part of PRs sonic-net#1125 and sonic-net#1126

This was referenced Nov 11, 2019

Natmgrd changes in sonic-swss sub module to support NAT feature. #1059

Merged

Natsyncd changes in sonic-swss submodule to support NAT feature. #1126

Merged

AkhileshSamineni added a commit to AkhileshSamineni/sonic-swss that referenced this pull request Nov 13, 2019

Removed pytest related files and added those to PR sonic-net#1125

7a0ccd1

Signed-off-by: Akhilesh Samineni <[email protected]>

arlakshm reviewed Nov 15, 2019

View reviewed changes

arlakshm reviewed Nov 16, 2019

View reviewed changes

kirankella mentioned this pull request Nov 21, 2019

Changes in sonic-sairedis repo to support the NAT feature. sonic-net/sonic-sairedis#519

Merged

prsunny reviewed Nov 22, 2019

View reviewed changes

AkhileshSamineni added a commit to AkhileshSamineni/sonic-swss that referenced this pull request Dec 10, 2019

Removed the Orchagent and Natsyncd related files as those are covered…

fd733b8

… as part of PRs sonic-net#1125 and sonic-net#1126

AkhileshSamineni added a commit to AkhileshSamineni/sonic-swss that referenced this pull request Dec 10, 2019

Removed pytest related files and added those to PR sonic-net#1125

5b09c41

Signed-off-by: Akhilesh Samineni <[email protected]>

AkhileshSamineni added 12 commits December 10, 2019 07:17

Orchagent changes in sonic-swss submodule to support NAT feature.

88fe411

- Added OA and zone related changes. Signed-off-by: Akhilesh Samineni <[email protected]>

Added aclorch changes for "do_not_nat" action.

e8b9dab

Signed-off-by: Akhilesh Samineni <[email protected]>

Fixed compilation issue.

f59ec87

Signed-off-by: Akhilesh Samineni <[email protected]>

Removed debug framework change to avoid compilation error

8246acb

Signed-off-by: Akhilesh Samineni <[email protected]>

Addressed review comments.

346f59a

Signed-off-by: Akhilesh Samineni <[email protected]>

Added changes to verify the NAT is supported before setting nat zone.

ff2c78c

Signed-off-by: Akhilesh Samineni <[email protected]>

Addressed review comments and added 'nat miss' traps in copp json.

3d2b900

Signed-off-by: Akhilesh Samineni <[email protected]>

Fixed compilation issue.

946e53d

Signed-off-by: Akhilesh Samineni <[email protected]>

Addressed review comments.

f9a8302

Signed-off-by: Akhilesh Samineni <[email protected]>

Added platform changes to avoid nexthop traching in OA.

63ebb3a

Signed-off-by: Akhilesh Samineni <[email protected]>

Added pytest testcases for NAT.

3350b85

Signed-off-by: Akhilesh Samineni <[email protected]>

Fixed compilation issue.

286ca21

Signed-off-by: Akhilesh Samineni <[email protected]>

AkhileshSamineni force-pushed the natorch_changes_in_sonic branch from e5acb9b to 286ca21 Compare December 10, 2019 15:29

Fixed compilation issue after forced push.

982abd6

Signed-off-by: Akhilesh Samineni <[email protected]>

prsunny previously approved these changes Dec 20, 2019

View reviewed changes

arlakshm previously approved these changes Dec 20, 2019

View reviewed changes

Merge branch 'master' into natorch_changes_in_sonic

5ad47af

AkhileshSamineni dismissed stale reviews from arlakshm and prsunny via 5ad47af December 21, 2019 06:41

arlakshm approved these changes Jan 17, 2020

View reviewed changes

prsunny approved these changes Jan 17, 2020

View reviewed changes

rlhui merged commit ea8b1da into sonic-net:master Jan 19, 2020

lguohan added a commit that referenced this pull request Jan 30, 2020

Orchagent changes in sonic-swss submodule to support NAT feature. (#1125

1cedf6b

) Signed-off-by: Akhilesh Samineni [email protected]

rlhui added the request_for_201911_branch label Jan 31, 2020

abdosi added the Included in 201911 Branch label Feb 4, 2020

abdosi pushed a commit that referenced this pull request Feb 4, 2020

Orchagent changes in sonic-swss submodule to support NAT feature. (#1125

f8661b9

) Signed-off-by: Akhilesh Samineni [email protected]

msosyak mentioned this pull request Feb 17, 2020

[COPP] Fix trap group name sonic-net/sonic-mgmt#1396

Merged

1 task

AkhileshSamineni mentioned this pull request Apr 30, 2020

Update NAT conntrack entries from natmgr instead of natorch #1274

Merged

		sai_samplepacket_api_t* sai_samplepacket_api;
		sai_debug_counter_api_t* sai_debug_counter_api;

Orchagent changes in sonic-swss submodule to support NAT feature. #1125

Orchagent changes in sonic-swss submodule to support NAT feature. #1125

Conversation

AkhileshSamineni commented Nov 11, 2019 • edited Loading

arlakshm Nov 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlakshm Nov 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirankella commented Nov 24, 2019

AkhileshSamineni commented Nov 24, 2019

arlakshm commented Dec 20, 2019

AkhileshSamineni commented Dec 21, 2019 • edited Loading

AkhileshSamineni commented Jan 10, 2020

AkhileshSamineni commented Jan 12, 2020

AkhileshSamineni commented Jan 13, 2020

AkhileshSamineni commented Jan 16, 2020

kirankella commented Jan 17, 2020

kirankella commented Jan 19, 2020

AkhileshSamineni commented Nov 11, 2019 •

edited

Loading

arlakshm Nov 15, 2019 •

edited

Loading

arlakshm Nov 16, 2019 •

edited

Loading

AkhileshSamineni commented Dec 21, 2019 •

edited

Loading