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.

Sign up for GitHub

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

determin inactive users and clean their PUNs too #3942

Merged
merged 6 commits into from
Dec 17, 2024
Merged

determin inactive users and clean their PUNs too #3942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
acd0817
determin inactive users and clean their PUNs too
johrstrom Nov 7, 2024
79e9338
fix test
johrstrom Nov 7, 2024
7bd9286
nginx_stage: clean up invalid users' PID file directories later
CSC-swesters Nov 11, 2024
eb8a11d
more conservative sleep time
johrstrom Dec 12, 2024
69c8854
reword this messgae
johrstrom Dec 12, 2024
dc2d3c6
fixup this warn message
johrstrom Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions nginx_stage/lib/nginx_stage.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,21 @@ def self.active_users
end.compact
end

# List of inactive users.
# @return [Array<String>] the list of inactive users.
def self.inactive_users
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a separate function for enumerating inactive_users causes duplicate work. It would probably be possible to gather two lists of results in one single walk-through? I.e., put valid_users in one list if the User.new(name) function doesn't raise an error, or put the name in an invalid_users list if it raises. Then return a list of two lists.

I suppose this approach would require a few code changes in other places, but in my opinion, that would be okay. I.e., we would have:

# Pseudo ruby, not sure if this is usable:

currently_active_users = NginxStage.active_users
# sanity check that the length of currently_active_users is 2.
valid_users = currently_active_users[0]
invalid_users = currently_active_users[1]
# Now handle valid and invalid users separately
# ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I should be changing the API as it's being used in other places. I guess that's the risk that I'm trading for duplication. I.e., I'd rather have duplication than possible errors/mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your concern. However, I think with sufficient test coverage and carefulness, a change like this shouldn't be an issue. nginx_stage is still quite limited in scope compared to the main ondemand application.

It's not a hill I'll die on though, so I'm willing to let this slide. Perhaps one could make a separate issue for it, so that future refactoring can take it into account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea the issue is with the 4.0 timeline so I don't have time to write tests or be super careful. If you're happy to defer it, I am too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good 👍 My other review comment I'm more concerned about, so I'm looking forward to your thoughts on that. Hopefully the suggested commit helps.

Dir[pun_pid_path(user: '*')].map do |v|
name = v[/#{pun_pid_path(user: '(.+)')}/, 1]
User.new(name)

# return nil here becasue we actually _want_ to rescue.
# i.e., a User is inactive if it _can't_ be instanitated.
nil
rescue ArgumentError => e
name
end.compact
end

# Get a hash of all the staged app configs
# @example List of all staged app configs
# staged_apps
Expand Down
35 changes: 35 additions & 0 deletions nginx_stage/lib/nginx_stage/generators/nginx_clean_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,41 @@ class NginxCleanGenerator < Generator
$stderr.puts "#{$!.to_s}"
end
end

pid_parent_dirs_to_remove_later = []
NginxStage.inactive_users.each do |u|
begin
puts "#{u} (disabled)"
pid_path = PidFile.new NginxStage.pun_pid_path(user: u)

# Send a SIGTERM to the master nginx process to kill the PUN.
# 'nginx stop' won't work, since getpwnam(3) will cause an error.
`kill -s TERM #{pid_path.pid}`
FileUtils.rm(NginxStage.pun_secret_key_base_path(user: u).to_s)
FileUtils.rm(NginxStage.pun_config_path(user: u).to_s)
pid_path_parent_dir = Pathname.new(pid_path.to_s).parent
pid_parent_dirs_to_remove_later.push(pid_path_parent_dir)
rescue StandardError => e
warn "Error trying to clean up disabled user #{u}: #{e.message}"
end
end

# Remove the PID path parent directories now that the nginx processes have
# had time to clean up their Passenger PID file and socket.
pid_parent_dirs_to_remove_later.each do |dir|
begin
begin
FileUtils.rmdir(dir)
rescue Errno::ENOTEMPTY
# Wait for a short time, while Nginx cleans up its PID file.
sleep(0.5)
# Then try again once.
FileUtils.rmdir(dir)
end
rescue StandardError => e
warn "Error trying to clean up the PID file directory '#{dir}' of disabled user: #{e.message}"
end
end
end

def cleanup_stale_files(pid_path, socket)
Expand Down
7 changes: 6 additions & 1 deletion spec/e2e/nginx_stage_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ def browser
on hosts, 'mkdir /var/run/ondemand-nginx/deleted_user'
on hosts, 'chmod 600 /var/run/ondemand-nginx/deleted_user'
on hosts, 'echo -n 11111111 > /var/run/ondemand-nginx/deleted_user/passenger.pid'
on hosts, 'echo -n 11111111 > /var/lib/ondemand-nginx/config/puns/deleted_user.conf'
on hosts, 'echo -n 11111111 > /var/lib/ondemand-nginx/config/puns/deleted_user.secret_key_base.txt'
end

after(:all) do
Expand All @@ -31,7 +33,10 @@ def browser

# Note there's no error here about 'deleted_user'
on hosts, '/opt/ood/nginx_stage/sbin/nginx_stage nginx_clean --force' do
assert_equal stdout, "ood\n"
assert_equal stdout, "ood\ndeleted_user (disabled)\n"
refute(File.exists?('/var/run/ondemand-nginx/deleted_user'))
refute(File.exists?('/var/lib/ondemand-nginx/config/puns/deleted_user.conf'))
refute(File.exists?('/var/lib/ondemand-nginx/config/puns/deleted_user.secret_key_base.txt'))
end
end
end
Expand Down
Loading