From abd1b708fdd72021c4761ef54056ad86c329e8cb Mon Sep 17 00:00:00 2001
From: Nick LaMuro <nicklamuro@gmail.com>
Date: Thu, 29 Jun 2017 16:59:20 -0500
Subject: [PATCH] Fix psuedo heartbeating when HB file missing

With the previous version of this code, the validate_heartbeat code:

    def validate_heartbeat(w)
      last_heartbeat = workers_last_heartbeat(w)

      if w.last_heartbeat.nil?
        last_heartbeat ||= Time.now.utc
        w.update_attributes(:last_heartbeat => last_heartbeat)
      elsif !last_heartbeat.nil? && last_heartbeat > w.last_heartbeat
        w.update_attributes(:last_heartbeat => last_heartbeat)
      end
    end

Would update the `last_heartbeat` value for the worker every time
`validate_heartbeat` was called, which is not the intent.

When working correctly, the validate_heartbeat code should:

  * If no value is set in the DB, initialize it to either the time
    pulled from workers_last_heartbeat, or the current time (this should
    only happen the first time this method is called for this worker)
  * If workers_last_heartbeat is not nil, and it is a more resent
    heartbeat time, update the DB with the new value

Otherwise, assume we have the most up to date heartbeat value in the DB.

This change just returns `nil` if we don't have anything to provide from
the heartbeat file.  This is fine since the `validate_heartbeat` method
will take care of any initialization of the last_heartbeat value in the
DB, so workers_last_heartbeat_to_file is just an existence check, plus a
value if there is one.
---
 .../miq_server/worker_management/heartbeat.rb |  2 +-
 .../worker_management/heartbeat_spec.rb       | 70 +++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/app/models/miq_server/worker_management/heartbeat.rb b/app/models/miq_server/worker_management/heartbeat.rb
index 8def3fd1b7b..58f9bf547af 100644
--- a/app/models/miq_server/worker_management/heartbeat.rb
+++ b/app/models/miq_server/worker_management/heartbeat.rb
@@ -108,6 +108,6 @@ def workers_last_heartbeat_to_drb(w)
   end
 
   def workers_last_heartbeat_to_file(w)
-    File.exist?(w.heartbeat_file) ? File.mtime(w.heartbeat_file).utc : Time.now.utc
+    File.mtime(w.heartbeat_file).utc if File.exist?(w.heartbeat_file)
   end
 end
diff --git a/spec/models/miq_server/worker_management/heartbeat_spec.rb b/spec/models/miq_server/worker_management/heartbeat_spec.rb
index 40f81d5a685..a509f5f116e 100644
--- a/spec/models/miq_server/worker_management/heartbeat_spec.rb
+++ b/spec/models/miq_server/worker_management/heartbeat_spec.rb
@@ -15,5 +15,75 @@
         expect(worker.reload.last_heartbeat).to be_within(1.second).of(t)
       end
     end
+
+    # Iterating by 5 each time to allow enough spacing to be more than 1 second
+    # apart when using be_within(x).of(t)
+    context "when using file based heartbeating" do
+      let!(:first_heartbeat) { Time.now.utc }
+      let!(:heartbeat_file)  { "/path/to/worker.hb" }
+
+      around do |example|
+        ENV["WORKER_HEARTBEAT_METHOD"] = "file"
+        ENV["WORKER_HEARTBEAT_FILE"]   = heartbeat_file
+        example.run
+        ENV.delete("WORKER_HEARTBEAT_METHOD")
+        ENV.delete("WORKER_HEARTBEAT_FILE")
+      end
+
+      context "with an existing heartbeat file" do
+        it "sets initial and subsequent heartbeats" do
+          expect(File).to receive(:exist?).with(heartbeat_file).and_return(true, true)
+          expect(File).to receive(:mtime).with(heartbeat_file).and_return(first_heartbeat, first_heartbeat + 5)
+
+          [0, 5].each do |i|
+            Timecop.freeze(first_heartbeat) do
+              miq_server.worker_heartbeat(pid)
+              miq_server.validate_heartbeat(worker)
+            end
+
+            expect(worker.reload.last_heartbeat).to be_within(1.second).of(first_heartbeat + i)
+          end
+        end
+      end
+
+      context "with a missing heartbeat file" do
+        it "sets initial heartbeat only" do
+          expect(File).to receive(:exist?).with(heartbeat_file).and_return(false).exactly(4).times
+          expect(File).to receive(:mtime).with(heartbeat_file).never
+
+          # This has different results first iteration of the loop compared to
+          # the rest:
+          #   1. Sets the initial heartbeat
+          #   2. Doesn't update the worker's last_heartbeat value after that
+          #
+          # So the result from the database should not change after the first
+          # iteration of the loop
+          [0, 5, 10, 15].each do |i|
+            Timecop.freeze(first_heartbeat + i) do
+              miq_server.worker_heartbeat(pid)
+              miq_server.validate_heartbeat(worker)
+            end
+
+            expect(worker.reload.last_heartbeat).to be_within(1.second).of(first_heartbeat)
+          end
+        end
+      end
+
+      context "with a missing heartbeat file on the first validate" do
+        it "sets initial heartbeat default, and updates the heartbeat from the file second" do
+          expect(File).to receive(:exist?).with(heartbeat_file).and_return(false, true)
+          expect(File).to receive(:mtime).with(heartbeat_file).and_return(first_heartbeat + 5)
+
+          [0, 5].each do |i|
+            Timecop.freeze(first_heartbeat) do
+              miq_server.worker_heartbeat(pid)
+              miq_server.validate_heartbeat(worker)
+            end
+
+            expect(worker.reload.last_heartbeat).to be_within(1.second).of(first_heartbeat + i)
+          end
+        end
+      end
+    end
   end
 end