From bd3289031168edc0cce58b7596950edb399c669e Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Fri, 15 Nov 2019 18:05:09 -0500 Subject: [PATCH] Drop support for forking workers, use spawn by default Make systemd optional on systemd enabled systems Api/web service worker needs ui-classic as it can try to read an existing UI session, which can contain serialized classes from ui-classic. Previously, we tried removing fork here: https://github.com/ManageIQ/manageiq/pull/16130 It was reverted here: https://github.com/ManageIQ/manageiq/pull/16154 Some of the followups needed to fix the original problems including passing down ems_id to per ems workers, resolved in: https://github.com/ManageIQ/manageiq/pull/16199 and https://github.com/ManageIQ/manageiq/pull/18648 At this point, things should just work. --- Gemfile | 1 - app/models/miq_web_service_worker.rb | 5 ++- app/models/miq_worker.rb | 62 ++------------------------- lib/extensions/ar_application_name.rb | 4 -- lib/workers/bin/run_single_worker.rb | 2 +- spec/models/miq_worker_spec.rb | 4 +- 6 files changed, 10 insertions(+), 68 deletions(-) diff --git a/Gemfile b/Gemfile index 1e396912dff9..e9e80275f784 100644 --- a/Gemfile +++ b/Gemfile @@ -57,7 +57,6 @@ gem "manageiq-postgres_ha_admin", "~>3.1", :require => false gem "memoist", "~>0.15.0", :require => false gem "mime-types", "~>3.0", :path => File.expand_path("mime-types-redirector", __dir__) gem "more_core_extensions", "~>3.7" -gem "nakayoshi_fork", "~>0.0.3" # provides a more CoW friendly fork (GC a few times before fork) gem "net-ldap", "~>0.16.1", :require => false gem "net-ping", "~>1.7.4", :require => false gem "openscap", "~>0.4.8", :require => false diff --git a/app/models/miq_web_service_worker.rb b/app/models/miq_web_service_worker.rb index c2d19ba61c82..24fc516ed3cb 100644 --- a/app/models/miq_web_service_worker.rb +++ b/app/models/miq_web_service_worker.rb @@ -17,7 +17,10 @@ def self.supports_container? end def self.bundler_groups - %w[manageiq_default graphql_api] + # TODO: The api process now looks at the existing UI session as of: https://github.com/ManageIQ/manageiq-api/pull/543 + # ui-classic should not be but is serialializing its classes into session, so we need to have access to them for deserialization + # sandboxes;FC:-ActiveSupport::HashWithIndifferentAccess{I"dashboard;FC;q{I"perf_options;FS:0ApplicationController::Performance::Options$typ0:daily_date0:hourly_date0: days0: + %w[manageiq_default ui_dependencies graphql_api] end def self.kill_priority diff --git a/app/models/miq_worker.rb b/app/models/miq_worker.rb index 92e6c1ae7c35..3c7a4f5206a7 100644 --- a/app/models/miq_worker.rb +++ b/app/models/miq_worker.rb @@ -307,48 +307,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 - close_drb_pool_connections - renice(Process.pid) - CodeCoverage.run_hook - 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 - - # Close all open DRb connections so that connections in the parent's memory space - # which is shared due to forking the child process do not pollute the child's DRb - # connection pool. This can lead to errors when the children connect to a server - # and get an incorrect response back. - # - # ref: https://bugs.ruby-lang.org/issues/2718 - def self.close_drb_pool_connections - require 'drb' - - # HACK: DRb doesn't provide an interface to close open pool connections. - # - # Once that is added this should be replaced. - DRb::DRbConn.instance_variable_get(:@mutex).synchronize do - DRb::DRbConn.instance_variable_get(:@pool).each(&:close) - DRb::DRbConn.instance_variable_set(:@pool, []) - end - end # Overriding queue_name as now some queue names can be # arrays of names for some workers not just a singular name. @@ -383,14 +341,12 @@ def systemd_worker? end def start_runner - if ENV['MIQ_SPAWN_WORKERS'] || !Process.respond_to?(:fork) - start_runner_via_spawn - elsif systemd_worker? + if ENV['MIQ_SYSTEMD_WORKERS'] && systemd_worker? start_systemd_worker elsif containerized_worker? start_runner_via_container else - start_runner_via_fork + start_runner_via_spawn end end @@ -398,24 +354,12 @@ def start_runner_via_container create_container_objects end - 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, ems_id = nil) raise ArgumentError, "No guid provided" unless guid require 'awesome_spawn' cmd = "#{Gem.ruby} #{runner_script}" - cmd = "nice #{nice_increment} #{cmd}" if ENV["APPLIANCE"] + cmd = "nice -n #{nice_increment} #{cmd}" if ENV["APPLIANCE"] options = {:guid => guid, :heartbeat => nil} if ems_id diff --git a/lib/extensions/ar_application_name.rb b/lib/extensions/ar_application_name.rb index bcfbd029669f..b0e5bca2fbcf 100644 --- a/lib/extensions/ar_application_name.rb +++ b/lib/extensions/ar_application_name.rb @@ -4,10 +4,6 @@ module ArApplicationName # connections in the pool. We do this because reconnect! on an instance of the # AR adapter created prior to our change to PGAPPNAME will have old connection # options, which will reset our application_name. - # - # Because we fork workers from the server, if we don't disconnect the pool, - # any call to reconnect! on a connection will cause the worker's connection - # to have the server's application_name. def self.name=(name) # TODO: this is postgresql specific ENV['PGAPPNAME'] = name diff --git a/lib/workers/bin/run_single_worker.rb b/lib/workers/bin/run_single_worker.rb index fae9eeb31edb..cb7865c016ba 100755 --- a/lib/workers/bin/run_single_worker.rb +++ b/lib/workers/bin/run_single_worker.rb @@ -89,7 +89,7 @@ def all_role_names exit 1 end -worker_class.before_fork +worker_class.preload_for_worker_role if worker_class.respond_to?(:preload_for_worker_role) unless options[:dry_run] create_options = {:pid => Process.pid} runner_options = {} diff --git a/spec/models/miq_worker_spec.rb b/spec/models/miq_worker_spec.rb index 032cefd90ec5..882cd522d992 100644 --- a/spec/models/miq_worker_spec.rb +++ b/spec/models/miq_worker_spec.rb @@ -382,12 +382,12 @@ def check_has_required_role(worker_role_names, expected_result) it "with ENV['APPLIANCE']" do begin - allow(MiqWorker).to receive(:nice_increment).and_return("+10") + allow(MiqWorker).to receive(:nice_increment).and_return("10") allow(@worker).to receive(:worker_options).and_return(:ems_id => 1234, :guid => @worker.guid) old_env = ENV.delete('APPLIANCE') ENV['APPLIANCE'] = 'true' cmd = @worker.command_line - expect(cmd).to start_with("nice +10") + expect(cmd).to start_with("nice -n 10") expect(cmd).to include("--ems-id 1234") expect(cmd).to include("--guid #{@worker.guid}") expect(cmd).to include("--heartbeat")