Support worker heartbeat to a local file instead of Drb. #15377

gtanzillo · 2017-06-14T21:22:54Z

For the new architecture, we'd like to move away for using Drb. This PR takes a step in that direction by enabling workers to heartbeat to a file instead of Drb.

https://www.pivotaltracker.com/story/show/147230943

NickLaMuro

Like what you got hear. Simple, and should do the trick. My comments are pedantic as always.

NickLaMuro · 2017-06-15T16:39:24Z

app/models/miq_worker/runner.rb

+  end
+
+  def heartbeat_to_file
+    require 'fileutils'


My two cents with this:

Just include it at the top of the file. This costs us around a megabyte to include it, and there is a good chance it is already being included by some other gem anyway.

If we do it this way, it means we are making this require "fileutils" call on every heartbeat_to_file call, and I would personally want to make this as lightweight of an operation as possible (though, I think subsequent requires are pretty good at being quick, but still).

NickLaMuro · 2017-06-15T16:40:01Z

app/models/miq_worker.rb

@@ -219,6 +219,10 @@ def worker_settings(options = {})
    self.class.fetch_worker_settings_from_server(miq_server, options)
  end

+  def heartbeat_file
+    ENV["WORKER_HEARTBEAT_FILE"] || Rails.root.join("tmp", guid + ".hb")


Wat!? No string interpolation... but using string concatenation! For shame...

😭 I'll fix that!

gtanzillo · 2017-06-16T19:23:32Z

app/models/miq_worker.rb

@@ -219,6 +219,10 @@ def worker_settings(options = {})
    self.class.fetch_worker_settings_from_server(miq_server, options)
  end

+  def heartbeat_file
+    ENV["WORKER_HEARTBEAT_FILE"] || Rails.root.join("tmp", "#{guid}.hb")


TODO: Need to clean up heartbeat files.

Morbid curiosity (and since I got pinged on this thread), why?

Is an old Heartbeat file (like a stale worker record) a problem? Not saying we shouldn't clean things up, but just curious if there are issues you have identified where cleaning these up is necessary to resolve them.

No issue other just having it clean up after itself. Every time a worker is started it will get a new guid and a new file so they could accumulate fast.

I think you got pinged because you previously added comments.

I think you got pinged because you previously added comments.

Yup, knew that, but poorly conveyed. Pure curiosity on something that I am actively following and trying not to act like a dic-tator 🥔 .

Sorry @NickLaMuro, I misread you comment and though you were asking why you got pinged, lol. I maded the change to cleanup the heartbeat file create on prior runs.

gtanzillo · 2017-06-16T19:36:44Z

app/models/miq_worker/runner.rb

+    send(heartbeat_method)
+
+    do_heartbeat_work
+  rescue DRb::DRbError => err


This should be moved down in the drb method

gtanzillo · 2017-06-16T19:37:28Z

app/models/miq_server/worker_management/heartbeat.rb

@@ -93,9 +93,17 @@ def validate_heartbeat(w)

  private

-  def workers_last_heartbeat(pid)
+  def workers_last_heartbeat(w)
+    send("workers_last_heartbeat_to_#{get_heartbeat_method(w)}", w)


Use ternary operator here instead of building method names.

gtanzillo · 2017-06-22T21:11:25Z

@jrafanie This should be ready to merge

jrafanie · 2017-06-23T16:30:35Z

app/models/miq_server/worker_management/heartbeat.rb

    end
  end
+
+  def workers_last_heartbeat_to_file(w)
+    File.mtime(w.heartbeat_file).utc


@gtanzillo Do you think we probably need to do a File.exist? check here? Is it possible we run this method either:

before a worker creates the file

after the file is removed but the worker row remains

Is this possible?

We don't have to tackle this here since we're defaulting to DRb, it's just a thought I had.

Good thought @jrafanie. I'll add that check because I don't think it should just assume it exists.

NickLaMuro

@gtanzillo just some pedantic thoughts I had while looking this over. Probably not worth holding up the merge of this PR to address.

NickLaMuro · 2017-06-23T18:31:31Z

app/models/miq_server/worker_management/heartbeat.rb

  private

-  def workers_last_heartbeat(pid)
+  def workers_last_heartbeat(w)
+    ENV["WORKER_HEARTBEAT_METHOD"] == "file" ? workers_last_heartbeat_to_file(w) : workers_last_heartbeat_to_drb(w)


@gtanzillo This is pedantic, but while you are addressing @jrafanie 's comment, I feel like these methods should use from instead of to:

workers_last_heartbeat_to_file => workers_last_heartbeat_from_file

workers_last_heartbeat_to_drb => workers_last_heartbeat_from_drb

I thought of that too (I'm telling the truth!) and discussed it with @jrafanie and we thought that consistency in method names would be better than having them semantically correct.

NickLaMuro · 2017-06-23T18:35:18Z

app/models/miq_worker.rb

@@ -219,6 +219,10 @@ def worker_settings(options = {})
    self.class.fetch_worker_settings_from_server(miq_server, options)
  end

+  def heartbeat_file
+    ENV["WORKER_HEARTBEAT_FILE"] || Rails.root.join("tmp", "#{guid}.hb")


Should we memoize this value into an instance variable?

def heartbeat_file @heartbeat_file ||= ENV["WORKER_HEARTBEAT_FILE"] || Rails.root.join("tmp", "#{guid}.hb") end

NickLaMuro · 2017-06-23T18:36:27Z

app/models/miq_server.rb

@@ -152,6 +152,7 @@ def start
    #############################################################
    # Start all the configured workers
    #############################################################
+    clean_heartbeat_files


Is this something we want to do conditionally? By that, I mean only worry about this if we are using a file based heartbeat? This can be done within the method of course, just thought I would bring it up.

Thanks @NickLaMuro I though about that but didn't think it would hurt to just do it blindly since it's only done once at server startup.

https://www.pivotaltracker.com/story/show/147230943

miq-bot · 2017-06-23T19:53:00Z

Checked commit gtanzillo@34a11ab with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 2 offenses detected

app/models/miq_worker/runner.rb

⚠️ - Line 368, Col 3 - Lint/RescueException - Avoid rescuing the Exception class. Perhaps you meant to rescue StandardError?
❗ - Line 388, Col 15 - Rails/SkipsModelValidations - Avoid using touch because it skips validations.

gtanzillo changed the title ~~[WIP][DO NOT MERGE] Prototype of worker heartbreaking to a local file.~~ [WIP][DO NOT MERGE] Prototype of worker heartbreating to a local file. Jun 14, 2017

gtanzillo changed the title ~~[WIP][DO NOT MERGE] Prototype of worker heartbreating to a local file.~~ [WIP][DO NOT MERGE] Prototype of worker heartbeating to a local file. Jun 14, 2017

gtanzillo force-pushed the rearch-heartbeat-to-file branch from 47ea14b to de2e90e Compare June 14, 2017 21:23

miq-bot added the wip label Jun 14, 2017

NickLaMuro reviewed Jun 15, 2017

View reviewed changes

gtanzillo force-pushed the rearch-heartbeat-to-file branch from de2e90e to 0760cba Compare June 16, 2017 14:06

gtanzillo commented Jun 16, 2017

View reviewed changes

gtanzillo force-pushed the rearch-heartbeat-to-file branch from 0760cba to 55851ce Compare June 19, 2017 01:25

gtanzillo changed the title ~~[WIP][DO NOT MERGE] Prototype of worker heartbeating to a local file.~~ Prototype of worker heartbeating to a local file. Jun 19, 2017

gtanzillo added core/workers enhancement and removed wip labels Jun 19, 2017

gtanzillo changed the title ~~Prototype of worker heartbeating to a local file.~~ Support worker heartbeat to a local file instead of Drb. Jun 19, 2017

gtanzillo force-pushed the rearch-heartbeat-to-file branch from 55851ce to b8f15ad Compare June 22, 2017 21:04

gtanzillo assigned jrafanie Jun 22, 2017

jrafanie reviewed Jun 23, 2017

View reviewed changes

NickLaMuro reviewed Jun 23, 2017

View reviewed changes

gtanzillo force-pushed the rearch-heartbeat-to-file branch from b8f15ad to 4668215 Compare June 23, 2017 19:19

NickLaMuro approved these changes Jun 23, 2017

View reviewed changes

Support for using a file instead of Drb for worker heartbeating.

34a11ab

https://www.pivotaltracker.com/story/show/147230943

gtanzillo force-pushed the rearch-heartbeat-to-file branch from 4668215 to 34a11ab Compare June 23, 2017 19:46

jrafanie merged commit a0b650f into ManageIQ:master Jun 23, 2017

jrafanie added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 23, 2017

NickLaMuro mentioned this pull request Jun 29, 2017

Fix pseudo heartbeating when HB file missing #15483

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support worker heartbeat to a local file instead of Drb. #15377

Support worker heartbeat to a local file instead of Drb. #15377

gtanzillo commented Jun 14, 2017 •

edited

Loading

NickLaMuro left a comment

NickLaMuro Jun 15, 2017

NickLaMuro Jun 15, 2017

gtanzillo Jun 16, 2017

gtanzillo Jun 16, 2017

NickLaMuro Jun 16, 2017 •

edited

Loading

gtanzillo Jun 16, 2017

NickLaMuro Jun 16, 2017

gtanzillo Jun 19, 2017

gtanzillo Jun 16, 2017

gtanzillo Jun 16, 2017

gtanzillo commented Jun 22, 2017

jrafanie Jun 23, 2017

gtanzillo Jun 23, 2017

NickLaMuro left a comment

NickLaMuro Jun 23, 2017

gtanzillo Jun 23, 2017

NickLaMuro Jun 23, 2017

NickLaMuro Jun 23, 2017

gtanzillo Jun 23, 2017

miq-bot commented Jun 23, 2017

Support worker heartbeat to a local file instead of Drb. #15377

Support worker heartbeat to a local file instead of Drb. #15377

Conversation

gtanzillo commented Jun 14, 2017 • edited Loading

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtanzillo commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 23, 2017

gtanzillo commented Jun 14, 2017 •

edited

Loading

NickLaMuro Jun 16, 2017 •

edited

Loading