Finish simplifying NTP configuration using Settings

app/models/miq_server/ntp_management.rb

@@ -3,32 +3,14 @@
 module MiqServer::NtpManagement
   extend ActiveSupport::Concern
 
-  def server_ntp_settings
-    # Get the ntp servers from the vmdb.yml first, zone second, else use some defaults
-    ntp = ntp_config
-    if server_ntp_settings_blank?(ntp)
-      zone.ntp_settings
-    else
-      ntp[:source] = :server
-      ntp
-    end
-  end
-
-  def ntp_config
-    get_config("vmdb").config[:ntp]
-  end
-
-  def server_ntp_settings_blank?(ntp)
-    # verify the ntp settings are like this and not blank:  {:ntp => {:server => ['blah'], :timeout => 5}}
-    ntp.values.flatten.compact.blank? rescue true
-  end
-
   # Called when zone ntp settings changed... run by the appropriate server
   # Also, called in start of miq_server and on a configuration change for the server
-  def ntp_reload(ntp_settings = server_ntp_settings)
+  def ntp_reload
     # matches ntp_reload_queue's guard clause
     return if !MiqEnvironment::Command.is_appliance? || MiqEnvironment::Command.is_container?
 
+    ntp_settings = get_config("vmdb").config[:ntp]
+
     if @ntp_settings && @ntp_settings == ntp_settings
       _log.info("Skipping reload of ntp settings since they are unchanged")
       return
@@ -39,15 +21,6 @@ def ntp_reload(ntp_settings = server_ntp_settings)
       return
     end
 
-    if ntp_settings.delete(:source) != :server && !server_ntp_settings_blank?(ntp_config)
-      _log.info("Skipping reload of ntp settings from zone since this server is configured with it's own ntp settings")
-      return
-    end
-
-    if ntp_settings[:server].nil?
-      _log.warn("No ntp server settings to synchronize")
-      return
-    end
     _log.info("Synchronizing ntp settings: #{ntp_settings.inspect}")
     apply_ntp_server_settings(ntp_settings)
     @ntp_settings = ntp_settings

app/models/miq_server/queue_management.rb

@@ -64,7 +64,6 @@ def ntp_reload_queue
       :method_name => "ntp_reload",
       :server_guid => guid,
       :priority    => MiqQueue::HIGH_PRIORITY,
-      :args => [server_ntp_settings],
       :zone        => my_zone
     )
   end

app/models/zone.rb

@@ -22,7 +22,6 @@ class Zone < ApplicationRecord
   virtual_has_many :active_miq_servers, :class_name => "MiqServer"
 
   before_destroy :check_zone_in_use_on_destroy
-  after_update   :queue_ntp_reload_if_changed
 
   include AuthenticationMixin
 
@@ -53,11 +52,6 @@ def miq_region
     MiqRegion.find_by(:region => region_id)
   end
 
-  def ntp_settings
-    # Return ntp settings if populated otherwise return the defaults
-    settings.fetch_path(:ntp, :server).present? ? settings[:ntp] : settings_for_resource.ntp.to_h
-  end
-
   def assigned_roles
     miq_servers.flat_map(&:assigned_roles).uniq.compact
   end
@@ -190,32 +184,18 @@ def any_started_miq_servers?
     miq_servers.any?(&:started?)
   end
 
+  def ntp_reload_queue
+    servers = active_miq_servers
+    return if servers.blank?
+    _log.info("Zone: [#{name}], Queueing ntp_reload for [#{servers.length}] active_miq_servers, ids: #{servers.collect(&:id)}")
+
+    servers.each(&:ntp_reload_queue)
+  end
+
   protected
 
   def check_zone_in_use_on_destroy
     raise _("cannot delete default zone") if name == "default"
     raise _("zone name '%{name}' is used by a server") % {:name => name} unless miq_servers.blank?
   end
-
-  private
-
-  def queue_ntp_reload_if_changed
-    return if settings_was[:ntp] == ntp_settings
-
-    servers = active_miq_servers
-    return if servers.blank?
-    _log.info("Zone: [#{name}], Queueing ntp_reload for [#{servers.length}] active_miq_servers, ids: #{servers.collect(&:id)}")
-
-    servers.each do |s|
-      MiqQueue.put_deprecated(
-        :class_name  => "MiqServer",
-        :instance_id => s.id,
-        :method_name => "ntp_reload",
-        :args        => [ntp_settings],
-        :server_guid => s.guid,
-        :priority    => MiqQueue::HIGH_PRIORITY,
-        :zone        => name
-      )
-    end
-  end
 end

spec/models/miq_server_spec.rb

@@ -174,32 +174,17 @@
           @miq_server.ntp_reload
         end
 
-        it "syncs with server settings with zone and server configured" do
-          @zone.update_attribute(:settings, :ntp => zone_ntp)
-          stub_settings(:ntp => server_ntp)
-
+        it "syncs the settings" do
           expect(LinuxAdmin::Chrony).to receive(:new).and_return(chrony)
           expect(chrony).to receive(:clear_servers)
-          expect(chrony).to receive(:add_servers).with(*server_ntp[:server])
+          expect(chrony).to receive(:add_servers).with("0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org")
           @miq_server.ntp_reload
         end
 
-        it "syncs with zone settings if server not configured" do
-          @zone.update_attribute(:settings, :ntp => zone_ntp)
-          stub_settings({})
+        it "only changes the config file if there are changes" do
+          expect(@miq_server).to receive(:apply_ntp_server_settings).once
 
-          expect(LinuxAdmin::Chrony).to receive(:new).and_return(chrony)
-          expect(chrony).to receive(:clear_servers)
-          expect(chrony).to receive(:add_servers).with(*zone_ntp[:server])
           @miq_server.ntp_reload
-        end
-
-        it "syncs with default zone settings if server and zone not configured" do
-          @zone.update_attribute(:settings, {})
-
-          expect(LinuxAdmin::Chrony).to receive(:new).and_return(chrony)
-          expect(chrony).to receive(:clear_servers)
-          expect(chrony).to receive(:add_servers).with("0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org")
           @miq_server.ntp_reload
         end
       end

spec/models/zone_spec.rb

@@ -141,47 +141,6 @@
     end
   end
 
-  context "#ntp_settings" do
-    let(:zone) { described_class.new(:name => "test", :description => "test description") }
-
-    it "no settings returns default NTP settings" do
-      zone.save!
-
-      expect(zone.ntp_settings).to eq(:server => ["0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org"])
-    end
-
-    it "with :ntp key returns what was set" do
-      zone.settings[:ntp] = {:server => ["tock.example.com"]}
-
-      expect(zone.ntp_settings).to eq(:server => ["tock.example.com"])
-    end
-  end
-
-  context "#after_save callback" do
-    before do
-      _, _, @zone = EvmSpecHelper.create_guid_miq_server_zone
-
-      @zone.update_attributes(:settings => {:ntp => {:server => ["tick.example.com"]}})
-      MiqQueue.where(:class_name => "MiqServer", :method_name => "ntp_reload").destroy_all
-    end
-
-    it "settings changed queues ntp reload" do
-      expect_any_instance_of(described_class).to receive(:queue_ntp_reload_if_changed).once.and_call_original
-
-      @zone.update_attributes(:settings => {:ntp => {:server => ["tock.example.com"]}})
-
-      expect(MiqQueue.where(:class_name => "MiqServer", :method_name => "ntp_reload").count).to eq(1)
-    end
-
-    it "settings not changed does not queue ntp reload" do
-      expect_any_instance_of(described_class).to receive(:queue_ntp_reload_if_changed).once.and_call_original
-
-      @zone.update_attributes(:settings => {:ntp => {:server => ["tick.example.com"]}})
-
-      expect(MiqQueue.where(:class_name => "MiqServer", :method_name => "ntp_reload").count).to eq(0)
-    end
-  end
-
   it "#settings should always be a hash" do
     expect(described_class.new.settings).to be_kind_of(Hash)
   end
@@ -210,4 +169,26 @@
       end
     end
   end
+
+  context "#ntp_reload_queue" do
+    it "queues a ntp reload for all active servers in the zone" do
+      expect(MiqEnvironment::Command).to receive(:is_appliance?).and_return(true)
+      expect(MiqEnvironment::Command).to receive(:is_container?).and_return(false)
+      zone     = FactoryGirl.create(:zone)
+      server_1 = FactoryGirl.create(:miq_server, :zone => zone)
+      FactoryGirl.create(:miq_server, :zone => zone, :status => "stopped")
+
+      zone.ntp_reload_queue
+
+      expect(MiqQueue.count).to eq(1)
+      expect(
+        MiqQueue.where(
+          :class_name  => "MiqServer",
+          :instance_id => server_1.id,
+          :method_name => "ntp_reload",
+          :server_guid => server_1.guid,
+        ).count
+      ).to eq(1)
+    end
+  end
 end