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

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c12dd97
Add conversion_host.id to task options
fabiendupont Mar 10, 2019
16364c3
Fix task presence check. Move task/conversion host association after …
fabiendupont Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions app/models/conversion_host/configurations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,20 @@ def enable(params)
params = params.symbolize_keys
_log.info("Enabling a conversion_host with parameters: #{params}")

params.delete(:task_id) # In case this is being called through *_queue which will stick in a :task_id
params.delete(:miq_task_id) # The miq_queue.activate_miq_task will stick in a :miq_task_id
task_id = params.delete(:task_id) # In case this is being called through *_queue which will stick in a :task_id
miq_task_id = params.delete(:miq_task_id) # The miq_queue.activate_miq_task will stick in a :miq_task_id
task_id ||= miq_task_id
Copy link
Member

Choose a reason for hiding this comment

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

What? no no lets just fix enable_queue to set :miq_task_id and standardize on a single key

Copy link
Author

Choose a reason for hiding this comment

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

Yup. I didn't like it neither. How would you fix enable_queue ?

Copy link
Member

@agrare agrare Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Actually it looks like the only caller (./manageiq-api/app/controllers/api/conversion_hosts_controller.rb) just does task_id = ConversionHost.enable_queue(data.except('auth_user'), data['auth_user']) so where were you seeing a task_id parameter coming from? Because from that it looks like it gets the task_id back rather than passing one in.

Copy link
Author

Choose a reason for hiding this comment

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

I took that from existing code... @djberg96 could you shed some light here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the enable_queue method will return the task_id on a create request, but we don't have a direct way to associate the task back to the conversion instance after that.

Copy link
Member

Choose a reason for hiding this comment

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

But enable_queue isn't passing in the task_id as a parameter right? So the only option should be miq_task_id

Copy link
Contributor

Choose a reason for hiding this comment

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

They're injected into the params via the MiqTask.generic_action_with_callback at https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_task.rb#L290-L292

Copy link
Member

Choose a reason for hiding this comment

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

But those are different hashes, only task_id is passed in to the method opts


vddk_url = params.delete(:param_v2v_vddk_package_url)

conversion_host = new(params)

unless task_id.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably change this to if task_id.present? in case it comes through as an empty param instead of nil. I've come to be paranoid about such things when dealing with http.

task = MiqTask.find(task_id)
task.options[:conversion_host_id] = conversion_host.id
Copy link
Member

Choose a reason for hiding this comment

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

Is options right? I thought it was context...

Copy link
Author

Choose a reason for hiding this comment

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

We've used options so much that it sounded like the logical choice.
@djberg96 would you recommend context_data over options ?

Copy link
Member

Choose a reason for hiding this comment

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

I mean is options even an attribute of an miq_task?

>> MiqTask.create!.options
NoMethodError (undefined method `options' for #<MiqTask:0x000055dc41439fe0>)

Copy link
Member

Choose a reason for hiding this comment

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

@fdupont-redhat you're probably thinking of an MiqRequestTask, that has an options column but MiqTask does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@agrare Now I'm curious what the difference between those two.

Copy link
Member

Choose a reason for hiding this comment

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

They're different models?

task.save!
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with this, but I'm thinking it should only happen if we know the conversion_host.save! was successful.

Copy link
Member

Choose a reason for hiding this comment

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

Won't .save! raise an exception and not get here if it failed?

conversion_host.enable_conversion_host_role(vddk_url)
conversion_host.save!
conversion_host
Expand Down