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

Add power operations to physical server #5

Merged
merged 1 commit into from
Jun 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class PhysicalInfraManager < ManageIQ::Providers::PhysicalInfraManager

include Vmdb::Logging
include ManagerMixin
include_concern "Operations"

has_many :physical_server_details,
:class_name => "AssetDetail",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module ManageIQ::Providers::Redfish
module PhysicalInfraManager::Operations
extend ActiveSupport::Concern

include_concern "Power"
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
module ManageIQ::Providers::Redfish
module PhysicalInfraManager::Operations::Power
# Keep this in sync with app/models/physical_server/operations/power.rb in
# core and ResetType enum in Redfish Resource type. Name of the method
# comes from the core and the action name used in the reset call from the
# ResetType enum.
#
# NOTE: Not all reset operations are implemented on all servers, so any of
# the methods listed here can fail. We need to find a way to let those
# failures bubble up to the user interface somehow or risk having a
# completely useless tool.

def power_on(server, _options)
reset_server(server, "On")
Copy link
Member

Choose a reason for hiding this comment

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

I was going to say reset_server didn't make sense for the function name, but then I saw the API call was ComputerSystem.Reset so 👍

end

def power_off(server, _options)
reset_server(server, "GracefulShutdown")
end

def power_off_now(server, _options)
reset_server(server, "ForceOff")
end

def restart(server, _options)
reset_server(server, "GracefulRestart")
end

def restart_now(server, _options)
reset_server(server, "ForceRestart")
end

def restart_to_sys_setup(_args, _options)
$redfish_log.error("Restarting to system setup is not supported.")
Copy link
Member

Choose a reason for hiding this comment

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

Hm we should have a supports feature for this so the UI/API can check if the method is supported before calling it, something to do in a followup as I'm not sure any of that exists for physical server power ops.

end

def restart_mgmt_controller(_server, _options)
# TODO(tadeboro): This operation is not well defined, since server can
# (and usually is) managed by more that one manager.
$redfish_log.error("Restarting management controller is not supported.")
Copy link
Member

Choose a reason for hiding this comment

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

Same here

end

private

def reset_server(server, reset_type)
$redfish_log.info("Requesting #{reset_type} for #{server.ems_ref}.")
with_provider_connection do |client|
system = client.find(server.ems_ref)
if system.nil?
$redfish_log.error("#{server.ems_ref} does not exist anymore.")
return
end

response = system.Actions["#ComputerSystem.Reset"].post(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move such const strings into more resilient form? At least a predefined const that will define this string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel that extracting a string just for the sake of extraction is useless and in this particular case harmful to the readability and comprehension of the code. At the moment, this is the only place that uses this operation name. If we need to restart server from more places than one in the future, we will extract the whole "restart the server" functionality into separate function and not replicate the post call invocation all over the place, still keeping this operation name confined to a single place.

:field => "target", :payload => { "ResetType" => reset_type }
)
if [200, 202, 204].include?(response.status)
$redfish_log.info("#{reset_type} for #{server.ems_ref} started.")
Copy link
Member

Choose a reason for hiding this comment

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

Is there a task id/ref returned that can be used to wait for the operation to complete? If so we might want to optionally wait for that to finish since most of our operations are expected to be synchronous.

else
$redfish_log.error("#{reset_type} for #{server.ems_ref} failed.")
Copy link
Member

Choose a reason for hiding this comment

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

When the API call fails is there any error message returned that we can print to help diagnose the failure?

end
end
end
end
end
3 changes: 3 additions & 0 deletions spec/factories/ext_management_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
end

trait :vcr do
security_protocol "ssl"
port 8889

hostname do
# Keep in sync with filter_sensitive_data in spec/spec_helper.rb!
Rails.application.secrets.redfish.try(:[], "host") || "redfishhost"
Expand Down
9 changes: 9 additions & 0 deletions spec/factories/physical_server.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
FactoryGirl.define do
factory :redfish_physical_server,
:class => ManageIQ::Providers::Redfish::PhysicalInfraManager::PhysicalServer,
:parent => :physical_server do
trait :vcr do
ems_ref "/redfish/v1/Systems/System.Embedded.1"
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
describe ManageIQ::Providers::Redfish::PhysicalInfraManager do
let(:server) { FactoryGirl.create(:redfish_physical_server, :vcr) }
subject(:ems) do
FactoryGirl.create(:ems_redfish_physical_infra, :vcr)
end

describe "#power_on", :vcr do
it "powers on the system" do
ems.power_on(server, nil)
end
end

describe "#power_off", :vcr do
it "powers off the system" do
ems.power_off(server, nil)
end
end

describe "#power_off_now", :vcr do
it "powers off the system immediately" do
ems.power_off_now(server, nil)
end
end

describe "#restart", :vcr do
it "restarts the system" do
ems.restart(server, nil)
end
end

describe "#restart_now", :vcr do
it "restarts the system immediately" do
ems.restart_now(server, nil)
end
end

describe "#restart_to_sys_setup", :vcr do
it "restarts to system setup" do
ems.restart_to_sys_setup(server, nil)
end
end

describe "#restart_mgmt_controller", :vcr do
it "restarts management controller" do
ems.restart_mgmt_controller(server, nil)
end
end
end

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading