Merge pull request #19140 from Fryguy/refactor_ansible_runner_workflow

Refactor AnsibleRunnerWorkflow to DRY it up

app/models/manageiq/providers/ansible_playbook_workflow.rb

@@ -1,49 +1,12 @@
 class ManageIQ::Providers::AnsiblePlaybookWorkflow < ManageIQ::Providers::AnsibleRunnerWorkflow
-  def self.job_options(env_vars, extra_vars, playbook_options, timeout, poll_interval, hosts, credentials, verbosity, become_enabled)
-    {
-      :become_enabled => become_enabled,
-      :credentials    => credentials,
-      :env_vars       => env_vars,
-      :extra_vars     => extra_vars,
-      :hosts          => hosts,
-      :playbook_path  => playbook_options[:playbook_path],
-      :timeout        => timeout,
-      :poll_interval  => poll_interval,
-      :verbosity      => verbosity
-    }
+  def execution_type
+    "playbook"
   end
 
-  def pre_playbook
-    # A step before running the playbook for any optional setup tasks
-    queue_signal(:run_playbook)
-  end
-
-  def run_playbook
+  def launch_runner
     env_vars, extra_vars, playbook_path = options.values_at(:env_vars, :extra_vars, :playbook_path)
     kwargs = options.slice(:credentials, :hosts, :verbosity, :become_enabled)
 
-    response = Ansible::Runner.run_async(env_vars, extra_vars, playbook_path, kwargs)
-    if response.nil?
-      queue_signal(:abort, "Failed to run ansible playbook", "error")
-    else
-      context[:ansible_runner_response] = response.dump
-
-      started_on = Time.now.utc
-      update_attributes!(:context => context, :started_on => started_on)
-      miq_task.update_attributes!(:started_on => started_on)
-
-      queue_signal(:poll_runner)
-    end
+    Ansible::Runner.run_async(env_vars, extra_vars, playbook_path, kwargs)
   end
-
-  def load_transitions
-    super.tap do |transactions|
-      transactions.merge!(
-        :start        => {'waiting_to_start' => 'pre_playbook'},
-        :run_playbook => {'pre_playbook'     => 'running'},
-      )
-    end
-  end
-
-  alias start pre_playbook
 end

app/models/manageiq/providers/ansible_role_workflow.rb

@@ -1,50 +1,12 @@
 class ManageIQ::Providers::AnsibleRoleWorkflow < ManageIQ::Providers::AnsibleRunnerWorkflow
-  def self.job_options(env_vars, extra_vars, role_options, timeout, poll_interval, hosts, credentials, verbosity, become_enabled)
-    {
-      :become_enabled  => become_enabled,
-      :credentials     => credentials,
-      :env_vars        => env_vars,
-      :extra_vars      => extra_vars,
-      :hosts           => hosts,
-      :role_name       => role_options[:role_name],
-      :roles_path      => role_options[:roles_path],
-      :role_skip_facts => role_options[:role_skip_facts],
-      :timeout         => timeout,
-      :poll_interval   => poll_interval,
-      :verbosity       => verbosity
-    }
+  def execution_type
+    "role"
   end
 
-  def pre_role
-    # A step before running the playbook for any optional setup tasks
-    queue_signal(:run_role)
-  end
-
-  def run_role
+  def launch_runner
     env_vars, extra_vars, role_name, roles_path, role_skip_facts = options.values_at(:env_vars, :extra_vars, :role_name, :roles_path, :role_skip_facts)
     role_skip_facts = true if role_skip_facts.nil?
-    response = Ansible::Runner.run_role_async(env_vars, extra_vars, role_name, :roles_path => roles_path, :role_skip_facts => role_skip_facts)
-    if response.nil?
-      queue_signal(:abort, "Failed to run ansible role", "error")
-    else
-      context[:ansible_runner_response] = response.dump
-
-      started_on = Time.now.utc
-      update_attributes!(:context => context, :started_on => started_on)
-      miq_task.update_attributes!(:started_on => started_on)
 
-      queue_signal(:poll_runner)
-    end
+    Ansible::Runner.run_role_async(env_vars, extra_vars, role_name, :roles_path => roles_path, :role_skip_facts => role_skip_facts)
   end
-
-  def load_transitions
-    super.tap do |transactions|
-      transactions.merge!(
-        :start    => {'waiting_to_start' => 'pre_role'},
-        :run_role => {'pre_role'         => 'running' },
-      )
-    end
-  end
-
-  alias start pre_role
 end

app/models/manageiq/providers/ansible_runner_workflow.rb

@@ -2,17 +2,49 @@ class ManageIQ::Providers::AnsibleRunnerWorkflow < Job
   def self.create_job(env_vars, extra_vars, role_or_playbook_options,
                       hosts = ["localhost"], credentials = [],
                       timeout: 1.hour, poll_interval: 1.second, verbosity: 0, become_enabled: false)
-    options = job_options(env_vars, extra_vars, role_or_playbook_options, timeout,
-                          poll_interval, hosts, credentials, verbosity, become_enabled)
-    super(name, options)
+    super(name, role_or_playbook_options.merge(
+      :become_enabled => become_enabled,
+      :credentials    => credentials,
+      :env_vars       => env_vars,
+      :extra_vars     => extra_vars,
+      :hosts          => hosts,
+      :timeout        => timeout,
+      :poll_interval  => poll_interval,
+      :verbosity      => verbosity
+    ))
   end
 
   def current_job_timeout(_timeout_adjustment = 1)
     options[:timeout] || super
   end
 
-  def job_options(options)
-    raise(NotImplementedError, 'abstract')
+  def execution_type
+    raise NotImplementedError, "must be implemented in a subclass"
+  end
+
+  def pre_execute
+    # A step before running the playbook/role for any optional setup tasks
+    queue_signal(:execute)
+  end
+
+  def launch_runner
+    raise NotImplementedError, "must be implemented in a subclass"
+  end
+
+  def execute
+    response = launch_runner
+
+    if response.nil?
+      queue_signal(:abort, "Failed to run ansible #{execution_type}", "error")
+    else
+      context[:ansible_runner_response] = response.dump
+
+      started_on = Time.now.utc
+      update!(:context => context, :started_on => started_on)
+      miq_task.update!(:started_on => started_on)
+
+      queue_signal(:poll_runner)
+    end
   end
 
   def poll_runner
@@ -21,7 +53,7 @@ def poll_runner
       if started_on + options[:timeout] < Time.now.utc
         response.stop
 
-        queue_signal(:abort, "Playbook has been running longer than timeout", "error")
+        queue_signal(:abort, "ansible #{execution_type} has been running longer than timeout", "error")
       else
         queue_signal(:poll_runner, :deliver_on => deliver_on)
       end
@@ -32,26 +64,26 @@ def poll_runner
       context[:ansible_runner_stdout]      = result.parsed_stdout
 
       if result.return_code != 0
-        set_status("Playbook failed", "error")
-        _log.warn("Playbook failed:\n#{result.parsed_stdout.join("\n")}")
+        set_status("ansible #{execution_type} failed", "error")
+        _log.warn("ansible #{execution_type} failed:\n#{result.parsed_stdout.join("\n")}")
       else
-        set_status("Playbook completed with no errors", "ok")
+        set_status("ansible #{execution_type} completed with no errors", "ok")
       end
-      queue_signal(:post_playbook)
+      queue_signal(:post_execute)
     end
   end
 
-  def post_playbook
-    # A step after running the playbook for any optional cleanup tasks
+  def post_execute
+    # A step after running the playbook/role for any optional cleanup tasks
     queue_signal(:finish, message, status)
   end
 
-  def fail_unimplamented
-    raise(NotImplementedError, "this is an abstract class, use a subclass that implaments a 'start' method")
+  def start
+    # Cannot use alias otherwise subclasses can't override
+    pre_execute
   end
 
   alias initializing dispatch_start
-  alias start        fail_unimplamented
   alias finish       process_finished
   alias abort_job    process_abort
   alias cancel       process_cancel
@@ -73,7 +105,7 @@ def queue_signal(*args, deliver_on: nil)
       :task_id     => guid,
       :args        => args,
       :deliver_on  => deliver_on,
-      :server_guid => MiqServer.my_server.guid,
+      :server_guid => MiqServer.my_server.guid
     )
   end
 
@@ -85,13 +117,15 @@ def load_transitions
     self.state ||= 'initialize'
 
     {
-      :initializing  => {'initialize'       => 'waiting_to_start'},
-      :poll_runner   => {'running'          => 'running'},
-      :post_playbook => {'running'          => 'post_playbook'},
-      :finish        => {'*'                => 'finished'},
-      :abort_job     => {'*'                => 'aborting'},
-      :cancel        => {'*'                => 'canceling'},
-      :error         => {'*'                => '*'}
+      :initializing => {'initialize'       => 'waiting_to_start'},
+      :start        => {'waiting_to_start' => 'pre_execute'},
+      :execute      => {'pre_execute'      => 'running'},
+      :poll_runner  => {'running'          => 'running'},
+      :post_execute => {'running'          => 'post_execute'},
+      :finish       => {'*'                => 'finished'},
+      :abort_job    => {'*'                => 'aborting'},
+      :cancel       => {'*'                => 'canceling'},
+      :error        => {'*'                => '*'}
     }
   end
 end

spec/models/manageiq/providers/ansible_playbook_workflow_spec.rb

@@ -1,6 +1,6 @@
 describe ManageIQ::Providers::AnsiblePlaybookWorkflow do
   let(:job)     { described_class.create_job(*options).tap { |job| job.state = state } }
-  let(:options) { [{"ENV" => "VAR"}, %w[arg1 arg2], {:playbook_path => "/path/to/playbook"}, %w[192.0.2.0 192.0.2.1], {:verbosity => 3}] }
+  let(:options) { [{"ENV" => "VAR"}, {"arg1" => "val1"}, {:playbook_path => "/path/to/playbook"}, %w[192.0.2.0 192.0.2.1], {:verbosity => 3}] }
   let(:state)   { "waiting_to_start" }
 
   context ".create_job" do
@@ -10,7 +10,7 @@
   end
 
   context ".signal" do
-    %w(start pre_playbook run_playbook poll_runner post_playbook finish abort_job cancel error).each do |signal|
+    %w[start pre_execute execute poll_runner post_execute finish abort_job cancel error].each do |signal|
       shared_examples_for "allows #{signal} signal" do
         it signal.to_s do
           expect(job).to receive(signal.to_sym)
@@ -19,7 +19,7 @@
       end
     end
 
-    %w(start pre_playbook run_playbook poll_runner post_playbook).each do |signal|
+    %w[start pre_execute execute poll_runner post_execute].each do |signal|
       shared_examples_for "doesn't allow #{signal} signal" do
         it signal.to_s do
           expect { job.signal(signal.to_sym) }.to raise_error(RuntimeError, /#{signal} is not permitted at state #{job.state}/)
@@ -36,23 +36,23 @@
       it_behaves_like "allows cancel signal"
       it_behaves_like "allows error signal"
 
-      it_behaves_like "doesn't allow run_playbook signal"
+      it_behaves_like "doesn't allow execute signal"
       it_behaves_like "doesn't allow poll_runner signal"
-      it_behaves_like "doesn't allow post_playbook signal"
+      it_behaves_like "doesn't allow post_execute signal"
     end
 
-    context "pre_playbook" do
-      let(:state) { "pre_playbook" }
+    context "pre_execute" do
+      let(:state) { "pre_execute" }
 
-      it_behaves_like "allows run_playbook signal"
+      it_behaves_like "allows execute signal"
       it_behaves_like "allows finish signal"
       it_behaves_like "allows abort_job signal"
       it_behaves_like "allows cancel signal"
       it_behaves_like "allows error signal"
 
       it_behaves_like "doesn't allow start signal"
       it_behaves_like "doesn't allow poll_runner signal"
-      it_behaves_like "doesn't allow post_playbook signal"
+      it_behaves_like "doesn't allow post_execute signal"
     end
 
     context "running" do
@@ -65,34 +65,34 @@
       it_behaves_like "allows error signal"
 
       it_behaves_like "doesn't allow start signal"
-      it_behaves_like "doesn't allow pre_playbook signal"
+      it_behaves_like "doesn't allow pre_execute signal"
     end
 
-    context "post_playbook" do
-      let(:state) { "post_playbook" }
+    context "post_execute" do
+      let(:state) { "post_execute" }
 
       it_behaves_like "allows finish signal"
       it_behaves_like "allows abort_job signal"
       it_behaves_like "allows cancel signal"
       it_behaves_like "allows error signal"
 
       it_behaves_like "doesn't allow start signal"
-      it_behaves_like "doesn't allow pre_playbook signal"
-      it_behaves_like "doesn't allow run_playbook signal"
+      it_behaves_like "doesn't allow pre_execute signal"
+      it_behaves_like "doesn't allow execute signal"
       it_behaves_like "doesn't allow poll_runner signal"
-      it_behaves_like "doesn't allow post_playbook signal"
+      it_behaves_like "doesn't allow post_execute signal"
     end
   end
 
-  context ".run_playbook" do
-    let(:state) { "pre_playbook" }
+  context ".execute" do
+    let(:state) { "pre_execute" }
     let(:response_async) { Ansible::Runner::ResponseAsync.new(:base_dir => "/path/to/results") }
 
     it "ansible-runner succeeds" do
       response_async = Ansible::Runner::ResponseAsync.new(:base_dir => "/path/to/results")
       runner_options = [
         {"ENV" => "VAR"},
-        %w[arg1 arg2],
+        {"arg1" => "val1"},
         "/path/to/playbook",
         {
           :become_enabled => false,
@@ -105,7 +105,7 @@
       expect(Ansible::Runner).to receive(:run_async).with(*runner_options).and_return(response_async)
       expect(job).to receive(:queue_signal).with(:poll_runner)
 
-      job.signal(:run_playbook)
+      job.signal(:execute)
 
       expect(job.context[:ansible_runner_response]).to eq(response_async.dump)
     end
@@ -114,7 +114,7 @@
       expect(Ansible::Runner).to receive(:run_async).and_return(nil)
       expect(job).to receive(:queue_signal).with(:abort, "Failed to run ansible playbook", "error")
 
-      job.signal(:run_playbook)
+      job.signal(:execute)
     end
   end
 
@@ -135,7 +135,7 @@
 
       response = Ansible::Runner::Response.new(response_async.dump.merge(:return_code => 0))
       expect(response_async).to receive(:response).and_return(response)
-      expect(job).to receive(:queue_signal).with(:post_playbook)
+      expect(job).to receive(:queue_signal).with(:post_execute)
 
       job.signal(:poll_runner)
     end
@@ -155,14 +155,14 @@
       Timecop.travel(time) do
         expect(response_async).to receive(:running?).and_return(true)
         expect(response_async).to receive(:stop)
-        expect(job).to receive(:queue_signal).with(:abort, "Playbook has been running longer than timeout", "error")
+        expect(job).to receive(:queue_signal).with(:abort, "ansible playbook has been running longer than timeout", "error")
 
         job.signal(:poll_runner)
       end
     end
 
     context ".deliver_on" do
-      let(:options) { [{"ENV" => "VAR"}, %w[arg1 arg2], {:playbook_path => "/path/to/playbook"}, %w[192.0.2.0 192.0.2.1], :poll_interval => 5.minutes] }
+      let(:options) { [{"ENV" => "VAR"}, {"arg1" => "val1"}, {:playbook_path => "/path/to/playbook"}, %w[192.0.2.0 192.0.2.1], :poll_interval => 5.minutes] }
 
       it "uses the option to queue poll_runner" do
         now = Time.now.utc