Fix workers not restarting when auth/endpoints are changed.

app/models/ext_management_system.rb

@@ -176,18 +176,41 @@ def self.create_from_params(params, endpoints, authentications)
 
   def edit_with_params(params, endpoints, authentications)
     tap do |ems|
+      endpoints_changed       = false
+      authentications_changed = false
+
+      endpoint_changes       = {}
+      authentication_changes = {}
+
       transaction do
         # Remove endpoints/attributes that are not arriving in the arguments above
-        ems.endpoints.where.not(:role => nil).where.not(:role => endpoints.map { |ep| ep['role'] }).delete_all
-        ems.authentications.where.not(:authtype => nil).where.not(:authtype => authentications.map { |au| au['authtype'] }).delete_all
+        endpoints_to_delete       = ems.endpoints.where.not(:role => nil).where.not(:role => endpoints.map { |ep| ep['role'] })
+        authentications_to_delete = ems.authentications.where.not(:authtype => nil).where.not(:authtype => authentications.map { |au| au['authtype'] })
+
+        endpoints_changed       ||= endpoints_to_delete.delete_all > 0
+        authentications_changed ||= authentications_to_delete.delete_all > 0
 
         ems.assign_attributes(params)
-        ems.endpoints = endpoints.map(&method(:assign_nested_endpoint))
-        ems.authentications = authentications.map(&method(:assign_nested_authentication))
+        ems.endpoints       = endpoints.map       { |ep| assign_nested_endpoint(ep) }
+        ems.authentications = authentications.map { |auth| assign_nested_authentication(auth) }
+
+        endpoint_changes = ems.endpoints.select(&:changed?).to_h do |ep|
+          [ep.role.to_sym, ep.changes]
+        end
+
+        authentication_changes = ems.authentications.select(&:changed?).to_h do |auth|
+          [auth.authtype.to_sym, auth.changes]
+        end
+
+        endpoints_changed       ||= endpoint_changes.present?
+        authentications_changed ||= authentication_changes.present?
 
         ems.provider.save! if ems.provider.present? && ems.provider.changed?
         ems.save!
       end
+
+      after_update_endpoints(endpoint_changes)            if endpoints_changed
+      after_update_authentication(authentication_changes) if authentications_changed
     end
   end
 
@@ -834,8 +857,12 @@ def perf_capture_enabled?
   # Some workers hold open a connection to the provider and thus do not
   # automatically pick up authentication changes.  These workers have to be
   # restarted manually for the new credentials to be used.
-  def after_update_authentication
-    stop_event_monitor_queue_on_credential_change
+  def after_update_authentication(changes)
+    stop_event_monitor_queue_on_credential_change(changes)
+  end
+
+  def after_update_endpoints(changes)
+    stop_event_monitor_queue_on_change(changes)
   end
 
   ###################################
@@ -847,6 +874,14 @@ def self.event_monitor_class
   end
   delegate :event_monitor_class, :to => :class
 
+  def endpoint_role_for_events
+    :default
+  end
+
+  def authtype_for_events
+    default_authentication_type
+  end
+
   def event_monitor
     return if event_monitor_class.nil?
     event_monitor_class.find_by_ems(self).first
@@ -874,15 +909,15 @@ def stop_event_monitor_queue
     )
   end
 
-  def stop_event_monitor_queue_on_change
-    if event_monitor_class && !self.new_record? && default_endpoint.changed.include_any?("hostname", "ipaddress")
+  def stop_event_monitor_queue_on_change(changes)
+    if event_monitor_class && !new_record? && changes[endpoint_role_for_events]&.keys&.include_any?("hostname", "ipaddress")
       _log.info("EMS: [#{name}], Hostname or IP address has changed, stopping Event Monitor.  It will be restarted by the WorkerMonitor.")
       stop_event_monitor_queue
     end
   end
 
-  def stop_event_monitor_queue_on_credential_change
-    if event_monitor_class && !self.new_record? && self.credentials_changed?
+  def stop_event_monitor_queue_on_credential_change(changes)
+    if event_monitor_class && !new_record? && changes[authtype_for_events].present?
       _log.info("EMS: [#{name}], Credentials have changed, stopping Event Monitor.  It will be restarted by the WorkerMonitor.")
       stop_event_monitor_queue
     end
@@ -908,6 +943,14 @@ def self.refresh_worker_class
   end
   delegate :refresh_worker_class, :to => :class
 
+  def endpoint_role_for_refresh
+    :default
+  end
+
+  def authtype_for_refresh
+    default_authentication_type
+  end
+
   def refresh_worker
     return if refresh_worker_class.nil?
 
@@ -938,15 +981,15 @@ def stop_refresh_worker_queue
     )
   end
 
-  def stop_refresh_worker_queue_on_change
-    if refresh_worker_class && !self.new_record? && default_endpoint.changed.include_any?("hostname", "ipaddress")
+  def stop_refresh_worker_queue_on_change(changes)
+    if refresh_worker_class && !new_record? && changes["default"]&.keys&.include_any?("hostname", "ipaddress")
       _log.info("EMS: [#{name}], Hostname or IP address has changed, stopping Refresh Worker.  It will be restarted by the WorkerMonitor.")
       stop_refresh_worker_queue
     end
   end
 
-  def stop_refresh_worker_queue_on_credential_change
-    if refresh_worker_class && !self.new_record? && self.credentials_changed?
+  def stop_refresh_worker_queue_on_credential_change(changes)
+    if refresh_worker_class && !new_record? && changes[authtype_for_refresh].present?
       _log.info("EMS: [#{name}], Credentials have changed, stopping Refresh Worker.  It will be restarted by the WorkerMonitor.")
       stop_refresh_worker_queue
     end

app/models/manageiq/providers/cloud_manager.rb

@@ -76,8 +76,8 @@ def open_browser
       MiqSystem.open_browser(browser_url)
     end
 
-    def stop_event_monitor_queue_on_credential_change
-      if event_monitor_class && !self.new_record? && self.credentials_changed?
+    def stop_event_monitor_queue_on_credential_change(changes)
+      if event_monitor_class && !new_record? && changes["default"]&.keys&.include_any?("hostname", "ipaddress")
         _log.info("EMS: [#{name}], Credentials have changed, stopping Event Monitor.  It will be restarted by the WorkerMonitor.")
         stop_event_monitor_queue
         network_manager.stop_event_monitor_queue if respond_to?(:network_manager) && network_manager

app/models/mixins/authentication_mixin.rb

@@ -183,12 +183,10 @@ def update_authentication(data, options = {})
 
     options.reverse_merge!(:save => true)
 
-    @orig_credentials ||= auth_user_pwd || "none"
-
     # Invoke before callback
     before_update_authentication if self.respond_to?(:before_update_authentication) && options[:save]
 
-    data.each_pair do |type, value|
+    authentication_changes = data.each_pair.map do |type, value|
       cred = authentication_type(type)
       current = {:new => nil, :old => nil}
 
@@ -245,18 +243,17 @@ def update_authentication(data, options = {})
       cred.auth_key        = value[:auth_key]
       cred.service_account = value[:service_account].presence
 
+      changes = [type.to_sym, cred.changes] if cred.changed?
+
       cred.save if options[:save] && id
-    end
 
-    # Invoke callback
-    after_update_authentication if self.respond_to?(:after_update_authentication) && options[:save]
-    @orig_credentials = nil if options[:save]
-  end
+      changes
+    end.compact.to_h
+
+    return if authentication_changes.blank? || !options[:save]
 
-  def credentials_changed?
-    @orig_credentials ||= auth_user_pwd || "none"
-    new_credentials = auth_user_pwd || "none"
-    @orig_credentials != new_credentials
+    # Invoke callback
+    after_update_authentication(authentication_changes) if respond_to?(:after_update_authentication)
   end
 
   def authentication_type(type)

spec/models/ext_management_system_spec.rb

@@ -833,6 +833,163 @@ def deliver_queue_message(queue_message = MiqQueue.order(:id).first)
     end
   end
 
+  describe "#edit_with_params" do
+    let(:zone) { EvmSpecHelper.local_miq_server.zone }
+    let(:ems)  do
+      FactoryBot.create(:ext_management_system, :with_authentication, :zone => zone).tap do |ems|
+        # assign_nested_authentication automatically sets the name to "#{self.class.name} #{name}"
+        # set it here to prevent spurious authentication record changes
+        ems.default_authentication.update!(:name => "#{ems.class.name} #{ems.name}")
+      end
+    end
+
+    let(:params)          { {"name" => ems.name, "zone" => ems.zone} }
+    let(:endpoints)       { [default_endpoint] }
+    let(:authentications) { [default_authentication] }
+
+    let(:default_endpoint)       { {"role" => "default", "hostname" => ems.default_endpoint.hostname, "ipaddress" => ems.default_endpoint.ipaddress} }
+    let(:default_authentication) { {"authtype" => "default", "userid" => ems.default_authentication.userid, "password" => ems.default_authentication.password} }
+
+    context "with no changes" do
+      it "doesn't call endpoints or authentications changed callbacks" do
+        expect(ems).not_to receive(:after_update_endpoints)
+        expect(ems).not_to receive(:after_update_authentication)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+    end
+
+    context "changing an ext_management_system record attribute" do
+      let(:params) { {:name => "new-name", :zone => ems.zone} }
+
+      it "changes the name" do
+        ems.edit_with_params(params, endpoints, authentications)
+        expect(ems.reload.name).to eq("new-name")
+      end
+    end
+
+    context "adding an endpoint" do
+      let(:endpoints) { [default_endpoint, {"role" => "metrics", "hostname" => "metrics"}] }
+
+      it "creates the new endpoint" do
+        ems.edit_with_params(params, endpoints, authentications)
+
+        ems.reload
+
+        expect(ems.endpoints.pluck(:role)).to match_array(["default", "metrics"])
+      end
+
+      it "calls after_update_endpoints" do
+        expect(ems).to receive(:after_update_endpoints)
+        expect(ems).not_to receive(:after_update_authentication)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+    end
+
+    context "deleting an endpoint" do
+      before { ems.endpoints.create!(:role => "metrics", :hostname => "metrics") }
+
+      it "deletes the unused endpoint" do
+        ems.edit_with_params(params, endpoints, authentications)
+
+        ems.reload
+        expect(ems.endpoints.pluck(:role)).to match_array(["default"])
+      end
+
+      it "calls after_update_endpoints" do
+        expect(ems).to receive(:after_update_endpoints)
+        expect(ems).not_to receive(:after_update_authentication)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+    end
+
+    context "updating an endpoint" do
+      let(:default_endpoint) { {"role" => "default", "hostname" => "new-hostname", "ipaddress" => ems.default_endpoint.ipaddress} }
+
+      it "updates the default hostname" do
+        ems.edit_with_params(params, endpoints, authentications)
+
+        ems.reload
+        expect(ems.default_endpoint.hostname).to eq("new-hostname")
+      end
+
+      it "calls after_update_endpoints" do
+        expect(ems).to receive(:after_update_endpoints)
+        expect(ems).not_to receive(:after_update_authentication)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+
+      it "stops the event monitor" do
+        expect(ems).to receive(:stop_event_monitor_queue)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+    end
+
+    context "adding an authentication" do
+      let(:authentications) { [default_authentication, {:authtype => "metrics", :auth_key => "secret"}] }
+
+      it "creates the authentication" do
+        ems.edit_with_params(params, endpoints, authentications)
+
+        ems.reload
+        expect(ems.authentications.pluck(:authtype)).to match_array(["default", "metrics"])
+      end
+
+      it "calls after_update_authentication" do
+        expect(ems).to receive(:after_update_authentication)
+        expect(ems).not_to receive(:after_update_endpoints)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+    end
+
+    context "deleting an authentication" do
+      before { ems.authentications.create!(:authtype => "metrics", :auth_key => "secret") }
+
+      it "deletes the authentication" do
+        ems.edit_with_params(params, endpoints, authentications)
+
+        ems.reload
+        expect(ems.authentications.pluck(:authtype)).to match_array(["default"])
+      end
+
+      it "calls after_update_authentication" do
+        expect(ems).to receive(:after_update_authentication)
+        expect(ems).not_to receive(:after_update_endpoints)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+    end
+
+    context "updating an authentication" do
+      let(:default_authentication) { {"authtype" => "default", "userid" => ems.default_authentication.userid, "password" => "more-secret"} }
+
+      it "updates the password" do
+        ems.edit_with_params(params, endpoints, authentications)
+
+        ems.reload
+        expect(ems.default_authentication.password).to eq("more-secret")
+      end
+
+      it "calls after_update_authentication" do
+        expect(ems).to receive(:after_update_authentication)
+        expect(ems).not_to receive(:after_update_endpoints)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+
+      it "stops the event monitor" do
+        expect(ems).to receive(:stop_event_monitor_queue)
+
+        ems.edit_with_params(params, endpoints, authentications)
+      end
+    end
+  end
+
   context "virtual column :supports_block_storage (direct supports)" do
     it "returns false if block storage is not supported" do
       ems = FactoryBot.create(:ext_management_system)