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

Support cancellation for miq_request and miq_request_task #17687

Merged
merged 1 commit into from
Jul 11, 2018
Merged

Support cancellation for miq_request and miq_request_task #17687

merged 1 commit into from
Jul 11, 2018

Conversation

bzwei
Copy link
Contributor

@bzwei bzwei commented Jul 10, 2018 •
edited by gmcculloug
Loading

There is immediate need from v2v for cancellation at both request and task levels. It is also a long demanded feature.

This PR adds MiqRequest#cancel_request and MiqRequestTask#cancel_task methods for the API controller to call in order to receive a user action to cancel a request or task.

Methods MiqRequest#canceling and MiqRequestTask#canceling are for automate to check whether user has requested to cancel. Then automate can attempt to stop its work.

These methods are initially not implemented for all types except transformation plan. The implementation for transformation plan is simply to store flag cancel_requested in options. This is a temporary solution. Ultimately we want to add canceling and canceled to allowed states for both request and task.

In this temporary solution we do not define how to report the state change or progress of the cancellation. @fdupont-redhat and @vconzola's team can agree on the keys and messages reading from options.

Related Links: ManageIQ/manageiq-v2v#370

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

cc @bdunne @fdupont-redhat @vconzola @priley86

@bzwei
Copy link
Contributor Author

bzwei commented Jul 10, 2018

@miq-bot assign @gmcculloug

@miq-bot miq-bot added the wip label Jul 10, 2018
gmcculloug
gmcculloug reviewed Jul 10, 2018
View reviewed changes
app/models/miq_request.rb Outdated
@@ -590,6 +590,14 @@ def self.display_name(number = 1)
n_('Request', 'Requests', number)
end

def cancel_request
raise raise _("This type of request cannot be canceled.")
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate raise keyword

bdunne
bdunne reviewed Jul 10, 2018
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

Why wouldn't this be a regular state in the state machine?

app/models/miq_request.rb Outdated
@@ -590,6 +590,14 @@ def self.display_name(number = 1)
n_('Request', 'Requests', number)
end

def cancel_request
Copy link
Member

Choose a reason for hiding this comment

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

Why not just cancel?

Copy link
Member

Choose a reason for hiding this comment

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

I had the same thought. It would be better if we had a name that could be applied consistently across models. Suggest using cancel which will match the exposed api action.

app/models/miq_request_task.rb Outdated
@@ -206,6 +206,14 @@ def self.display_name(number = 1)
n_('Request Task', 'Request Tasks', number)
end

def cancel_task
Copy link
Member

Choose a reason for hiding this comment

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

Same here

gmcculloug
gmcculloug reviewed Jul 10, 2018
View reviewed changes
app/models/miq_request.rb Outdated
@@ -590,6 +590,14 @@ def self.display_name(number = 1)
n_('Request', 'Requests', number)
end

def cancel_request
raise _("This type of request cannot be canceled.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested message: "Cancel operation is not supported for #{self.class.name}"

app/models/miq_request.rb Outdated
@@ -590,6 +590,14 @@ def self.display_name(number = 1)
n_('Request', 'Requests', number)
end

def cancel_request
Copy link
Member

Choose a reason for hiding this comment

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

I had the same thought. It would be better if we had a name that could be applied consistently across models. Suggest using cancel which will match the exposed api action.

app/models/miq_request_task.rb Outdated
@@ -206,6 +206,14 @@ def self.display_name(number = 1)
n_('Request Task', 'Request Tasks', number)
end

def cancel_task
raise _("This type of task cannot be canceled")
Copy link
Member

Choose a reason for hiding this comment

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

Rework message.

@bzwei
Copy link
Contributor Author

bzwei commented Jul 10, 2018

@bdunne We will eventually move it to a regular state. This feature is needed by v2v now but we expect much more work to be done in order to introduce a new state.

@miq-bot
Copy link
Member

miq-bot commented Jul 10, 2018

Checked commit bzwei@674d86c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 👍

@bdunne
Copy link
Member

bdunne commented Jul 10, 2018

Would kill be a more descriptive method name?

@gmcculloug
Copy link
Member

I would say cancel is the preferred name. This change is adding a way to flag a request/task as being requested to cancel by the user and then it is up to the request/task to respond properly.

There is work being added to do the cleanup (not just kill) the task here: ManageIQ/manageiq-content#357

@bzwei bzwei changed the title [WIP] Support cancellation for miq_request and miq_request_task Support cancellation for miq_request and miq_request_task Jul 11, 2018
@bzwei
Copy link
Contributor Author

bzwei commented Jul 11, 2018

@bdunne can you add API action for cancellation?

@miq-bot miq-bot removed the wip label Jul 11, 2018
@gmcculloug
Copy link
Member

@agrare Would this feature be a candidate for adding a supports flag for or is that largely defined for provider level actions?

The cancel action will be on all miq_request/miq_request_task objects but only supported/implemented on service_template_transformation_plan objects.

@agrare
Copy link
Member

agrare commented Jul 11, 2018

@gmcculloug I think this is fine the way it is for now, if we expose cancel to the UI and it is only supported by some providers then we might think about adding a supports feature for it.

@gmcculloug gmcculloug merged commit d676ff5 into ManageIQ:master Jul 11, 2018
@gmcculloug gmcculloug added this to the Sprint 90 Ending Jul 16, 2018 milestone Jul 11, 2018
@bzwei bzwei deleted the miq_request_cancel branch July 11, 2018 20:19
@bdunne bdunne mentioned this pull request Jul 13, 2018
Merged
@JPrause
Copy link
Member

JPrause commented Jul 17, 2018

@miq-bot add_label blocker

@ghost ghost mentioned this pull request Jul 31, 2018
Merged
simaishi pushed a commit that referenced this pull request Jul 31, 2018
@gmcculloug @simaishi
Merge pull request #17687 from bzwei/miq_request_cancel
e22387a 
Support cancellation for miq_request and miq_request_task
(cherry picked from commit d676ff5)

https://bugzilla.redhat.com/show_bug.cgi?id=1610533
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit e22387a84958be350f76052817be02e89a7ffab5
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Jul 11 15:59:59 2018 -0400

    Merge pull request #17687 from bzwei/miq_request_cancel
    
    Support cancellation for miq_request and miq_request_task
    (cherry picked from commit d676ff58e41e587270f58efefaf4a01f173f6206)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1610533

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug left review comments

@bdunne bdunne bdunne left review comments

Assignees

@gmcculloug gmcculloug

Labels
blocker enhancement gaprindashvili/backported v2v
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bzwei @miq-bot @bdunne @gmcculloug @agrare @JPrause @simaishi