Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check for messages key in prefetch_below_threshold? #15620

Merged
merged 1 commit into from
Jul 21, 2017
Merged

Check for messages key in prefetch_below_threshold? #15620

merged 1 commit into from
Jul 21, 2017

Conversation

djberg96
Copy link
Contributor

A customer log showed a worker failure that seems to be caused by a failed .nil? check in the MiqServer::WorkerManagement::Dequeue#prefetch_below_threshold? method. Below is a snippet from their evm.log:

[----] I, [2017-07-10T03:44:03.429517 #2976:1267130]  INFO -- : MIQ(MiqScheduleWorker::Runner#do_work) Number of scheduled items to be processed: 2.
[----] E, [2017-07-10T03:44:08.642178 #2697:1267130] ERROR -- : MIQ(MiqServer#monitor) undefined method `length' for nil:NilClass
[----] E, [2017-07-10T03:44:08.642290 #2697:1267130] ERROR -- : [NoMethodError]: undefined method `length' for nil:NilClass  Method:[rescue in monitor]
[----] E, [2017-07-10T03:44:08.642362 #2697:1267130] ERROR -- : /var/www/miq/vmdb/app/models/miq_server/worker_management/dequeue.rb:65:in `block in prefetch_below_threshold?'
/opt/rh/rh-ruby23/root/usr/share/ruby/sync.rb:234:in `block in sync_synchronize'
/opt/rh/rh-ruby23/root/usr/share/ruby/sync.rb:231:in `handle_interrupt'
/opt/rh/rh-ruby23/root/usr/share/ruby/sync.rb:231:in `sync_synchronize'
/var/www/miq/vmdb/app/models/miq_server/worker_management/dequeue.rb:63:in `prefetch_below_threshold?'
/var/www/miq/vmdb/app/models/miq_server/worker_management/dequeue.rb:104:in `block (2 levels) in populate_queue_messages'
/var/www/miq/vmdb/app/models/miq_server/worker_management/dequeue.rb:103:in `each'
/var/www/miq/vmdb/app/models/miq_server/worker_management/dequeue.rb:103:in `block in populate_queue_messages'
/opt/rh/rh-ruby23/root/usr/share/ruby/sync.rb:234:in `block in sync_synchronize'
/opt/rh/rh-ruby23/root/usr/share/ruby/sync.rb:231:in `handle_interrupt'
/opt/rh/rh-ruby23/root/usr/share/ruby/sync.rb:231:in `sync_synchronize'
/var/www/miq/vmdb/app/models/miq_server/worker_management/dequeue.rb:102:in `populate_queue_messages'
/var/www/miq/vmdb/app/models/miq_server.rb:349:in `block in monitor'
/opt/rh/cfme-gemset/bundler/gems/manageiq-gems-pending-e0f3ea8755bf/lib/gems/pending/util/extensions/miq-benchmark.rb:11:in `realtime_store'
/opt/rh/cfme-gemset/bundler/gems/manageiq-gems-pending-e0f3ea8755bf/lib/gems/pending/util/extensions/miq-benchmark.rb:30:in `realtime_block'
/var/www/miq/vmdb/app/models/miq_server.rb:349:in `monitor'
/var/www/miq/vmdb/app/models/miq_server.rb:370:in `block (2 levels) in monitor_loop'
/opt/rh/cfme-gemset/bundler/gems/manageiq-gems-pending-e0f3ea8755bf/lib/gems/pending/util/extensions/miq-benchmark.rb:11:in `realtime_store'
/opt/rh/cfme-gemset/bundler/gems/manageiq-gems-pending-e0f3ea8755bf/lib/gems/pending/util/extensions/miq-benchmark.rb:30:in `realtime_block'
/var/www/miq/vmdb/app/models/miq_server.rb:370:in `block in monitor_loop'
/var/www/miq/vmdb/app/models/miq_server.rb:369:in `loop'
/var/www/miq/vmdb/app/models/miq_server.rb:369:in `monitor_loop'
/var/www/miq/vmdb/app/models/miq_server.rb:252:in `start'
/var/www/miq/vmdb/lib/workers/evm_server.rb:65:in `start'
/var/www/miq/vmdb/lib/workers/evm_server.rb:91:in `start'

The proposed change just adds an explicit check on @queue_messages[queue_name][:messages].

Attempts to resolve https://bugzilla.redhat.com/show_bug.cgi?id=1471232

@djberg96
Copy link
Contributor Author

@gtanzillo Please take a look.

@bronaghs
Copy link

@miq-bot assign @gtanzillo

@Fryguy
Copy link
Member

Fryguy commented Jul 20, 2017

@bdunne @carbonin Can you look at this since you been in this code lately?

Fryguy
Fryguy reviewed Jul 20, 2017
View reviewed changes
app/models/miq_server/worker_management/dequeue.rb Outdated
@@ -61,7 +61,7 @@ def prefetch_stale_threshold

def prefetch_below_threshold?(queue_name, wcount)
@queue_messages_lock.synchronize(:SH) do
return false if @queue_messages[queue_name].nil?
return false if @queue_messages[queue_name].nil? || @queue_messages[queue_name][:messages].nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case prefer fetch_path

return false if @queue_messages.fetch_path(queue_name, :messages).nil?

Alternately, if you are saying the key may or may not exist (as opposed to key existing with a nil value), then you can use key_path?

return false unless @queue_messages.key_path?(queue_name, :messages)

@djberg96
Copy link
Contributor Author

@Fryguy done.

@miq-bot
Copy link
Member

miq-bot commented Jul 20, 2017

Checked commit https://github.com/djberg96/manageiq/commit/8c3d35f9603453c142c5afa1fd9d15c4e6a4bab9 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

bdunne
bdunne approved these changes Jul 20, 2017
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

gtanzillo
gtanzillo approved these changes Jul 21, 2017
View reviewed changes
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@gtanzillo gtanzillo added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 21, 2017
@gtanzillo gtanzillo merged commit 6e08686 into ManageIQ:master Jul 21, 2017
@djberg96
Copy link
Contributor Author

@miq-bot add_label fine/yes

@djberg96
Copy link
Contributor Author

@miq-bot add_label euwe/yes

simaishi pushed a commit that referenced this pull request Aug 11, 2017
@gtanzillo @simaishi
Merge pull request #15620 from djberg96/prefetch_below_threshold
cd68df4 
Check for messages key in prefetch_below_threshold?
(cherry picked from commit 6e08686)

https://bugzilla.redhat.com/show_bug.cgi?id=1480629
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit cd68df49e0dcbd7cee356e991009e4138a447fbe
Author: Gregg Tanzillo <[email protected]>
Date:   Thu Jul 20 20:45:23 2017 -0400

    Merge pull request #15620 from djberg96/prefetch_below_threshold
    
    Check for messages key in prefetch_below_threshold?
    (cherry picked from commit 6e08686bb2b11bad81778eba9fd6baa9fe69b41c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1480629

simaishi pushed a commit that referenced this pull request Aug 15, 2017
@gtanzillo @simaishi
Merge pull request #15620 from djberg96/prefetch_below_threshold
b56a4f6 
Check for messages key in prefetch_below_threshold?
(cherry picked from commit 6e08686)

https://bugzilla.redhat.com/show_bug.cgi?id=1480630
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit b56a4f6100c4231bdde6297be563cfc7b5626b75
Author: Gregg Tanzillo <[email protected]>
Date:   Thu Jul 20 20:45:23 2017 -0400

    Merge pull request #15620 from djberg96/prefetch_below_threshold
    
    Check for messages key in prefetch_below_threshold?
    (cherry picked from commit 6e08686bb2b11bad81778eba9fd6baa9fe69b41c)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1480630

d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@gtanzillo @d-m-u
Merge pull request ManageIQ#15620 from djberg96/prefetch_below_threshold
ae7d4a0 
Check for messages key in prefetch_below_threshold?
(cherry picked from commit 6e08686)

https://bugzilla.redhat.com/show_bug.cgi?id=1480629
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@bdunne bdunne bdunne approved these changes

@gtanzillo gtanzillo gtanzillo approved these changes

Assignees

@gtanzillo gtanzillo

Labels
bug core/queue euwe/backported fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@djberg96 @bronaghs @Fryguy @miq-bot @simaishi @gtanzillo @bdunne