Bmc fixes #264

logicminds · 2015-03-11T02:55:26Z

Fixes 9709
Fixes 9710
Fixes 9711
Fixes 9740

* updates rubyipmi to use version 0.9.0 or later as its now required

logicminds · 2015-03-11T03:03:40Z

This will require rubyipmi 0.9.1 which was just released today.

logicminds · 2015-03-11T03:06:04Z

[test]

logicminds · 2015-03-11T03:12:29Z

An example of what this provider can be seen here.

http://projects.theforeman.org/projects/smart-proxy/wiki/API#BMC-Controls (Troubleshooting section)

dmitri-d · 2015-03-11T15:42:56Z

modules/bmc/ipmi.rb

        end
      end

-      def self.providers_installed?
-        return Rubyipmi.providers_installed?
+      def self.providers_installed


This is a bit confusing -- the convention is that methods with a ? suffix return a boolean, but here it appears to return a list...

Yes, I know. I noticed this the other day in Rubyipmi and I plan to fix it but it will have to wait since its a breaking change. I have updated this in smart-proxy but this this will most likely be fixed in 0.11.0 of rubyipmi.

logicminds · 2015-03-11T21:11:30Z

@witlessbird regarding the private method. It appears after some testing that that even when I remove the private decorator all methods are still marked private. I am not sure why the class is still setting the methods private so there is something that I don't understand. This looks to be the case will all the other smart proxy modules as well. So as far as refactoring this to have a public bmc_setup method I am not sure this is possible unless its removed from the class and place inside a Proxy::BMC module which might not be ideal and might introduce other issues like authorization.

[3] pry(#<BmcApiTest>)> Proxy::BMC::Api.new.bmc_setup
NoMethodError: undefined method `bmc_setup' for #<Sinatra::Wrapper:0x007fa5619ebef8>
from (pry):3:in `test_api_recovers_from_missing_provider'
[4] pry(#<BmcApiTest>)>
 Proxy::BMC::Api.new.send(:bmc_setup)
NoMethodError: undefined method `bmc_setup' for #<Sinatra::Wrapper:0x007fa5632969a8>
from (pry):5:in `test_api_recovers_from_missing_provider'

dmitri-d · 2015-03-12T10:47:00Z

sinatra overrides ::Sinatra::Base.new, as a result you don't actually get an instance of your controller back. Call ::Proxy::BMC::Api.new! instead.

lzap · 2015-03-13T09:40:29Z

modules/bmc/bmc_api.rb

@@ -1,30 +1,48 @@
+require 'bmc/ipmi'


You are adding global require here, this will cause issues on discovery-image. Please load dependencies only when ipmi is enabled by configuration later on, not here.

It's actually ok here, as bmc_api will only get loaded if the module was enabled. https://github.com/theforeman/smart-proxy/blob/develop/lib/smart_proxy.rb#L100 is the bit that takes care of this...

lzap · 2015-03-13T09:52:57Z

Do you have an advice how to test this without BMC-enabled hardware? I guess there could be some mock mode or something in the library or IPMI stack.

logicminds · 2015-03-13T15:34:59Z

@lzap About the only thing that I came up with is a tool that feeds back STDOUT and STDERR along with an error code given STDIN. But I did not finish writing that tool and its still a private repo.

* previously when IPMI errors occured at the Rubyipmi or CLI level there was no good way to troubleshoot the issue. With this addition we can now watch an erorrs Rubyipmi spits out by sending Rubyipmi a custom logger.

logicminds · 2015-03-13T15:58:48Z

I could probably add a mock provider to rubyipmi which could then be used in smart-proxy when the mock provider is selected. I don't know when I would have time to do this though.

logicminds · 2015-03-17T23:14:09Z

[test]

logicminds · 2015-03-18T21:04:36Z

@witlessbird @lzap Jenkins seems to be failing tests, but I can get them to fail on my system. Is this a known issue with Jenkins and doing forced updates?

logicminds · 2015-03-18T21:08:58Z

This is ready to merge, I have addressed all the issues discussed above and refactored the code.

domcleal · 2015-03-19T08:17:05Z

I think it's because we don't have freeipmi installed on our slaves. We installed ipmitool as the existing tests assume that's present, but I also get failures locally without freeipmi:

[  3/309] BmcApiTest#test_api_bmc_setup_returns_new_ipmi_proxy_given_freeipmiwhich: no ipmipower in (/home/dcleal/.rvm/gems/ruby-2.1.0@proxy/bin:/home/dcleal/.rvm/gems/ruby-2.1.0@global/bin:/home/dcleal/.rvm/rubies/ruby-2.1.0/bin:/home/dcleal/.rvm/bin:/home/dcleal/bin:/ho
me/dcleal/go/bin:/home/dcleal/.gem/home/bin:/home/dcleal/code/puppet/puppet/bin:/home/dcleal/code/facter/bin:/usr/lib64/qt-3.3/bin:/usr/lib64/ccache:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/bin:/sbin/code/augeas/augeas-sandbox:/sbin)
 = 0.01 s
  1) Error:
BmcApiTest#test_api_bmc_setup_returns_new_ipmi_proxy_given_freeipmi:
NoMethodError: undefined method `accept?' for nil:NilClass
    /home/dcleal/code/foreman/smart-proxy/lib/proxy/helpers.rb:20:in `log_halt'
    /home/dcleal/code/foreman/smart-proxy/modules/bmc/bmc_api.rb:283:in `rescue in bmc_setup'
    /home/dcleal/code/foreman/smart-proxy/modules/bmc/bmc_api.rb:250:in `bmc_setup'
    /home/dcleal/code/foreman/smart-proxy/test/bmc/bmc_api_test.rb:89:in `test_api_bmc_setup_returns_new_ipmi_proxy_given_freeipmi'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:1265:in `run'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit/testcase.rb:17:in `run'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:940:in `block in _run_suite'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:933:in `map'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:933:in `_run_suite'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit.rb:663:in `block in _run_suites'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit.rb:661:in `each'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit.rb:661:in `_run_suites'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:884:in `_run_anything'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:1092:in `run_tests'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:1079:in `block in _run'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:1078:in `each'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:1078:in `_run'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/minitest/unit.rb:1066:in `run'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit.rb:27:in `run'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit.rb:780:in `run'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit.rb:372:in `block (2 levels) in autorun'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit.rb:33:in `run_once'
    /home/dcleal/.rvm/rubies/ruby-2.1.0/lib/ruby/2.1.0/test/unit.rb:371:in `block in autorun'
[  8/309] BmcApiTest#test_api_can_get_installed_providerswhich: no ipmipower in (/home/dcleal/.rvm/gems/ruby-2.1.0@proxy/bin:/home/dcleal/.rvm/gems/ruby-2.1.0@global/bin:/home/dcleal/.rvm/rubies/ruby-2.1.0/bin:/home/dcleal/.rvm/bin:/home/dcleal/bin:/home/dcleal/go/bin:/ho
me/dcleal/.gem/home/bin:/home/dcleal/code/puppet/puppet/bin:/home/dcleal/code/facter/bin:/usr/lib64/qt-3.3/bin:/usr/lib64/ccache:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/bin:/sbin/code/augeas/augeas-sandbox:/sbin)
 = 0.01 s
  2) Failure:
BmcApiTest#test_api_can_get_installed_providers [/home/dcleal/code/foreman/smart-proxy/test/bmc/bmc_api_test.rb:157]:
<["freeipmi", "ipmitool", "shell"]> expected but was
<["ipmitool", "shell"]>.

I'm rolling it out now, will retest in a bit.

dmitri-d · 2015-03-19T11:06:12Z

I'm seeing a local test failure when both freeipmi and ipmitool have been installed:

[ 28/309] BmcApiTest#test_api_recovers_from_nil_provider = 0.07 s                                                      
  1) Failure:
test_api_recovers_from_nil_provider(BmcApiTest) [/home/wb/sandbox/smart-proxy/test/bmc/bmc_api_test.rb:122]:
Last response was not ok

domcleal · 2015-03-19T11:09:55Z

[test]

domcleal · 2015-03-19T11:14:38Z

Unit tests pass now on Jenkins, I wonder why it fails for @witlessbird. However there are some rubocop warnings, see the link on http://ci.theforeman.org/job/test_proxy_develop_pr_rubocop/200/

dmitri-d · 2015-03-19T12:20:41Z

I looked into the failure I'm seeing:

in test_api_recovers_from_nil_provider, https://github.com/logicminds/smart-proxy/blob/bmc_fixes/modules/bmc/bmc_api.rb#L41 returns an instance of Proxy::BMC::Shell
Proxy::BMC::Shell doesn't implement #test method, and throws NotImplementedError
NotImplementedError is not a StandardError, therefore rescue at https://github.com/logicminds/smart-proxy/blob/bmc_fixes/modules/bmc/bmc_api.rb#L43 doesn't catch it.

Not sure why in my case test gets an instance of Proxy::BMC::Shell. More importantly, implementation of either Proxy::BMC::Base or Proxy::BMC::Api has to change, as it is currently possible for hard to troubleshoot failures to occur.

I think either api has to catch Exception, not just StandardError, or interface compliance check in Proxy::BMC::Base has to be done differently.

logicminds · 2015-03-19T18:24:26Z

On Mar 19, 2015, at 5:20 AM, Dmitri Dolguikh notifications@github.com wrote:

I looked into the failure I'm seeing:

• in test_api_recovers_from_nil_provider, https://github.com/logicminds/smart-proxy/blob/bmc_fixes/modules/bmc/bmc_api.rb#L41 returns an instance of Proxy::BMC::Shell

I can reproduce this when I enable :bmc_default_provider: shell in the config/bmc.yml file. So this is probably what you have in your config file.

• Proxy::BMC::Shell doesn't implement #test method, and throws NotImplementedError

Easy fix could catch.

• NotImplementedError is not a StandardError, therefore rescue at https://github.com/logicminds/smart-proxy/blob/bmc_fixes/modules/bmc/bmc_api.rb#L43 doesn't catch it.
I think something else is going on here because the error is still of exception class. NotImplementedError < ScriptError < Exception.

I have been mocking the settings provider value for some tests, but not all tests, so the default would apply if the value is not mocked.

Not sure why in my case test gets an instance of Proxy::BMC::Shell. More importantly, implementation of either Proxy::BMC::Base or Proxy::BMC::Api has to change, as it is currently possible for hard to troubleshoot failures to occur.

You are getting an instance of shell because that is what you have set as your shell provider and because nil was expected in the test but I didn’t mock the settings value it was picking up your settings value.
Adding this as a mock Proxy::BMC::Plugin.settings.stubs(:bmc_default_provider).returns(nil) fixes the test and ensures nothing is picked up from the settings file.

I think either api has to catch Exception, not just StandardError, or interface compliance check in Proxy::BMC::Base has to be done differently.

—
Reply to this email directly or view it on GitHub.

dmitri-d · 2015-03-19T18:28:16Z

@logicminds: rescue => e form will only catch StandardError exceptions. NotImplementedError doesn't have StandardError in its inheritance chain, so none of the exceptions thrown in the ::Proxy::BMC::Base will be caught.

logicminds · 2015-03-19T18:43:40Z

@witlessbird makes sense now. https://robots.thoughtbot.com/rescue-standarderror-not-exception

So than we could assume all other methods that the shell provider does not implement will also suffer from this same issue. Should we file a new ticket for this issue, since its out of scope?

* updates rubyipmi to 0.9.1 * adds new bmc api call /bmc/:host/test that tests the connection * adds test method to shell provider

* adds additional logic around providers * implements /providers, /providers/installed, /host, / * the / returns a list of resources to use to help the user * catches a case where if the provider is nil, we can default to a provider and warn the user * changes the bmc_setup method to be public instead of private. * refactors the providers_installed? rubyipmi command to providers_installed

…n't implement the api call * catches the NotImplementedError properely and returns 501 http code * fixes rubocop warnings

logicminds · 2015-03-19T20:11:44Z

[test]

I just fixed all rubocop warnings and pushed another rubyipmi gem version to fix some style issues. I also opened issue 9840 because it was unrelated but still important for future provider support. I would expect all tests to pass on any system.

dmitri-d · 2015-03-20T11:18:21Z

merged in at e917190, 3fba6e1, ec00558, 0d70f58, 369793b. Thanks, @logicminds!

Fixes #9709 - allow the bmc module to log all impi calls

300b7b2

* updates rubyipmi to use version 0.9.0 or later as its now required

theforeman-bot added Needs testing Not yet reviewed labels Mar 11, 2015

logicminds mentioned this pull request Mar 11, 2015

Fixes #9709 - allow the bmc module to log all impi calls #263

Closed

logicminds force-pushed the bmc_fixes branch from a5f26d9 to 4fca5da Compare March 11, 2015 03:01

logicminds force-pushed the bmc_fixes branch from 4fca5da to 91bfba7 Compare March 11, 2015 03:45

dmitri-d reviewed Mar 11, 2015
View reviewed changes

logicminds force-pushed the bmc_fixes branch 3 times, most recently from 0ef2b14 to ba22482 Compare March 11, 2015 21:02

logicminds force-pushed the bmc_fixes branch from ba22482 to e7b8778 Compare March 11, 2015 21:23

lzap reviewed Mar 13, 2015
View reviewed changes

Fixes #9709 - allow the bmc module to log all impi calls

bef36e5

* previously when IPMI errors occured at the Rubyipmi or CLI level there was no good way to troubleshoot the issue. With this addition we can now watch an erorrs Rubyipmi spits out by sending Rubyipmi a custom logger.

logicminds force-pushed the bmc_fixes branch 2 times, most recently from c81a738 to 876d174 Compare March 13, 2015 15:55

logicminds force-pushed the bmc_fixes branch from 876d174 to e56620d Compare March 17, 2015 22:57

domcleal pushed a commit to theforeman/foreman-infra that referenced this pull request Mar 19, 2015

Install freeipmi for theforeman/smart-proxy#264

427b849

logicminds added 3 commits March 19, 2015 12:15

Fixes #9710 - add rubyipmi test_connection api call

9579ebc

* updates rubyipmi to 0.9.1 * adds new bmc api call /bmc/:host/test that tests the connection * adds test method to shell provider

Fixes #9840 - BMC providers should return 501 error code when they do…

92a6617

…n't implement the api call * catches the NotImplementedError properely and returns 501 http code * fixes rubocop warnings

logicminds force-pushed the bmc_fixes branch from e56620d to 92a6617 Compare March 19, 2015 19:56

dmitri-d closed this Mar 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bmc fixes #264

Bmc fixes #264

logicminds commented Mar 11, 2015

logicminds commented Mar 11, 2015

logicminds commented Mar 11, 2015

logicminds commented Mar 11, 2015

dmitri-d Mar 11, 2015

logicminds Mar 11, 2015

logicminds commented Mar 11, 2015

dmitri-d commented Mar 12, 2015

lzap Mar 13, 2015

dmitri-d Mar 13, 2015

lzap commented Mar 13, 2015

logicminds commented Mar 13, 2015

logicminds commented Mar 13, 2015

logicminds commented Mar 17, 2015

logicminds commented Mar 18, 2015

logicminds commented Mar 18, 2015

domcleal commented Mar 19, 2015

dmitri-d commented Mar 19, 2015

domcleal commented Mar 19, 2015

domcleal commented Mar 19, 2015

dmitri-d commented Mar 19, 2015

logicminds commented Mar 19, 2015

dmitri-d commented Mar 19, 2015

logicminds commented Mar 19, 2015

logicminds commented Mar 19, 2015

dmitri-d commented Mar 20, 2015

Bmc fixes #264

Bmc fixes #264

Conversation

logicminds commented Mar 11, 2015

logicminds commented Mar 11, 2015

logicminds commented Mar 11, 2015

logicminds commented Mar 11, 2015

dmitri-d Mar 11, 2015

Choose a reason for hiding this comment

logicminds Mar 11, 2015

Choose a reason for hiding this comment

logicminds commented Mar 11, 2015

dmitri-d commented Mar 12, 2015

lzap Mar 13, 2015

Choose a reason for hiding this comment

dmitri-d Mar 13, 2015

Choose a reason for hiding this comment

lzap commented Mar 13, 2015

logicminds commented Mar 13, 2015

logicminds commented Mar 13, 2015

logicminds commented Mar 17, 2015

logicminds commented Mar 18, 2015

logicminds commented Mar 18, 2015

domcleal commented Mar 19, 2015

dmitri-d commented Mar 19, 2015

domcleal commented Mar 19, 2015

domcleal commented Mar 19, 2015

dmitri-d commented Mar 19, 2015

logicminds commented Mar 19, 2015

dmitri-d commented Mar 19, 2015

logicminds commented Mar 19, 2015

logicminds commented Mar 19, 2015

dmitri-d commented Mar 20, 2015