Create service template with Ansible Tower after first creating a new job template #13896

syncrou · 2017-02-13T22:29:38Z

Running from console with these options and subsequent command:

options = { :name => 'db_console', :description => 'test ansible', :service_template_catalog_id => ServiceTemplateCatalog.last.id, :config_info => { :provision => { :new_dialog_name => 'hello_world', :hosts => 'blah', :credential => '<real credential id associated this tower instance>', :playbook_id => '<id of playbook>'}}}

ServiceTemplateAnsiblePlaybook.create_job_templates(options[:name], options[:description], options[:config_info], auth_user)

You should build a job_template on the provider that looks something like this:

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

syncrou · 2017-02-13T22:33:14Z

@miq-bot add_label enhancement, providers/ansible_tower

@miq-bot assign @bzwei

syncrou · 2017-02-13T22:34:41Z

cc - @bdunne, @gmcculloug

syncrou · 2017-02-13T22:36:15Z

@miq-bot add_label euwe/no

bzwei · 2017-02-13T22:36:48Z

app/models/service_template_ansible_playbook.rb

@@ -1,5 +1,5 @@
 class ServiceTemplateAnsiblePlaybook < ServiceTemplateGeneric
-  def self.default_provisioning_entry_point(_service_type)
+  def self.default_provisioning_entry_point(_service_type = nil)


no need to change

bzwei · 2017-02-13T22:44:25Z

app/models/service_template_ansible_playbook.rb

+    Hash.new.tap do |hash|
+      [:provision, :retirement, :reconfigure].each do |action|
+        next unless config_info[action]
+        hash[action.to_sym] = { :configuration_template => create_job_template("#{service_name}_#{action}", description, config_info[action], auth_user) }


action is already a symbol. For the job template name we may need to add a random number to ensure the uniqueness.

@bzwei - Do we have a standard procedure for generating a random number? My original thought was something like rand(100) but wasn't sure if that gives us enough permutations.

bzwei · 2017-02-13T22:47:38Z

app/models/service_template_ansible_playbook.rb

-    # tower = playbook.manager
+  def self.create_job_templates(service_name, description, config_info, auth_user)
+    {}.tap do |hash|
+      [:provision, :retirement, :reconfigure].each do |action|


[:provision, :retirement, :reconfigure].each_with_object({}) do |action, hash|

bzwei · 2017-02-13T22:47:54Z

app/models/service_template_ansible_playbook.rb

+    {}.tap do |hash|
+      [:provision, :retirement, :reconfigure].each do |action|
+        next unless config_info[action]
+        hash[action.to_sym] = { :configuration_template => create_job_template("#{service_name}_#{action}", description, config_info[action], auth_user) }


action is already a symbol

bzwei · 2017-02-13T22:48:57Z

app/models/service_template_ansible_playbook.rb

+    {}.tap do |hash|
+      [:provision, :retirement, :reconfigure].each do |action|
+        next unless config_info[action]
+        hash[action.to_sym] = { :configuration_template => create_job_template("#{service_name}_#{action}", description, config_info[action], auth_user) }


For the job template name we need to add a random number to make the name unique. I prefer to make it a postfix.

Should we instead use something like "ManageIQ_Service_#{service.id}_#{action}"? I don't think we should pick any random number.

Creating job template is the first step. We don't have any id at this stage. Having the service name as part of job template name is better for debugging. If we don't want random number we could try sequential number until the tower no longer rejects. Random number is probably better for performance's sake.

Prefer a GUID, and then you can also make sure the same GUID is in the database table in a guid column?

I'm not against adding the name of the ServiceTemplate for troubleshooting purposes, but it isn't unique, therefore we can't use it to guarantee uniqueness, which is why I suggest using the records id. Is there a reason we can't create an empty ServiceTemplate (to be updated later in the process) before we create the Job Templates, just for the purpose of obtaining the unique id?

Or is there some other thing we can tie to? Like a service request id or something?

@bdunne Is there a race condition with refresh with that? That is, if you create a service template, then later refresh comes along, could it delete it out from underneath you?

@Fryguy ServiceTemplate is our concept, it isn't backed by a provider or updated on refresh.

This is how the job template looks like MIQ-PLAYBOOK-DEMO_PROVISION_HD2G8J13, the last segment _HD2G8J13 is auto generated by a random algorithm. It looks good to me. This is just a hidden name, not meaningful to user but could be helpful for debugging.

I am OK to use UUID, just think appending to an existing name make it looks too long; or we could use UUID alone for the name.

Using the ID or UUID of the service_template make sense to me since they are already part of the model. (Remember that with a region id the ID value will be almost as long as the UUID.) But if the name length is not a problem in Tower then I do not think we should be too concerned with it here.

If this is the only thing blocking this PR can we open an issue (and/or pivotal story) to address it in a followup PR. From a discussion with @syncrou it sounds like creating the service_template first will require changes to another existing PR as well.

bzwei · 2017-02-13T22:59:42Z

app/models/service_template_ansible_playbook.rb

+      :ask_limit_on_launch      => true,
+      :ask_inventory_on_launch  => true,
+      :ask_credential_on_launch => true
+    }.merge(info.slice(:extra_vars))


:extra_vars hash needs to be converted to JSON string

bdunne

Overall looks good.

bdunne · 2017-02-13T22:36:13Z

app/models/service_template_ansible_playbook.rb

+    Hash.new.tap do |hash|
+      [:provision, :retirement, :reconfigure].each do |action|
+        next unless config_info[action]
+        hash[action.to_sym] = { :configuration_template => create_job_template("#{service_name}_#{action}", description, config_info[action], auth_user) }


I don't think the .to_sym on action is necessary since it's already a symbol.

bdunne · 2017-02-13T22:48:32Z

app/models/service_template_ansible_playbook.rb

+    {}.tap do |hash|
+      [:provision, :retirement, :reconfigure].each do |action|
+        next unless config_info[action]
+        hash[action.to_sym] = { :configuration_template => create_job_template("#{service_name}_#{action}", description, config_info[action], auth_user) }


I don't think the action.to_sym needs the to_sym since action is already a symbol

bdunne · 2017-02-13T22:50:22Z

app/models/service_template_ansible_playbook.rb

-    # tower = playbook.manager
+  def self.create_job_templates(service_name, description, config_info, auth_user)
+    {}.tap do |hash|
+      [:provision, :retirement, :reconfigure].each do |action|


Prefer [:provision, :retirement, :reconfigure].each_with_object do |action, hash| to avoid tapping the hash above.

bdunne · 2017-02-13T23:05:08Z

spec/models/service_template_ansible_playbook_spec.rb

+      info = catalog_item_options[:config_info][:provision]
+      _tower, params = described_class.build_parameter_list(name, description, info)
+
+      expect(params[:name]).to eq name


This could be a single expectation like:

expect(params).to have_attributes( :name => name, :description => description, ...etc... )

This change allows for a Queued refresh after building the JobTemplate in the provider https://www.pivotaltracker.com/story/show/137306185

Accept an auth_user or default to 'system' as a backup Always call my_zone https://www.pivotaltracker.com/story/show/137306185

bzwei · 2017-02-14T15:36:27Z

spec/models/service_template_ansible_playbook_spec.rb

+      expect(params).to have_attributes(
+        :name               => name,
+        :description        => description,
+        :extra_vars         => nil,


should do a compact to remove nils.

bzwei · 2017-02-14T15:40:37Z

app/models/service_template_ansible_playbook.rb

  end
-  private_class_method :create_job_template


all the methods you added should be private

syncrou · 2017-02-14T16:52:20Z

@bzwei, @bdunne - I believe I have addressed all the feedback

bzwei · 2017-02-14T17:15:29Z

app/models/service_template_ansible_playbook.rb

+    [:provision, :retirement, :reconfigure].each_with_object({}) do |action, hash|
+      next unless config_info[action]
+      job_template_name = unique_job_template_name(service_name, action)
+      hash[action] = { :configuration_template => create_job_template(job_template_name.to_s, description, config_info[action], auth_user) }


.to_s is redundant

Fryguy · 2017-02-14T20:22:28Z

app/models/service_template_ansible_playbook.rb

+  private_class_method :build_parameter_list
+
+  def self.unique_job_template_name(service_name, action)
+    "#{service_name}_#{action}_#{rand(36**8).to_s(36)}"


Comment from earlier was lost on change: #13896 (comment)

The question was which of the following is best:

random value (made up from a number - current code here)

random UUID

something else that's unique but identifiable, like service_template_id.

@bdunne I believe you wanted 3, but there were questions on what is available. Can you elaborate a bit? Also, if service_template_id is available, is that sufficient for uniqueness? That is, is there a 1:1 between a service template and the job template? If so, I'd prefer 3 as well

service_template_id is not available here. The goal is to have a unique name. Combining the name with a random value/UUID, the chance of collision is minimal if not absolutely 0. If we cannot guarantee the service_template_id to be unique due to region or database reset, the chance of collision is even bigger.

Either way the issue is not a major concern for this PR.

bdunne · 2017-02-14T23:13:03Z

...odels/manageiq/providers/ansible_tower/automation_manager/configuration_script/api_create.rb

@@ -2,21 +2,23 @@ class ManageIQ::Providers::AnsibleTower::AutomationManager::ConfigurationScript
  module ApiCreate
    def create_in_provider(manager_id, params)
      manager = ExtManagementSystem.find(manager_id)
+      raise "authentication status is not ok" unless manager.authentication_status_ok?


Why do we need this? Shouldn't with_provider_connection raise and if that somehow got through, shouldn't create! raise?

This is discovered through test. You can reproduce and try a better solution. I have a valid tower but the authentication_status_ok? returns false because the appliance (dev machine) was disconnected from VPN before. The job template can be created successfully because it does not check for authentication status, but refresh cannot pass through. The created job template is useless because we cannot get it in VMDB.

We can attempt to sync the current authentication status. This line can be a temporary solution before we implement a better way.

miq-bot · 2017-02-15T15:32:30Z

Checked commits syncrou/manageiq@4d57beb~...c81dbcb 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-15T16:02:16Z

@bdunne, @gmcculloug - All requested changes are in. Can we hit the magic green button?

bdunne

👍 LGTM

syncrou changed the title ~~Create service template with tower 2~~ Create service template with Ansible Tower after first creating a new job template Feb 13, 2017

miq-bot added enhancement providers/ansible_tower labels Feb 13, 2017

miq-bot assigned bzwei Feb 13, 2017

miq-bot added the euwe/no label Feb 13, 2017

bzwei reviewed Feb 13, 2017

View reviewed changes

syncrou force-pushed the create_service_template_with_tower_2 branch 3 times, most recently from c81518e to 41c824c Compare February 13, 2017 22:43

bzwei reviewed Feb 13, 2017

View reviewed changes

bdunne requested changes Feb 13, 2017

View reviewed changes

syncrou added 3 commits February 14, 2017 10:20

Build Job Template as a queued item

4d57beb

This change allows for a Queued refresh after building the JobTemplate in the provider https://www.pivotaltracker.com/story/show/137306185

Adjust queued methods

72fcd14

Accept an auth_user or default to 'system' as a backup Always call my_zone https://www.pivotaltracker.com/story/show/137306185

Add specs around queued job_template creation

f3e8adc

syncrou force-pushed the create_service_template_with_tower_2 branch from 41c824c to f3e8adc Compare February 14, 2017 15:23

bzwei reviewed Feb 14, 2017

View reviewed changes

syncrou force-pushed the create_service_template_with_tower_2 branch from 97423f8 to aa5896e Compare February 14, 2017 20:12

Fryguy reviewed Feb 14, 2017

View reviewed changes

syncrou force-pushed the create_service_template_with_tower_2 branch from a763d81 to 86e76c6 Compare February 14, 2017 22:58

bdunne reviewed Feb 14, 2017

View reviewed changes

If we find :extra_vars set the value to json

c81dbcb

syncrou force-pushed the create_service_template_with_tower_2 branch from 86e76c6 to c81dbcb Compare February 15, 2017 15:28

bdunne approved these changes Feb 15, 2017

View reviewed changes

bzwei approved these changes Feb 15, 2017

View reviewed changes

gmcculloug assigned gmcculloug and unassigned bzwei Feb 15, 2017

gmcculloug merged commit fc185c2 into ManageIQ:master Feb 15, 2017

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

Fryguy added the providers/automation label Jan 24, 2020

Create service template with Ansible Tower after first creating a new job template #13896

Create service template with Ansible Tower after first creating a new job template #13896

Conversation

syncrou commented Feb 13, 2017 • edited Loading

syncrou commented Feb 13, 2017

syncrou commented Feb 13, 2017

syncrou commented Feb 13, 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

bdunne left a 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

syncrou commented Feb 14, 2017

Choose a reason for hiding this comment

Fryguy Feb 14, 2017 • edited Loading

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 15, 2017

syncrou commented Feb 15, 2017

bdunne left a comment

Choose a reason for hiding this comment

syncrou commented Feb 13, 2017 •

edited

Loading

Fryguy Feb 14, 2017 •

edited

Loading