Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to not miss the entire set of counters to be added in addObject for CounterContext::updateSupportedCount #1493

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

judyjoseph
Copy link

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()

if (isCounterSupported(counter))

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@judyjoseph judyjoseph changed the title Remove check to see if m_supportedCounters is empty in CounterContext::updateSupportedCount Remove !m_supportedCounters.empty check in CounterContext::updateSupportedCount Jan 7, 2025
@Junchao-Mellanox
Copy link
Contributor

Maybe we can set always_check_supported_counters to true for macsec counters?

@judyjoseph
Copy link
Author

Maybe we can set always_check_supported_counters to true for macsec counters?

I thought of this approach, but from code I see it resets the current m_supportedCounters counter list -- which will not help, as it clears the existing list ?

    if (always_check_supported_counters)
    {
        m_supportedCounters.clear();
    }

@Junchao-Mellanox
Copy link
Contributor

Removing the check will cause it check supported counter each time adding an object. I am afraid it will cause performance degradation. Could you please add one more attribute like dont_clear_support_counter and set it to true for macsec counter?

@judyjoseph judyjoseph changed the title Remove !m_supportedCounters.empty check in CounterContext::updateSupportedCount Fix to not miss the entire set of counters to be added in addObject for CounterContext::updateSupportedCount Jan 7, 2025
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@judyjoseph judyjoseph requested a review from kcudnik January 7, 2025 21:10
@judyjoseph
Copy link
Author

judyjoseph commented Jan 7, 2025

Removing the check will cause it check supported counter each time adding an object. I am afraid it will cause performance degradation. Could you please add one more attribute like dont_clear_support_counter and set it to true for macsec counter?

Added a new flag 'dont_clear_support_counter', do review

@rlhui rlhui added the P0 label Jan 8, 2025
@rlhui rlhui requested a review from liamkearney-msft January 8, 2025 18:26
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liamkearney-msft
Copy link

can you add a snippit of the macsec counters tests run on this change? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants