[V2V] Datastore validation in Transformation Mapping #19204

thearifismail · 2019-08-26T17:32:21Z

This PR validates that the source and target datastores (storages) are associated with the parent EmsCluster. This time around the validation is done as a part of after_create step. This was necessary to work with the transformation_mapping API

Target Datastores
RHV: Class Storage, Filters: Must belong to target cluster
OSP: Class CloudVolume, Filters: Must belong to target CloudTenant's ExtManagementSystem

Source Datastores
VMW: Class Storage, Filters: Must belong to one of the source clusters

https://trello.com/c/1x31IdD0/241-spike-mapping-and-plan-api-validation-of-input
https://bugzilla.redhat.com/show_bug.cgi?id=1666840

…ster

thearifismail · 2019-08-26T17:36:47Z

@miq-bot add_reviewer @djberg96 @agrare

miq-bot · 2019-08-26T17:37:17Z

@thearifismail 'djberg96 agrare' is an invalid reviewer, ignoring...

thearifismail · 2019-08-26T17:48:38Z

@miq-bot add_reviewer @djberg96

thearifismail · 2019-08-26T17:56:56Z

@miq-bot add_labels transformation, ivanchuk/yes, hammer/yes

miq-bot · 2019-08-26T17:57:14Z

@thearifismail Cannot apply the following label because they are not recognized: ivanchuck/yes

agrare · 2019-08-26T18:53:50Z

app/models/transformation_mapping_item.rb

+  # cleanup if transformation mapping or any of its items are invalid
+  def cleanup (tm)
+    tmis = TransformationMappingItem.where(:tranformation_mapping_id => tm.id)
+    tm.delete


I don't think this type of cleanup is something you want to do within a validation block...rails will rollback the transaction if something fails validation cc @djberg96

The issue is that in order to perform the storage validation we need the parent TransformationMapping object first, that's why it's being done as an after_create, so it's not a baked in validation, but rather our custom handler.

If there's a way combine a Rails validation callback with after_create that I'm not aware of, please let me know.

I tried adding validate after after_create but the Transformation_Mappingbut the validation would fail because the TransformationMapping API implementation could not handle the new implementation. That's why I had to customize the cleanup to ensure their cleanup.

The issue is that in order to perform the storage validation we need the parent TransformationMapping object first

Is that not available when using validate ? I took this branch and changed it from after_create to validate and deleted the calls to cleanup and I can see the parent transformation_mapping in the validation block.

the TransformationMapping API implementation could not handle the new implementation. That's why I had to customize the cleanup to ensure their cleanup.

An aside, It is a little odd that validation on a single sub-item will destroy the entire tree above it IMO, its not obvious to me that this would happen.

Another aside, if you did tm.destroy with proper cascade delete you shouldn't have to individually .delete the child records. Calling just .delete also has the high likelihood of leaving orphaned records. When in doubt use .destroy

Replaced tm.delete with tm.destroy and removed the explicit cleanup method. Could not test the new implementation using the WebUI and byebug and relied on testing in rails console using tm = TransformationMapping.find(<ID>) and then tm.destory. Veried that all the transformation_mapping_items in the plan were deleted.

…troy and removed explicit cleanup method.

agrare · 2019-08-26T23:42:41Z

app/models/transformation_mapping_item.rb


  validate :source_cluster,      :if => -> { source.kind_of?(EmsCluster) }
  validate :destination_cluster, :if => -> { destination.kind_of?(EmsCluster) || destination.kind_of?(CloudTenant) }

+  after_create :validate_source_datastore,    :if => -> { source.kind_of?(Storage) }
+  after_create :validate_destination_datastore, :if => -> { destination.kind_of?(Storage) || destination.kind_of?(CloudVolume) }


@thearifismail can you share the workflow or reproduce in specs the situation where you don't have access to the transformation_mapping in the validate_source_datastore or validate_destination_datastore methods when being used as validate instead of after_create?

I tested this in specs which is the create the transformation mapping, then create the individual items as well as the initializing without creating the transformation_mapping and all the items then .save! the top level which saves the associations in one transaction.

Both worked as expected, I had access to the transformation_mapping from the validation method just in the latter case it didn't have an ID yet but when the validation failed it rolled back the TransformationMapping record.

agrare · 2019-08-26T23:43:27Z

app/models/transformation_mapping_item.rb

+
+    unless src_cluster_storages.include?(source_storage)
+      errors.add(:source, "Source cluster storages must include source storage: #{source_storage}")
+      tm.destroy


I still think it is weird that a validation on a sub-item will destroy the whole tree

A transformation map has three pairs of source and destination items. They are EmsClusters, Storages, and Lans. For a transformation map to work, all six have to be valid. A single invalid item renders all others useless. Say clusters, storages, and source lan validation was successful but the destination lan was bad, all the previous 5 items become useless and have to be purged. That's why I have implemented this way. If you can suggest a more elegant way, please share with me.

If you can suggest a more elegant way, please share with me.

Absolutely, if you make this a validation instead of an after_create and you save all in one transaction then:

>> vc = ManageIQ::Providers::Vmware::InfraManager.first >> rhv = ManageIQ::Providers::Redhat::InfraManager.first >> source_cluster = vc.ems_clusters.first >> dest_cluster = rhv.ems_clusters.first >> source_lan = vc.lans.first >> dest_lan = rhv.lans.first >> source_storage = Storage.create!(:name => "unattached storage") >> dest_storage = rhv.storages.first >> tm = TransformationMapping.new(:name => "my transformation") >> tm.transformation_mapping_items.new(:source => source_cluster, :destination => dest_cluster) >> tm.transformation_mapping_items.new(:source => source_lan, :destination => dest_lan) >> tm.transformation_mapping_items.new(:source => source_storage, :destination => dest_storage) >> tm.save! (0.4ms) BEGIN (0.3ms) ROLLBACK ActiveRecord::RecordInvalid (Validation failed: TransformationMapping: Transformation mapping items is invalid)

So any validation failures prevent the entire mapping from being save:

>> TransformationMapping.first => nil

If you can't save the whole mapping&items in one transaction then I recommend having the caller catch the exception and destroy the mapping:

>> tm = TransformationMapping.create!(:name => "my transformation") >> tm.transformation_mapping_items.create!(:source => source_cluster, :destination => dest_cluster) >> tm.transformation_mapping_items.create!(:source => source_lan, :destination => dest_lan) >> begin >> tm.transformation_mapping_items.create!(:source => source_storage, :destination => dest_storage) >> rescue ActiveRecord::RecordInvalid >> tm.destroy! >> end

djberg96 · 2019-08-27T14:08:54Z

@agrare Looks like we need an update on the api side: ManageIQ/manageiq-api#664

agrare · 2019-08-27T14:15:19Z

That's an interesting way to do it, you could also use the association to create it.

So instead of effectively doing:

tmi = TransformationMappingItem.new(mapping, source, dest)
mapping.transformation_mapping_items.append(tmi)
mapping.save!

This is equivalent to

mapping.transformation_mappint_items.create!(source, dest)

djberg96 · 2019-08-27T14:25:22Z

@agrare Multiple ways to skin a cat I guess, I just went with what was already there. :)

agrare · 2019-08-27T14:27:45Z

Yeah, but definitely if you didn't give it any context of the mapping then it wouldn't be available in the validation method. So given that can we change this to validate and drop the tm.destroy?

djberg96 · 2019-08-27T14:31:05Z

@agrare Oh, maybe I wasn't sure what you were responding to. Once the api change is made, we should be able to change this back to a standard validation.

agrare · 2019-08-27T14:32:56Z

👍 sounds good

agrare · 2019-08-27T14:33:44Z

And yeah wasn't saying your PR was weird, just the way it was done already was strange

app/models/transformation_mapping_item.rb

agrare · 2019-08-27T20:56:32Z

@thearifismail looking good, couple small comments and can you add specs covering the case where the the mapping item is invalid

…or invalid mapping items

miq-bot · 2019-08-28T15:56:51Z

Checked commits thearifismail/manageiq@31081c8~...50c0ca0 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

[V2V] Datastore validation in Transformation Mapping

agrare · 2019-08-28T16:49:10Z

Thanks @thearifismail !

There are going to be some other dependencies to get this backported to hammer cleanly, can you list the other PRs that need to be backported as well for @simaishi

thearifismail · 2019-08-28T19:33:57Z

PRs:
#19220
ManageIQ/manageiq-api#664
#19168
#18986

[V2V] Datastore validation in Transformation Mapping (cherry picked from commit 3ea5480) https://bugzilla.redhat.com/show_bug.cgi?id=1768517

simaishi · 2019-11-04T17:57:17Z

Ivanchuk backport details:

$ git log -1
commit 3f62e68babd523f2464dda17868395c08426435e
Author: Adam Grare <agrare@redhat.com>
Date:   Wed Aug 28 12:47:43 2019 -0400

    Merge pull request #19204 from thearifismail/new_dev_5
    
    [V2V] Datastore validation in Transformation Mapping
    
    (cherry picked from commit 3ea54802461a28a5687e5e1fcf3a7a4947a569b9)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1768517

new datastore validations to ensure their association with the EmsClu…

31081c8

…ster

thearifismail mentioned this pull request Aug 26, 2019

[V2V] Added datastore validation for creating infra mapping #18835

Closed

miq-bot added the wip label Aug 26, 2019

thearifismail changed the title ~~[WIP][V2V] Datastore validation in Transformation Mapping~~ [V2V] Datastore validation in Transformation Mapping Aug 26, 2019

miq-bot requested a review from djberg96 August 26, 2019 17:49

miq-bot removed the wip label Aug 26, 2019

miq-bot added hammer/yes v2v labels Aug 26, 2019

agrare self-assigned this Aug 26, 2019

agrare reviewed Aug 26, 2019

View reviewed changes

miq-bot added the ivanchuk/yes label Aug 26, 2019

resplaced TransformationMapping.delete with TransformationMapping.des…

3cddd9f

…troy and removed explicit cleanup method.

agrare reviewed Aug 26, 2019

View reviewed changes

djberg96 mentioned this pull request Aug 27, 2019

Explicitly pass transformation_mapping when generating mapping items ManageIQ/manageiq-api#664

Closed

removed explicit destroy calls not needed anymore

fd5f595

agrare reviewed Aug 27, 2019

View reviewed changes

app/models/transformation_mapping_item.rb Outdated Show resolved Hide resolved

agrare reviewed Aug 27, 2019

View reviewed changes

app/models/transformation_mapping_item.rb Show resolved Hide resolved

replaced flatten calls with more efficient flat_map and added specs f…

50c0ca0

…or invalid mapping items

agrare approved these changes Aug 28, 2019

View reviewed changes

agrare added a commit that referenced this pull request Aug 28, 2019

Merge pull request #19204 from thearifismail/new_dev_5

3ea5480

[V2V] Datastore validation in Transformation Mapping

agrare merged commit 50c0ca0 into ManageIQ:master Aug 28, 2019

agrare added this to the Sprint 119 Ending Sep 2, 2019 milestone Aug 28, 2019

thearifismail mentioned this pull request Aug 28, 2019

[V2V] Lan validation in Transformation Mapping #19220

Merged

lpichler mentioned this pull request Aug 29, 2019

Create TransformationMapping before Item (FIX CI FAILURE) ManageIQ/manageiq-api#666

Merged

agrare mentioned this pull request Nov 1, 2019

[V2V] Refactor ServiceTemplateTransformationPlanTask specs. #19168

Merged

simaishi pushed a commit that referenced this pull request Nov 4, 2019

Merge pull request #19204 from thearifismail/new_dev_5

3f62e68

[V2V] Datastore validation in Transformation Mapping (cherry picked from commit 3ea5480) https://bugzilla.redhat.com/show_bug.cgi?id=1768517

simaishi added ivanchuk/backported and removed ivanchuk/yes labels Nov 4, 2019

Fryguy added hammer/yes? and removed hammer/yes labels Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V2V] Datastore validation in Transformation Mapping #19204

[V2V] Datastore validation in Transformation Mapping #19204

thearifismail commented Aug 26, 2019 •

edited by simaishi

Loading

thearifismail commented Aug 26, 2019

miq-bot commented Aug 26, 2019

thearifismail commented Aug 26, 2019

thearifismail commented Aug 26, 2019 •

edited

Loading

miq-bot commented Aug 26, 2019

agrare Aug 26, 2019

djberg96 Aug 26, 2019

thearifismail Aug 26, 2019 •

edited

Loading

agrare Aug 26, 2019

agrare Aug 26, 2019

thearifismail Aug 26, 2019

agrare Aug 26, 2019

agrare Aug 26, 2019

thearifismail Aug 27, 2019

agrare Aug 27, 2019

djberg96 commented Aug 27, 2019

agrare commented Aug 27, 2019

djberg96 commented Aug 27, 2019

agrare commented Aug 27, 2019

djberg96 commented Aug 27, 2019

agrare commented Aug 27, 2019

agrare commented Aug 27, 2019

agrare commented Aug 27, 2019

miq-bot commented Aug 28, 2019

agrare commented Aug 28, 2019

thearifismail commented Aug 28, 2019

simaishi commented Nov 4, 2019

[V2V] Datastore validation in Transformation Mapping #19204

[V2V] Datastore validation in Transformation Mapping #19204

Conversation

thearifismail commented Aug 26, 2019 • edited by simaishi Loading

thearifismail commented Aug 26, 2019

miq-bot commented Aug 26, 2019

thearifismail commented Aug 26, 2019

thearifismail commented Aug 26, 2019 • edited Loading

miq-bot commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thearifismail Aug 26, 2019 • 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

djberg96 commented Aug 27, 2019

agrare commented Aug 27, 2019

djberg96 commented Aug 27, 2019

agrare commented Aug 27, 2019

djberg96 commented Aug 27, 2019

agrare commented Aug 27, 2019

agrare commented Aug 27, 2019

agrare commented Aug 27, 2019

miq-bot commented Aug 28, 2019

agrare commented Aug 28, 2019

thearifismail commented Aug 28, 2019

simaishi commented Nov 4, 2019

thearifismail commented Aug 26, 2019 •

edited by simaishi

Loading

thearifismail commented Aug 26, 2019 •

edited

Loading

thearifismail Aug 26, 2019 •

edited

Loading