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.

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

Don't ignore errors in Vm#running_processes #17220

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Mar 28, 2018

The Vm#running_processes method currently logs all errors/exceptions
instead of raising them up. This causes the tasks to look like they
were successful even though there are warnings in the log about
mis-configuration which should be corrected by the user.

The bottom two tasks were from before this change, they actually failed but look successful in the task list. The top two tasks were from after this change.

screenshot from 2018-03-28 09-30-10

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1534023

@agrare agrare added the bug label Mar 28, 2018
@agrare
Copy link
Member Author

agrare commented Mar 28, 2018

Not sure who best to review this as the code is so ancient, @bdunne what do you think?

@agrare agrare force-pushed the bz_1534023_running_processes_dont_eat_exception branch 2 times, most recently from 6ecb7ee to b88bc68 Compare March 28, 2018 13:53
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 unless WMIHelper.connectServer raises if it fails to connect...

@bdunne bdunne requested a review from djberg96 March 28, 2018 14:01
@agrare
Copy link
Member Author

agrare commented Mar 28, 2018

unless WMIHelper.connectServer raises if it fails to connect...

Hm right we want it to try the next IP address, but then if they all fail we want to raise an error...

@agrare
Copy link
Member Author

agrare commented Mar 28, 2018

I gave it some bogus creds and the connect doesn't fail, it does throw an exception deep down in manageiq-gems-pending-e29e0b0d8abd/lib/gems/pending/util/win32/miq-wmi-linux.rb:

[----] E, [2018-03-28T10:07:39.904641 #18528:2ab91025af50] ERROR -- : [NoMethodError]: undefined method `include?' for nil:NilClass  Method:[block in method_missing]
[----] E, [2018-03-28T10:07:39.905703 #18528:2ab91025af50] ERROR -- : /home/agrare/.gem/ruby/2.4.3/bundler/gems/manageiq-gems-pending-e29e0b0d8abd/lib/gems/pending/util/win32/miq-wmi-linux.rb:77:in `run_query'
/home/agrare/.gem/ruby/2.4.3/bundler/gems/manageiq-gems-pending-e29e0b0d8abd/lib/gems/pending/util/win32/miq-wmi-linux.rb:140:in `get_instance'
/home/agrare/.gem/ruby/2.4.3/bundler/gems/manageiq-gems-pending-e29e0b0d8abd/lib/gems/pending/util/miq-process.rb:171:in `process_list_wmi'
/home/agrare/.gem/ruby/2.4.3/bundler/gems/manageiq-gems-pending-e29e0b0d8abd/lib/gems/pending/util/miq-process.rb:154:in `process_list_all'
/home/agrare/src/manageiq/manageiq/app/models/vm.rb:93:in `block in running_processes'
/home/agrare/src/manageiq/manageiq/app/models/vm.rb:88:in `each'
/home/agrare/src/manageiq/manageiq/app/models/vm.rb:88:in `running_processes'

@agrare
Copy link
Member Author

agrare commented Mar 28, 2018

@bdunne What do you think about just raising the validation errors since that should catch most of the issues

The Vm#running_processes method currently logs all errors/exceptions
instead of raising them up.  This causes the tasks to look like they
were successful even though there are warnings in the log about
mis-configuration which should be corrected by the user.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1534023
@agrare agrare force-pushed the bz_1534023_running_processes_dont_eat_exception branch from b88bc68 to b187c43 Compare March 29, 2018 12:26
@miq-bot
Copy link
Member

miq-bot commented Mar 29, 2018

Checked commit agrare@b187c43 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@bdunne bdunne merged commit c6c282a into ManageIQ:master Mar 29, 2018
@bdunne bdunne added this to the Sprint 83 Ending Apr 9, 2018 milestone Mar 29, 2018
@bdunne bdunne self-assigned this Mar 29, 2018
@agrare agrare deleted the bz_1534023_running_processes_dont_eat_exception branch March 29, 2018 14:58
@agrare
Copy link
Member Author

agrare commented Mar 29, 2018

Thanks @bdunne !

simaishi pushed a commit that referenced this pull request Apr 2, 2018
…nt_eat_exception

Don't ignore errors in Vm#running_processes
(cherry picked from commit c6c282a)

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

simaishi commented Apr 2, 2018

Gaprindashvili backport details:

$ git log -1
commit 6a4e2701e58fa36696ce8222add20c7678cadc05
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Thu Mar 29 10:53:08 2018 -0400

    Merge pull request #17220 from agrare/bz_1534023_running_processes_dont_eat_exception
    
    Don't ignore errors in Vm#running_processes
    (cherry picked from commit c6c282a59b4e4d700a3af14526cd397928f9ee52)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1562780

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants