Changes in sonic-sairedis repo to support the NAT feature.#519
Changes in sonic-sairedis repo to support the NAT feature.#519lguohan merged 3 commits intosonic-net:masterfrom
Conversation
stepanblyschak
left a comment
stepanblyschak
left a comment
There was a problem hiding this comment.
LGTM overall, please check indentation inconsistencies and extra newlines
|
|
||
| if (status == SAI_STATUS_SUCCESS) | ||
| { | ||
| meta_generic_validation_post_get(meta_key, nat_entry->switch_id, attr_count, attr_list); |
There was a problem hiding this comment.
inconsistent indentation here
|
|
||
| if (status == SAI_STATUS_SUCCESS) | ||
| { | ||
| meta_generic_validation_post_remove(meta_key); |
There was a problem hiding this comment.
inconsistent indentation here
| if (nat_entry == NULL) | ||
| { | ||
| SWSS_LOG_ERROR("nat_entry pointer is NULL"); | ||
|
|
There was a problem hiding this comment.
extra newline. Same for many lines
There was a problem hiding this comment.
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.
vslib/src/sai_vs_nat.cpp
Outdated
| sai_status_t vs_remove_nat_entry( | ||
| _In_ const sai_nat_entry_t *nat_entry) | ||
| { | ||
| MUTEX(); |
There was a problem hiding this comment.
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
kirankella
force-pushed
the
nat_changes_in_sonic
branch
from
123395c to
c2cb0ec
Compare
|
Resolved merge conflict with latest master and updated vs library for nat pytests.. |
| * | ||
| * @return Best match object if found or nullptr. | ||
| */ | ||
| std::shared_ptr<SaiObj> findCurrentBestMatchForNatEntry( |
There was a problem hiding this comment.
@kcudnik , can you check the comparison logic here?
There was a problem hiding this comment.
looks good but some unittest would be helpful
| } | ||
| } | ||
|
|
||
| void processNatEntries() |
There was a problem hiding this comment.
what do you mean "the code" ?
There was a problem hiding this comment.
I mean this function processNatEntries.
There was a problem hiding this comment.
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 ?
|
retest this please |
kcudnik
left a comment
kcudnik
left a comment
There was a problem hiding this comment.
looks very good, could you add unittests for that comparison logic ? they could be in separate PR to not make this too big
| 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 |
There was a problem hiding this comment.
why thos need to be skipped?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
if thats the case then those attributes should be read only
| * | ||
| * @return Best match object if found or nullptr. | ||
| */ | ||
| std::shared_ptr<SaiObj> findCurrentBestMatchForNatEntry( |
There was a problem hiding this comment.
looks good but some unittest would be helpful
| } | ||
| } | ||
|
|
||
| void processNatEntries() |
There was a problem hiding this comment.
what do you mean "the code" ?
|
retest this please |
|
@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
abdosi
added
the
Request for 201911 Branch
* Changes in sonic-sairedis repo to support the NAT feature. Signed-off-by: [email protected]
…#519) * Changes in sonic-sairedis repo to support the NAT feature. Signed-off-by: [email protected]
6 participants
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: [email protected]