Natsyncd changes in sonic-swss submodule to support NAT feature. #1126
Conversation
| { | ||
| m_AppRestartAssist = new AppRestartAssist(pipelineAppDB, "neighsyncd", "swss", DEFAULT_NEIGHSYNC_WARMSTART_TIMER); |
There was a problem hiding this comment.
Can please elaborate why we need the changes in neighsyncd and warmrestartAssist ?
There was a problem hiding this comment.
@arlakshm The AppRestartAssist class was handling the reconciliation logic only for a single app-db table. This class is enhanced to handle reconciliation logic for multiple app-db tables, if the user of this class is the producer for multiple app-db ables as in the case of NAT. Earlier neighsyncd is the producer for a single app-db table and passing the app-db table to AppRestartAssist. Since this class is enhanced now, neighsyncd code is modified to call the new APIs.
natsyncd/natsync.cpp
Outdated
| /* If a matching Static NAPT entry exists in the APP_DB, | ||
| * it has higher priority than the dynamic napt entry. */ | ||
| SWSS_LOG_INFO("SNAPT %s: static entry exists, not processing the NAPT notification", opStr.c_str()); | ||
| if (m_AppRestartAssist->isWarmStartInProgress()) |
There was a problem hiding this comment.
This condition may not be hit ? on the top we are checking if (! m_AppRestartAssist->isWarmStartInProgress())
There was a problem hiding this comment.
Yes, this is dead code, will remove it.
natsyncd/natsync.cpp
Outdated
| } | ||
| else | ||
| { | ||
| if (m_AppRestartAssist->isWarmStartInProgress()) |
There was a problem hiding this comment.
Yes, this is dead code, will remove it.
| * - If SNAPT and DNAPT conditions are met or if there is no static | ||
| * or dynamic Twice NAT entry, then it is a Twice NAPT entry. | ||
| */ | ||
| int NatSync::addNatEntry(struct nfnl_ct *ct, struct naptEntry &entry, bool addFlag) |
There was a problem hiding this comment.
There are lot of DB gets and checks being done in this function. Will there be a performance impact in-terms of Netlink message loss, when the scale is high ?
There was a problem hiding this comment.
Per NAT entry, atmost 2 db gets are done and not seen netlink message loss with 600 pps traffic.
DB gets are done to avoid storing another copy of nat tables (apart from appl_db) within natsyncd.
natsyncd/natsyncd.cpp
Outdated
| nfnl.registerGroup(NFNLGRP_CONNTRACK_UPDATE); | ||
| nfnl.registerGroup(NFNLGRP_CONNTRACK_DESTROY); | ||
|
|
||
| cout << "Listens to conntrack messages..." << endl; |
| { | ||
| /* If a matching Static Twice NAT entry exists in the APP_DB, | ||
| * it has higher priority than the dynamic twice nat entry. */ | ||
| if ((fvField(iter) == "entry_type") && (fvValue(iter) == "static")) |
There was a problem hiding this comment.
How was it even trapped if a static entry exists?
There was a problem hiding this comment.
For static snat entry, the packet can come up in case of h/w L3 destination lookup failure. To handle such cases, we need the SNAT rule for static entry in the iptables.
- 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: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
AkhileshSamineni
force-pushed
the
natsyncd_changes_in_sonic
branch
from
2ef39c6
to
3ca530c
Compare
|
Retest this please. |
|
retest this please. |
|
Retest this please. |
2 similar comments
|
Retest this please. |
|
Retest this please. |
Add the missing import of `netaddr` for `show ip interfaces`. Signed-off-by: Longxiang Lyu <lolv@microsoft.com>
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 : #1059 and #1125
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