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] Set context data for the task associated with conversion host creation #18541

Merged
merged 4 commits into from
Mar 29, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54746a1
Set context data for the task associated with conversion host creation.
Mar 11, 2019
6cb8de7
Explicitly reject key information as context data.
Mar 12, 2019
313ae94
Modify context_data to set the conversion_host id, and set the reques…
djberg96 Mar 27, 2019
af27955
Indicate unused variable in reject block for rubocop.
djberg96 Mar 27, 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
32 changes: 27 additions & 5 deletions app/models/conversion_host/configurations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,21 @@ def queue_configuration(op, instance_id, resource, params, auth_user = nil)
:args => [params, auth_user]
}

MiqTask.generic_action_with_callback(task_opts, queue_opts)
task_id = MiqTask.generic_action_with_callback(task_opts, queue_opts)
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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)
)

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 actually had this already started, so I've created a WIP here: #18578

Copy link
Member

Choose a reason for hiding this comment

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

Awesome!


# Set the context_data after the fact because the above call only accepts
# certain options while ignoring the rest. We also don't want to store
# any ssh key information. Useful for a retry option in the UI, and
# informational purposes in general.
#
MiqTask.find(task_id).tap do |task|
Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the plan, yes.

params = params&.except(:task_id, :miq_task_id)
hash = {:request_params => params&.reject { |key, _value| key.to_s.end_with?('private_key') }}
task.context_data = hash
task.save
end

task_id
end

def enable_queue(params, auth_user = nil)
Expand All @@ -43,13 +57,14 @@ def enable(params, auth_user = nil)
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
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

vmware_vddk_package_url = params.delete(:vmware_vddk_package_url)
params[:vddk_transport_supported] = !vmware_vddk_package_url.nil?
params[:vddk_transport_supported] = vmware_vddk_package_url.present?

vmware_ssh_private_key = params.delete(:vmware_ssh_private_key)
params[:ssh_transport_supported] = !vmware_ssh_private_key.nil?
params[:ssh_transport_supported] = vmware_ssh_private_key.present?

ssh_key = params.delete(:conversion_host_ssh_private_key)

Expand All @@ -65,6 +80,13 @@ def enable(params, auth_user = nil)

conversion_host.enable_conversion_host_role(vmware_vddk_package_url, vmware_ssh_private_key)
conversion_host.save!

if miq_task_id
MiqTask.find(miq_task_id).tap do |task|
task.context_data.to_h[:conversion_host_id] = conversion_host.id
task.save
end
end
end
rescue StandardError => error
raise
Expand Down
11 changes: 10 additions & 1 deletion spec/models/conversion_host/configurations_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@

it "to queue with a task" do
task_id = described_class.enable_queue(params)
expect(MiqTask.find(task_id)).to have_attributes(:name => expected_task_action)
expected_context_data = {:request_params => params.except(:resource)}

expect(MiqTask.find(task_id)).to have_attributes(:name => expected_task_action, :context_data => expected_context_data)
expect(MiqQueue.first).to have_attributes(
:args => [params.merge(:task_id => task_id).except(:resource), nil],
:class_name => described_class.name,
Expand All @@ -129,6 +131,13 @@
:zone => vm.ext_management_system.my_zone
)
end

it "rejects ssh key information as context data" do
task_id = described_class.enable_queue(params.merge(:conversion_host_ssh_private_key => 'xxx', :vmware_ssh_private_key => 'yyy'))
expected_context_data = {:request_params => params.except(:resource)}

expect(MiqTask.find(task_id)).to have_attributes(:name => expected_task_action, :context_data => expected_context_data)
end
end

context "#disable_queue" do
Expand Down