-
Notifications
You must be signed in to change notification settings - Fork 900
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
Fork workers no more 🔥 🔥 #16130
Fork workers no more 🔥 🔥 #16130
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -304,29 +304,6 @@ def send_message_to_worker_monitor(message, *args) | |
) | ||
end | ||
|
||
def self.before_fork | ||
preload_for_worker_role if respond_to?(:preload_for_worker_role) | ||
end | ||
|
||
def self.after_fork | ||
close_pg_sockets_inherited_from_parent | ||
DRb.stop_service | ||
renice(Process.pid) | ||
end | ||
|
||
# When we fork, the children inherits the parent's file descriptors | ||
# so we need to close any inherited raw pg sockets in the child. | ||
def self.close_pg_sockets_inherited_from_parent | ||
owner_to_pool = ActiveRecord::Base.connection_handler.instance_variable_get(:@owner_to_pool) | ||
owner_to_pool[Process.ppid].values.compact.each do |pool| | ||
pool.connections.each do |conn| | ||
socket = conn.raw_connection.socket | ||
_log.info("Closing socket: #{socket}") | ||
IO.for_fd(socket).close | ||
end | ||
end | ||
end | ||
|
||
# Overriding queue_name as now some queue names can be | ||
# arrays of names for some workers not just a singular name. | ||
# We use JSON.parse as the array of names is stored as a string. | ||
|
@@ -339,26 +316,6 @@ def queue_name | |
end | ||
end | ||
|
||
def start_runner | ||
if ENV['MIQ_SPAWN_WORKERS'] || !Process.respond_to?(:fork) | ||
start_runner_via_spawn | ||
else | ||
start_runner_via_fork | ||
end | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mess up @carbonin 's containers fork? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, but don't worry about that. The rebase is going to be a disaster either way 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. womp womp |
||
|
||
def start_runner_via_fork | ||
self.class.before_fork | ||
pid = fork(:cow_friendly => true) do | ||
self.class.after_fork | ||
self.class::Runner.start_worker(worker_options) | ||
exit! | ||
end | ||
|
||
Process.detach(pid) | ||
pid | ||
end | ||
|
||
def self.build_command_line(guid) | ||
command_line = "#{Gem.ruby} #{runner_script} --heartbeat --guid=#{guid} #{name}" | ||
ENV['APPLIANCE'] ? "nice #{nice_increment} #{command_line}" : command_line | ||
|
@@ -370,7 +327,7 @@ def self.runner_script | |
script | ||
end | ||
|
||
def start_runner_via_spawn | ||
def start_runner | ||
pid = Kernel.spawn(self.class.build_command_line(guid), [:out, :err] => [Rails.root.join("log", "evm.log"), "a"]) | ||
Process.detach(pid) | ||
pid | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,7 +55,7 @@ | |
require File.expand_path("../../../config/environment", __dir__) | ||
|
||
worker_class = worker_class.constantize | ||
worker_class.before_fork | ||
worker_class.preload_for_worker_role if worker_class.respond_to?(:preload_for_worker_role) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed, thanks @Fryguy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might soon not be needed since I think the only thing that actually makes use of this is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scratch that, guess we configure the secret token in that line... Nevermind... 😞 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry... grepping and responding as I find new stuff... sorry for the spam, but found something else: https://github.com/ManageIQ/manageiq/blob/874cad6/lib/vmdb/initializer.rb#L13 Does that mean we do this twice anyway? (Also, for what it is worth, this code isn't hidden somewhere else in our other repos it seems: https://github.com/search?q=org%3AManageIQ+preload_for_worker_role&type=Code ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@NickLaMuro Good catch. We gate that if you're a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yup, for sure, and wasn't trying to suggest we do that in this PR. The |
||
unless options[:dry_run] | ||
create_options = {:pid => Process.pid} | ||
runner_options = {} | ||
|
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.
Is this still useful for other uses of fork, such as the parallel gem?
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.
I doubt it. The only place that cared about trying to eek out more shared memory was worker creation. We can keep it but this line was not added for users of parallel .