Allow state db to take modified entries made to the tunnel decap table#3960
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
3fe4837 to
93a087a
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR enables synchronization of tunnel decap table modifications to STATE_DB, preventing database inconsistencies between STATE_DB, ASIC_DB, and APPL_DB. The implementation tracks field changes via a state_changed flag and publishes updates to STATE_DB when modifications occur to existing tunnel entries.
Key Changes
- Added
state_changedflag tracking indoDecapTunnelTask()to detect when tunnel fields are modified - Set the flag when modifiable fields (dscp_mode, ttl_mode, QoS maps) are updated on existing tunnels
- Added STATE_DB synchronization call when changes are detected
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| orchagent/tunneldecaporch.cpp | Implements state change tracking and STATE_DB synchronization for tunnel decap table modifications |
| tests/mock_tests/tunneldecaporch_ut.cpp | Adds comprehensive unit tests for TunnelDecapOrch functionality including creation, modification, and deletion scenarios |
| tests/mock_tests/Makefile.am | Registers the new tunneldecaporch_ut.cpp test file in the build system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
191dc36 to
03d3d78
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@kperumalbfn can you pls check why the checks won't run? The agents don't seem to be grabbing the workers before the 45 min timeout |
|
@prsunny @kperumalbfn can you pls review? |
orchagent/tunneldecaporch.cpp
Outdated
| } | ||
| } | ||
|
|
||
| if (exists && state_changed) { |
There was a problem hiding this comment.
Why you want an addition 'state_changed variable? why not simply check 'exists' and set STATE_DB?
There was a problem hiding this comment.
If none of the fields were updated, then there's no point updating the state db which this state_changed variable keeps track of
There was a problem hiding this comment.
Its fine, the current check is 'if exists' its anyways setting the SAI attribute.
There was a problem hiding this comment.
not necessarily, some attributes like ecn are not set since they are not supported in SAI and still have the exists keyword check; the state_changed variable ensures that if the SAI attribute is not being changed, then the state db does not take in a false change
There was a problem hiding this comment.
Could you add a comment that ECN_MODE SET attribute is not supported in SAI, so that we will revisit this check after updating SAI headers.
There was a problem hiding this comment.
also, this is not a frequent operation of setting the fields, so imo, if 'exists' its still ok to just update STATE_DB. it will be a no-op
03d3d78 to
c1b78ce
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
84ef20b to
5c6bfe0
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
orchagent/tunneldecaporch.cpp
Outdated
| } | ||
| } | ||
|
|
||
| if (exists) { |
There was a problem hiding this comment.
no need for valid since multiple attributes can be changed in a single call to this function. for instance, dscp and ecn value can be in SET mode where dscp set is allowed and SAI change is made to change asic attribute but ecn mode set is not allowed and the valid flag is set to false. in this case, I still want the dscp mode change to be reflected in the state db. Moreover, state db changes are only reflected if the cached tunnel table has changes to it which doesn't happen when valid = false.
There was a problem hiding this comment.
ok, i was thinking of this specific case - SWSS_LOG_ERROR("unknown decap tunnel table attribute '%s'.", fvField(i).c_str()); where valid will be false. But agree with you considering the current code flow of setting valid field
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…cap table (sonic-net#3960)" This reverts commit 820eb74.
sonic-net#3960) What I did Allow state db to take modified entries made to the tunnel decap table Why I did it Prevent DB inconsistency between state, asic and appl db How I verified it redis-cli -n 6 HGETALL "TUNNEL_DECAP_TABLE|IPINIP_V6_TUNNEL" "tunnel_type" "IPINIP" "dscp_mode" "pipe" "ecn_mode" "standard" "ttl_mode" "pipe" /var/log/syslog.1:2025 Nov 10 19:18:32.730385 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_TUNNEL' have been synchronised in STATE_DB /var/log/syslog.1:2025 Nov 10 19:18:32.740298 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_V6_TUNNEL' have been synchronised in STATE_DB Signed-off-by: Kalash Nainwal <kalash@nexthop.ai>
sonic-net#3960) What I did Allow state db to take modified entries made to the tunnel decap table Why I did it Prevent DB inconsistency between state, asic and appl db How I verified it redis-cli -n 6 HGETALL "TUNNEL_DECAP_TABLE|IPINIP_V6_TUNNEL" "tunnel_type" "IPINIP" "dscp_mode" "pipe" "ecn_mode" "standard" "ttl_mode" "pipe" /var/log/syslog.1:2025 Nov 10 19:18:32.730385 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_TUNNEL' have been synchronised in STATE_DB /var/log/syslog.1:2025 Nov 10 19:18:32.740298 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_V6_TUNNEL' have been synchronised in STATE_DB
sonic-net#3960) What I did Allow state db to take modified entries made to the tunnel decap table Why I did it Prevent DB inconsistency between state, asic and appl db How I verified it redis-cli -n 6 HGETALL "TUNNEL_DECAP_TABLE|IPINIP_V6_TUNNEL" "tunnel_type" "IPINIP" "dscp_mode" "pipe" "ecn_mode" "standard" "ttl_mode" "pipe" /var/log/syslog.1:2025 Nov 10 19:18:32.730385 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_TUNNEL' have been synchronised in STATE_DB /var/log/syslog.1:2025 Nov 10 19:18:32.740298 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_V6_TUNNEL' have been synchronised in STATE_DB
sonic-net#3960) What I did Allow state db to take modified entries made to the tunnel decap table Why I did it Prevent DB inconsistency between state, asic and appl db How I verified it redis-cli -n 6 HGETALL "TUNNEL_DECAP_TABLE|IPINIP_V6_TUNNEL" "tunnel_type" "IPINIP" "dscp_mode" "pipe" "ecn_mode" "standard" "ttl_mode" "pipe" /var/log/syslog.1:2025 Nov 10 19:18:32.730385 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_TUNNEL' have been synchronised in STATE_DB /var/log/syslog.1:2025 Nov 10 19:18:32.740298 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_V6_TUNNEL' have been synchronised in STATE_DB Signed-off-by: Lawrence Lee <lawlee@microsoft.com>
sonic-net#3960) What I did Allow state db to take modified entries made to the tunnel decap table Why I did it Prevent DB inconsistency between state, asic and appl db How I verified it redis-cli -n 6 HGETALL "TUNNEL_DECAP_TABLE|IPINIP_V6_TUNNEL" "tunnel_type" "IPINIP" "dscp_mode" "pipe" "ecn_mode" "standard" "ttl_mode" "pipe" /var/log/syslog.1:2025 Nov 10 19:18:32.730385 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_TUNNEL' have been synchronised in STATE_DB /var/log/syslog.1:2025 Nov 10 19:18:32.740298 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_V6_TUNNEL' have been synchronised in STATE_DB Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>
What I did
Allow state db to take modified entries made to the tunnel decap table
Why I did it
Prevent DB inconsistency between state, asic and appl db
How I verified it
redis-cli -n 6 HGETALL "TUNNEL_DECAP_TABLE|IPINIP_V6_TUNNEL"
/var/log/syslog.1:2025 Nov 10 19:18:32.730385 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_TUNNEL' have been synchronised in STATE_DB
/var/log/syslog.1:2025 Nov 10 19:18:32.740298 bjw-can-2700-2 NOTICE swss#orchagent: :- doDecapTunnelTask: Fields for TUNNEL_DECAP_TABLE entry 'IPINIP_V6_TUNNEL' have been synchronised in STATE_DB
Added comprehensive unit tests too which pass
Details if related