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

kirankella · 2019-09-24T08:57:02Z

Changes in the SAI redis APIs and the syncd to support NAT.

Related PRs for this feature:
sonic-buildimage repo - sonic-net/sonic-buildimage#3494
sonic-swss repo - sonic-net/sonic-swss#1059
sonic-swss repo - sonic-net/sonic-swss#1125
sonic-swss repo - sonic-net/sonic-swss#1126
sonic-utilities repo - sonic-net/sonic-utilities#645
sonic-linux-kernel repo - sonic-net/sonic-linux-kernel#100
sonic-swss-common repo - sonic-net/sonic-swss-common#304

Requirment is that the SAI library implements the SAI NAT specification (SAI 1.5).

Signed-off-by: kiran.kella@broadcom.com

stepanblyschak

LGTM overall, please check indentation inconsistencies and extra newlines

stepanblyschak · 2019-10-16T13:59:58Z

meta/sai_meta.cpp

+
+    if (status == SAI_STATUS_SUCCESS)
+    {
+      meta_generic_validation_post_get(meta_key, nat_entry->switch_id, attr_count, attr_list);


inconsistent indentation here

stepanblyschak · 2019-10-16T14:00:33Z

meta/sai_meta.cpp

+
+    if (status == SAI_STATUS_SUCCESS)
+    {
+      meta_generic_validation_post_remove(meta_key);


inconsistent indentation here

stepanblyschak · 2019-10-16T14:04:10Z

meta/sai_meta.cpp

+    if (nat_entry == NULL)
+    {
+        SWSS_LOG_ERROR("nat_entry pointer is NULL");
+


extra newline. Same for many lines

Inline with newlines added in other similar functions like meta_sai_create_fdb_entry and meta_sai_validate_fdb_entry. Let me know if the newlines are not needed.

stepanblyschak · 2019-10-16T14:06:35Z

vslib/src/sai_vs_nat.cpp

+sai_status_t vs_remove_nat_entry(
+        _In_ const sai_nat_entry_t *nat_entry)
+{
+   MUTEX();


3-spaces indentation here.

- Added natsyncd and warmboot related changes. Link to NAT HLD : https://github.com/Azure/SONiC/blob/master/doc/nat/nat_design_spec.md Depends on: sonic-swss : 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

Signed-off-by: kiran.kella@broadcom.com

kirankella · 2019-11-13T04:49:09Z

Resolved merge conflict with latest master and updated vs library for nat pytests..

lguohan · 2019-11-13T05:38:57Z

syncd/syncd_applyview.cpp

+ *
+ * @return Best match object if found or nullptr.
+ */
+std::shared_ptr<SaiObj> findCurrentBestMatchForNatEntry(


@kcudnik , can you check the comparison logic here?

looks good but some unittest would be helpful

@kirankella, can you add unit test?

lguohan · 2019-11-13T05:39:37Z

syncd/syncd_hard_reinit.cpp

@@ -1098,6 +1099,45 @@ void processRoutes(bool defaultOnly)
    }
 }

+void processNatEntries()


@kcudnik , can you the code logic here?

what do you mean "the code" ?

I mean this function processNatEntries.

i dont understand the question "can you the code logic here?" what this mean ? do you want to to review the code (obvious since i review entire PR) can you explain more ?

lguohan · 2019-11-13T05:40:28Z

retest this please

meta/saiserialize.cpp

kcudnik

looks very good, could you add unittests for that comparison logic ? they could be in separate PR to not make this too big

meta/saiserialize.cpp

kcudnik · 2019-11-13T09:57:38Z

syncd/syncd_applyview.cpp

+                        case SAI_NAT_ENTRY_ATTR_HIT_BIT_COR:
+                        case SAI_NAT_ENTRY_ATTR_HIT_BIT:
+
+                            // when reading asic view, ignore Nat entry hit-bit attribute


why thos need to be skipped?

@kcudnik These 2 attributes in a NAT entry are not required in the comparison. They are dynamic attributes meant to be used for querying the active hardware status for the traffic flows.

if thats the case then those attributes should be read only

kcudnik · 2019-11-13T09:59:21Z

syncd/syncd_applyview.cpp

+ *
+ * @return Best match object if found or nullptr.
+ */
+std::shared_ptr<SaiObj> findCurrentBestMatchForNatEntry(


looks good but some unittest would be helpful

kcudnik · 2019-11-13T09:59:57Z

syncd/syncd_hard_reinit.cpp

@@ -1098,6 +1099,45 @@ void processRoutes(bool defaultOnly)
    }
 }

+void processNatEntries()


what do you mean "the code" ?

kirankella · 2019-11-19T17:01:56Z

looks very good, could you add unittests for that comparison logic ? they could be in separate PR to not make this too big

@kcudnik @lguohan We can add the unittests in a separate PR like you mentioned.

lguohan · 2019-11-20T06:10:14Z

retest this please

kcudnik · 2019-11-20T12:23:19Z

@kirankella can you address my comments?

- Added natsyncd and warmboot related changes. Link to NAT HLD : https://github.com/Azure/SONiC/blob/master/doc/nat/nat_design_spec.md Depends on: sonic-swss : 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

* Changes in sonic-sairedis repo to support the NAT feature. Signed-off-by: kiran.kella@broadcom.com

…#519) * Changes in sonic-sairedis repo to support the NAT feature. Signed-off-by: kiran.kella@broadcom.com

AkhileshSamineni mentioned this pull request Sep 24, 2019

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

Merged

stepanblyschak suggested changes Oct 16, 2019

View reviewed changes

stepanblyschak approved these changes Oct 18, 2019

View reviewed changes

marian-pritsak approved these changes Oct 28, 2019

View reviewed changes

AkhileshSamineni mentioned this pull request Nov 11, 2019

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

Merged

AkhileshSamineni mentioned this pull request Nov 11, 2019

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

Merged

kirankella added 2 commits November 12, 2019 20:41

Changes in sonic-sairedis repo to support the NAT feature.

f1db4f9

Signed-off-by: kiran.kella@broadcom.com

Fixed vs test reported spellcheck errors

c2cb0ec

kirankella force-pushed the nat_changes_in_sonic branch from 123395c to c2cb0ec Compare November 13, 2019 04:45

lguohan reviewed Nov 13, 2019

View reviewed changes

lguohan requested a review from kcudnik November 13, 2019 05:39

kcudnik reviewed Nov 13, 2019

View reviewed changes

meta/saiserialize.cpp Outdated Show resolved Hide resolved

kcudnik requested changes Nov 13, 2019

View reviewed changes

addressed review comments

45f88a2

kcudnik approved these changes Nov 20, 2019

View reviewed changes

lguohan merged commit 5ef1764 into sonic-net:master Nov 22, 2019

abdosi added the Request for 201911 Branch Label for changes in master that applies for 201911 Branch also label Dec 31, 2019

abdosi pushed a commit that referenced this pull request Jan 3, 2020

Changes in sonic-sairedis repo to support the NAT feature. (#519)

579ee27

* Changes in sonic-sairedis repo to support the NAT feature. Signed-off-by: kiran.kella@broadcom.com

abdosi added the Included in 201911 Branch label Jan 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kirankella commented Sep 24, 2019 •

edited

Loading

stepanblyschak left a comment

stepanblyschak Oct 16, 2019

kirankella Oct 18, 2019

stepanblyschak Oct 16, 2019

kirankella Oct 18, 2019

stepanblyschak Oct 16, 2019

kirankella Oct 18, 2019

stepanblyschak Oct 16, 2019

kirankella Oct 18, 2019

kirankella commented Nov 13, 2019

lguohan Nov 13, 2019

kcudnik Nov 13, 2019

lguohan Nov 13, 2019

lguohan Nov 13, 2019

kcudnik Nov 13, 2019

lguohan Nov 13, 2019

kcudnik Nov 13, 2019

lguohan commented Nov 13, 2019

kcudnik left a comment

kcudnik Nov 13, 2019

kirankella Nov 20, 2019

kcudnik Nov 20, 2019

kcudnik Nov 13, 2019

kcudnik Nov 13, 2019

kirankella commented Nov 19, 2019

lguohan commented Nov 20, 2019

kcudnik commented Nov 20, 2019

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

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

Conversation

kirankella commented Sep 24, 2019 • edited Loading

stepanblyschak left a 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 13, 2019

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

lguohan commented Nov 13, 2019

kcudnik left a 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 19, 2019

lguohan commented Nov 20, 2019

kcudnik commented Nov 20, 2019

kirankella commented Sep 24, 2019 •

edited

Loading