Cleanup after Ansible runner. #19383

lfu · 2019-10-09T13:40:53Z

Ansible runner support has been added in Ivanchuk.
This pull request is to clean up the Ansible playbook service to get rid of the temporary Ansible template implementation.

Blocks ManageIQ/manageiq-automation_engine#378
Requires #19432
Includes ManageIQ/manageiq-content#594
Includes ManageIQ/manageiq-automation_engine#383
Includes #19466

https://bugzilla.redhat.com/show_bug.cgi?id=1765682

@miq-bot assign @tinaafitz
@miq-bot add_label services, changelog/yes, ivanchuk/yes, hammer/no

Fryguy · 2019-10-11T16:29:56Z

cc @NickLaMuro Please review

Fryguy · 2019-10-11T16:31:08Z

app/models/manageiq/providers/embedded_ansible/automation_manager/playbook.rb

-    $log.log_hashes(jt_options)
-    job_template_klass.raw_create_in_provider(manager, jt_options)
-  end
+  DEFAULT_EXECUTION_TTL = 100.minutes # automate state machine aborts after 100 retries at a minite interval


minor typo minite -> minute

Fryguy · 2019-10-11T16:31:44Z

app/models/manageiq/providers/embedded_ansible/automation_manager/playbook.rb

+      hash[k] = match_data ? ManageIQ::Password.decrypt(v.gsub(/password::/, '')) : v
+    end
+
+    extra_vars


No need to set a temporary variable and return it because it would be returned anyway.

To be fair, this was a copy-paste from what was done in EmbeddedAnsible::AutomationManager::ConfigurationScript previously.

NickLaMuro

I am still wrapping my head around a lot of this, but my initial reaction to this change is I don't think this should be something that we should be pushing into the ivanchuk branch. I am fine with it conceptually (I think), but making such a massive code delete like this should be saved for a later release so the QA effort that has already been done on the existing code isn't wasted.

</my_2cents>

NickLaMuro

So I did take the resulting patch of this PR and apply it to fresh appliance to try it out. Everything works as far as I can tell, so I am good with merging this into master.

I still haven't heard as response regarding my "2 cents" to not have this as part of ivanchuk, but in addition, I found a unused method as part of my more thorough review. But other than that, I am cool with these changes, and it also proves I am still pretty unfamiliar to the architecture surrounding all of this code.

app/models/manageiq/providers/embedded_ansible/automation_manager/job.rb

lfu · 2019-10-15T17:15:40Z

@NickLaMuro Thanks for reviewing and testing.
@gmcculloug and @tinaafitz would decide if this PR would be back ported to ivanchuk.

NickLaMuro

Per @lfu 's comment:

#19383 (comment)

this decision will be made separately, so I don't think my review should be keeping this from going into master. Approving.

tinaafitz · 2019-10-23T20:31:27Z

@mkanoor Please review.

mkanoor · 2019-10-23T20:43:10Z

app/models/manageiq/providers/embedded_ansible/automation_manager/job.rb

@@ -96,7 +98,7 @@ def retireable?
  # If extra_vars are passed through automate, all keys are considered as attributes and
  # converted to lower case. Need to convert them back to original definitions in the
  # job template through survey_spec or variables
-  def self.reconcile_extra_vars_keys(_template, options)
+  def self.reconcile_extra_vars_keys(_playbook, options)


@lfu is this just returning back the options that were passed in

Seems that is the case here.

@lfu can we get rid of it

If it is ok with @gmcculloug. I promised him this is a refactor PR.

@lfu Since this is a private method it seems like a straight-forward change that would be fine for this PR.

miq-bot · 2019-10-24T20:06:33Z

Checked commit lfu@3b3184c with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
9 files checked, 0 offenses detected
Everything looks fine. 🏆

mzazrivec · 2019-10-25T07:48:05Z

With this change in place, the server application no longer works. #19432 seems to be fixing it.

martinpovolny · 2019-10-25T08:15:22Z

I am wondering why is this ivanchuk/yes. Making #19432 the same though.

gmcculloug · 2019-10-25T12:43:21Z

@martinpovolny Thanks for pointing out the release flag. I have discussed the viability of back-porting this with @dmetzger57 as it would be nice to keep Ivanchuk in-sync with master this early on but will come down to when we can get it properly tested. Without a BZ this will not get back-ported so leaving it currently set to ivanchuk/yes until a decision is made, which might be today.

Cleanup after Ansible runner. (cherry picked from commit 1e1ef97) https://bugzilla.redhat.com/show_bug.cgi?id=1784103

simaishi · 2019-12-17T18:49:18Z

Ivanchuk backport details:

$ git log -1
commit d1148e84ef28f16d97be0e8f1d59b59bc060b0a5
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Oct 24 17:44:30 2019 -0400

    Merge pull request #19383 from lfu/ansible_runner_cleanup

    Cleanup after Ansible runner.

    (cherry picked from commit 1e1ef97add1d60ab52347b882c562684e0a64fad)

    https://bugzilla.redhat.com/show_bug.cgi?id=1784103

miq-bot assigned tinaafitz Oct 9, 2019

miq-bot added changelog/yes enhancement hammer/no ivanchuk/yes lifecycle/services labels Oct 9, 2019

lfu mentioned this pull request Oct 9, 2019

Cleanup after Ansible runner. ManageIQ/manageiq-automation_engine#378

Merged

lfu force-pushed the ansible_runner_cleanup branch from 4eb0c8e to bba6458 Compare October 9, 2019 21:09

Fryguy reviewed Oct 11, 2019

View reviewed changes

Fryguy requested a review from NickLaMuro October 11, 2019 16:32

NickLaMuro suggested changes Oct 11, 2019

View reviewed changes

lfu force-pushed the ansible_runner_cleanup branch from bba6458 to c8e686d Compare October 14, 2019 13:13

NickLaMuro suggested changes Oct 14, 2019

View reviewed changes

app/models/manageiq/providers/embedded_ansible/automation_manager/job.rb Show resolved Hide resolved

NickLaMuro approved these changes Oct 16, 2019

View reviewed changes

miq-bot added the unmergeable label Oct 22, 2019

lfu force-pushed the ansible_runner_cleanup branch from c8e686d to f64693f Compare October 22, 2019 23:14

gmcculloug assigned gmcculloug and unassigned tinaafitz Oct 23, 2019

miq-bot removed the unmergeable label Oct 23, 2019

mkanoor reviewed Oct 23, 2019

View reviewed changes

Cleanup after Ansible runner.

3b3184c

lfu force-pushed the ansible_runner_cleanup branch from f64693f to 3b3184c Compare October 24, 2019 20:03

mkanoor approved these changes Oct 24, 2019

View reviewed changes

gmcculloug merged commit 1e1ef97 into ManageIQ:master Oct 24, 2019

gmcculloug added this to the Sprint 123 Ending Oct 28, 2019 milestone Oct 24, 2019

mzazrivec mentioned this pull request Oct 25, 2019

Remove require_nested ConfigurationScript #19432

Merged

gmcculloug added refactoring core/embedded ansible and removed enhancement labels Oct 25, 2019

This was referenced Oct 28, 2019

Cleanup after Ansible runner integration ManageIQ/manageiq-content#594

Merged

Cleanup after Ansible runner. ManageIQ/manageiq-automation_engine#383

Merged

lfu deleted the ansible_runner_cleanup branch November 4, 2019 15:22

lfu mentioned this pull request Nov 6, 2019

Remove embedded ansible PlaybookRunner. #19466

Merged

simaishi pushed a commit that referenced this pull request Dec 17, 2019

Merge pull request #19383 from lfu/ansible_runner_cleanup

d1148e8

Cleanup after Ansible runner. (cherry picked from commit 1e1ef97) https://bugzilla.redhat.com/show_bug.cgi?id=1784103

simaishi added ivanchuk/backported and removed ivanchuk/yes labels Dec 17, 2019

NickLaMuro mentioned this pull request Jun 15, 2020

[EmbeddedAnsible] TODO Cleanup #19276

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup after Ansible runner. #19383

Cleanup after Ansible runner. #19383

lfu commented Oct 9, 2019 •

edited

Loading

Fryguy commented Oct 11, 2019 •

edited

Loading

Fryguy Oct 11, 2019

Fryguy Oct 11, 2019

NickLaMuro Oct 11, 2019

NickLaMuro left a comment

NickLaMuro left a comment •

edited

Loading

lfu commented Oct 15, 2019

NickLaMuro left a comment

tinaafitz commented Oct 23, 2019

mkanoor Oct 23, 2019

lfu Oct 24, 2019

mkanoor Oct 24, 2019

lfu Oct 24, 2019

gmcculloug Oct 24, 2019

miq-bot commented Oct 24, 2019

mzazrivec commented Oct 25, 2019

martinpovolny commented Oct 25, 2019

gmcculloug commented Oct 25, 2019

simaishi commented Dec 17, 2019

Cleanup after Ansible runner. #19383

Cleanup after Ansible runner. #19383

Conversation

lfu commented Oct 9, 2019 • edited Loading

Fryguy commented Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro left a comment • edited Loading

Choose a reason for hiding this comment

lfu commented Oct 15, 2019

NickLaMuro left a comment

Choose a reason for hiding this comment

tinaafitz commented Oct 23, 2019

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

miq-bot commented Oct 24, 2019

mzazrivec commented Oct 25, 2019

martinpovolny commented Oct 25, 2019

gmcculloug commented Oct 25, 2019

simaishi commented Dec 17, 2019

lfu commented Oct 9, 2019 •

edited

Loading

Fryguy commented Oct 11, 2019 •

edited

Loading

NickLaMuro left a comment •

edited

Loading