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

Tag associated resource for conversion hosts #18505

Merged
merged 4 commits into from
Mar 5, 2019
Merged

Tag associated resource for conversion hosts #18505

merged 4 commits into from
Mar 5, 2019

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Mar 1, 2019

A resource that's been designated as a conversion host should be tagged and identifiable as such. This PR modifies the ConversionHost model so that on create or delete it is tagged appropriately. I also added some comments and specs.

Note that I modified the existing tagging methods slightly so that it can only be v2v_transformation_host/trueor v2v_transformation_host/false. Previously it could end up with both tags.

In addition, it only sets v2v_transformation_method/vddk or v2v_transformation_method/ssh if they're actually supported. Those are deleted if the conversion host is deleted.

Part of https://bugzilla.redhat.com/show_bug.cgi?id=1622728
https://bugzilla.redhat.com/show_bug.cgi?id=1695855

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 1, 2019

@fdupont-redhat @mturley Please take a look.

@ghost
Copy link

ghost commented Mar 1, 2019

I would have seen this on the enable/disable actions. We can have a conversion host and easily know if it's configured.

@miq-bot
Copy link
Member

miq-bot commented Mar 1, 2019

Checked commits https://github.com/djberg96/manageiq/compare/2daceddc313e72a23d054d2de8d2e6c86f7f6c30~...70dd7bd368214e3445c5710d11c40f2ab7474854 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 1, 2019

@miq-bot add_labels transformation, enhancement

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 1, 2019

@miq-bot add_label hammer/yes

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 4, 2019

@miq_bot add_reviewer @agrare

@agrare agrare self-assigned this Mar 4, 2019
@mturley
Copy link
Contributor

mturley commented Mar 4, 2019

To Fabien's point:

I would have seen this on the enable/disable actions. We can have a conversion host and easily know if it's configured.

Is it possible to have a conversion host that is not configured, at least via the REST API? Create/enable is one request, and the disable request also destroys the record, right?

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 4, 2019

@mturley ATM, It's only possible through manual fiddling, probably by using the non-async methods directly on an active record instance on the console. Yes, disable destroys the record.

Anticipating your next question, perhaps you're wondering why tag the resource at all if it's not a conversion host? My thinking was that, this way, we could still tell that it had been converted to a conversion host at some point in its lifecycle.

@mturley
Copy link
Contributor

mturley commented Mar 4, 2019

That makes sense! Perfect, thanks.

Having this tag will be nice for excluding already-configured hosts from selection in the wizard, which I was anticipating having to do by checking each one against the conversion_hosts results. 👍

@mturley
Copy link
Contributor

mturley commented Mar 4, 2019

On a related question, @djberg96 , what would happen if a user requested conversion host enablement on a host that is already configured, or on a host which is in the process of being configured?

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 4, 2019

At the moment, I can only guess that the python script that performs the actual enablement would bomb. If that doesn't do any sort of validation, then the answer is.....I don't know.

So, good point. Once tagging support is added, I can add another validation that you can't enable an already enabled conversion host, which we can check by looking at its tags.

@mturley
Copy link
Contributor

mturley commented Mar 4, 2019

Ok. I'm going to filter out those hosts from the wizard, so it should be next to impossible for this to happen via the UI, but technically possible (e.g. user A opens the wizard and loads eligible hosts, user B enables one of them, user A tries to finish the wizard with the same host). Ideally that API request should fail (403 Forbidden, or maybe 400 Bad Request). Also ideally, it should fail even if the host isn't enabled, if there is a task in progress to enable that host (although I'm sure that's more of a stretch).

@ghost
Copy link

ghost commented Mar 4, 2019

The Python script you mention is ansible-playbook. The enablement is done by a playbook to which we pass extra vars, basically the values from the wizard. And we tried to make it idempotent so we should be able to run it twice or more.

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 5, 2019

@fdupont-redhat Ok, so we don't need to really worry about it. Good to know, thanks.

@ghost
Copy link

ghost commented Mar 5, 2019

@djberg96 Yes, the main challenge I see is collecting the log of the playbook.

# Wrapper method for the various tag_resource_as_xxx methods.
#--
# TODO: Do we need this?
#
def tag_resource_as(status)
send("tag_resource_as_#{status}")
end
Copy link
Member

Choose a reason for hiding this comment

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

Yeah lets delete this if it isn't used...it doesn't look that helpful

@agrare agrare merged commit a7b4e8e into ManageIQ:master Mar 5, 2019
@agrare agrare added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 5, 2019
simaishi pushed a commit that referenced this pull request Apr 22, 2019
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit e45c1c40b58508ffa8402da3d68efd9eb210fcc8
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Mar 5 16:52:01 2019 -0500

    Merge pull request #18505 from djberg96/conversion_host_resource_tag
    
    Tag associated resource for conversion hosts
    
    (cherry picked from commit a7b4e8eda1e82f5317dbe0c71c2fd5dc8ea328a5)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1702033

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.

5 participants