Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[V2V] Add VM validation for warm migration eligibility and updated specs to … #19401

Merged
merged 11 commits into from
Oct 29, 2019
Merged

[V2V] Add VM validation for warm migration eligibility and updated specs to … #19401

merged 11 commits into from
Oct 29, 2019

Conversation

thearifismail
Copy link
Contributor

@thearifismail thearifismail commented Oct 15, 2019 •
edited
Loading

For warm migration, we need to limit the list of selectable VMs in the migration plan wizard. This is done by adding warm_migration_incompatible property to VMs in app/models/service_template_transformation_plan/validate_config_info.rb . The criteria is that the VM must not have snapshots.

https://bugzilla.redhat.com/show_bug.cgi?id=1760040

@thearifismail
Copy link
Contributor Author

@miq-bot add_reviewers @djberg96, @agrare

@miq-bot
Copy link
Member

miq-bot commented Oct 15, 2019

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

@djberg96
Copy link
Contributor

I generally favor positives over negatives, because then you end up with double negatives. In other words, rather than read this:

vm_options[:warm_migration_incompatible] = false

It's easier to read this:

vm_options[:warm_migration_compatible] = true

@thearifismail
Copy link
Contributor Author

thearifismail commented Oct 15, 2019 •
edited
Loading

Done. Now it has vm_options[:warm_migration_compatible] = !vm_object.snapshots.present?

@djberg96
Copy link
Contributor

vm_options[:warm_migration_compatible] = !vm_object.snapshots.present?

This can be written as:

vm_options[:warm_migration_compatible] = vm_object.snapshots.blank?

@thearifismail
Copy link
Contributor Author

@miq-bot add_reviewer @agrare
@miq-bot add_reviewer @fdupont-redhat

@miq-bot miq-bot requested a review from agrare October 16, 2019 13:57
@miq-bot
Copy link
Member

miq-bot commented Oct 16, 2019

@thearifismail 'fdupont-redhat' is an invalid reviewer, ignoring...

@thearifismail
Copy link
Contributor Author

@miq-bot add_label transformation

@miq-bot miq-bot added the v2v label Oct 16, 2019
@ghost
Copy link

ghost commented Oct 16, 2019

I would have expected the validation code to be in app/models/transformation_mapping/vm_migration_validator.rb, because that's where the other validations are. Basically, I would add a new method to check if VM can be warm migrated, with only one check. Then, I'd call it from validate_vm. One example is validate_vm_name.

Then, the question is: how do you know that warm migration should be checked ? The identify_vms doesn't have arguments, while the UI should tell the backend that it has to check for warm migration...

@agrare agrare self-assigned this Oct 16, 2019
@thearifismail
Copy link
Contributor Author

thearifismail commented Oct 16, 2019 •
edited
Loading

I would have expected the validation code to be in app/models/transformation_mapping/vm_migration_validator.rb, because that's where the other validations are. Basically, I would add a new method to check if VM can be warm migrated, with only one check. Then, I'd call it from validate_vm. One example is validate_vm_name.

Then, the question is: how do you know that warm migration should be checked ? The identify_vms doesn't have arguments, while the UI should tell the backend that it has to check for warm migration...

To ensure that warm migration check did not block cold migrations, the reference to migration was needed in app/models/transformation_mapping/vm_migration_validator.rb. After consultation with UI dev, adding vm_options[:warm_migration_compatible] flag in app/models/service_template_transformation_plan/validate_config_info.rb came up as the better place that met the UI need.

@ghost
Copy link

ghost commented Oct 16, 2019

As long as the UI is able to get the information, I'm fine with that.

@ghost
Copy link

ghost commented Oct 16, 2019

In the spec, I don't see any test where the value is false. There should be at least one.

@djberg96
Copy link
Contributor

@thearifismail I'm guessing you don't want your byebug history as part of the PR. ;)

@thearifismail
Copy link
Contributor Author

Removed it already.

agrare
agrare reviewed Oct 21, 2019
View reviewed changes
app/models/service_template_transformation_plan/validate_config_info.rb Outdated
@@ -63,6 +63,7 @@ def validate_config_info(options)
raise _("Invalid VM found #{vm_obj.name}") if vm_obj.invalid?

vm_options = {}
vm_options[:warm_migration_compatible] = vm_obj.snapshots.blank?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that having snapshots is the only thing that would prevent warm migration which might not always be the case. How about a supports feature mixin on the (I assume only VMware VM) model? Then this becomes vm_object.supports_warm_migration?

@thearifismail thearifismail mentioned this pull request Oct 22, 2019
Merged
@thearifismail
Copy link
Contributor Author

Some gems needed to get updated. Running bundle update followed by bundle exec rspec successfully executed all specs.

@thearifismail thearifismail force-pushed the bug1760040 branch 2 times, most recently from 2c7f2ca to 99d4c7f Compare October 26, 2019 17:40
agrare
agrare reviewed Oct 28, 2019
View reviewed changes
spec/models/service_template_transformation_plan_spec.rb Outdated
expect(service_template.vm_resources.find_by(:resource_id => vm2.id).options)
.to eq("pre_ansible_playbook_service_template_id" => apst.id, "post_ansible_playbook_service_template_id" => apst.id, "osp_security_group_id" => security_group1.id, "osp_flavor_id" => flavor1.id)

stub_const("VirtualMachine", Class.new(ManageIQ::Providers::InfraManager::Vm) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you namespace this? ManageIQ::Providers::InfraManager::WarmMigrationVm or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Changed VirtualMachine to ManageIQ::Providers::InfraManager::WarmMigrationVm.

agrare
agrare reviewed Oct 28, 2019
View reviewed changes
spec/models/service_template_transformation_plan_spec.rb Outdated
expect(service_template.vm_resources.find_by(:resource_id => vm3.id).options)
.to eq("pre_ansible_playbook_service_template_id" => apst.id, "post_ansible_playbook_service_template_id" => apst.id, "osp_security_group_id" => security_group2.id, "osp_flavor_id" => flavor2.id)

stub_const("VirtualMachine", Class.new(ManageIQ::Providers::InfraManager::Vm) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in a before do; end block at the top so you don't have to re-declare it everytime.
You can also e.g. let(:vm_with_snapshots) { VirtualMachine.new() } so you can reuse the instance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

agrare
agrare reviewed Oct 28, 2019
View reviewed changes
spec/models/service_template_transformation_plan_spec.rb Outdated
end)

vm1_options = service_template.vm_resources.find_by(:resource_id => vm1.id).options
vm1_options["warm_migration_compatible"] = VirtualMachine.new(:snapshots => vm1.snapshots).supports_warm_migrate?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using vm1.snapshots could this warm-migration compatible class be vm1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did start with vm1 but using VirtualMachine.new(:vm => vm1).supports_warm_migrate? or VirtualMachine.new(:vm_or_template => vm1).supports_warm_migrate? would complain that :vm, vm_or_template were UNKNOWN. Using :snapshot was the only I could get it work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, instead of let(:vm1) { FactoryBot.create(:vm_or_template) } you do let(:vm1) { VirtualMachine.new }

djberg96
djberg96 reviewed Oct 28, 2019
View reviewed changes
spec/models/service_template_transformation_plan_spec.rb Outdated
before do
stub_const("ManageIQ::Providers::InfraManager::WarmMigrationVm", Class.new(ManageIQ::Providers::InfraManager::Vm) do
supports :warm_migrate do
unsupported_reason_add(:warm_migrate, _('Warm migratiobn can not migrate a VM with snapshots')) if snapshots.present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"migratiobn"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

agrare
agrare reviewed Oct 28, 2019
View reviewed changes
spec/models/service_template_transformation_plan_spec.rb Outdated
.to eq("pre_ansible_playbook_service_template_id" => apst.id, "post_ansible_playbook_service_template_id" => apst.id, "osp_security_group_id" => security_group1.id, "osp_flavor_id" => flavor1.id)

vm1_options = service_template.vm_resources.find_by(:resource_id => vm1.id).options
vm1_options["warm_migration_compatible"] = ManageIQ::Providers::InfraManager::WarmMigrationVm.new(:snapshots => vm1.snapshots).supports_warm_migrate?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be set by the actual code instead of being set in the tests?

@ghost
Copy link

ghost commented Oct 29, 2019

Quickly chatted with @agrare. Using the vm_vmware factory in the spec would be ok if we commit to move ServiceTemplateTransformationPlan* classes to manageiq-v2v repo. I created the following issue to track that effort: ManageIQ/manageiq-v2v#1058, and self-assigned it.

@miq-bot
Copy link
Member

miq-bot commented Oct 29, 2019

Checked commits thearifismail/manageiq@80b367f~...e43f670 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@agrare
Copy link
Member

agrare commented Oct 29, 2019

Kicking the cross-repo tests ManageIQ/manageiq-cross_repo-tests#1

@agrare agrare closed this Oct 29, 2019
@agrare agrare reopened this Oct 29, 2019
agrare
agrare approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to merge this now because these classes are going to be extracted into the v2v repo shortly

@agrare agrare merged commit 4d3d171 into ManageIQ:master Oct 29, 2019
@agrare agrare added this to the Sprint 124 Ending Nov 11, 2019 milestone Oct 29, 2019
@simaishi
Copy link
Contributor

@fdupont-redhat ivanchuk/yes?

@ghost
Copy link

ghost commented Jan 13, 2020

@miq-bot add-label ivanchuk/yes

simaishi pushed a commit that referenced this pull request Feb 21, 2020
@agrare @simaishi
Merge pull request #19401 from thearifismail/bug1760040
5d23be7 
[V2V] Add VM validation for warm migration eligibility and updated specs to …

(cherry picked from commit 4d3d171)

https://bugzilla.redhat.com/show_bug.cgi?id=1790538
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 5d23be72d69cc6cccd71a84316da0a0600f3adeb
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Oct 29 15:43:00 2019 -0400

    Merge pull request #19401 from thearifismail/bug1760040

    [V2V] Add VM validation for warm migration eligibility and updated specs to …

    (cherry picked from commit 4d3d17113bfdcdd45e7cca8b3099df9388d83f06)

    https://bugzilla.redhat.com/show_bug.cgi?id=1790538

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djberg96 djberg96 djberg96 left review comments

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
enhancement ivanchuk/backported v2v
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thearifismail @miq-bot @djberg96 @agrare @simaishi