-
Notifications
You must be signed in to change notification settings - Fork 108
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
use ps to count sessions instead of lsof #3511
Conversation
Thanks, this looks promising! I'll have time to test this later in the week and I can provide more feedback after that. I'd like to see the I'm not well versed in Rails, so I don't see how that is used. But would it be possible to fix this in the same manner, i.e., using |
Sure thing! Updated in this PR. |
module SessionFinder | ||
def session_count(user) | ||
`ps -o cmd -u #{user}`.split("\n").select do |command| | ||
command.match?(/Passenger \w+App:/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to leave comment about what this could match, as I presume you want to match like Passenger RubyApp:
or Passenger NodeApp:
. Would be interesting to see if this match works for non-Ruby and non-NodeJS apps, such as Python apps maybe.
I'd recommend for the |
Yes, that's an excellent suggestion. +1 for that. If I'm not mistaken, these code paths are not part of the creation of new PUNs, right? So the only times it might hang should be when the
Thanks! Would it still be possible @johrstrom to actually remove the old I still didn't understand whether the
If nothing uses it, maybe it would be better to remove this also altogether? 🙂 |
@@ -59,8 +62,9 @@ class NginxCleanGenerator < Generator | |||
next if (user && user != u.to_s) | |||
pid_path = PidFile.new NginxStage.pun_pid_path(user: u) | |||
socket = SocketFile.new NginxStage.pun_socket_path(user: u) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this would still run lsof
as the SocketFile initializer runs @processes = get_processes
, although the output itself is not used. There is the same case for nginx_show_generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is done. I believe @CSC-swesters also wanted it removed, which it is now.
Co-authored-by: treydock <[email protected]>
All hooks get run when you use a generator. So So running that, I get something like this which is where you see those
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Use the ps command to count sessions instead of lsof. This was because lsof may cause issues in some environments with permissions (like containers) and indeed may be quite slow. So this changes that for simplicity and speed. --------- Co-authored-by: treydock <[email protected]>
This uses
ps
to find the active sessions instead oflsof
.It should/could fix #3111.