Always use file based heartbeat #19666

carbonin · 2019-12-19T23:09:16Z

Based on #19665

Fryguy · 2020-01-02T19:59:27Z

#19665 is merged

Fryguy · 2020-01-02T20:03:45Z

My only concern with the heartbeat file is that since we've only used it in a container so far, it had no chance for file collisions, since it's only one worker per container. So, I'm not sure if lines like this matter now -

manageiq/lib/workers/miq_defaults.rb

Line 22 in 2fb2389

guid ||= "miq_worker"

carbonin · 2020-01-06T19:43:50Z

@Fryguy That one is a bit misleading ... MiqWorker defines its own heartbeat_file method here

The MiqDefaults version is really just responsible for defining the location of the file and happens to deal with the env var as well.

This separates getting messages from the server over drb from the actual heartbeat which happens to the file.

The config change timestamp is stored in memcached and is checked after each worker heartbeat. Sending this message over drb isn't necessary anymore

…ectly The indirection here was put in place to match the existing drb message processing as closely as possible, now that that has been removed this can be simplified.

Additionally remove spec for #process_message which was really a spec for #sync_config

carbonin · 2020-01-07T19:05:21Z

Waiting on ManageIQ/vmware_web_service#70 to be released so that the broker doesn't fail with:

[----] E, [2020-01-07T11:52:58.338666 #21305:2ae59a9565f0] ERROR -- : MIQ(MiqVimBrokerWorker::Runner) ID [463] PID [21305] GUID [36f41131-9bcb-4605-a314-fc3f5b419009] Error heartbeating because NameError: uninitialized constant Timeout::ExitException
/home/ncarboni/.gem/ruby/2.5.5/gems/vmware_web_service-0.4.7/lib/VMwareWebService/broker_timeout.rb:39:in `timeout'
/home/ncarboni/.gem/ruby/2.5.5/gems/dalli-2.7.6/lib/dalli/socket.rb:131:in `open'
/home/ncarboni/.gem/ruby/2.5.5/gems/dalli-2.7.6/lib/dalli/server.rb:586:in `connect'
/home/ncarboni/.gem/ruby/2.5.5/gems/dalli-2.7.6/lib/dalli/server.rb:95:in `alive?'
/home/ncarboni/.gem/ruby/2.5.5/gems/dalli-2.7.6/lib/dalli/options.rb:24:in `block in alive?'
/home/ncarboni/.rubies/ruby-2.5.5/lib/ruby/2.5.0/monitor.rb:226:in `mon_synchronize'
/home/ncarboni/.gem/ruby/2.5.5/gems/dalli-2.7.6/lib/dalli/options.rb:23:in `alive?'
/home/ncarboni/.gem/ruby/2.5.5/gems/dalli-2.7.6/lib/dalli/ring.rb:42:in `server_for_key'
/home/ncarboni/.gem/ruby/2.5.5/gems/dalli-2.7.6/lib/dalli/client.rb:361:in `perform'
/home/ncarboni/.gem/ruby/2.5.5/gems/dalli-2.7.6/lib/dalli/client.rb:58:in `get'
/home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:378:in `server_last_change'
/home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:362:in `config_out_of_date?'
/home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:325:in `heartbeat'
/home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:295:in `block in do_work_loop'
/home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:293:in `loop'
/home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:293:in `do_work_loop'
/home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:137:in `run'
/home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:118:in `start'
/home/ncarboni/Source/manageiq/lib/workers/bin/run_single_worker.rb:113:in `<main>' Worker exiting.

This was a problem for the EventCatcher when sync_config was called as a part of worker initialization. The EventCatcher uses its ems hostname as a part of its log prefix, but the ems isn't set up until after the first call to sync_config. This was leading to errors when the worker was starting: [----] I, [2019-12-19T17:51:48.407958 ManageIQ#11054:2ab1a6d9a5f8] INFO -- : Starting ManageIQ::Providers::Vmware::InfraManager::EventCatcher with runner options {:ems_id=>"2", :guid=>"43a315ba-39ff-4326-99a6-89b3af48b1ee"} [----] I, [2019-12-19T17:51:48.451836 ManageIQ#11054:2ab1a6d9a5f8] INFO -- : Deleting worker record for ManageIQ::Providers::Vmware::InfraManager::EventCatcher, id 310 /home/ncarboni/Source/manageiq/app/models/manageiq/providers/base_manager/event_catcher/runner.rb:63:in `log_prefix': undefined method `hostname' for nil:NilClass (NoMethodError) from /home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:242:in `sync_config' from /home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:52:in `worker_initialization' from /home/ncarboni/Source/manageiq/app/models/miq_worker/runner.rb:42:in `initialize' from /home/ncarboni/Source/manageiq/lib/workers/bin/run_single_worker.rb:113:in `new' from /home/ncarboni/Source/manageiq/lib/workers/bin/run_single_worker.rb:113:in `<main>'

carbonin · 2020-01-07T21:52:02Z

@Fryguy this should be ready for real review.

carbonin · 2020-01-07T21:53:55Z

Also, this is related to the pods and zones epic ManageIQ/manageiq-pods#353

jrafanie · 2020-01-07T22:47:19Z

app/models/miq_server/worker_management/monitor.rb

@@ -32,8 +32,6 @@ def monitor_workers
      persist_last_heartbeat(worker)
      # Check the worker record for heartbeat timeouts
      next unless validate_worker(worker)
-      # Tell the valid workers to sync config if needed
-      worker_set_message(worker, "sync_config") if resync_needed


😮 Awesome

~~though, how do the worker know when config is changed now?~~

jrafanie · 2020-01-07T22:49:26Z

app/models/miq_worker/runner.rb

+
+    if config_out_of_date?
+      _log.info("#{log_prefix} Synchronizing configuration...")
+      sync_config


lol, there it is. Ignore my above question.

chessbyte · 2020-01-08T02:19:39Z

app/models/miq_worker/runner.rb

    worker_monitor_drb.worker_get_messages(@worker.pid).each do |msg, *args|
      process_message(msg, *args)
    end
  rescue DRb::DRbError => err
-    do_exit("Error heartbeating to MiqServer because #{err.class.name}: #{err.message}", 1)
+    do_exit("Error processing messages from MiqServer because #{err.class.name}: #{err.message}", 1)


if this PR is about removing DRb-based heartbeating, why do we have rescue DRb::DRbError here?

This method in particular is not for heartbeat. This fetches messages set for the worker in the @workers hash over drb.

The vim broker is the only worker that still uses this functionality (previously it was also used for config sync and stopping workers), so this can all be removed once the vim broker is gone.

To clarify, processing messages and heartbeating are two different operations that we happen to execute at the same time.

thanks for the clarification @carbonin

app/models/miq_server/worker_management/monitor.rb

…ymore The side-effects of this method are still needed

miq-bot · 2020-01-08T18:20:53Z

Checked commits carbonin/manageiq@849eec9~...fe52aad with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. ⭐

carbonin added enhancement technical debt wip core/workers labels Dec 19, 2019

Fryguy self-assigned this Jan 2, 2020

carbonin force-pushed the always_use_file_based_heartbeat branch from ea28379 to 928af24 Compare January 6, 2020 19:38

carbonin force-pushed the always_use_file_based_heartbeat branch from 928af24 to efb4593 Compare January 6, 2020 20:28

carbonin added 6 commits January 6, 2020 17:14

Ensure worker heartbeat file is removed in run_single_worker.rb

849eec9

Heartbeat only to a file, not to drb

20f9cc5

This separates getting messages from the server over drb from the actual heartbeat which happens to the file.

Don't tell workers to sync config over drb

5df4609

The config change timestamp is stored in memcached and is checked after each worker heartbeat. Sending this message over drb isn't necessary anymore

Rename #get_messages to #config_out_of_date? and call sync_config dir…

9c4d135

…ectly The indirection here was put in place to match the existing drb message processing as closely as possible, now that that has been removed this can be simplified.

Only remove drb heartbeat tests and setup

d594f36

Make specs for #get_messages specs for #config_out_of_date?

7a0d6b9

Additionally remove spec for #process_message which was really a spec for #sync_config

carbonin mentioned this pull request Jan 6, 2020

The event catcher fails for an host with a null hostname ManageIQ/manageiq-providers-vmware#505

Closed

carbonin force-pushed the always_use_file_based_heartbeat branch from 17076f9 to 010055a Compare January 7, 2020 20:11

carbonin changed the title ~~[WIP] Always use file based heartbeat~~ Always use file based heartbeat Jan 7, 2020

miq-bot removed the wip label Jan 7, 2020

carbonin mentioned this pull request Jan 7, 2020

[Podification] Make it work with Zones ManageIQ/manageiq-pods#353

Closed

jrafanie reviewed Jan 7, 2020

View reviewed changes

chessbyte reviewed Jan 8, 2020

View reviewed changes

carbonin commented Jan 8, 2020

View reviewed changes

app/models/miq_server/worker_management/monitor.rb Outdated Show resolved Hide resolved

Rename sync_needed? to sync_monitor as it doesn't return a boolean an…

fe52aad

…ymore The side-effects of this method are still needed

Fryguy merged commit 776b041 into ManageIQ:master Jan 14, 2020

Fryguy added this to the Sprint 128 Ending Jan 20, 2020 milestone Jan 14, 2020

carbonin deleted the always_use_file_based_heartbeat branch April 23, 2020 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always use file based heartbeat #19666

Always use file based heartbeat #19666

carbonin commented Dec 19, 2019

Fryguy commented Jan 2, 2020

Fryguy commented Jan 2, 2020

carbonin commented Jan 6, 2020

carbonin commented Jan 7, 2020

carbonin commented Jan 7, 2020

carbonin commented Jan 7, 2020

jrafanie Jan 7, 2020

jrafanie Jan 7, 2020 •

edited

Loading

jrafanie Jan 7, 2020

chessbyte Jan 8, 2020

carbonin Jan 8, 2020

carbonin Jan 8, 2020

chessbyte Jan 8, 2020

miq-bot commented Jan 8, 2020

Always use file based heartbeat #19666

Always use file based heartbeat #19666

Conversation

carbonin commented Dec 19, 2019

Fryguy commented Jan 2, 2020

Fryguy commented Jan 2, 2020

carbonin commented Jan 6, 2020

carbonin commented Jan 7, 2020

carbonin commented Jan 7, 2020

carbonin commented Jan 7, 2020

jrafanie Jan 7, 2020

Choose a reason for hiding this comment

jrafanie Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

jrafanie Jan 7, 2020

Choose a reason for hiding this comment

chessbyte Jan 8, 2020

Choose a reason for hiding this comment

carbonin Jan 8, 2020

Choose a reason for hiding this comment

carbonin Jan 8, 2020

Choose a reason for hiding this comment

chessbyte Jan 8, 2020

Choose a reason for hiding this comment

miq-bot commented Jan 8, 2020

jrafanie Jan 7, 2020 •

edited

Loading