[action] [PR:1493] Fix to not miss the entire set of counters to be added in addObject for CounterContext::updateSupportedCount #1503
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…or CounterContext::updateSupportedCount Fixes issue: sonic-net/sonic-buildimage#21232 In MACSEC there are two set of counters one for INGRESS another for EGRESS which gets mapped to the same COUNTER type - CounterType::MACSEC_SA ( https://github.com/sonic-net/sonic-swss/blob/c20902f3195b5bf8a941045e131aa1b863b69fd0/orchagent/macsecorch.cpp#L2145 ) In the releases after 202205 we started seeing this behavior where the MACSEC RX counters were missing as mentioned in the issue 21232. Further debugging pointed to issue seen after this PR : sonic-net#1073 was merged. In this case when macsec orch tries to addCounter for INGRESS SA after the EGRESS SA, and it don't go through as the m_supportedCounters is not empty. For a fix I am removing the check in CounterContext::updateSupportedCount, which I think is ok as we anyways do a check later on in getSupportedCounters() API using isCounterSupported() before calling collectData() https://github.com/sonic-net/sonic-sairedis/blob/1684aecf7fcc0ecd01aab9bbef855ac483ae2b62/syncd/FlexCounter.cpp#L939 After the fix the macsec SA ingress counters are seen ``` --------------------------------------- ---------------------------------------------------------------- MACsec port(Ethernet216) --------------------- --------------- cipher_suite GCM-AES-XPN-256 enable true enable_encrypt true enable_protect true enable_replay_protect false profile MACSEC_PROFILE replay_window 0 send_sci true --------------------- --------------- MACsec Egress SC (xxx) ----------- - encoding_an 0 ----------- - MACsec Egress SA (0) ------------------------------------- ---------------------------------------------------------------- auth_key xxx next_pn 1 sak xxx salt xxx ssci 1 SAI_MACSEC_SA_ATTR_CURRENT_XPN 99 SAI_MACSEC_SA_STAT_OCTETS_ENCRYPTED 17947 SAI_MACSEC_SA_STAT_OCTETS_PROTECTED 0 SAI_MACSEC_SA_STAT_OUT_PKTS_ENCRYPTED 98 SAI_MACSEC_SA_STAT_OUT_PKTS_PROTECTED 0 ------------------------------------- ---------------------------------------------------------------- MACsec Ingress SC (xxx) MACsec Ingress SA (0) --------------------------------------- ---------------------------------------------------------------- active true auth_key xxx lowest_acceptable_pn 1 sak xxx salt xxx ssci 2 SAI_MACSEC_SA_ATTR_CURRENT_XPN 203 SAI_MACSEC_SA_STAT_IN_PKTS_DELAYED 0 <<<<<<<<<<<<<<<<<<<< SAI_MACSEC_SA_STAT_IN_PKTS_INVALID 0 SAI_MACSEC_SA_STAT_IN_PKTS_LATE 0 SAI_MACSEC_SA_STAT_IN_PKTS_NOT_USING_SA 0 SAI_MACSEC_SA_STAT_IN_PKTS_NOT_VALID 0 SAI_MACSEC_SA_STAT_IN_PKTS_OK 5 SAI_MACSEC_SA_STAT_IN_PKTS_UNCHECKED 0 SAI_MACSEC_SA_STAT_IN_PKTS_UNUSED_SA 0 SAI_MACSEC_SA_STAT_OCTETS_ENCRYPTED 512 SAI_MACSEC_SA_STAT_OCTETS_PROTECTED 0 <<<<<<<<<<<<<<<<<<<<< --------------------------------------- ---------------------------------------------------------------- ```
|
Original PR: #1493 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kcudnik
approved these changes
Jan 17, 2025
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@kcudnik @bingwang-ms these tests vsTest/TestAsan are not getting started for this cherry-pick PR . Is this something known - can you pls check thanks ! |
|
no clue whats going on herE :( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issue: sonic-net/sonic-buildimage#21232
In MACSEC there are two set of counters one for INGRESS another for EGRESS which gets mapped to the same COUNTER type - CounterType::MACSEC_SA ( https://github.com/sonic-net/sonic-swss/blob/c20902f3195b5bf8a941045e131aa1b863b69fd0/orchagent/macsecorch.cpp#L2145 )
In the releases after 202205 we started seeing this behavior where the MACSEC RX counters were missing as mentioned in the issue 21232. Further debugging pointed to issue seen after this PR : #1073 was merged.
In this case when macsec orch tries to addCounter for INGRESS SA after the EGRESS SA, and it don't go through as the m_supportedCounters is not empty.
For a fix I am removing the check in CounterContext::updateSupportedCount, which I think is ok as we anyways do a check later on in getSupportedCounters() API using isCounterSupported() before calling collectData()
sonic-sairedis/syncd/FlexCounter.cpp
Line 939 in 1684aec
After the fix the macsec SA ingress counters are seen