Fixes #2595 - Add more Rubyipmi methods to BMC API #521

Deltik · 2017-04-20T18:09:56Z

Replaces: #505
Replaces: 051b285

This commit implements the same features as
051b285, with some differences:

Added unit tests for all new features
Added interfaces to modules/bmc/base.rb
Cleared up some ambiguous method names
Changed some unit test stubs to expects
Fixed bugs in the original implementation

New API methods:

GET /bmc/:host/fru
Shows available actions: "list", "serial", "manufacturer", "model",
and "asset_tag"
GET /bmc/:host/bmc
Shows available actions: "info", "guid", and "version"
GET /bmc/:host/sensors
Shows available actions: "list", "count", "names", "fanlist",
"templist", and "get"
GET /bmc/:host/lan/snmp
Shows BMC host's SNMP community string
GET /bmc/:host/lan/vlanid
Shows BMC host's VLAN ID
GET /bmc/:host/lan/ipsrc
Shows whether BMC host's LAN was configured "static" or "dhcp"
GET /bmc/:host/lan/print
Shows BMC hosts's LAN configuration
PUT /bmc/:host/chassis/power/reset
Performs power reset on chassis
GET /bmc/:host/fru/list
Shows all of host's FRU information. Includes workaround for
undocumented IBM/Lenovo IPMI bug.
GET /bmc/:host/fru/serial
Shows host's serial number
GET /bmc/:host/fru/manufacturer
Shows host's manufacturer
GET /bmc/:host/fru/model
Shows host's product model
GET /bmc/:host/fru/asset_tag
Shows host's asset tag string, if Rubyipmi has this method. It's in
the spec, but doesn't seem to be implemented today. See spec here.
GET /bmc/:host/bmc/info
Shows information about the BMC
GET /bmc/:host/bmc/guid
Shows system's GUID
GET /bmc/:host/version
Shows BMC's version
PUT /bmc/:host/bmc/reset?type=warm
Performs warm reset on BMC
PUT /bmc/:host/bmc/reset?type=cold
Performs cold reset on BMC
GET /bmc/:host/sensors/list
Shows all of host's sensors
GET /bmc/:host/sensors/count
Shows number of sensors on host
GET /bmc/:host/sensors/names
Shows list of sensor names on host
GET /bmc/:host/sensors/fanlist
Shows all of host's fan sensors
GET /bmc/:host/sensors/templist
Shows all of host's temperature sensors
GET /bmc/:host/sensors/get
Shows available option "sensor" for this action
GET /bmc/:host/sensors/get/:sensor
Shows information about sensor :sensor on host

dmitri-d · 2017-04-21T11:19:37Z

[test]

dmitri-d · 2017-05-04T10:01:50Z

modules/bmc/ipmi.rb

+      def frulist
+        frulist = host.fru.list
+        # Detect quirk on ipmitool
+        raise 'Quirk' if frulist.empty?


I find this hard to read/follow. Can 'quirk' happen with ipmitool only? If so, why not return the error message on line 218 right away?

This has been fixed in 7fbe331.

dmitri-d · 2017-05-04T10:03:25Z

modules/bmc/ipmi.rb

+          # returns "invalid byte sequence in UTF-8". The second time does a
+          # successful fru list.
+          return host.fru.list if host.is_a? Rubyipmi::Freeipmi::Connection
+        rescue


The call above can throw RuntimeErrors only?

I'm catching all exceptions and feeding their messages through to the client.

dmitri-d · 2017-05-04T10:04:54Z

modules/bmc/ipmi.rb

+        raise 'Quirk' if frulist.empty?
+        frulist
+      rescue Exception => e
+        msg = "Unknown error getting fru list."


Is this the case? Wouldn't exception contain the details of the error that caused it?

This has been fixed in 7fbe331. Exceptions are now returned as

Error getting fru list: Exception#message

dmitri-d · 2017-05-04T10:06:29Z

modules/bmc/ipmi.rb

+        end
+        # Handle ipmitool
+        return "#{msg} Try bmc_provider=freeipmi for possible quirk workaround." if e.message == 'Quirk'
+        return msg


This is also hard to follow. I'd suggest returning an error message at the top of the rescue block.

This has been fixed in 7fbe331.

Replaces: #505 Replaces: 051b285 This commit implements the same features as 051b285, with some differences: - Added unit tests for all new features - Added interfaces to `modules/bmc/base.rb` - Cleared up some ambiguous method names - Changed some unit test stubs to expects - Fixed bugs in the original implementation New API methods: - GET /bmc/:host/fru Shows available actions: "list", "serial", "manufacturer", "model", and "asset_tag" - GET /bmc/:host/bmc Shows available actions: "info", "guid", and "version" - GET /bmc/:host/sensors Shows available actions: "list", "count", "names", "fanlist", "templist", and "get" - GET /bmc/:host/lan/snmp Shows BMC host's SNMP community string - GET /bmc/:host/lan/vlanid Shows BMC host's VLAN ID - GET /bmc/:host/lan/ipsrc Shows whether BMC host's LAN was configured "static" or "dhcp" - GET /bmc/:host/lan/print Shows BMC hosts's LAN configuration - PUT /bmc/:host/chassis/power/reset Performs power reset on chassis - GET /bmc/:host/fru/list Shows all of host's FRU information. Includes workaround for undocumented IBM/Lenovo IPMI bug. - GET /bmc/:host/fru/serial Shows host's serial number - GET /bmc/:host/fru/manufacturer Shows host's manufacturer - GET /bmc/:host/fru/model Shows host's product model - GET /bmc/:host/fru/asset_tag Shows host's asset tag string, if Rubyipmi has this method. It's in the spec, but doesn't seem to be implemented today. See logicminds/rubyipmi/blob/master/spec/unit/ipmitool/fru_spec.rb - GET /bmc/:host/bmc/info Shows information about the BMC - GET /bmc/:host/bmc/guid Shows system's GUID - GET /bmc/:host/version Shows BMC's version - PUT /bmc/:host/bmc/reset?type=warm Performs warm reset on BMC - PUT /bmc/:host/bmc/reset?type=cold Performs cold reset on BMC - GET /bmc/:host/sensors/list Shows all of host's sensors - GET /bmc/:host/sensors/count Shows number of sensors on host - GET /bmc/:host/sensors/names Shows list of sensor names on host - GET /bmc/:host/sensors/fanlist Shows all of host's fan sensors - GET /bmc/:host/sensors/templist Shows all of host's temperature sensors - GET /bmc/:host/sensors/get Shows available option "sensor" for this action - GET /bmc/:host/sensors/get/:sensor Shows information about sensor :sensor on host

dmitri-d · 2017-05-05T13:35:22Z

[test]

dmitri-d · 2017-05-05T13:45:52Z

Looks good, waiting for the results of the build.

dmitri-d · 2017-05-05T13:52:06Z

Merged, thanks @Deltik.

dmitri-d · 2017-05-05T14:01:23Z

@Deltik: did you have anything in mind re: changes to Foreman UI?

Deltik · 2017-05-05T15:09:53Z

@witlessbird: Foreman could use this additional functionality as part of

Foreman » Hosts » All hosts » (any host with a BMC) » BMC

Currently, when showing any host with a BMC, Foreman makes these requests in the background to the associated Smart Proxy:

GET /bmc/BMC_IP_ADDRESS/chassis/power/status HTTP/1.1
GET /bmc/BMC_IP_ADDRESS/lan/ip HTTP/1.1
GET /bmc/BMC_IP_ADDRESS/lan/netmask HTTP/1.1
GET /bmc/BMC_IP_ADDRESS/lan/mac HTTP/1.1
GET /bmc/BMC_IP_ADDRESS/lan/gateway HTTP/1.1

, where BMC_IP_ADDRESS is the IP address of the BMC.

I'm thinking there could be buttons added to fetch additional information from BMC that take longer than a few seconds, such as GET /bmc/BMC_IP_ADDRESS/fru/list, which takes 15 seconds for me using ipmitool and 2 seconds for me using freeipmi.

There could also be a button to fetch sensor information. And a button to perform a BMC reset.

I haven't thought this through much, but Foreman can use these new API methods for additional BMC functionality.

We can also fix an issue with this:

Currently, "Cycle" means the same thing as "Reset" because they both call PUT /bmc/BMC_IP_ADDRESS/chassis/power/cycle, but now, "Reset" can be changed to mean /bmc/BMC_IP_ADDRESS/chassis/power/reset.

theforeman-bot added Needs testing Not yet reviewed labels Apr 20, 2017

dmitri-d mentioned this pull request Apr 21, 2017

Fixes #2595: Expose more existing rubyipmi features as BMC smart-prox… #505

Closed

dmitri-d mentioned this pull request May 4, 2017

Fixes #19205 - Add IPMI IP scanning to BMC API #519

Closed

dmitri-d suggested changes May 4, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels May 4, 2017

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels May 4, 2017

dmitri-d approved these changes May 5, 2017

View reviewed changes

dmitri-d removed the Needs re-review label May 5, 2017

dmitri-d merged commit a9332b6 into theforeman:develop May 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2595 - Add more Rubyipmi methods to BMC API #521

Fixes #2595 - Add more Rubyipmi methods to BMC API #521

Deltik commented Apr 20, 2017

dmitri-d commented Apr 21, 2017

dmitri-d May 4, 2017

Deltik May 4, 2017

dmitri-d May 4, 2017

Deltik May 4, 2017

dmitri-d May 4, 2017

Deltik May 4, 2017

dmitri-d May 4, 2017

Deltik May 4, 2017

dmitri-d commented May 5, 2017

dmitri-d commented May 5, 2017

dmitri-d commented May 5, 2017

dmitri-d commented May 5, 2017

Deltik commented May 5, 2017

Fixes #2595 - Add more Rubyipmi methods to BMC API #521

Fixes #2595 - Add more Rubyipmi methods to BMC API #521

Conversation

Deltik commented Apr 20, 2017

dmitri-d commented Apr 21, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented May 5, 2017

dmitri-d commented May 5, 2017

dmitri-d commented May 5, 2017

dmitri-d commented May 5, 2017

Deltik commented May 5, 2017