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] Allow downloading wrapper log file #18506

Merged
merged 2 commits into from
Mar 5, 2019

Conversation

ghost
Copy link

@ghost ghost commented Mar 1, 2019

In the current implementation, we provide a download button in the UI that allows downloading the virt-v2v log file. It's been reported by field that the virt-v2v-wrapper log is more relevant and provides better error messages. This PR allows downloading the wrapper log.

The transformation_log method becomes more generic by accepting a log_type argument. The default log_type value is v2v to not break code calling it. The transformation_log_queue method gates the log type to allow only v2v and wrapper. The migration log controller has been modified to leverage this new feature in ManageIQ/manageiq-v2v#887.

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

@ghost
Copy link
Author

ghost commented Mar 1, 2019

@miq-bot add-label transformation, enhancement, hammer/yes
@miq-bot add-reviewer djberg96
@miq-bot add-reviewer agrare

@@ -132,20 +132,21 @@ def transformation_log

# Intend to be called by UI to display transformation log. The log is stored in MiqTask#task_results
# Since the task_results may contain a large block of data, it is desired to remove the task upon receiving the data
def transformation_log_queue(userid = nil)
def transformation_log_queue(userid = nil, log_type = 'v2v')
raise "Transformation log type '#{log_type}' not supported" unless %w(v2v wrapper).include?(log_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick, I'd vote for a def valid_transformation_log_types method that returned this array instead of hardcoding it. That way down the road, we could just modify that method and/or overload it if needed. Not a showstopper, though.

@ghost ghost force-pushed the v2v_download_wrapper_log branch from 9e45c41 to c691b59 Compare March 1, 2019 17:56
@miq-bot
Copy link
Member

miq-bot commented Mar 1, 2019

Checked commits fabiendupont/manageiq@39f465b~...c691b59 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. ⭐

@@ -113,16 +113,20 @@ def destination_security_group
SecurityGroup.find_by(:id => vm_resource.options["osp_security_group_id"])
end

def transformation_log
def valid_transformation_log_types
%w(v2v wrapper)
Copy link
Member

Choose a reason for hiding this comment

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

Is the wrapper log a superset of the v2v log? Wondering if there's any reason why someone would want to download both.

Copy link
Author

Choose a reason for hiding this comment

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

Nope. It's a separate log. virt-v2v log is very verbose and difficult to interpret by the user. virt-v2v-wrapper log provides more comprehensive error messages when possible. When not, virt-v2v log is still a good place to look at. The support may also be interested in both when a customer opens a case.

return create_error_status_task(userid, msg).id
end

_log.info("Queuing the download of transformation log for #{description} with ID [#{id}]")
_log.info("Queuing the download of #{log_type} log for #{description} with ID [#{id}]")
task_options = {:userid => userid, :action => 'transformation_log'}
queue_options = {:class_name => self.class,
:method_name => 'transformation_log',
:instance_id => id,
:priority => MiqQueue::HIGH_PRIORITY,
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this change but...why is this HIGH_PRIORITY?? @fdupont-redhat can you put in a follow-up PR to remove the priority property? (NORMAL is the default so we dont' need to pass anything in)

Copy link
Author

Choose a reason for hiding this comment

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

Created the PR: #18524

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM just that one issue needs to be addressed in a follow-up

@agrare agrare merged commit 5307629 into ManageIQ:master Mar 5, 2019
@agrare agrare added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 5, 2019
@ghost ghost deleted the v2v_download_wrapper_log branch March 5, 2019 21:22
simaishi pushed a commit that referenced this pull request Mar 6, 2019
@simaishi
Copy link
Contributor

simaishi commented Mar 6, 2019

Hammer backport details:

$ git log -1
commit 10259fb2c98a883d32bf3b4276bba3a7f811345a
Author: Adam Grare <[email protected]>
Date:   Tue Mar 5 14:06:59 2019 -0500

    Merge pull request #18506 from fdupont-redhat/v2v_download_wrapper_log
    
    [V2V] Allow downloading wrapper log file
    
    (cherry picked from commit 53076295d673ff9f62a3f20b22b1644cf4b8ce15)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1686045

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