Create temporary inventory when execute a playbook #14008

bzwei · 2017-02-21T21:17:18Z

Create a temporary inventory and add hosts to it before job execution in the preprocess step.
Delete the temporary inventory after job execution in the postprocess step

Every job template is created with default inventory Demo Inventory which contains localhost. Demo Inventory comes with tower installation. We can change it if the embedded tower will configure a formal localhost inventory.

Temporary is created with default organization which has id = 1. Again we can change it if the embedded tower will configure a formal default organization.

Will skip the temporary inventory creation/deletion if the playbook runs at localhost.

https://www.pivotaltracker.com/story/show/140142225

bzwei · 2017-02-21T21:17:39Z

@gmcculloug @bdunne @syncrou

syncrou · 2017-02-21T22:31:22Z

app/models/service_ansible_playbook.rb

+
+ def create_inventory(action, hosts)
+ manager(action).with_provider_connection do |connection|
+ inventory = connection.api.inventories.create(:name => inventory_name(action), :organization => 1)


Would we want .create! and destroy! - so we could bubble up an Exception?

bdunne · 2017-02-21T22:36:05Z

app/models/service_ansible_playbook.rb

+
+ def create_inventory(action, hosts)
+ manager(action).with_provider_connection do |connection|
+ inventory = connection.api.inventories.create(:name => inventory_name(action), :organization => 1)


Using .tap will further indent the hosts.split call making the nesting more visible.

def create_inventory(action, hosts) manager(action).with_provider_connection do |connection| connection.api.inventories.create(:name => inventory_name(action), :organization => 1).tap do |inventory| hosts.split(',').each do |host| connection.api.hosts.create(:name => host, :inventory => inventory.id) end end end end

Maybe the host creation logic should be split into another private method to reduce complexity, what do you think?

bdunne · 2017-02-21T22:37:18Z

app/models/service_ansible_playbook.rb

@@ -73,4 +90,29 @@ def extra_vars_from_dialog

 params.blank? ? {} : {:extra_vars => params}
 end
+
+ def create_inventory(action, hosts)


The name of this method could be improved, maybe create_inventory_and_hosts, since it is creating more than just an Ansible Tower Inventory.

bdunne · 2017-02-21T22:40:51Z

app/models/service_template_ansible_playbook.rb

@@ -89,7 +89,7 @@ def self.build_parameter_list(name, description, info)
 :description => description || '',
 :project => playbook.configuration_script_source.manager_ref,
 :playbook => playbook.name,
- :inventory => tower.inventory_root_groups.first.ems_ref,
+ :inventory => tower.inventory_root_groups.find_by!(:name => 'Demo Inventory').ems_ref,


The inventory name should probably be a constant on the manager. It can then be reused by the setup scripting that we discussed with @gtanzillo @carbonin and @gmcculloug.

Not sure where should the constant be replaced. We can refactor once the solution is finalized.

@bzwei Agreed, please open a pivotal story to track this work.

https://www.pivotaltracker.com/story/show/140418429

bdunne · 2017-02-21T22:44:34Z

spec/models/service_ansible_playbook_spec.rb

@@ -12,40 +15,95 @@
 end

 let(:executed_service) do
- basic_service.tap do |service|
+ FactoryGirl.create(:service_ansible_playbook, :options => provision_options).tap do |service|
 allow(service).to receive(:job).with(action).and_return(tower_job)


Shouldn't this build the relation rather than stubbing it?

We cannot stub method for tower_job later in the tests if build a relation here.

Then it sounds like it shouldn't be shared, but don't fix in this PR.

bdunne · 2017-02-21T22:49:50Z

spec/models/service_ansible_playbook_spec.rb

- )
+ context 'basic service' do
+ it 'prepares job options from service template' do
+ expect(basic_service).to receive(:create_inventory)


You could probably write this expectation like this, then you wouldn't have to wrap the line and rubocop would be much happier.

hosts = config_info_options.fetch_path(:config_info, :provision, :hosts) expect(basic_service).to receive(:create_inventory).with(action, hosts).and_return(double(:id => 10))

bdunne · 2017-02-21T22:59:05Z

app/models/service_ansible_playbook.rb

- :credential_id => dialog_options['dialog_credential_id'],
- :hosts => dialog_options['dialog_hosts']
+ :credential_id => dialog_options['dialog_credential'],
+ :hosts => (dialog_options['dialog_hosts'] || '').strip.presence


Why add the || ''? #presence will still return nil...

if dialog_options['dialog_hosts'] is nil

dialog_options['dialog_hosts'].to_s.strip.presence

bdunne · 2017-02-21T23:01:56Z

app/models/service_ansible_playbook.rb

+
+ def create_inventory(action, hosts)
+ manager(action).with_provider_connection do |connection|
+ inventory = connection.api.inventories.create!(:name => inventory_name(action), :organization => 1)


You could do:

def create_inventory(action, hosts) manager(action).with_provider_connection do |connection| connection.api.inventories.create!(:name => inventory_name(action), :organization => 1).tap do |inventory| hosts.split(',').each do |host| connection.api.hosts.create(:name => host, :inventory => inventory.id) end end end end

The tap further indents the host creation and better represents the flow.

bdunne · 2017-02-21T23:05:33Z

app/models/service_ansible_playbook.rb

@@ -38,30 +39,46 @@ def job(action)
 service_resources.find_by!(:name => action, :resource_type => 'OrchestrationStack').try(:resource)
 end

+ def postprocess(action)
+ hosts = options.fetch_path(job_option_key(action), :inventory)
+ delete_inventory(action) unless hosts.blank? || hosts == 'localhost'


Why not delete the inventory if it only contains 'localhost'?

localhost uses the default inventory that cannot be deleted.

bdunne · 2017-02-22T15:54:02Z

spec/models/service_ansible_playbook_spec.rb

@@ -12,40 +15,95 @@
 end

 let(:executed_service) do
- basic_service.tap do |service|
+ FactoryGirl.create(:service_ansible_playbook, :options => provision_options).tap do |service|
 allow(service).to receive(:job).with(action).and_return(tower_job)


Then it sounds like it shouldn't be shared, but don't fix in this PR.

syncrou · 2017-02-22T16:47:37Z

app/models/service_ansible_playbook.rb

+ manager(action).with_provider_connection do |connection|
+ connection.api.inventories.create!(:name => inventory_name(action), :organization => 1).tap do |inventory|
+ hosts.split(',').each do |host|
+ connection.api.hosts.create(:name => host, :inventory => inventory.id)


s/create/create!/

miq-bot · 2017-02-22T17:07:21Z

Checked commits bzwei/manageiq@0dd941a~...c76637f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 🏆

syncrou · 2017-02-22T19:06:01Z

👍 Looks good to me (once it can get to green.)

gmcculloug self-assigned this Feb 21, 2017

gmcculloug requested a review from bdunne February 21, 2017 21:21

gmcculloug added enhancement euwe/no lifecycle/services labels Feb 21, 2017

syncrou reviewed Feb 21, 2017

View reviewed changes

Select Demo Inventory as the default

0dd941a

bzwei force-pushed the playbook_hosts branch from 1cc5072 to 9c0739d Compare February 21, 2017 22:57

bdunne requested changes Feb 21, 2017

View reviewed changes

bzwei force-pushed the playbook_hosts branch 4 times, most recently from 07d0ea4 to 423fe43 Compare February 22, 2017 15:07

bdunne approved these changes Feb 22, 2017

View reviewed changes

syncrou reviewed Feb 22, 2017

View reviewed changes

Create temporary inventory when execute a playbook

c76637f

bzwei force-pushed the playbook_hosts branch from 423fe43 to c76637f Compare February 22, 2017 17:01

gmcculloug merged commit e85e92e into ManageIQ:master Feb 23, 2017

gmcculloug added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 23, 2017

bzwei deleted the playbook_hosts branch February 23, 2017 16:19

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create temporary inventory when execute a playbook #14008

Create temporary inventory when execute a playbook #14008

bzwei commented Feb 21, 2017 •

edited by gmcculloug

Loading

bzwei commented Feb 21, 2017

syncrou Feb 21, 2017

bdunne Feb 21, 2017

bdunne Feb 21, 2017

bdunne Feb 21, 2017

bdunne Feb 21, 2017

bzwei Feb 21, 2017

gmcculloug Feb 22, 2017

bzwei Feb 22, 2017

bdunne Feb 21, 2017

bzwei Feb 22, 2017

bdunne Feb 22, 2017

bdunne Feb 21, 2017

bdunne Feb 21, 2017

bzwei Feb 21, 2017

bdunne Feb 21, 2017

bdunne Feb 21, 2017

bdunne Feb 21, 2017

bzwei Feb 21, 2017

bdunne Feb 22, 2017

syncrou Feb 22, 2017

miq-bot commented Feb 22, 2017

syncrou commented Feb 22, 2017

Create temporary inventory when execute a playbook #14008

Create temporary inventory when execute a playbook #14008

Conversation

bzwei commented Feb 21, 2017 • edited by gmcculloug Loading

bzwei commented Feb 21, 2017

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

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

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

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 Feb 22, 2017

syncrou commented Feb 22, 2017

bzwei commented Feb 21, 2017 •

edited by gmcculloug

Loading