[#370] Cancel a migration's in-progress tasks #517

AparnaKarve · 2018-07-26T01:59:40Z

Fixes #370

This is a continuation to @priley86 's and @mturley 's work...
Includes -

Canceling Migration Tasks
Displays the fa-ban icon for canceled tasks
Notification about the the Cancellation
Managing the checkboxes and Cancel Migration button state appropriately

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

To be merged after ManageIQ/manageiq-api#421

michaelkro · 2018-07-26T11:50:41Z

app/javascript/react/screens/App/Plan/PlanActions.js

+
+          const cancelSuccessMsg = __('Migration canceled successfully for VMs: ');
+          const vmNames = [];
+          tasks.map(task => vmNames.push(task.vmName));


I think here, we can just do

const vmNames = tasks.map(task => task.vmName)

michaelkro · 2018-07-26T11:55:01Z

app/javascript/react/screens/App/Plan/PlanActions.js

+          const cancelSuccessMsg = __('Migration canceled successfully for VMs: ');
+          const vmNames = [];
+          tasks.map(task => vmNames.push(task.vmName));
+          const cancelSuccessMsgWithVMNames = `${cancelSuccessMsg} ${vmNames}`;


Minor nit, but if we pass an array (with multiple entries) into the template literal, there will not be spaces after each comma

Agreed, it might be worth doing something like this:

const vmNamesStr = vmNames.join(', '); const cancelSuccessMsgWithVMNames = `${cancelSuccessMsg} ${vmNamesStr}`;

edit: to combine with the above comment, it could all just be:

const vmNames = tasks.map(task => task.vmName).join(', '); const cancelSuccessMsgWithVMNames = `${cancelSuccessMsg} ${vmNames}`;

Thanks for the tip.
Addressed this in the last commit

michaelkro

The bulk selector gets checked after canceling a task (this is actually observable in the first screenshot). Steps to reproduce

Cancel a task
Notice that the bulk selector is now checked

michaelkro · 2018-07-26T12:36:09Z

app/javascript/react/screens/App/Plan/components/PlanRequestDetailList/PlanRequestDetailList.js

+    // gather the selected tasks in state and feed them through
+    const { selectedCancelTasks } = this.state;
+    const { cancelPlanRequestTasksAction, cancelPlanRequestTasksUrl } = this.props;
+    cancelPlanRequestTasksAction(cancelPlanRequestTasksUrl, selectedCancelTasks);


We might want to refetch the tasks here. Currently, after confirming the cancel, the success notification appears, but we need to wait for the next poll before the ListItem is updated

did not do a refetch here.

The listview reflects the cancelled state immediately upon a successful return from the API POST cancel action.
I have used markedForCancellation redux state to mark the cancelled items in the listview before the task is officially marked with cancel_requested=true by the API.

michaelkro

Observation about the bulk selector & checkbox state after cancellation. Does not affect functionality, but wondering what the intended behavior is for this. Steps to reproduce

Cancel a task
Notice that the checkbox for the ListItem is disabled, but checked
Using the bulk selector, unselect all
Now the checkbox for the ListItem is not checked

michaelkro

Related to comment

Can attempt to cancel a already cancelled task. Steps to reproduce

Cancel a task. Notice that the checkbox for the ListItem is disabled, but checked.
Attempt to cancel another task
In the confirm modal, the already cancelled plan appears along with the new one

AparnaKarve · 2018-07-26T14:09:17Z

@michaelkro I think the expected behavior should be -

bulk selector should always be selectable --

it will be a no-op, if all the tasks are either already canceled or completed
Otherwise it will select/unselect tasks that are not canceled/completed

From your observation above, [#370] Cancel a migration's in-progress tasks #517 (review), the confirm modal, should not mention an already cancelled plan along with the new one

Let me know if you can think of more.

AllenBW

eeekkkk so trying this out.. latest miq master its not a happy camper...

a post to /api/request_tasks with the body {"action":"cancel","resources":[{"id":"173"}]} returns {"error":{"kind":"bad_request","message":"Unsupported Action cancel for the request_tasks collection specified","klass":"Api::BadRequestError"}}

Maybe there's a branch somewhere that i neeeeed to be on?

AllenBW · 2018-07-26T13:15:01Z

app/javascript/react/screens/App/Plan/PlanConstants.js

@@ -29,7 +30,8 @@ export const STATUS_MESSAGE_KEYS = {
  VALIDATING: 'Validating',
  VM_MIGRATIONS_COMPLETED: 'VM Transformations completed',
  VM_MIGRATED: 'Virtual machine migrated',
-  VM_MIGRATIONS_FAILED: 'VM Transformations failed'
+  VM_MIGRATIONS_FAILED: 'VM Transformations failed',
+  CANCELLED: 'VM cancelled'


Maybe want to say VM Migration canceled here and elsewhere...we follow the pattern of item/action/result elsewhere 🤔

I have removed this constant for now, since we don't know yet what string the state machine actually uses for a cancelled VM.

This will become more clear after some appliance testing and once Fabien's cancellation implementation is complete

AllenBW · 2018-07-26T13:19:42Z

app/javascript/react/screens/App/Plan/PlanReducer.js

@@ -79,6 +81,10 @@ const processVMTasks = vmTasks => {
      taskDetails.options.playbooks = task.options.playbooks;
    }

+    if (task.options.cancel_requested) {


This should be done on the item creation above, say line 69,

cancel_requested: task.options.cancel_requested

(but I don't know what the value of that attribute is, so maybe direct assignment doesn't make sense)

done in the last commit

AllenBW · 2018-07-26T13:39:17Z

app/javascript/react/screens/App/Plan/components/PlanRequestDetailList/PlanRequestDetailList.js

+            </React.Fragment>
+          }
+          cancelButtonLabel={__('No')}
+          confirmButtonLabel={__('Cancel Migrations')}


Confirmation buttons thus far have been single word actions, without specifying the item on which the action is taken, I know that this might have been specified in the mock... but it seems to introduce a design inconsistency @vconzola thoughts?

@AllenBW This is a classic UX conundrum - how to confirm an action when that action is cancelling something. The normal button labels (OK, Cancel) lead to confusion - Does Cancel cancel or cancel the Cancel. So I ran this by Stacey (our info architect) and this is what we came up with. Maybe not consistent, but in this case necessary.

@AllenBW @vconzola Just my $0.02, but I feel like the single-word restriction is specifically confusing in this "cancelling the cancel" case. I've been happy with products that I see use the opposite tactic when one word isn't enough-- very explicit and verbose buttons.

"Yes, cancel these migrations" / "No, do not cancel anything" or something to that effect.

I think using one word where it's not confusing, and then just going straight for a sentence otherwise instead of trying to find the two word button that works, is good. End of non-designer opinion.

AparnaKarve · 2018-07-26T14:25:04Z

@AllenBW You will need ManageIQ/manageiq-api#421

AllenBW · 2018-07-26T14:31:06Z

Ahhhh that's the missing piece, thanks @AparnaKarve !

AllenBW · 2018-07-26T14:59:54Z

app/javascript/react/screens/App/Plan/components/PlanRequestDetailList/PlanRequestDetailList.js

+    const { selectedCancelTasks } = this.state;
+    const { cancelPlanRequestTasksAction, cancelPlanRequestTasksUrl } = this.props;
+    cancelPlanRequestTasksAction(cancelPlanRequestTasksUrl, selectedCancelTasks);
+    this.setState({ showConfirmCancel: false });


Modal window isn't hiding after firing this 😕

Can you check again? The modal seems to be hiding OK for me.

AllenBW · 2018-07-26T15:03:13Z

It might be something wacky I did... or desired behavior... but after canceling two in progress migrations... they get the plan appears to be stuck in inprogress purgatory... meaning... it just hangs out inprogress tab... with no action aside from download logs available

AparnaKarve · 2018-07-26T15:13:13Z

I have noticed some strange behavior with Cancel locally as well... hence I think the real valid test of canceling a migration should be done on an appliance - it seemed to work a lot better on an appliance yesterday.

So based on the testing that can be done locally, I will try and fix some of the UI/UX issues that @michaelkro noticed above, but the bulk of the testing needs to be done on an appliance.

I will try and setup an appliance sometime today for testing this.

vconzola · 2018-07-26T15:16:32Z

@michaelkro I concur with @AparnaKarve expected behavior comment above.

mturley · 2018-07-26T17:05:04Z

Working on some of the quirky behavior around the checkboxes, since it's related to stuff I did. I'll have a commit for @AparnaKarve to cherry-pick shortly

mturley · 2018-07-26T19:03:38Z

Posting this here for posterity, it might conflict with @AparnaKarve's redux stuff, but it should fix some behavior around the bulk selector: mturley@a65868d

AparnaKarve · 2018-07-27T13:32:27Z

After implementing the redux approach, came across a few use cases where the user can sneak in a Cancel on an already canceled task (in the brief window until the next polling happens)

Trying to address some of these cases...

AparnaKarve · 2018-07-27T15:19:40Z

@michaelkro @AllenBW @mturley Please test/review when you get a chance.

the plan appears to be stuck in inprogress purgatory... meaning... it just hangs out inprogress tab

@AllenBW We usually don't see this issue on appliances... although I have tried to address this in c3fe254,
86acf0d and
407f8b9

I will try and setup an appliance to test this, but note that, currently migrations are failing on the appliances in the "Pre-Migration" step - we need an automate PR to fix this

Note for future reference: `taskCancelled` indicates that a task has already been cancelled (inferred from the API output) whereas `taskCancelling` indicates a task that was marked for cancellation in the UI Both states will display the `fa-ban` icon in the listview and display the text `Canceled Migration` for the task

- Remove completed task from selection - Apply cancellation for incomplete tasks only

unknown status messages are usually about stalled migrations, hence the status message - `VM migrations stalled`

mturley · 2018-07-27T15:42:38Z

Thank you so much for your hard work on this @AparnaKarve.

michaelkro · 2018-07-27T15:42:57Z

app/javascript/react/screens/App/Plan/PlanActions.js

+export const cancelPlanRequestTasksAction = (url, tasks) => dispatch => {
+  const completedTasks = [];
+  const incompleteTasks = [];
+  tasks.map(t => (t.completed ? completedTasks.push(t) : incompleteTasks.push(t)));


While this ultimately has the desired effect, I think we may want to use forEach here. In my experience, we use map when we want to do something with the returned array. In this case, it seems that we are iterating through tasks (rather than mapping it to a new array to be used later)

Beat me to it!

Addressed in the last commit

mturley

Not going to request changes, because these are pretty minor nit picks for readability.

mturley · 2018-07-27T15:47:09Z

app/javascript/react/screens/App/Plan/PlanActions.js

+export const cancelPlanRequestTasksAction = (url, tasks) => dispatch => {
+  const completedTasks = [];
+  const incompleteTasks = [];
+  tasks.map(t => (t.completed ? completedTasks.push(t) : incompleteTasks.push(t)));


Since you don't use map's return value here, it could be a forEach instead.. but it really doesn't matter, and if this is the only thing I can find to comment on, I think your code looks great!

mturley · 2018-07-27T16:03:12Z

app/javascript/react/screens/App/Plan/PlanReducer.js

@@ -50,21 +56,32 @@ const excludeDownloadDoneTaskId = (allDownloadLogInProgressTaskIds, taskId) =>
 const includeDownloadInProgressTaskId = (allDownloadLogInProgressTaskIds, taskId) =>
  allDownloadLogInProgressTaskIds ? allDownloadLogInProgressTaskIds.concat(taskId) : [taskId];

+const removeCancelledTasksFromSelection = (allSelectedTasksForCancel, alreadyCancelledTasks) => {


This is fine, but a little odd looking, and will run in n^2 time (I think? been a while since big-O lectures). Here's my approach on this one, taking the map out and only using one filter, and then using every which will short circuit if it sees a non-matching element.

Not tested, but:

const removeCancelledTasksFromSelection = (allSelectedTasksForCancel, alreadyCancelledTasks) => ( allSelectedTasksForCancel.filter(selectedTask => alreadyCancelledTasks.every(cancelledTask => selectedTask.id !== cancelledTask.id) ) );

Not that we need to optimize for runtime here, but I think that's more readable too.

Good suggestion.

I have applied this change in the last commit.

michaelkro

Not a blocker

Bulk selector becomes checked when there are no selectable tasks

mturley · 2018-07-27T16:10:38Z

@michaelkro I think that's the bug I thought we were seeing at first yesterday, which should be fixed by my commit here in the old implementation: mturley@a65868d. @AparnaKarve I agree that it's not a blocker, but you could port this line from my commit into your logic to fix this.

const allSelected = !noneSelected && selectedCancelTasks.length === incompleteTasks.length;

(it's the !noneSelected that we need)

mturley · 2018-07-27T16:17:41Z

app/javascript/react/screens/App/Plan/components/PlanRequestDetailList/PlanRequestDetailList.js

+    return {
+      filteredTasks,
+      incompleteTasks,
+      allSelected: selectedTasksForCancel.length === incompleteTasks.length,


Ah, @AparnaKarve I didn't notice you based your work on a slightly older version of getCancelSelectionState. I pushed a newer one on Wednesday that fixed the bug @michaelkro was just reporting (select all checkbox is checked when there are no selectable tasks).

We just need to pull noneSelected up into a variable, and then do allSelected: !noneSelected && ...

Fixed the bulk selector issues in the last commit

michaelkro · 2018-07-27T16:49:05Z

I've been trying (unsuccessfully) to find what is making this happen, but it looks like the listview resorts after a cancellation request (after the next poll)

AllenBW

LG2M!!

Any final input @michaelkro @mturley ?

michaelkro

The resorting thing is the only thing that I'm a little concerned about, but it could be due to local environment quirks. I'm ok with handling that as a separate issue

AparnaKarve · 2018-07-27T17:02:28Z

Ok, so just a reminder that we need the API PR merged in first before this.

While I have some time until the API PR is merged, let me check what more I can address from the above comments...
(plus squashing some commits is also still TBD)

@michaelkro The resorting thing is independent of this PR (KK also mentioned to me about it once) - so yes, it could be handled as a separate issue.

mturley · 2018-07-27T17:05:05Z

Code all looks good to me, I haven't tested your new commits locally yet since yesterday morning though. While you're wrapping up I'll see if I can find any more issues.

mturley

LGTM! Thanks again @AparnaKarve @michaelkro and @priley86.

AparnaKarve · 2018-07-27T17:52:20Z

Thanks everyone for the reviews and testing!

I have done yet another round of addressing feedback - 5f9c89b
... but I am gonna stop now.

Currently keeping an eye on the API PR... will let you all know when this can be merged.

mturley

Looks great! Thanks so much for taking this over @AparnaKarve .

mturley · 2018-07-30T16:19:54Z

app/javascript/react/screens/App/Plan/PlanReducer.js

-      (allSelectedTasksForCancel = allSelectedTasksForCancel.filter(element => element.id !== cancelledTask.id))
+const removeCancelledTasksFromSelection = (allSelectedTasksForCancel, alreadyCancelledTasks) =>
+  allSelectedTasksForCancel.filter(selectedTask =>
+    alreadyCancelledTasks.every(cancelledTask => selectedTask.id !== cancelledTask.id)


🎉 I love the lesser-known Array methods :)

mturley · 2018-07-30T16:20:38Z

app/javascript/react/screens/App/Plan/components/PlanRequestDetailList/PlanRequestDetailList.js

@@ -285,7 +285,7 @@ class PlanRequestDetailList extends React.Component {
    return {
      filteredTasks,
      incompleteTasks,
-      allSelected: selectedTasksForCancel.length === incompleteTasks.length,
+      allSelected: selectedTasksForCancel.length === incompleteTasks.length && selectedTasksForCancel.length > 0,


…igrations [ManageIQ#370] Cancel a migration's in-progress tasks

simaishi · 2018-08-01T21:10:51Z

Backported to Gaprindashvili via #525

AparnaKarve force-pushed the cancel_in_progress_migrations branch from 6e426f3 to 025b580 Compare July 26, 2018 02:02

AparnaKarve added blocker gaprindashvili/yes labels Jul 26, 2018

AllenBW added the enhancement label Jul 26, 2018

AllenBW requested review from michaelkro, mturley and AllenBW July 26, 2018 11:25

AllenBW self-assigned this Jul 26, 2018

michaelkro reviewed Jul 26, 2018

View reviewed changes

AllenBW reviewed Jul 26, 2018

View reviewed changes

mturley mentioned this pull request Jul 26, 2018

[WIP][#370] Cancel a migration's tasks #511

Closed

AparnaKarve force-pushed the cancel_in_progress_migrations branch 2 times, most recently from 12b7d3f to 7c28590 Compare July 27, 2018 14:55

AparnaKarve added 4 commits July 27, 2018 08:39

edge case - task completed just prior to cancellation

143f807

- Remove completed task from selection - Apply cancellation for incomplete tasks only

edge case - Address unknown status messages

e672472

unknown status messages are usually about stalled migrations, hence the status message - `VM migrations stalled`

PR feedback

4af01f9

michaelkro reviewed Jul 27, 2018

View reviewed changes

mturley reviewed Jul 27, 2018

View reviewed changes

michaelkro reviewed Jul 27, 2018

View reviewed changes

mturley reviewed Jul 27, 2018

View reviewed changes

AllenBW approved these changes Jul 27, 2018

View reviewed changes

michaelkro approved these changes Jul 27, 2018

View reviewed changes

mturley approved these changes Jul 27, 2018

View reviewed changes

PR feedback - take two!

5f9c89b

AparnaKarve force-pushed the cancel_in_progress_migrations branch from 7c28590 to 5f9c89b Compare July 27, 2018 17:43

AllenBW added the waiting-on-another-issue label Jul 27, 2018

mturley approved these changes Jul 30, 2018

View reviewed changes

michaelkro removed the waiting-on-another-issue label Jul 30, 2018

michaelkro merged commit 445c6a1 into ManageIQ:master Jul 30, 2018

AparnaKarve deleted the cancel_in_progress_migrations branch July 30, 2018 23:12

AparnaKarve pushed a commit to AparnaKarve/manageiq-v2v that referenced this pull request Jul 31, 2018

Merge pull request ManageIQ#517 from AparnaKarve/cancel_in_progress_m…

3cb28d8

…igrations [ManageIQ#370] Cancel a migration's in-progress tasks

AparnaKarve mentioned this pull request Jul 31, 2018

[GAPRINDASHVILI] Cancel a migration's in-progress tasks #525

Merged

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Aug 1, 2018

[#370] Cancel a migration's in-progress tasks #517

[#370] Cancel a migration's in-progress tasks #517

Conversation

AparnaKarve commented Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkro left a comment

Choose a reason for hiding this comment

michaelkro left a comment • edited Loading

Choose a reason for hiding this comment

AparnaKarve commented Jul 26, 2018

AllenBW left a comment • edited Loading

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

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

AparnaKarve commented Jul 26, 2018

AllenBW commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllenBW commented Jul 26, 2018

AparnaKarve commented Jul 26, 2018

vconzola commented Jul 26, 2018

mturley commented Jul 26, 2018

mturley commented Jul 26, 2018

AparnaKarve commented Jul 27, 2018

AparnaKarve commented Jul 27, 2018

mturley commented Jul 27, 2018

michaelkro Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkro left a comment

Choose a reason for hiding this comment

mturley commented Jul 27, 2018 • edited Loading

mturley Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkro commented Jul 27, 2018

AllenBW left a comment

Choose a reason for hiding this comment

michaelkro left a comment

Choose a reason for hiding this comment

AparnaKarve commented Jul 27, 2018

mturley commented Jul 27, 2018

mturley left a comment

Choose a reason for hiding this comment

AparnaKarve commented Jul 27, 2018

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simaishi commented Aug 1, 2018

AparnaKarve commented Jul 26, 2018 •

edited

Loading

mturley Jul 26, 2018 •

edited

Loading

michaelkro left a comment •

edited

Loading

AllenBW left a comment •

edited

Loading

michaelkro Jul 27, 2018 •

edited

Loading

mturley Jul 27, 2018 •

edited

Loading

mturley commented Jul 27, 2018 •

edited

Loading

mturley Jul 27, 2018 •

edited

Loading