Enable cancel operation for service template transformation plan request #17825

hsong-rh · 2018-08-09T19:24:00Z

This PR enable cancellation for V2V request by using the new introduced cancelation_status column in miq_requests table.

cancelation_status has three valid values: cancel_requested, canceling and canceled. After UI gets cancel request, it will mark to cancel_requested. Following checking will change it to canceling, canceled, based on subtasks' states.

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

hsong-rh · 2018-08-09T19:58:53Z

@bzwei @gmcculloug Please review.

bzwei · 2018-08-09T20:55:43Z

app/models/miq_request.rb

+    false
+  end
+
+  def do_cancel


make it a private method

bzwei · 2018-08-09T21:07:58Z

app/models/service_template_provision_request.rb

+  end
+
+  def do_cancel
+    update_attributes(:request_state => "finished", :status => "Error", :message => "Request is canceled by user.")


Move the logic to cancel. Here do_cancel does nothing.

bzwei · 2018-08-09T21:10:44Z

@hsong-rh Please change the title to wip.

bzwei · 2018-08-13T18:39:51Z

app/models/miq_request.rb

  validates_inclusion_of :approval_state, :in => %w(pending_approval approved denied), :message => "should be 'pending_approval', 'approved' or 'denied'"
  validates_inclusion_of :status,         :in => %w(Ok Warn Error Timeout Denied)
+  validates_inclusion_of :cancel_state,   :in => %w(pending_cancel processing finished), :message => "should be 'pending_cancel', 'processing' or 'finished'"


for allowed states, how about %w(cancel_requested canceling canceled)

bzwei · 2018-08-13T21:59:43Z

cc @fdupont-redhat @priley86
Automate should check and update task's cancel_state.
UI should check this column to show cancellation status instead of parsing options.

ghost · 2018-08-14T06:11:11Z

@bzwei How do I update task's cancel_state ? I can only method to check the cancel state, not to set it. Should I simply do `task.cancel_state = 'canceling' when I have successfully launched the cancellation state machine ?

Alos, this PR has code only for the model, but not for the service model. I guess there will be another PR for that.

bzwei · 2018-08-14T13:04:33Z

@fdupont-redhat yes. Another PR for service model is on the way. It will provide methods for automate to change cancel_state.

JPrause · 2018-08-14T13:22:03Z

@miq-bot add_label blocker

JPrause · 2018-08-14T13:22:38Z

@hsong-rh if this can be backported, please add the gaprindashvili/yes label.

gmcculloug

Instead of jamming more code into the miq_request.rb and miq_request_task.rb files it would be better to move this logic into new files using include_concern. There seems to be some potential to use a mixin as well as there are common elements in both.

Still need to review this more and it would be good to get together to discuss the columns involved as well as the state values.

hsong-rh · 2018-08-28T13:59:12Z

@bzwei @agrare @gmcculloug Please review.

hsong-rh · 2018-08-29T19:43:23Z

@agrare @bzwei @mkanoor @gmcculloug Changed StateMachine into StateMachineMixin and added corresponding spec tests. Please review and decide if need to separate into 2 PRs.

mkanoor · 2018-08-29T20:48:51Z

app/models/service_template_transformation_plan_request.rb

+      :pending   => {'pending' => 'pending'},
+      :queued    => {'pending' => 'queued'},
+      :active    => {'pending' => 'active',
+                     'active'  => 'active',


@hsong-rh
Is active => active needed?

Yes, one type of automated request needs it.

@hsong-rh do you know which one of the automate request. Also if the next_state == current_state meaning active == active can't we just skip that state change

hsong-rh · 2018-08-30T19:31:31Z

@bzwei @mkanoor @agrare @gmcculloug Changed to focus on request cancellation for V2V. Refactor for state machine mixin will be in separate PR. Please review.

gmcculloug

A few minor suggestions around the spec, but otherwise this looks good.

gmcculloug · 2018-09-12T21:36:03Z

spec/models/service_template_transformation_plan_request_spec.rb

@@ -38,4 +38,24 @@
      expect(ServiceResource.find_by(:resource => vms[0]).status).to eq(ServiceResource::STATUS_APPROVED)
    end
  end
+
+  context "gets cancel request" do


Suggest "when request gets canceled"

Also, you can move the call to cancel into a before block: before { request.cancel }

gmcculloug · 2018-09-12T21:41:31Z

spec/models/service_template_transformation_plan_request_spec.rb

@@ -38,4 +38,24 @@
      expect(ServiceResource.find_by(:resource => vms[0]).status).to eq(ServiceResource::STATUS_APPROVED)
    end
  end
+
+  context "gets cancel request" do
+    it "gets cancel_requested request" do


"cancelation status is set to requested"

gmcculloug · 2018-09-12T21:43:19Z

spec/models/service_template_transformation_plan_request_spec.rb

+      expect(request.canceled?).to be_falsey
+    end
+
+    it "calls do_cancel" do


"marks request as finished in error"

miq-bot · 2018-09-13T13:12:00Z

Checked commit hsong-rh@5f10870 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 0 offenses detected
Everything looks fine. 🏆

bzwei · 2018-09-13T18:41:52Z

@fdupont-redhat @priley86 Though this PR is related to https://bugzilla.redhat.com/show_bug.cgi?id=1614864, it will not back ported because it relies on schema migration.

To summarize the changes

We now support transformation plan cancellation at both request and task levels.
Cancel status is stored in a new column, namely cancel_status; thus UI should look at this attribute to show the canceling status. Valid values are cancel_requested, canceling, and canceled.
Automate should call task's cancel_requested? method to check whether user has requested a cancellation.
Automate should call task's canceling and canceled methods to report the status change.
Automate (and UI) should clean up and remove the cancel status in options field.

With this PR being merged the V2V cancellation will not work properly until automate and UI make the above modifications.

bzwei suggested changes Aug 9, 2018

View reviewed changes

hsong-rh changed the title ~~Enable cancel operation for service template transformation plan request~~ [WIP] Enable cancel operation for service template transformation plan request Aug 9, 2018

miq-bot added the wip label Aug 9, 2018

hsong-rh force-pushed the miq_request_cancel branch 5 times, most recently from d3fd4e6 to 9ac1276 Compare August 13, 2018 21:00

hsong-rh changed the title ~~[WIP] Enable cancel operation for service template transformation plan request~~ Enable cancel operation for service template transformation plan request Aug 13, 2018

miq-bot removed the wip label Aug 13, 2018

hsong-rh force-pushed the miq_request_cancel branch from 9ac1276 to 4a43e3f Compare August 13, 2018 21:43

bzwei approved these changes Aug 13, 2018

View reviewed changes

gmcculloug self-assigned this Aug 14, 2018

gmcculloug added bug v2v labels Aug 14, 2018

miq-bot added the blocker label Aug 14, 2018

gmcculloug reviewed Aug 14, 2018

View reviewed changes

hsong-rh mentioned this pull request Aug 14, 2018

Enable cancelation_status to be updated for service model ManageIQ/manageiq-automation_engine#212

Merged

gmcculloug removed the blocker label Aug 14, 2018

hsong-rh mentioned this pull request Aug 14, 2018

Add cancelation_status column in MiqRequest and MiqRequestTask ManageIQ/manageiq-schema#254

Merged

hsong-rh force-pushed the miq_request_cancel branch 2 times, most recently from c93e474 to a30adc5 Compare August 27, 2018 15:17

hsong-rh force-pushed the miq_request_cancel branch 3 times, most recently from 3e4c30e to 91a7b4b Compare August 29, 2018 19:01

mkanoor reviewed Aug 29, 2018

View reviewed changes

hsong-rh force-pushed the miq_request_cancel branch from 91a7b4b to d5c547b Compare August 30, 2018 18:31

hsong-rh changed the title ~~Enable cancel operation for service template transformation plan request~~ [WIP] Enable cancel operation for service template transformation plan request Aug 30, 2018

miq-bot added the wip label Aug 30, 2018

hsong-rh force-pushed the miq_request_cancel branch from d5c547b to d00da09 Compare September 5, 2018 19:20

hsong-rh changed the title ~~[WIP] Enable cancel operation for service template transformation plan request~~ Enable cancel operation for service template transformation plan request Sep 6, 2018

miq-bot removed the wip label Sep 6, 2018

hsong-rh force-pushed the miq_request_cancel branch from d00da09 to 1eadfcc Compare September 7, 2018 15:46

hsong-rh closed this Sep 11, 2018

hsong-rh reopened this Sep 11, 2018

hsong-rh force-pushed the miq_request_cancel branch 2 times, most recently from 65f62fd to 3452fe1 Compare September 12, 2018 17:52

bzwei approved these changes Sep 12, 2018

View reviewed changes

gmcculloug reviewed Sep 12, 2018

View reviewed changes

hsong-rh force-pushed the miq_request_cancel branch from 3452fe1 to 2db1d05 Compare September 13, 2018 12:56

Enable cancel operation for service template transformation plan request

5f10870

hsong-rh force-pushed the miq_request_cancel branch from 2db1d05 to 5f10870 Compare September 13, 2018 13:06

gmcculloug approved these changes Sep 13, 2018

View reviewed changes

gmcculloug merged commit 9f3c401 into ManageIQ:master Sep 13, 2018

gmcculloug added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 13, 2018

hsong-rh mentioned this pull request Sep 13, 2018

Exposes cancelation status to client side ManageIQ/manageiq-automation_engine#229

Merged

AparnaKarve mentioned this pull request Oct 12, 2018

[699] Fix cancellation issues based on new backend changes ManageIQ/manageiq-v2v#708

Merged

ghost mentioned this pull request Oct 29, 2018

Use task cancelation_status to trigger cancelation ManageIQ/manageiq-content#457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable cancel operation for service template transformation plan request #17825

Enable cancel operation for service template transformation plan request #17825

hsong-rh commented Aug 9, 2018 •

edited

Loading

hsong-rh commented Aug 9, 2018

bzwei Aug 9, 2018

bzwei Aug 9, 2018

bzwei commented Aug 9, 2018

bzwei Aug 13, 2018

bzwei commented Aug 13, 2018

ghost commented Aug 14, 2018

bzwei commented Aug 14, 2018

JPrause commented Aug 14, 2018

JPrause commented Aug 14, 2018

gmcculloug left a comment

hsong-rh commented Aug 28, 2018

hsong-rh commented Aug 29, 2018

mkanoor Aug 29, 2018

hsong-rh Aug 30, 2018

mkanoor Aug 30, 2018

hsong-rh commented Aug 30, 2018

gmcculloug left a comment

gmcculloug Sep 12, 2018

gmcculloug Sep 12, 2018

gmcculloug Sep 12, 2018

miq-bot commented Sep 13, 2018

bzwei commented Sep 13, 2018

Enable cancel operation for service template transformation plan request #17825

Enable cancel operation for service template transformation plan request #17825

Conversation

hsong-rh commented Aug 9, 2018 • edited Loading

hsong-rh commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Aug 9, 2018

Choose a reason for hiding this comment

bzwei commented Aug 13, 2018

ghost commented Aug 14, 2018

bzwei commented Aug 14, 2018

JPrause commented Aug 14, 2018

JPrause commented Aug 14, 2018

gmcculloug left a comment

Choose a reason for hiding this comment

hsong-rh commented Aug 28, 2018

hsong-rh commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsong-rh commented Aug 30, 2018

gmcculloug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 13, 2018

bzwei commented Sep 13, 2018

hsong-rh commented Aug 9, 2018 •

edited

Loading