From da9523ee2168da89511c9260528c7bb243bfb777 Mon Sep 17 00:00:00 2001 From: Joe Rafaniello Date: Fri, 17 Feb 2017 12:23:27 -0500 Subject: [PATCH] Configure apache balancer with up to 10 members at startup https://bugzilla.redhat.com/show_bug.cgi?id=1422988 Start Ui, Web Service, Web Socket, etc. puma workers bound to a port from STARTING_PORT to the maximum worker count port (3000 to 3009 if max worker count is 10). Configure apache at boot with these ports as balancer members. Fixes a failure after we start new puma workers and try to gracefully restart apache. The next request will fail since apache is waiting for active connections to close before restarting. The subsequent request will then be ok since the failure would cause the websocket connections to close, allowing apaache to restart fully. Previously, we would add and remove members in the balancer configuration when starting or stopping puma workers. We would then gracefully restart apache since the new workers wouldn't be used until apache reloaded the configuration. Note, we didn't do anything after removing members from the balancer configuration because apache's mod_proxy_balancer gracefully handles dead members by marking them as in Error and not retrying them for 60 seconds by default. Therefore, it's not necessary to restart apache to "remove" members. The problem is when we would try to add balancer members to the configuration and gracefully restart apache. It turns out, our web socket workers maintain active connections to apache so apache wouldn't restart until those connections were closed. Now, we take the idea mentioned above of the mod_proxy_balancer keeping track of which members are alive or in error by configuring up to 10, maximum_workers_count, members at server startup. We can then start and stop workers and let apache route traffic to the members that are alive. We no longer have to update the apache configuration and restart it when a worker starts or stops. Note, apache has a graceful reload option that could allow us to maintain an accurate list of balancer members as workers start and stop and tell apache workers to gracefully reload the configuration. This option was buggy until fixed in [1]. It also required us to keep touching the balancer configuration which we probably shouldn't have ben doing in the first place. [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=44736 --- .../miq_server/environment_management.rb | 2 +- .../mixins/miq_web_server_worker_mixin.rb | 53 +++++-------------- spec/models/miq_ui_worker_spec.rb | 21 ++++++++ 3 files changed, 36 insertions(+), 40 deletions(-) diff --git a/app/models/miq_server/environment_management.rb b/app/models/miq_server/environment_management.rb index 87b7e1726b8..6a3e5154ee4 100644 --- a/app/models/miq_server/environment_management.rb +++ b/app/models/miq_server/environment_management.rb @@ -85,10 +85,10 @@ def start_memcached def prep_apache_proxying return unless MiqEnvironment::Command.supports_apache? - MiqApache::Control.stop MiqUiWorker.install_apache_proxy_config MiqWebServiceWorker.install_apache_proxy_config MiqWebsocketWorker.install_apache_proxy_config + MiqApache::Control.restart end end diff --git a/app/models/mixins/miq_web_server_worker_mixin.rb b/app/models/mixins/miq_web_server_worker_mixin.rb index 76c96b45511..4904d870b44 100644 --- a/app/models/mixins/miq_web_server_worker_mixin.rb +++ b/app/models/mixins/miq_web_server_worker_mixin.rb @@ -1,4 +1,6 @@ require 'miq_apache' +class NoFreePortError < StandardError; end + module MiqWebServerWorkerMixin extend ActiveSupport::Concern @@ -8,6 +10,8 @@ module MiqWebServerWorkerMixin class << self attr_accessor :registered_ports end + + try(:maximum_workers_count=, 10) end module ClassMethods @@ -89,8 +93,6 @@ def sync_workers end end - modify_apache_ports(ports_hash, self::PROTOCOL) if MiqEnvironment::Command.supports_apache? - result end @@ -110,50 +112,23 @@ def install_apache_proxy_config _log.info("[#{options.inspect}") MiqApache::Conf.install_default_config(options) + add_apache_balancer_members end - def modify_apache_ports(ports_hash, protocol) - return unless MiqEnvironment::Command.supports_apache? - adds = Array(ports_hash[:adds]) - deletes = Array(ports_hash[:deletes]) - - # Remove any already registered - adds -= self.registered_ports - - return false if adds.empty? && deletes.empty? + def port_range + self::STARTING_PORT...(self::STARTING_PORT + maximum_workers_count) + end + def add_apache_balancer_members conf = MiqApache::Conf.instance(self::BALANCE_MEMBER_CONFIG_FILE) - - unless adds.empty? - _log.info("Adding port(s) #{adds.inspect}") - conf.add_ports(adds, protocol) - end - - unless deletes.empty? - _log.info("Removing port(s) #{deletes.inspect}") - conf.remove_ports(deletes, protocol) - end - - saved = conf.save - if saved - self.registered_ports += adds - self.registered_ports -= deletes - - # Update the apache load balancer regardless but only restart apache - # when adding a new port to the balancer. - MiqServer.my_server.queue_restart_apache unless adds.empty? - _log.info("Added/removed port(s) #{adds.inspect}/#{deletes.inspect}, registered ports after #{self.registered_ports.inspect}") - end - saved + conf.add_ports(port_range.to_a, self::PROTOCOL) + conf.save end def reserve_port(ports) - index = 0 - loop do - port = self::STARTING_PORT + index - return port unless ports.include?(port) - index += 1 - end + free_ports = port_range.to_a - ports + raise NoFreePortError if free_ports.empty? + free_ports.first end end diff --git a/spec/models/miq_ui_worker_spec.rb b/spec/models/miq_ui_worker_spec.rb index d4bd97e628c..a521cfc739c 100644 --- a/spec/models/miq_ui_worker_spec.rb +++ b/spec/models/miq_ui_worker_spec.rb @@ -26,4 +26,25 @@ expect(MiqUiWorker.all_ports_in_use).to eq([3000]) end end + + it ".port_range" do + expect(described_class.port_range.to_a).to eq((3000..3009).to_a) + end + + describe ".reserve_port" do + it "returns next free port" do + ports = (3000..3001).to_a + expect(described_class.reserve_port(ports)).to eq(3002) + end + + it "raises if no ports available" do + ports = (3000..3009).to_a + expect { described_class.reserve_port(ports) }.to raise_error(NoFreePortError) + end + + it "returns free port between used ports" do + ports = [3000, 3002] + expect(described_class.reserve_port(ports)).to eq(3001) + end + end end