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

CPU Spike because of redundant and flooded keyspace notifis handled #230

Merged
merged 2 commits into from
Aug 5, 2021

Conversation

vivekrnv
Copy link
Contributor

@vivekrnv vivekrnv commented Aug 3, 2021

Signed-off-by: Vivek Reddy Karri vkarri@nvidia.com

- What I did

Fixes #8293

- How I did it

Accumulated all the older notifications and did act only upon the latest notification discarding the others

- How to verify it

- Description for the changelog

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
@vivekrnv
Copy link
Contributor Author

vivekrnv commented Aug 3, 2021

@qiluo-msft, please review

"""
latest_update_map = {}
while True:
data, interface, if_index = poll_lldp_entry_updates(pubsub)
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poll_lldp_entry_updates

In future, pubsub may return just changed fields, not all the fields of a key. So the general solution should merge set/del entries in this function, instead of picking the last one. #Closed

Copy link
Contributor Author

@vivekrnv vivekrnv Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if i understood your suggestion correctly, a keyspace notif doesn't contain any info about the fields changed. Eg: __keyspace@0__:mykey del.

So, even if the lld_syncd logic was modified to send only one notif instead of mutiple. That'll be captured by this method. The actual logic of fetching the fv-pairs is in the downstream methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarify. I see the flow is implemented inside this file, instead of using a library. Sorry for misleading.

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
@dgsudharsan
Copy link
Collaborator

Request for 202106

@qiluo-msft qiluo-msft merged commit 43b5e1a into sonic-net:master Aug 5, 2021
qiluo-msft pushed a commit that referenced this pull request Aug 5, 2021
…230)

**- What I did**

Fixes [#8293](sonic-net/sonic-buildimage#8293)

**- How I did it**

Accumulated all the older notifications and did act only upon the latest notification discarding the others
@vivekrnv
Copy link
Contributor Author

vivekrnv commented Nov 8, 2021

@qiluo-msft, Can you backport this to 202106?

judyjoseph pushed a commit that referenced this pull request Nov 14, 2021
…230)

**- What I did**

Fixes [#8293](sonic-net/sonic-buildimage#8293)

**- How I did it**

Accumulated all the older notifications and did act only upon the latest notification discarding the others
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SNMP-SUBAGENT] CPU Spike because of redundant and flooded keyspace notifications
4 participants