(PUP-12061) Recalculate splay when splaylimit changes

If puppet.conf was modified, but splaylimit wasn't, then we recalculated
the splay offset. This goes against the desired behavior of only splaying when
the agent is initially started and running every runinterval seconds from that
point forward or until runinterval/splaylimit are changed in the future.

Now the SplayJob remembers its last splaylimit so that it only recalculates
splay if the limit changes. It's important to recalculate splay so that the
agent chooses a slot over the new range, either smaller or bigger than before.

Author: joshcooper

lib/puppet/daemon.rb

@@ -182,6 +182,7 @@ def run_event_loop
 
     reparse_run.disable if Puppet[:filetimeout] == 0
 
+    # these are added in a different order than they are defined
     @scheduler.run_loop([reparse_run, agent_run, signal_loop])
   end
 end

lib/puppet/scheduler/splay_job.rb

@@ -5,7 +5,7 @@ class SplayJob < Job
     attr_reader :splay
 
     def initialize(run_interval, splay_limit, &block)
-      @splay = calculate_splay(splay_limit)
+      @splay, @splay_limit = calculate_splay(splay_limit)
       super(run_interval, &block)
     end
 
@@ -31,13 +31,15 @@ def ready?(time)
     # @return @splay [Integer] a random integer less than or equal to the splay limit that represents the seconds to
     # delay before next agent run.
     def splay_limit=(splay_limit)
-      @splay = calculate_splay(splay_limit)
+      if @splay_limit != splay_limit
+        @splay, @splay_limit = calculate_splay(splay_limit)
+      end
     end
 
     private
 
     def calculate_splay(limit)
-      rand(limit + 1)
+      [rand(limit + 1), limit]
     end
   end
 end

spec/unit/daemon_spec.rb

@@ -83,6 +83,11 @@ def run_loop(jobs)
       expect(reparse_run).not_to be_enabled
     end
 
+    it "does not splay the agent run by default" do
+      daemon.start
+      expect(agent_run).to be_an_instance_of(Puppet::Scheduler::Job)
+    end
+
     describe "and calculating splay" do
       before do
         # Set file timeout so the daemon reparses
@@ -91,12 +96,24 @@ def run_loop(jobs)
       end
 
       it "recalculates when splaylimit changes" do
-        allow(agent).to receive(:run)
         daemon.start
-        first_splay = agent_run.splay
-        allow(Kernel).to receive(:rand).and_return(1738)
+
+        Puppet[:splaylimit] = 60
+        init_splay = agent_run.splay
+        next_splay = init_splay + 1
+        allow(agent_run).to receive(:rand).and_return(next_splay)
         reparse_run.run(Time.now)
-        expect(agent_run.splay).to_not eq(first_splay)
+
+        expect(agent_run.splay).to eq(next_splay)
+      end
+
+      it "does not change splay if splaylimit is unmodified" do
+        daemon.start
+
+        init_splay = agent_run.splay
+        reparse_run.run(Time.now)
+
+        expect(agent_run.splay).to eq(init_splay)
       end
     end
   end

spec/unit/scheduler/splay_job_spec.rb

@@ -31,4 +31,17 @@
     expect(job).to receive(:splay).and_return(6)
     expect(job.ready?(start_time)).not_to be
   end
+
+  it "does not apply a splay if the splaylimit is unchanged" do
+    old_splay = job.splay
+    job.splay_limit = splay_limit
+    expect(job.splay).to eq(old_splay)
+  end
+
+  it "applies a splay if the splaylimit is changed" do
+    new_splay = 999
+    allow(job).to receive(:rand).and_return(new_splay)
+    job.splay_limit = splay_limit + 1
+    expect(job.splay).to eq(new_splay)
+  end
 end