From 5bd59ad795ee3043c514469c71116557cd3a4b90 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Ple=C5=A1ko?= <miha.plesko@xlab.si>
Date: Thu, 23 May 2019 15:46:47 +0200
Subject: [PATCH] Introduce Request and Task for firmware update
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We're introducing a new operation on physical server: firmware update.
We want to use state machine-driven Request for it because it's a long
running procedure and we want to be doing it on more than one server
with a single click (and then track its progress).

That being said - the PR contains a new Request class and a new
Task class (with internal state machine). The actual state machine
implementation is done in provider codebase - depending on class of
the selected physical server.

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
---
 .../providers/physical_infra_manager.rb       |  4 +
 app/models/miq_request.rb                     |  3 +
 ...physical_server_firmware_update_request.rb | 50 ++++++++++
 .../physical_server_firmware_update_task.rb   | 30 ++++++
 .../state_machine.rb                          | 35 +++++++
 spec/factories/miq_request.rb                 |  3 +-
 spec/factories/miq_request_task.rb            |  3 +-
 spec/models/miq_request_spec.rb               | 29 +++---
 ...cal_server_firmware_update_request_spec.rb | 93 +++++++++++++++++++
 .../state_machine_spec.rb                     | 50 ++++++++++
 ...ysical_server_firmware_update_task_spec.rb | 18 ++++
 11 files changed, 302 insertions(+), 16 deletions(-)
 create mode 100644 app/models/physical_server_firmware_update_request.rb
 create mode 100644 app/models/physical_server_firmware_update_task.rb
 create mode 100644 app/models/physical_server_firmware_update_task/state_machine.rb
 create mode 100644 spec/models/physical_server_firmware_update_request_spec.rb
 create mode 100644 spec/models/physical_server_firmware_update_task/state_machine_spec.rb
 create mode 100644 spec/models/physical_server_firmware_update_task_spec.rb

diff --git a/app/models/manageiq/providers/physical_infra_manager.rb b/app/models/manageiq/providers/physical_infra_manager.rb
index 726ee2daccba..298c34c68779 100644
--- a/app/models/manageiq/providers/physical_infra_manager.rb
+++ b/app/models/manageiq/providers/physical_infra_manager.rb
@@ -110,6 +110,10 @@ def console_url
       raise MiqException::Error, _("Console not supported")
     end
 
+    def self.firmware_update_class
+      self::FirmwareUpdate
+    end
+
     def self.display_name(number = 1)
       n_('Physical Infrastructure Manager', 'Physical Infrastructure Managers', number)
     end
diff --git a/app/models/miq_request.rb b/app/models/miq_request.rb
index af56a64c7d4d..64a0775dffd0 100644
--- a/app/models/miq_request.rb
+++ b/app/models/miq_request.rb
@@ -89,6 +89,9 @@ class MiqRequest < ApplicationRecord
       :PhysicalServerProvisionRequest      => {
         :provision_physical_server => N_("Physical Server Provision")
       },
+      :PhysicalServerFirmwareUpdateRequest => {
+        :physical_server_firmware_update => N_("Physical Server Firmware Update")
+      },
       :ServiceRetireRequest                => {
         :service_retire => N_("Service Retire")
       },
diff --git a/app/models/physical_server_firmware_update_request.rb b/app/models/physical_server_firmware_update_request.rb
new file mode 100644
index 000000000000..ddda763e1e15
--- /dev/null
+++ b/app/models/physical_server_firmware_update_request.rb
@@ -0,0 +1,50 @@
+class PhysicalServerFirmwareUpdateRequest < MiqRequest
+  TASK_DESCRIPTION  = 'Physical Server Firmware Update'.freeze
+  SOURCE_CLASS_NAME = 'PhysicalServer'.freeze
+
+  def description
+    'Physical Server Firmware Update'
+  end
+
+  def my_role(_action = nil)
+    'ems_operations'
+  end
+
+  def self.request_task_class
+    PhysicalServerFirmwareUpdateTask
+  end
+
+  def self.new_request_task(attribs)
+    affected_ems(attribs).class.firmware_update_class.new(attribs)
+  end
+
+  def requested_task_idx
+    [-1] # we are only using one task per request not matter how many servers are affected
+  end
+
+  def self.affected_physical_servers(attribs)
+    ids = attribs.dig('options', :src_ids)
+    raise MiqException::MiqFirmwareUpdateError, 'At least one PhysicalServer is required' if ids&.empty?
+
+    PhysicalServer.where(:id => ids).tap do |servers|
+      unless servers.size == ids.size
+        raise MiqException::MiqFirmwareUpdateError, 'At least one PhysicalServer is missing'
+      end
+      unless servers.map(&:ems_id).uniq.size == 1
+        raise MiqException::MiqFirmwareUpdateError, 'All PhysicalServers need to belong to same EMS'
+      end
+    end
+  end
+
+  def self.affected_ems(attribs)
+    affected_physical_servers(attribs).first.ext_management_system
+  end
+
+  def affected_physical_servers
+    @affected_physical_servers ||= self.class.affected_physical_servers(attributes)
+  end
+
+  def affected_ems
+    @affected_ems ||= self.class.affected_ems(attributes)
+  end
+end
diff --git a/app/models/physical_server_firmware_update_task.rb b/app/models/physical_server_firmware_update_task.rb
new file mode 100644
index 000000000000..9f434334b9aa
--- /dev/null
+++ b/app/models/physical_server_firmware_update_task.rb
@@ -0,0 +1,30 @@
+class PhysicalServerFirmwareUpdateTask < MiqRequestTask
+  include_concern 'StateMachine'
+
+  validates :state, :inclusion => {
+    :in      => %w[pending queued active firmware_updated finished],
+    :message => 'should be pending, queued, active, firmware_updated or finished'
+  }
+
+  AUTOMATE_DRIVES = false
+
+  def description
+    'Physical Server Firmware Update'
+  end
+
+  def self.base_model
+    PhysicalServerFirmwareUpdateTask
+  end
+
+  def do_request
+    signal :run_firmware_update
+  end
+
+  def self.request_class
+    PhysicalServerFirmwareUpdateRequest
+  end
+
+  def self.display_name(number = 1)
+    n_('Firmware Update Task', 'Firmware Update Tasks', number)
+  end
+end
diff --git a/app/models/physical_server_firmware_update_task/state_machine.rb b/app/models/physical_server_firmware_update_task/state_machine.rb
new file mode 100644
index 000000000000..8c86d5b2b761
--- /dev/null
+++ b/app/models/physical_server_firmware_update_task/state_machine.rb
@@ -0,0 +1,35 @@
+module PhysicalServerFirmwareUpdateTask::StateMachine
+  def run_firmware_update
+    dump_obj(options, "MIQ(#{self.class.name}##{__method__}) options: ", $log, :info)
+    signal :start_firmware_update
+  end
+
+  def start_firmware_update
+    # Implement firmware update in subclass, user-defined values are stored in options field.
+    # Affected servers can be accessed via miq_request.affected_physical_servers
+    raise NotImplementedError, 'Must be implemented in subclass and signal :done_firmware_update when done'
+  end
+
+  def done_firmware_update
+    update_and_notify_parent(:message => msg('done updating firmware'))
+    signal :mark_as_completed
+  end
+
+  def mark_as_completed
+    update_and_notify_parent(:state => 'firmware_updated', :message => msg('firmware update completed'))
+    MiqEvent.raise_evm_event(self, 'generic_task_finish', :message => "Done updating firmware on PhysicalServer(s)")
+    signal :finish
+  end
+
+  def finish
+    if status != 'Error'
+      _log.info("Executing firmware update task: [#{description}]... Complete")
+    else
+      _log.info("Executing firmware update task: [#{description}]... Errored")
+    end
+  end
+
+  def msg(txt)
+    "Updating firmware for PhysicalServer(s): #{txt}"
+  end
+end
diff --git a/spec/factories/miq_request.rb b/spec/factories/miq_request.rb
index c088d55282da..0d8f6a9db299 100644
--- a/spec/factories/miq_request.rb
+++ b/spec/factories/miq_request.rb
@@ -16,7 +16,8 @@
     factory :miq_provision_request,              :class => "MiqProvisionRequest" do
       source { create(:miq_template) }
     end
-    factory :physical_server_provision_request,  :class => "PhysicalServerProvisionRequest"
+    factory :physical_server_provision_request,        :class => "PhysicalServerProvisionRequest"
+    factory :physical_server_firmware_update_request,  :class => "PhysicalServerFirmwareUpdateRequest"
 
     factory :service_template_transformation_plan_request, :class => "ServiceTemplateTransformationPlanRequest" do
       source { create(:service_template_transformation_plan) }
diff --git a/spec/factories/miq_request_task.rb b/spec/factories/miq_request_task.rb
index 0852fa892604..80dfc5d56a3c 100644
--- a/spec/factories/miq_request_task.rb
+++ b/spec/factories/miq_request_task.rb
@@ -30,7 +30,8 @@
   factory :miq_provision_openstack, :parent => :miq_provision_cloud, :class => "ManageIQ::Providers::Openstack::CloudManager::Provision"
 
   # Physical Infrastructure
-  factory :physical_server_provision_task, :parent => :miq_provision, :class => "PhysicalServerProvisionTask"
+  factory :physical_server_provision_task,       :parent => :miq_provision, :class => "PhysicalServerProvisionTask"
+  factory :physical_server_firmware_update_task, :parent => :miq_provision, :class => "PhysicalServerFirmwareUpdateTask"
 
   # Automate
   factory :automation_task, :parent => :miq_request_task, :class => "AutomationTask"
diff --git a/spec/models/miq_request_spec.rb b/spec/models/miq_request_spec.rb
index c88581564ca6..90f9c7224547 100644
--- a/spec/models/miq_request_spec.rb
+++ b/spec/models/miq_request_spec.rb
@@ -5,20 +5,21 @@
   context "CONSTANTS" do
     it "REQUEST_TYPES" do
       expected_request_types = {
-        :MiqProvisionRequest                      => {:template                   => "VM Provision", :clone_to_vm => "VM Clone", :clone_to_template => "VM Publish"},
-        :MiqProvisionRequestTemplate              => {:template                   => "VM Provision Template"},
-        :MiqProvisionConfiguredSystemRequest      => {:provision_via_foreman      => "#{ui_lookup(:ui_title => 'foreman')} Provision"},
-        :VmReconfigureRequest                     => {:vm_reconfigure             => "VM Reconfigure"},
-        :VmCloudReconfigureRequest                => {:vm_cloud_reconfigure       => "VM Cloud Reconfigure"},
-        :VmMigrateRequest                         => {:vm_migrate                 => "VM Migrate"},
-        :VmRetireRequest                          => {:vm_retire                  => "VM Retire"},
-        :ServiceRetireRequest                     => {:service_retire             => "Service Retire"},
-        :OrchestrationStackRetireRequest          => {:orchestration_stack_retire => "Orchestration Stack Retire"},
-        :AutomationRequest                        => {:automation                 => "Automation"},
-        :ServiceTemplateProvisionRequest          => {:clone_to_service           => "Service Provision"},
-        :ServiceReconfigureRequest                => {:service_reconfigure        => "Service Reconfigure"},
-        :PhysicalServerProvisionRequest           => {:provision_physical_server  => "Physical Server Provision"},
-        :ServiceTemplateTransformationPlanRequest => {:transformation_plan        => "Transformation Plan"}
+        :MiqProvisionRequest                      => {:template                        => "VM Provision", :clone_to_vm => "VM Clone", :clone_to_template => "VM Publish"},
+        :MiqProvisionRequestTemplate              => {:template                        => "VM Provision Template"},
+        :MiqProvisionConfiguredSystemRequest      => {:provision_via_foreman           => "#{ui_lookup(:ui_title => 'foreman')} Provision"},
+        :VmReconfigureRequest                     => {:vm_reconfigure                  => "VM Reconfigure"},
+        :VmCloudReconfigureRequest                => {:vm_cloud_reconfigure            => "VM Cloud Reconfigure"},
+        :VmMigrateRequest                         => {:vm_migrate                      => "VM Migrate"},
+        :VmRetireRequest                          => {:vm_retire                       => "VM Retire"},
+        :ServiceRetireRequest                     => {:service_retire                  => "Service Retire"},
+        :OrchestrationStackRetireRequest          => {:orchestration_stack_retire      => "Orchestration Stack Retire"},
+        :AutomationRequest                        => {:automation                      => "Automation"},
+        :ServiceTemplateProvisionRequest          => {:clone_to_service                => "Service Provision"},
+        :ServiceReconfigureRequest                => {:service_reconfigure             => "Service Reconfigure"},
+        :PhysicalServerProvisionRequest           => {:provision_physical_server       => "Physical Server Provision"},
+        :PhysicalServerFirmwareUpdateRequest      => {:physical_server_firmware_update => "Physical Server Firmware Update"},
+        :ServiceTemplateTransformationPlanRequest => {:transformation_plan             => "Transformation Plan"}
       }
 
       expect(described_class::REQUEST_TYPES).to eq(expected_request_types)
diff --git a/spec/models/physical_server_firmware_update_request_spec.rb b/spec/models/physical_server_firmware_update_request_spec.rb
new file mode 100644
index 000000000000..024fe960551f
--- /dev/null
+++ b/spec/models/physical_server_firmware_update_request_spec.rb
@@ -0,0 +1,93 @@
+describe PhysicalServerFirmwareUpdateRequest do
+  it '.TASK_DESCRIPTION' do
+    expect(described_class::TASK_DESCRIPTION).to eq('Physical Server Firmware Update')
+  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(PhysicalServerFirmwareUpdateTask)
+  end
+
+  it '#description' do
+    expect(subject.description).to eq('Physical Server Firmware Update')
+  end
+
+  it '#my_role' do
+    expect(subject.my_role).to eq('ems_operations')
+  end
+
+  it '#requested_task_idx' do
+    expect(subject.requested_task_idx).to eq([-1])
+  end
+
+  describe '.new_request_task' do
+    before { allow(ems.class).to receive(:firmware_update_class).and_return(task) }
+
+    let(:ems)    { FactoryBot.create(:ems_physical_infra) }
+    let(:task)   { double('TASK') }
+
+    it 'returns subclassed task' do
+      expect(described_class).to receive(:affected_ems).and_return(ems)
+      expect(task).to receive(:new).with('ATTRS')
+      described_class.new_request_task('ATTRS')
+    end
+  end
+
+  describe '.affected_physical_servers' do
+    let(:attrs)   { { 'options' => {:src_ids => src_ids} } }
+    let(:server1) { FactoryBot.create(:physical_server, :ems_id => 1) }
+    let(:server2) { FactoryBot.create(:physical_server, :ems_id => 2) }
+    let(:server3) { FactoryBot.create(:physical_server, :ems_id => 2) }
+
+    context 'when no src_ids are given' do
+      let(:src_ids) { [] }
+      it 'handled error is raised' do
+        expect { described_class.affected_physical_servers(attrs) }.to raise_error(MiqException::MiqFirmwareUpdateError)
+      end
+    end
+
+    context 'when invalid src_ids are given' do
+      let(:src_ids) { ['invalid'] }
+      it 'handled error is raised' do
+        expect { described_class.affected_physical_servers(attrs) }.to raise_error(MiqException::MiqFirmwareUpdateError)
+      end
+    end
+
+    context 'when servers belong to different ems' do
+      let(:src_ids) { [server1.id, server2.id] }
+      it 'handled error is raised' do
+        expect { described_class.affected_physical_servers(attrs) }.to raise_error(MiqException::MiqFirmwareUpdateError)
+      end
+    end
+
+    context 'when all okay' do
+      let(:src_ids) { [server2.id, server3.id] }
+      it 'server list is returned' do
+        expect(described_class.affected_physical_servers(attrs)).to eq([server2, server3])
+      end
+    end
+  end
+
+  it '#affected_physical_servers' do
+    expect(described_class).to receive(:affected_physical_servers).and_return('RES')
+    expect(subject.affected_physical_servers).to eq('RES')
+  end
+
+  describe '.affected_ems' do
+    let(:ems)    { FactoryBot.create(:ems_physical_infra) }
+    let(:server) { FactoryBot.create(:physical_server, :ems_id => ems.id) }
+
+    it 'when all okay' do
+      expect(described_class).to receive(:affected_physical_servers).and_return([server])
+      expect(described_class.affected_ems(nil)).to eq(ems)
+    end
+  end
+
+  it '#affected_ems' do
+    expect(described_class).to receive(:affected_ems).and_return('RES')
+    expect(subject.affected_ems).to eq('RES')
+  end
+end
diff --git a/spec/models/physical_server_firmware_update_task/state_machine_spec.rb b/spec/models/physical_server_firmware_update_task/state_machine_spec.rb
new file mode 100644
index 000000000000..2064fbd7072d
--- /dev/null
+++ b/spec/models/physical_server_firmware_update_task/state_machine_spec.rb
@@ -0,0 +1,50 @@
+describe PhysicalServerFirmwareUpdateTask do
+  let(:server)  { FactoryBot.create(:physical_server) }
+  let(:src_ids) { [server.id] }
+
+  subject { described_class.new(:options => { :src_ids => src_ids }) }
+
+  describe '#run_firmware_update' do
+    context 'when ok' do
+      it do
+        expect(subject).to receive(:signal).with(:start_firmware_update)
+        subject.run_firmware_update
+      end
+    end
+  end
+
+  it '#done_firmware_update' do
+    expect(subject).to receive(:signal).with(:mark_as_completed)
+    expect(subject).to receive(:update_and_notify_parent)
+    subject.done_firmware_update
+  end
+
+  it '#mark_as_completed' do
+    expect(subject).to receive(:signal).with(:finish)
+    expect(subject).to receive(:update_and_notify_parent)
+    expect(MiqAeEvent).to receive(:raise_evm_event)
+    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(: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(:status => 'Ok') }
+      it do
+        expect(log).to receive(:info).with(satisfy { |msg| msg.include?('... Complete') })
+        subject.finish
+      end
+    end
+  end
+end
diff --git a/spec/models/physical_server_firmware_update_task_spec.rb b/spec/models/physical_server_firmware_update_task_spec.rb
new file mode 100644
index 000000000000..a7490e81835d
--- /dev/null
+++ b/spec/models/physical_server_firmware_update_task_spec.rb
@@ -0,0 +1,18 @@
+describe PhysicalServerFirmwareUpdateTask do
+  it '#description' do
+    expect(subject.description).to eq('Physical Server Firmware Update')
+  end
+
+  it '.base_model' do
+    expect(described_class.base_model).to eq(PhysicalServerFirmwareUpdateTask)
+  end
+
+  it '.request_class' do
+    expect(described_class.request_class).to eq(PhysicalServerFirmwareUpdateRequest)
+  end
+
+  it '#do_request' do
+    expect(subject).to receive(:signal).with(:run_firmware_update)
+    subject.do_request
+  end
+end