[V2V] Set context data for the task associated with conversion host creation #18541

djberg96 · 2019-03-11T15:34:09Z

This PR modifies the ConversionHost::Configurations#enable_queue method so that the original params are stored in the context_data of the associated task. This is a serialized field, so it would be accessible as a hash of options.

A few things to head off potential questions about why this PR is written the way it is:

Q: What is this going to be used for?
A: The UI team would like it for a "retry" option. To do that they need the original parameters, which they can retrieve from the associated task.

Q: Why not just add context_data to the task_opts hash above?
A: Because MiqTask.generic_action_with_callback only recognizes certain options. The rest are dropped.

Q: Why use MiqTask.find on a task that you just created?
A: Because MiqTask.generic_action_with_callback returns a task_id instead of the full task object.

Q: Where are task_id and miq_task_id coming from, and why explicitly .except them?
A: They're injected at some point in the queue lifecycle, so I have to manually reject them because they aren't actually part of the params. The specs revealed that. Without that, the specs will fail.

~~https://bugzilla.redhat.com/show_bug.cgi?id=1622728~~
https://bugzilla.redhat.com/show_bug.cgi?id=1695886

djberg96 · 2019-03-11T15:34:27Z

@fdupont-redhat @mturley Please review.

mturley · 2019-03-11T15:49:02Z

So this will store all the params of the original request in context_data? Since we're going to be passing SSH private keys in that request, my concern is that we'll be exposing those to the public here. Maybe we need some way of optionally referencing the stored keys in the enable request as an alternative to passing them in explicitly, and some identifiers for the keys can be stored here?

Sorry if I'm misunderstanding.

mturley · 2019-03-11T15:53:27Z

If we need to break the SSH keys into a separate request that we do before the enable request, that would be fine. Not sure if that makes this easier.

djberg96 · 2019-03-11T16:48:29Z

Ah, true, forgot about private ssh keys. I don't know if we want to make that a separate request, though. I guess I would need some idea of the behavior of a retry option where the user had supplied a key. I can always just filter it out, too.

ghost · 2019-03-11T17:02:52Z

Wouldn't that be more practical to create the conversion host before queuing the enablement task ? That would give the conversion host id pretty early, while being quite fast. Upon creation, it would create the Authentication from the SSH key, so we could discard it from the params hash. Then the queue_configuration could update the created task with the params hash and the conversion host id in the task context_data. That would also allow the UI to simply call /api/conversion_hosts to list all the conversion hosts...

@mturley, about filtering out SSH keys, would it be possible to ask the user to provide the VMware hypervisors SSH key when retrying ? That would prevent storing the private key in clear text.

mturley · 2019-03-11T17:09:51Z

@fdupont-redhat we could pop a modal when you click Retry asking for the key again, sure. @vconzola what do you think about that from a UX perspective?

Wouldn't it also need to ask them for the Conversion Host SSH key again? (and only ask about the VMware hypervisors key if they selected SSH for a transformation method?)

vconzola · 2019-03-11T17:45:25Z

@mturley I'm ok with your suggestion of asking the user to re-enter the SSH key information again on Retry. Not ideal from a UX perspective, but reasonable since we don't want to store the key. We should add some explanatory text to the modal explaining why we're asking the user to enter it again. "Because we care about you,..." :-)

mturley · 2019-03-11T18:18:15Z

Added a comment describing the prompt for re-entering SSH keys in ManageIQ/manageiq-v2v#901. @djberg96 that works for us if you just want to filter those keys out of the params here. I think they'll be ssh_key and vmware_ssh_private_key now, according to @fdupont-redhat 's last email.

djberg96 · 2019-03-12T15:21:11Z

I'll just filter out anything with _key in it. :)

djberg96 · 2019-03-12T15:40:29Z

@mturley Updated.

djberg96 · 2019-03-12T15:42:20Z

@fdupont-redhat If I had seen what a problem this coupling would have been I would have pushed back harder in my original PR for the rest API. I think that ship has sailed, so I'm going to look for other ways.

djberg96 · 2019-03-14T17:59:52Z

@miq-bot add_label transformation, enhancement, hammer/yes

mturley · 2019-03-14T20:49:54Z

@djberg96 I just wanted to reference my other comment here: #18540 (comment)

If in that other PR there's no good way to use instance_id, maybe we need to use context_data for both the request params and the conversion host id, which would affect this PR.

djberg96 · 2019-03-14T21:17:30Z

@mturley Ok, I'll modify this one to include the conversion host id as part of the context data.

mturley · 2019-03-18T15:15:46Z

This should be listed in https://bugzilla.redhat.com/show_bug.cgi?id=1622728 I think

mturley · 2019-03-18T20:15:06Z

@djberg96 are you expecting to add conversion_host_id as a property alongside the request params in context_data, keeping it flat? or are we adding a nested object inside context_data for the request params? Either way is fine, just preparing a PR for the Retry button that will consume this.

djberg96 · 2019-03-18T20:47:23Z

@mturley Was going to make the params a nested property, e.g. {:conversion_host_id => 1, :request_params => { .... }.

agrare · 2019-03-19T15:17:58Z

app/models/conversion_host/configurations.rb

@@ -24,7 +25,21 @@ def queue_configuration(op, instance_id, resource, params, auth_user = nil)
 :zone => resource.ext_management_system.my_zone,
 :args => params
 }
- MiqTask.generic_action_with_callback(task_opts, queue_opts)
+
+ task_id = MiqTask.generic_action_with_callback(task_opts, queue_opts)


@djberg96 how about adding :context_data to the options hash and pass it to the MiqTask.create! here

I thought about making modifications there - I think that method could use some refactoring in general - but was worried about introducing some sort of unexpected regression. I'd like to do that in a separate PR if you don't mind. If/when that happens, we can return to this and refactor it.

I'd rather fix it first even if it is in a separate PR, a quick search of the callers should be enough to see if anyone is doing anything strange like passing in invalid keys.

I'd expect to be able to control any of the task attributes in the first argument, so something like:

Task.create!( { :state => STATE_QUEUED, :status => STATUS_OK, :message => msg }.merge(options) )

I actually had this already started, so I've created a WIP here: #18578

miq-bot · 2019-03-26T16:46:08Z

This pull request is not mergeable. Please rebase and repush.

mturley · 2019-03-26T21:17:44Z

I think this is now the only PR we're waiting for to enable the Conversion Hosts settings UI 🎉

djberg96 · 2019-03-26T21:19:13Z

I'll be focused on this one tomorrow. :)

…t params to their own key.

miq-bot · 2019-03-27T14:11:50Z

Checked commits https://github.com/djberg96/manageiq/compare/54746a1c10e85f716b34e5ee449c5d421607d224~...af2795546536fefff0e6178cb9bea3a23c78ac5f 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. ⭐

agrare · 2019-03-27T15:25:50Z

app/models/conversion_host/configurations.rb

+ # any ssh key information. Useful for a retry option in the UI, and
+ # informational purposes in general.
+ #
+ MiqTask.find(task_id).tap do |task|


@djberg96 so you want to use the old method so this can be backported then followup with a refactoring PR to just pass in the context_data?

That's the plan, yes.

…data [V2V] Set context data for the task associated with conversion host creation

ghost · 2019-04-03T21:49:37Z

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

…data [V2V] Set context data for the task associated with conversion host creation (cherry picked from commit 65617fa) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1702278

simaishi · 2019-04-25T13:26:17Z

Hammer backport details:

$ git log -1
commit 7ee3e9b4a47192a0e7f452da0900b470e6b6d03f
Author: Adam Grare <agrare@redhat.com>
Date:   Fri Mar 29 07:42:23 2019 -0400

    Merge pull request #18541 from djberg96/conversion_host_task_context_data
    
    [V2V] Set context data for the task associated with conversion host creation
    
    (cherry picked from commit 65617fa67713a3e640fb8029685c41c27550a75d)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1702278

mturley mentioned this pull request Mar 11, 2019

Conversion Host Enablement - List View - Retry button on a failed enablement task - Need API support ManageIQ/manageiq-v2v#901

Closed

agrare self-assigned this Mar 13, 2019

miq-bot added enhancement hammer/yes v2v labels Mar 14, 2019

djberg96 mentioned this pull request Mar 15, 2019

[V2V] Add conversion_host.id to task options #18540

Closed

mturley mentioned this pull request Mar 18, 2019

Conversion Host Configuration Wizard - rename API field from ssh_key to conversion_host_ssh_private_key ManageIQ/manageiq-v2v#913

Merged

agrare reviewed Mar 19, 2019

View reviewed changes

djberg96 mentioned this pull request Mar 21, 2019

Refactor the MiqTask#generic_action_with_callback method #18578

Merged

mturley mentioned this pull request Mar 21, 2019

[#901] Conversion Host Enablement - List View - Add Retry button with modal for re-entering SSH keys ManageIQ/manageiq-v2v#916

Merged

miq-bot added the unmergeable label Mar 26, 2019

Daniel Berger added 2 commits March 27, 2019 08:09

Set context data for the task associated with conversion host creation.

54746a1

Explicitly reject key information as context data.

6cb8de7

miq-bot removed the unmergeable label Mar 27, 2019

djberg96 added 2 commits March 27, 2019 09:45

Modify context_data to set the conversion_host id, and set the reques…

313ae94

…t params to their own key.

Indicate unused variable in reject block for rubocop.

af27955

agrare reviewed Mar 27, 2019

View reviewed changes

mturley mentioned this pull request Mar 27, 2019

Enable (un-hide) the Conversion Hosts tab of the Settings page ManageIQ/manageiq-v2v#922

Merged

agrare approved these changes Mar 29, 2019

View reviewed changes

agrare merged commit af27955 into ManageIQ:master Mar 29, 2019

agrare added a commit that referenced this pull request Mar 29, 2019

Merge pull request #18541 from djberg96/conversion_host_task_context_…

65617fa

…data [V2V] Set context data for the task associated with conversion host creation

agrare added this to the Sprint 108 Ending Apr 1, 2019 milestone Mar 29, 2019

ghost mentioned this pull request Apr 5, 2019

[V2V] Run the playbook on the appliance with the conversion host in inventory #18613

Merged

simaishi added hammer/backported and removed hammer/yes labels Apr 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V2V] Set context data for the task associated with conversion host creation #18541

[V2V] Set context data for the task associated with conversion host creation #18541

djberg96 commented Mar 11, 2019 •

edited

Loading

djberg96 commented Mar 11, 2019

mturley commented Mar 11, 2019

mturley commented Mar 11, 2019

djberg96 commented Mar 11, 2019

ghost commented Mar 11, 2019

mturley commented Mar 11, 2019

vconzola commented Mar 11, 2019

mturley commented Mar 11, 2019

djberg96 commented Mar 12, 2019

djberg96 commented Mar 12, 2019

djberg96 commented Mar 12, 2019

djberg96 commented Mar 14, 2019

mturley commented Mar 14, 2019

djberg96 commented Mar 14, 2019

mturley commented Mar 18, 2019

mturley commented Mar 18, 2019

djberg96 commented Mar 18, 2019

agrare Mar 19, 2019

djberg96 Mar 19, 2019

agrare Mar 21, 2019

djberg96 Mar 21, 2019

agrare Mar 21, 2019

miq-bot commented Mar 26, 2019

mturley commented Mar 26, 2019

djberg96 commented Mar 26, 2019

miq-bot commented Mar 27, 2019

agrare Mar 27, 2019

djberg96 Mar 29, 2019

ghost commented Apr 3, 2019

simaishi commented Apr 25, 2019

[V2V] Set context data for the task associated with conversion host creation #18541

[V2V] Set context data for the task associated with conversion host creation #18541

Conversation

djberg96 commented Mar 11, 2019 • edited Loading

djberg96 commented Mar 11, 2019

mturley commented Mar 11, 2019

mturley commented Mar 11, 2019

djberg96 commented Mar 11, 2019

ghost commented Mar 11, 2019

mturley commented Mar 11, 2019

vconzola commented Mar 11, 2019

mturley commented Mar 11, 2019

djberg96 commented Mar 12, 2019

djberg96 commented Mar 12, 2019

djberg96 commented Mar 12, 2019

djberg96 commented Mar 14, 2019

mturley commented Mar 14, 2019

djberg96 commented Mar 14, 2019

mturley commented Mar 18, 2019

mturley commented Mar 18, 2019

djberg96 commented Mar 18, 2019

agrare Mar 19, 2019

Choose a reason for hiding this comment

djberg96 Mar 19, 2019

Choose a reason for hiding this comment

agrare Mar 21, 2019

Choose a reason for hiding this comment

djberg96 Mar 21, 2019

Choose a reason for hiding this comment

agrare Mar 21, 2019

Choose a reason for hiding this comment

miq-bot commented Mar 26, 2019

mturley commented Mar 26, 2019

djberg96 commented Mar 26, 2019

miq-bot commented Mar 27, 2019

agrare Mar 27, 2019

Choose a reason for hiding this comment

djberg96 Mar 29, 2019

Choose a reason for hiding this comment

ghost commented Apr 3, 2019

simaishi commented Apr 25, 2019

djberg96 commented Mar 11, 2019 •

edited

Loading