Skip to content

Fix presence events on TCP transport #62827

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

Merged
merged 8 commits into from
Dec 8, 2022

Conversation

recmanj
Copy link

@recmanj recmanj commented Oct 6, 2022

What does this PR do?

Fixes presence events by removing a client's presence correctly.

What issues does this PR fix or reference?

Fixes: #62826

Previous Behavior

Presence events not fired when salt-minion status changes.

New Behavior

Presence events fired correctly when salt-minion status changes.

Merge requirements satisfied?

Commits signed with GPG?

No

@recmanj recmanj requested a review from a team as a code owner October 6, 2022 12:16
@recmanj recmanj requested review from waynew and removed request for a team October 6, 2022 12:16
@sakateka
Copy link
Contributor

sakateka commented Oct 7, 2022

This patch does even more, it fixes a memory leak.
Because of this regression, a large memory leak appears in salt-master.

USER        PID %CPU %MEM    VSZ   RSS TTY  STAT START   TIME COMMAND
salt  94353  0.1 21.0 78579320 4110420 ?               S    Oct05   3:34  \_ /usr/bin/salt-master PubServerChannel._publish_daemon

Leaking objects:

builtins.frame
collections.deque
salt.transport.tcp.Subscriber
salt.ext.tornado.gen.Runner
salt.ext.tornado.concurrent.Future
salt.ext.tornado.iostream.IOStream
salt.utils.msgpack.Unpacker

Screenshot 2022-10-07 at 11-26-21

The leak was discovered immediately after the update to 3005.
I researched a bit and also came to this place in the code.

Thank you @recmanj

@sakateka
Copy link
Contributor

sakateka commented Oct 7, 2022

@dwoz PTAL

@Ch3LL Ch3LL requested a review from dwoz October 13, 2022 19:17
@Ch3LL Ch3LL added the Sulfur v3006.0 release code name and version label Oct 13, 2022
twangboy
twangboy previously approved these changes Oct 24, 2022
whytewolf
whytewolf previously approved these changes Oct 27, 2022
@twangboy twangboy added this to the Sulphur v3006.0 milestone Oct 28, 2022
Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

I'm good with this change but it would be nice to have a test which verifies the presence callbacks are getting called.

@twangboy twangboy added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Oct 28, 2022
@garethgreenaway
Copy link
Contributor

@sakateka Just a heads up about the request to add an additional test for this change.

@Ch3LL
Copy link
Contributor

Ch3LL commented Nov 7, 2022

bump @recmanj are you willing to add test coverage here?

@recmanj
Copy link
Author

recmanj commented Nov 7, 2022

@Ch3LL on its way

@Ch3LL
Copy link
Contributor

Ch3LL commented Dec 5, 2022

We're wanting to tag for 3006 this friday. I would really love to try to get this in. Any luck on adding test coverage?

@recmanj
Copy link
Author

recmanj commented Dec 6, 2022

Will push them tomorrow afternoon, sorry

@recmanj recmanj dismissed stale reviews from whytewolf and twangboy via 27af1ce December 7, 2022 23:47
@Ch3LL Ch3LL removed the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Dec 8, 2022
@Ch3LL Ch3LL merged commit 3fff9ea into saltstack:master Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Presence events are not being fired on 3005+ (TCP transport)
7 participants