-
Notifications
You must be signed in to change notification settings - Fork 896
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
Physical server provisioning with internal state machine #18573
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,28 @@ | ||
class PhysicalServerProvisionRequest < MiqProvisionConfiguredSystemRequest | ||
class PhysicalServerProvisionRequest < MiqRequest | ||
TASK_DESCRIPTION = 'Physical Server Provisioning'.freeze | ||
SOURCE_CLASS_NAME = 'PhysicalServer'.freeze | ||
|
||
def description | ||
"Apply configuration pattern" | ||
'Physical Server Provisioning' | ||
end | ||
|
||
def src_configured_systems | ||
PhysicalServer.where(:id => options[:src_configured_system_ids]) | ||
def my_role(_action = nil) | ||
'ems_operations' | ||
end | ||
|
||
def self.request_task_class_from(_attribs) | ||
ManageIQ::Providers::Lenovo::PhysicalInfraManager::ProvisionTask | ||
def self.request_task_class | ||
PhysicalServerProvisionTask | ||
end | ||
|
||
def event_name(mode) | ||
"physical_server_provision_request_#{mode}" | ||
def self.new_request_task(attribs) | ||
source = source_physical_server(attribs[:source_id]) | ||
source.ext_management_system.class.provision_class(nil).new(attribs) | ||
end | ||
|
||
def originating_controller | ||
"physical_server" | ||
def self.source_physical_server(source_id) | ||
PhysicalServer.find_by(:id => source_id).tap do |source| | ||
raise MiqException::MiqProvisionError, "Unable to find source PhysicalServer with id [#{source_id}]" if source.nil? | ||
raise MiqException::MiqProvisionError, "Source PhysicalServer with id [#{source_id}] has no EMS, unable to provision" if source.ext_management_system.nil? | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
class PhysicalServerProvisionTask < MiqProvisionTask | ||
include_concern 'StateMachine' | ||
|
||
def description | ||
'Provision Physical Server' | ||
end | ||
|
||
def self.base_model | ||
PhysicalServerProvisionTask | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the change @lfu @gmcculloug that introduces standalone task name instead shared one (was |
||
end | ||
|
||
def self.request_class | ||
PhysicalServerProvisionRequest | ||
end | ||
|
||
def model_class | ||
PhysicalServer | ||
end | ||
|
||
def deliver_to_automate | ||
super('physical_server_provision', my_zone) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
module PhysicalServerProvisionTask::StateMachine | ||
def run_provision | ||
raise MiqException::MiqProvisionError, "Unable to find #{model_class} with id #{source_id.inspect}" if source.blank? | ||
dump_obj(options, "MIQ(#{self.class.name}##{__method__}) options: ", $log, :info) | ||
signal :start_provisioning | ||
end | ||
|
||
def start_provisioning | ||
update_and_notify_parent(:message => msg('start provisioning')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @miha-plesko why go through the trouble of updating the parent message if you're just going to raise immediately after? The way this is now you can't even do I'm going to merge as is because this case shouldn't ever happen There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @agrare my thinking was to improve logs for unimplemented provider (e.g. Lenovo): first log says "start provisioning" then it explodes "NotImplemented" which kind of reads nicely as "start provisioning is not implemented". However, I see your point - the piece of code is unusable as soon as one performs override from the provider. Followup here #18706, thanks for merging. |
||
# Implement provisioning in subclass, user-defined values are stored in options field. | ||
raise NotImplementedError, 'Must be implemented in subclass and signal :done_provisioning when done' | ||
end | ||
|
||
def done_provisioning | ||
update_and_notify_parent(:message => msg('done provisioning')) | ||
signal :mark_as_completed | ||
end | ||
|
||
def mark_as_completed | ||
update_and_notify_parent(:state => 'provisioned', :message => msg('provisioning completed')) | ||
MiqEvent.raise_evm_event(source, 'generic_task_finish', :message => "Done provisioning PhysicalServer") | ||
signal :finish | ||
end | ||
|
||
def finish | ||
if status != 'Error' | ||
_log.info("Executing provision task: [#{description}]... Complete") | ||
else | ||
_log.info("Executing provision task: [#{description}]... Errored") | ||
end | ||
end | ||
|
||
def msg(txt) | ||
"Provisioning PhysicalServer id=#{source.id}, name=#{source.name}, ems_ref=#{source.ems_ref}: #{txt}" | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
describe PhysicalServerProvisionRequest do | ||
it '.TASK_DESCRIPTION' do | ||
expect(described_class::TASK_DESCRIPTION).to eq('Physical Server Provisioning') | ||
end | ||
|
||
it '.SOURCE_CLASS_NAME' do | ||
expect(described_class::SOURCE_CLASS_NAME).to eq('PhysicalServer') | ||
end | ||
|
||
it '.request_task_class' do | ||
expect(described_class.request_task_class).to eq(PhysicalServerProvisionTask) | ||
end | ||
|
||
it '#description' do | ||
expect(subject.description).to eq('Physical Server Provisioning') | ||
end | ||
|
||
it '#my_role' do | ||
expect(subject.my_role).to eq('ems_operations') | ||
end | ||
|
||
describe '.new_request_task' do | ||
before do | ||
allow(ems.class).to receive(:provision_class).and_return(task) | ||
end | ||
|
||
let(:server) { FactoryBot.create(:physical_server, :ext_management_system => ems) } | ||
let(:ems) { FactoryBot.create(:ems_physical_infra) } | ||
let(:task) { double('TASK') } | ||
|
||
context 'when source is ok' do | ||
it do | ||
expect(task).to receive(:new).with(:source_id => server.id) | ||
described_class.new_request_task(:source_id => server.id) | ||
end | ||
end | ||
|
||
context 'when source is missing' do | ||
it do | ||
expect { described_class.new_request_task(:source_id => 'missing') }.to raise_error(MiqException::MiqProvisionError) | ||
end | ||
end | ||
|
||
context 'when source is lacking EMS' do | ||
before { server.update!(:ext_management_system => nil) } | ||
it do | ||
expect { described_class.new_request_task(:source_id => server.id) }.to raise_error(MiqException::MiqProvisionError) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
describe PhysicalServerProvisionTask do | ||
let(:server) { FactoryBot.create(:physical_server) } | ||
|
||
subject { described_class.new(:source => server) } | ||
|
||
describe '#run_provision' do | ||
context 'when missing source' do | ||
let(:server) { nil } | ||
it do | ||
expect { subject.run_provision }.to raise_error(MiqException::MiqProvisionError) | ||
end | ||
end | ||
|
||
context 'when ok' do | ||
it do | ||
expect(subject).to receive(:signal).with(:start_provisioning) | ||
subject.run_provision | ||
end | ||
end | ||
end | ||
|
||
it '#done_provisioning' do | ||
expect(subject).to receive(:signal).with(:mark_as_completed) | ||
expect(subject).to receive(:update_and_notify_parent) | ||
subject.done_provisioning | ||
end | ||
|
||
it '#mark_as_completed' do | ||
expect(subject).to receive(:signal).with(:finish) | ||
expect(subject).to receive(:update_and_notify_parent) | ||
subject.mark_as_completed | ||
end | ||
|
||
describe '#finish' do | ||
before { allow(subject).to receive(:_log).and_return(log) } | ||
|
||
let(:log) { double('LOG') } | ||
|
||
context 'when task has errored' do | ||
before { subject.update_attribute(:status, 'Error') } | ||
it do | ||
expect(log).to receive(:info).with(satisfy { |msg| msg.include?('Errored') }) | ||
subject.finish | ||
end | ||
end | ||
|
||
context 'when task has completed' do | ||
before { subject.update_attribute(:status, 'Ok') } | ||
it do | ||
expect(log).to receive(:info).with(satisfy { |msg| msg.include?('... Complete') }) | ||
subject.finish | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
describe PhysicalServerProvisionTask do | ||
it '#description' do | ||
expect(subject.description).to eq('Provision Physical Server') | ||
end | ||
|
||
it '#model_class' do | ||
expect(subject.model_class).to eq(PhysicalServer) | ||
end | ||
|
||
it '.request_class' do | ||
expect(described_class.request_class).to eq(PhysicalServerProvisionRequest) | ||
end | ||
|
||
describe '#deliver_to_automate' do | ||
before do | ||
allow(subject).to receive(:approved?).and_return(true) | ||
allow(subject).to receive(:get_user).and_return(double('USER').as_null_object) | ||
allow(subject).to receive(:my_zone).and_return(double('ZONE').as_null_object) | ||
end | ||
|
||
let(:request) { FactoryBot.create(:physical_server_provision_request) } | ||
|
||
subject { described_class.new(:miq_request => request) } | ||
|
||
it do | ||
expect(MiqQueue).to receive(:put).with(satisfy { |args| args[:class_name] == 'MiqAeEngine' }) | ||
subject.deliver_to_automate | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this method need to be overridden in sub-classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so as discussed on a call a week ago we're now allowing Task override from within provider repo (see L19 below). Overriden Task should inherit from this
PhysicalServerProvisionTask
.Here in Request, however, we cannot allow override because there is no way we can figure what provider does the Request belong to. But this is not a problem, because
self.request_task_class
is only used by Request to render Task description prefix, so IMO it's safe to leave it like this.