Add Service resource linking. #16082

lfu · 2017-10-02T13:18:58Z

Link provisioned VMs to the service.

@miq-bot assign @gmcculloug
@miq-bot add_label service, enhancement, fine/no

miq-bot · 2017-10-02T13:19:10Z

@lfu Cannot apply the following label because they are not recognized: service

bzwei · 2017-10-02T14:46:04Z

app/models/service.rb

@@ -65,6 +65,7 @@ class Service < ApplicationRecord
  include TenancyMixin
  include SupportsFeatureMixin
  include Metric::CiMixin
+  include ResourceLinking


Shall we name it ResourceLinkingMixin similar to lines above?

I was suggesting we use include_concern here.

bzwei · 2017-10-02T14:48:53Z

app/models/service/resource_linking.rb

+module Service::ResourceLinking
+  extend ActiveSupport::Concern
+
+  def add_provider_vms_queue(provider, array_of_uid_ems)


So this implementation intents to add resources by type? Next time if we want to link networks, we need to introduce new method add_provider_networks_queue and copy the same logic?

bzwei · 2017-10-02T14:49:57Z

app/models/service/resource_linking.rb

+      :userid => evm_owner.userid
+    }
+    queue_opts = {
+      :class_name  => "Service",


You can still use self.class.name to avoid hard coding.

bzwei · 2017-10-02T14:57:07Z

app/models/service/resource_linking.rb

+    array_of_uid_ems = array_of_uid_ems.kind_of?(Array) ? array_of_uid_ems.compact.uniq : [array_of_uid_ems]
+
+    if Vm.where(:uid_ems => array_of_uid_ems).length != array_of_uid_ems.length
+      task_ids = EmsRefresh.queue_refresh(provider, nil, :create_task => true)


May need to consult with the provider team whether we can avoid a full refresh.

bzwei · 2017-10-02T14:58:17Z

app/models/service/resource_linking.rb

+    found_vms.each { |vm| add_resource!(vm) }
+
+    not_found_vms = array_of_uid_ems - found_vms.pluck(:uid_ems)
+    _log.info("VMs not found to link to service ID [#{id}], name [#{name}]: #{not_found_vms.join("\n")}") unless not_found_vms.blank?


Consider increase the log level to warn or error

gmcculloug · 2017-10-02T15:27:59Z

app/models/service/resource_linking.rb

+      task_ids = EmsRefresh.queue_refresh(provider, nil, :create_task => true)
+
+      # Wait for the tasks to finish
+      task_ids.each { |tid| MiqTask.wait_for_taskid(tid) }


We should not be waiting for the provider refresh synchronously, instead it should be re-queueing the work and checking. Some providers could take more then 10 mins to complete a refresh. This also means we cannot use MiqTask.generic_action_with_callback.

cc @mkanoor

lfu · 2017-10-05T19:39:42Z

@gmcculloug @mkanoor @bzwei Please review the new logic. I will work on the specs.

gmcculloug · 2017-10-05T20:04:38Z

app/models/service/resource_linking.rb

+    _log.info("NAME [#{options[:name]}] for user #{evm_owner.userid}")
+
+    job = Service::LinkingWorkflow.create_job(options)
+    return job


No need for the return job line, the result of Service::LinkingWorkflow.create_job will be returned from the method unless it is raising an error.

The previous line should just be: Service::LinkingWorkflow.create_job(options)

gmcculloug · 2017-10-05T20:09:27Z

app/models/service/resource_linking.rb

+    raise _("no target_id defined for linking targets to service") unless options[:target_id]
+
+    array_of_uid_ems = options[:array_of_uid_ems]
+    options[:array_of_uid_ems] = array_of_uid_ems.kind_of?(Array) ? array_of_uid_ems.compact.uniq : [array_of_uid_ems]


These two lines can be simplified:
options[:array_of_uid_ems] = Array(options[:array_of_uid_ems]).compact.uniq

gmcculloug · 2017-10-05T20:17:37Z

app/models/service/resource_linking.rb

+  def add_provider_vms(options)
+    # required keys in options hash:
+    #   :target_id, the id of the provider to refresh
+    #   :array_of_uid_ems, an array of VMs' uid_ems


Does the external caller to the API need to know this name? Suggest using uid_ems_array.

Whoever calls this method needs to pass in the two parameters: target_id and uid_ems_array.

bzwei · 2017-10-05T20:37:03Z

app/models/service/linking_workflow.rb

+      }
+    end
+
+    def dispatch_start


I would rather not override this method. We can explicitly call start before return the job

bzwei · 2017-10-05T21:25:19Z

@lfu you can do a target refresh based on ems_ref, see example https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/cloud_manager/provision/state_machine.rb#L57

gmcculloug · 2017-10-06T16:35:36Z

app/models/service/resource_linking.rb

+  def add_provider_vms(options)
+    # required keys in options hash:
+    #   :target_id, the id of the provider to refresh
+    #   :uid_ems_array, an array of VMs' uid_ems


Why are we taking these as a hash and not just normal parameters? Seems we are doing the raise in normalize_inputs! for target_id when it should just be a normal argument (or keyword args). I realize you are merging the options but the existing normalize_inputs! method could return the hash that gets merged.

Maybe something like: add_provider(target_id:, uid_ems_array:) but depends on what the API is passing.

@jntullo Are you passing the provider object or just the ID from the API?

@gmcculloug I was passing in the actual provider object with the :uid_ems_array but see that it has changed

If we need the zone from the provider we'll need to load the object. Let's understand that first before we change what you are passing.

gmcculloug · 2017-10-06T16:40:11Z

app/models/service/resource_linking.rb

+      :userid       => evm_owner.userid,
+      :sync_key     => guid,
+      :service_id   => id,
+      :zone         => my_zone


Is zone needed for the provider refresh? If so this should be the providers zone not the service. Otherwise it would be good to eliminate all together.

This zone is for the new job, not for refresh.

gmcculloug · 2017-10-06T16:45:46Z

app/models/service/resource_linking.rb

+    raise _("no target_id defined for linking to service") unless options[:target_id]
+
+    uid_ems_array = options[:uid_ems_array] || []
+    options[:uid_ems_array] = uid_ems_array.kind_of?(Array) ? uid_ems_array.compact.uniq : [uid_ems_array]


Not sure if the suggestion to reduce this to these two lines to the follow was lost in the comments or if you are preferring the current approach.

Suggested change: options[: uid_ems_array] = Array(options[: uid_ems_array]).compact.uniq

gmcculloug

One minor comment but overall this looks good.

@bzwei @mkanoor Please take another look.

gmcculloug · 2017-10-10T17:29:40Z

app/models/service/resource_linking.rb

+  extend ActiveSupport::Concern
+
+  def add_provider_vms(provider, uid_ems_array)
+    vm_id_array = Array(uid_ems_array).compact.uniq


Minor, but the name vm_id_array is misleading as it is the uid that is expected here.

bzwei · 2017-10-10T21:14:07Z

app/models/service/linking_workflow.rb

+          :id => id, :name => name, :service_id => options[:service_id]
+        }
+        _log.error(msg)
+        signal(:abort, msg, 'error')


I don't see :abort, but only :abort_job or :error.

Never mind. I saw it in the state_machine mixin.

miq-bot · 2017-10-11T14:08:08Z

Checked commits lfu/manageiq@eac5d1f~...4146352 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 1 offense detected

app/models/service/linking_workflow.rb

❗ - Line 49, Col 5 - Style/Alias - Use alias instead of alias_method in a class body.

bzwei · 2017-10-12T14:55:42Z

spec/models/service/linking_workflow_spec.rb

+      :service_id    => service.id
+    }
+  end
+  let(:job) { described_class.create_job(options) }


Will the job be automatically deleted after test?

miq-bot assigned gmcculloug Oct 2, 2017

miq-bot added enhancement fine/no labels Oct 2, 2017

lfu force-pushed the service_resource_linking branch from ad3eb90 to 7847796 Compare October 2, 2017 13:55

gmcculloug requested review from mkanoor and bzwei October 2, 2017 14:20

bzwei reviewed Oct 2, 2017

View reviewed changes

gmcculloug reviewed Oct 2, 2017

View reviewed changes

jntullo mentioned this pull request Oct 3, 2017

Need the ability to add just created vms to a service ManageIQ/manageiq-api#82

Closed

lfu force-pushed the service_resource_linking branch from 7847796 to 7b07d99 Compare October 5, 2017 19:35

gmcculloug reviewed Oct 5, 2017

View reviewed changes

lfu force-pushed the service_resource_linking branch from 7b07d99 to d8efbb5 Compare October 5, 2017 20:27

bzwei reviewed Oct 5, 2017

View reviewed changes

lfu force-pushed the service_resource_linking branch from d8efbb5 to 5b04bcc Compare October 6, 2017 15:17

gmcculloug reviewed Oct 6, 2017

View reviewed changes

lfu force-pushed the service_resource_linking branch 6 times, most recently from 0f99bf4 to 03d6ca6 Compare October 10, 2017 13:43

gmcculloug reviewed Oct 10, 2017

View reviewed changes

lfu force-pushed the service_resource_linking branch from 03d6ca6 to f8bb682 Compare October 10, 2017 18:22

gmcculloug approved these changes Oct 10, 2017

View reviewed changes

bzwei reviewed Oct 10, 2017

View reviewed changes

lfu added 2 commits October 11, 2017 10:03

Add Service::LinkingWorkflow.

eac5d1f

Add Service::ResourceLinking.

4146352

lfu force-pushed the service_resource_linking branch from f8bb682 to 4146352 Compare October 11, 2017 14:03

bzwei approved these changes Oct 11, 2017

View reviewed changes

mkanoor approved these changes Oct 12, 2017

View reviewed changes

gmcculloug merged commit d4da91c into ManageIQ:master Oct 12, 2017

gmcculloug added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 12, 2017

bzwei reviewed Oct 12, 2017

View reviewed changes

gmcculloug mentioned this pull request Oct 12, 2017

add_provider_vms action for Services ManageIQ/manageiq-api#100

Merged

lfu deleted the service_resource_linking branch October 16, 2017 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Service resource linking. #16082

Add Service resource linking. #16082

lfu commented Oct 2, 2017

miq-bot commented Oct 2, 2017

bzwei Oct 2, 2017

gmcculloug Oct 2, 2017

bzwei Oct 2, 2017

bzwei Oct 2, 2017

bzwei Oct 2, 2017

bzwei Oct 2, 2017

gmcculloug Oct 2, 2017

lfu commented Oct 5, 2017

gmcculloug Oct 5, 2017

gmcculloug Oct 5, 2017

gmcculloug Oct 5, 2017

lfu Oct 6, 2017

bzwei Oct 5, 2017

bzwei commented Oct 5, 2017

gmcculloug Oct 6, 2017

jntullo Oct 6, 2017

gmcculloug Oct 6, 2017

gmcculloug Oct 6, 2017

lfu Oct 9, 2017

gmcculloug Oct 6, 2017

gmcculloug left a comment

gmcculloug Oct 10, 2017

bzwei Oct 10, 2017

bzwei Oct 10, 2017

miq-bot commented Oct 11, 2017

bzwei Oct 12, 2017

Add Service resource linking. #16082

Add Service resource linking. #16082

Conversation

lfu commented Oct 2, 2017

miq-bot commented Oct 2, 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

lfu commented Oct 5, 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

bzwei commented Oct 5, 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

gmcculloug 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

miq-bot commented Oct 11, 2017

Choose a reason for hiding this comment