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.

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

[V2V] Don't run Ansible playbook if conversion host resource is absent or archived #20133

Merged
merged 2 commits into from May 13, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 7, 2020

When the resource associated to a conversion host is nil, running the conversion host playbooks (check, disable, enable) will fail, because the tag_resource_as_* method will try to tag a nil object and raise. For the disable method, it has a side effect that the conversion host is not destroyed.

And before trying to tag the resource, it will also try to run the associated Ansible playbook that will also fail and be rescued for nothing.

This pull request checks that the resource is present and returns early if it not, avoiding unneeded load and failure, and allowing conversion host destruction. It also returns if the resource is archived as it means the VM / Host doesn't exist in the provider and the playbook will also fail.

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

@ghost ghost requested review from Fryguy and gtanzillo as code owners May 7, 2020 07:36
@ghost
Copy link
Author

ghost commented May 7, 2020

@miq-bot add-label v2v, bug, jansa/yes?, ivanchuk/yes

@miq-bot
Copy link
Member

miq-bot commented May 7, 2020

Checked commits fabiendupont/manageiq@9f67239~...0ee08d4 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare
Copy link
Member

agrare commented May 8, 2020

@fdupont-redhat when will the conversion_host resource be nil? I thought there was verification on this when creating the host and both VmOrTemplate and Host have dependent destroy on the belongs_to so if the host is deleted so too should the conversion host.

No issue with this change, just want to make sure we're not putting a band-aid on a bigger issue

@ghost
Copy link
Author

ghost commented May 8, 2020

@agrare it can be nil if the resource is removed from inventory. There's no cascading to conversion host object. It happened to QE to remove it before removing the conversion host, so I guess it could happen to users.

@agrare
Copy link
Member

agrare commented May 12, 2020

@fdupont-redhat but there is cascade deletion from the vm and host to the conversion host, vm and host

Also tested out real quick on the console:

>> vm.conversion_host
=> #<ConversionHost id: 2, name: "my-vm", address: nil, type: nil, resource_type: "VmOrTemplate", resource_id: 25, version: nil, max_concurrent_tasks: nil, vddk_transport_supported: nil, ssh_transport_supported: nil, created_at: "2020-05-12 13:32:59", updated_at: "2020-05-12 13:32:59", concurrent_transformation_limit: nil, cpu_limit: nil, memory_limit: nil, network_limit: nil, blockio_limit: nil>
>> ConversionHost.count
=> 1
>> vm.destroy!
>> ConversionHost.count
=> 0

@agrare
Copy link
Member

agrare commented May 13, 2020

I'll merge this to get this fixed but think we should look at how this was able to happen, maybe QE was doing .delete instead of .destroy manually on vm records?

@agrare agrare merged commit 5916d54 into ManageIQ:master May 13, 2020
simaishi pushed a commit that referenced this pull request May 21, 2020
[V2V] Don't run Ansible playbook if conversion host resource is absent or archived

(cherry picked from commit 5916d54)

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

Ivanchuk backport details:

$ git log -1
Satoes-MacBook-Pro:manageiq simaishi (ivanchuk) $ git logb
commit 7e2c4a7386309067db9a069ee1a787d1ff924658
Author: Adam Grare <agrare@redhat.com>
Date:   Wed May 13 08:36:49 2020 -0400

    Merge pull request #20133 from fdupont-redhat/v2v_bz_1810406

    [V2V] Don't run Ansible playbook if conversion host resource is absent or archived

    (cherry picked from commit 5916d54d21a9ea3b32abbecb5479e19643fe2ad7)

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

simaishi pushed a commit that referenced this pull request May 21, 2020
[V2V] Don't run Ansible playbook if conversion host resource is absent or archived

(cherry picked from commit 5916d54)

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

Jansa backport details:

$ git log -1
commit bd09bd05d4deba5421c6b6e196dc634086d13fcd
Author: Adam Grare <agrare@redhat.com>
Date:   Wed May 13 08:36:49 2020 -0400

    Merge pull request #20133 from fdupont-redhat/v2v_bz_1810406

    [V2V] Don't run Ansible playbook if conversion host resource is absent or archived

    (cherry picked from commit 5916d54d21a9ea3b32abbecb5479e19643fe2ad7)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants