Fix ServiceTemplateTransformationPlan edit action #17989

AparnaKarve · 2018-09-17T20:31:17Z

While adding a new v2v UI feature for editing a Migration Plan (ServiceTemplateTransformationPlan) in ManageIQ/manageiq-v2v#620, we noticed a couple of issues related to VM discovery, such as -

new VMs added to an existing Migration plan during Edit, were being discovered again
VMs that were being removed from an existing Migration plan during Edit, were not being discovered by other plans

Above issues have been fixed in this PR

AparnaKarve · 2018-09-17T20:31:53Z

@lfu @bzwei Can you please review?

lfu · 2018-09-18T19:35:14Z

app/models/service_template_transformation_plan/validate_config_info.rb

@@ -0,0 +1,51 @@
+module ServiceTemplateTransformationPlan::ValidateConfigInfo
+  def self.included(base)


Please use ActiveSupport::Concern.

Added that. Thanks.

AparnaKarve · 2018-09-18T23:24:18Z

@lfu In the last commit, I have handled a use case where

miq_requests exist
or
only the basic attributes (like name/description) are supplied by the UI, and options[:config_info] is nil.

This use case covers editing plans that are completed or archived.
For plans that are completed or archived, the config_info cannot be altered, and all we can edit are the basic attributes like name/description.

AparnaKarve · 2018-09-19T20:00:37Z

@mturley Thank you for your testing so far!

mturley · 2018-09-20T17:55:34Z

@lfu @bzwei sorry to bother you guys, but is there anything we can do to move this along? it's blocking ManageIQ/manageiq-v2v#620 and we have a code freeze on Monday.

AparnaKarve · 2018-09-20T18:23:29Z

@lfu had a suggestion that for updating a plan which has not been executed yet, could we just delete the old resources, then add the new resource and update the config info -- which is a great suggestion and in fact what I initially intended to do.
But I was just not sure if we should be doing vm_resources.destroy_all ... to delete the old resources.

Anyway I tried this, which seems to work -

def update_catalog_item(options, auth_user = nil)
  if miq_requests.any? || options[:config_info].nil?
    options[:config_info] = self.options[:config_info]
  end

  added_vms_enhanced_config_info = validate_config_info(options)

  transaction do
    update_from_options(options)
    vm_resources.destroy_all
    reload.tap do |service_template|
      service_template.add_resource(added_vms_enhanced_config_info[:transformation_mapping])
      added_vms_enhanced_config_info[:vms].each { |vm_hash| service_template.add_resource(vm_hash[:vm], :status => ServiceResource::STATUS_QUEUED, :options => vm_hash[:options]) }
      service_template.create_resource_actions(added_vms_enhanced_config_info)
    end
  end
end

@lfu What do you think of the above method?

lfu · 2018-09-20T18:57:30Z

def update_catalog_item(options, auth_user = nil)
  if miq_requests.any? || options[:config_info].nil?

What's this for? Is this checking necessary? I thought options[:config_info] should be populated with data unless someone is going to remove all previous selections.

    options[:config_info] = self.options[:config_info]
  end

  added_vms_enhanced_config_info = validate_config_info(options)

  transaction do
    update_from_options(options)
    vm_resources.destroy_all
    reload.tap do |service_template|

Do we need reload here?

      service_template.add_resource(added_vms_enhanced_config_info[:transformation_mapping])

Need to remove the old transformation_mapping resource first.

      added_vms_enhanced_config_info[:vms].each { |vm_hash| service_template.add_resource(vm_hash[:vm], :status => ServiceResource::STATUS_QUEUED, :options => vm_hash[:options]) }
      service_template.create_resource_actions(added_vms_enhanced_config_info)

Should this be service_template.update_resource_actions

AparnaKarve · 2018-09-20T21:55:38Z

I have updated with a working version of what we discussed outside of the PR.

Once we finalize on the code, I can squash some of the related commits.

bzwei · 2018-09-21T13:29:51Z

app/models/service_template_transformation_plan.rb

-    pre_service_id  = config_info[:pre_service].try(:id) || config_info[:pre_service_id]
-    post_service_id = config_info[:post_service].try(:id) || config_info[:post_service_id]
+  def update_catalog_item(options, _auth_user = nil)
+    if miq_requests.any? || options[:config_info].nil?


if miq_requests exists, do you allow an update?
If requests have been made and there were failed VMs, then user may want a retry. Before a retry they may want to update the VM list. Therefore it seems update should be allowed. But what about request is on-going? I think it is a question to PM.
Anyway, if it is not allowed, you should raise an error here. If allowed, you should use new options, not the existing one.

Currently we are allowing Edits that includes Name/Description and VMs on only those plans that haven't been executed yet.
We won't be allowing editing the associated infrastructure mapping.

Edits won't be allowed for plans in-progress.

As far as failed and completed plans go, we would be supporting only partial edits that will let the user edit attributes like Name/Description

@mturley can you corroborate that?

If you only allow name/description changes for certain cases, you should not continue to update other fields by deleting vms and add them back. It will have unexpected result if there is a request running, or even the plan has completed or failed.

You said "Edits won't be allowed for plans in-progress", but I don't see it is reflected in the code. Maybe on UI it is not allowed, but it should be forbidden in backend also.

@AparnaKarve is correct, for failed and completed plans, the UI as designed only supports editing the name and description.

I do want to point out that although we currently don't allow editing the associated infrastructure mapping, that is mostly a UI restriction (it complicates the state of the wizard), and @vconzola mentioned we might want to allow that in the future (which basically would mean replacing an entire plan).

@bzwei Addressed some of your above concerns in the last update (last commit)
Can you verify?

To confirm when things can and cannot be edited, see this table: https://docs.google.com/document/d/1B-iFdNKqUgw44GPHc0TIUEDc9MdKiBll2pK6PTve__M/edit

bzwei · 2018-09-21T13:34:44Z

app/models/service_template_transformation_plan.rb

+      vm_resources.destroy_all
+      reload.tap do |service_template|
+        added_vms_enhanced_config_info[:vms].each { |vm_hash| service_template.add_resource(vm_hash[:vm], :status => ServiceResource::STATUS_QUEUED, :options => vm_hash[:options]) }
+        service_template.create_resource_actions(added_vms_enhanced_config_info)


do you need to create_resource_actions every time? If yes, shouldn't you delete them first?

Do we have a method for deleting resource actions? If yes, then I think we should call it here.

Although note that we only want to delete the vm resources and rebuild them.
As mentioned above #17989 (comment), we don't want to update (and hence delete) the associated infrastructure mapping.

I do need create_resource_actions every time, otherwise the ServiceResource records associated with the plan are not created.
Keeping the ServiceResource records up-to-date is critical for accurate VM discovery.

From what I have observed so far it looks like the ServiceResource records that were previously associated with a plan are auto-destroyed when a call to create_resource_actions is made -- so we do not need an explicit delete in this case.

AparnaKarve · 2018-09-24T21:04:37Z

@lfu Please review the latest update.

If all looks good, I would like to squash all the commits related to update_catalog_item

bzwei · 2018-09-24T21:08:35Z

@jerryk55 @roliveri please review

blomquisg · 2018-09-25T20:57:39Z

app/models/service_template_transformation_plan/validate_config_info.rb

+      raise _('Must provide an existing transformation mapping') if mapping.blank?
+
+      pre_service_id  = config_info[:pre_service].try(:id) || config_info[:pre_service_id]
+      post_service_id = config_info[:post_service].try(:id) || config_info[:post_service_id]


I believe that @michaelkro might be including other details in the config_info including the OSP security group and the OSP flavor.

Though it might need to be in the actions section below. Be sure to follow up and see where that needs to be added.

Good point.

This code is a direct transplant from service_template_transformation_plan.rb -- so there were no changes made to the existing behavior as such. Although adding any OSP specific info in this PR itself may not be a bad idea.

@bzwei Let me know if any OSP specific info needs to be added in actions?

Currently we have an outstanding UI PR - ManageIQ/manageiq-v2v#658 where we are adding osp_security_group and osp_flavor to actions as part of the POST body for service_templates

@bzwei Is there anything that needs to be done here with regard to the 2 new additions - osp_security_group and osp_flavor ?

osp_ options do not impact actions, so no need to update actions
They will need to be updated in service_resource per vm. I would recommend another PR for both add/update such options.

blomquisg · 2018-09-25T20:59:22Z

@bzwei are you 👍 on this? Or, just don't have time to review? I just want to be sure where it currently stands.

bzwei · 2018-09-25T21:46:11Z

app/models/service_template_transformation_plan.rb

@@ -65,46 +66,23 @@ def self.create_catalog_item(options, _auth_user = nil)
    end
  end

-  private
+  def update_catalog_item(options, _auth_user = nil)
+    raise _("Editing a plan in progress is prohibited") if %w(active pending).include?(miq_requests.last.try(:request_state))


Without proper sorting miq_requests.last does not guarantee to be the latest request.

Addressed this in the last update

bzwei · 2018-09-25T21:50:53Z

app/models/service_template_transformation_plan.rb

-      :provision              => config_info[:provision] || {}
-    }
+    transaction do
+      vm_resources.destroy_all


Are we allowed to change the selection of infrastructure mapping? why not?

We don't allow editing the associated infrastructure mapping.

I think the rationale here is that if one needs to change the infrastructure mapping for a plan, then creating a brand new plan with the desired infra mapping is a better approach.
Plus it's easier to deal with a static infrastructure mapping in the Wizard... as @mturley mentioned above #17989 (comment), it complicates the state of the wizard.

UI may forbid updating the infrastructure mapping, but what about API or a direct call for update that includes a changed mapping? We should either log a warning or raise an error if such change is detected.

I re-read #17989 (comment), and I think that we should consider supporting the infrastructure mapping update purely from a backend standpoint (API or a direct call, like you said)

UI may forbid updating it today, but that restriction may not apply in the future (based on the comment)

added support for updating associated transformation mapping

bzwei · 2018-09-26T13:20:35Z

Need @roliveri or @jerryk55 final review before merging.

AparnaKarve · 2018-09-26T15:40:41Z

@miq_bot add_label hammer/yes

`validate_config_info` needs to be a Class method for Create and Instance method for Update

and edit transformation mapping

miq-bot · 2018-09-27T15:41:57Z

Checked commits AparnaKarve/manageiq@c132327~...8a8bd75 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

AparnaKarve · 2018-09-27T16:32:19Z

In the latest update here, I have consolidated (squashed) some of the related commits.

@bzwei @lfu Thank you for the review.

@gmcculloug / @blomquisg This should be good to go

AparnaKarve · 2018-09-27T16:49:34Z

@miq-bot add_label hammer/yes

AparnaKarve · 2018-09-28T16:41:54Z

@roliveri We have some frontend work depending on this PR.
Can you review and merge?

…formation_plan_edit Fix ServiceTemplateTransformationPlan edit action (cherry picked from commit e89f4a1)

simaishi · 2018-10-02T13:19:38Z

Hammer backport details:

$ git log -1
commit e658b2ac875faf710a2eccac953cb9fca1d63539
Author: Richard Oliveri <[email protected]>
Date:   Mon Oct 1 13:30:10 2018 -0400

    Merge pull request #17989 from AparnaKarve/fix_service_template_transformation_plan_edit
    
    Fix ServiceTemplateTransformationPlan edit action
    
    (cherry picked from commit e89f4a1ceb13088d2fde1157a31491e8e6d4a95d)

AparnaKarve mentioned this pull request Sep 17, 2018

[#607] Edit an existing RHV migration plan ManageIQ/manageiq-v2v#620

Merged

10 tasks

lfu reviewed Sep 18, 2018

View reviewed changes

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from 1612d90 to f9e5eaf Compare September 18, 2018 23:20

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch 3 times, most recently from 9607c23 to 1f0f425 Compare September 19, 2018 19:56

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from 1f0f425 to caaf4dc Compare September 19, 2018 23:29

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from caaf4dc to cb85190 Compare September 20, 2018 21:54

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from cb85190 to e245d0c Compare September 20, 2018 21:58

bzwei reviewed Sep 21, 2018

View reviewed changes

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from e245d0c to d3dbdfb Compare September 21, 2018 16:43

This was referenced Sep 24, 2018

Edit an existing Migration Plan ManageIQ/manageiq-v2v#607

Closed

[#641] Add EditPlanNameModal for editing name and description of completed plans ManageIQ/manageiq-v2v#663

Merged

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from d3dbdfb to 2a685fe Compare September 24, 2018 21:01

blomquisg reviewed Sep 25, 2018

View reviewed changes

bzwei reviewed Sep 25, 2018

View reviewed changes

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from 2a685fe to f1429c3 Compare September 25, 2018 22:36

bzwei approved these changes Sep 27, 2018

View reviewed changes

Extract validate_config_info in a module

c132327

`validate_config_info` needs to be a Class method for Create and Instance method for Update

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from db80951 to 711d199 Compare September 27, 2018 15:28

AparnaKarve added 2 commits September 27, 2018 08:35

Add instance method update_catalog_item

8db4aba

specs for all edit use cases - add, remove, add & remove VMs

8a8bd75

and edit transformation mapping

AparnaKarve force-pushed the fix_service_template_transformation_plan_edit branch from 711d199 to 8a8bd75 Compare September 27, 2018 15:38

miq-bot added the hammer/yes label Sep 27, 2018

gmcculloug assigned roliveri Sep 27, 2018

This was referenced Sep 28, 2018

[#607] Make OSP Instance Properties step compatible with Edit Plan and enable Edit Plan ManageIQ/manageiq-v2v#674

Merged

Enable Edit Plan menu item ManageIQ/manageiq-v2v#675

Closed

roliveri approved these changes Oct 1, 2018

View reviewed changes

roliveri merged commit e89f4a1 into ManageIQ:master Oct 1, 2018

AparnaKarve deleted the fix_service_template_transformation_plan_edit branch October 1, 2018 17:32

simaishi pushed a commit that referenced this pull request Oct 2, 2018

Merge pull request #17989 from AparnaKarve/fix_service_template_trans…

e658b2a

…formation_plan_edit Fix ServiceTemplateTransformationPlan edit action (cherry picked from commit e89f4a1)

simaishi added hammer/backported and removed hammer/yes labels Oct 2, 2018

mfeifer added this to the Sprint 96 Ending Oct 8, 2018 milestone Jan 27, 2020

		@@ -0,0 +1,51 @@
		module ServiceTemplateTransformationPlan::ValidateConfigInfo
		def self.included(base)

Fix ServiceTemplateTransformationPlan edit action #17989

Fix ServiceTemplateTransformationPlan edit action #17989

Conversation

AparnaKarve commented Sep 17, 2018

AparnaKarve commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Sep 18, 2018

AparnaKarve commented Sep 19, 2018

mturley commented Sep 20, 2018

AparnaKarve commented Sep 20, 2018

lfu commented Sep 20, 2018

AparnaKarve commented Sep 20, 2018 • 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

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

AparnaKarve Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

AparnaKarve commented Sep 24, 2018 • edited Loading

bzwei commented Sep 24, 2018

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

blomquisg commented Sep 25, 2018

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

bzwei commented Sep 26, 2018

AparnaKarve commented Sep 26, 2018

miq-bot commented Sep 27, 2018

AparnaKarve commented Sep 27, 2018

AparnaKarve commented Sep 27, 2018

AparnaKarve commented Sep 28, 2018

simaishi commented Oct 2, 2018

AparnaKarve commented Sep 20, 2018 •

edited

Loading

AparnaKarve Sep 21, 2018 •

edited

Loading

AparnaKarve commented Sep 24, 2018 •

edited

Loading