[#437] Resolve all Retry Migration issues #468

AparnaKarve · 2018-07-03T23:15:24Z

Updates Progress bar in the main Migration area
Displays correct number of VMs in the Completed List
Displays the VMs that were successfully migrated in the previous requests in Plan Detail screen

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

AparnaKarve · 2018-07-03T23:19:57Z

With @kedark3 's assistance, an appliance was set up where a Plan had 2 out of 3 VMs migrated successfully.
A Retry operation was tried on this appliance for this Plan to validate the PR - results were successful.

AllenBW · 2018-07-04T12:32:04Z

app/javascript/react/screens/App/common/commonUtilitiesHelper.js

+      {}
+    ),
+
+  flattenArray: (arrayItems, flattenedArray) =>


looks like we could use flatten here https://lodash.com/docs/4.17.4#flatten?

I'm going to have to respectfully disagree here. I think it's preferred to use es6 methods when the functions are trivial and there is not much more to implement using a native function. Individual Lodash imports will soon be stripped by Webpack tree shaking techniques and loaded individually, however I still don't think that's a reason to prefer them (since they often times add more code on the loading end). In these cases I prefer to extend the native functions which removes the need for those imports. I have seen cases where lodash functions do provide a lot of value (sortBy for example is non trivial), but these are specific to the use case...

I will believe this was mentioned in several Foreman PRs as well...

Ooooo! We disagree indeed. I don't have performance metrics to compare, so I cannot assert one method is "better than another" 🤷‍♂️ BUT the less code we have to maintain, watch something go wrong with, the better. Both of these functions have been written before, I see no strong case for us to rewrite them.

☝️ being said, this is a blocker, my personal preference to not rewrite commonly functions will not hinder approval of this

Yea, obviously we are still loading Lodash and not stripping it...so it's already included and would be minimal impact on this project, but long term I still think it's considered bad practice now by most communities (kind of like using jQuery at this point, we avoid it when we can but obviously we still rely on it in some places). Reference:
https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_flatten

If we end up converting things to lodash, it becomes preferential to do what Foreman did w/ the lodash babel plugin. Went ahead and referenced that in 3776
but happy to open a separate issue if we care at all about the performance of lodash.

Love love the link, thanks for sharing!

Ok mmm I would make one humble request, that we keep the naming consistent, renaming this to flatten would be nice 🤔

sure, not a problem!

AllenBW · 2018-07-04T12:36:22Z

app/javascript/react/screens/App/common/commonUtilitiesHelper.js

+  getMostRecentEntityByCreationDate: entities =>
+    entities.reduce((prev, current) => (prev.created_on > current.created_on ? prev : current)),
+
+  groupBy: (items, key) =>


So this function exists in lodash, a tool we use elsewhere in v2v, https://lodash.com/docs/4.17.4#groupBy

Wonder if using ☝️ might be a lil less code

same as above...

AparnaKarve · 2018-07-05T15:04:42Z

On an unrelated topic to the above discussion, there might be 2 open items to the Retry issue (depending on how we look at it)

Item 1 -
Consider this scenario -
In the first migration attempt, 2 out of 3 VMs migrated successfully.
When I Retry the same plan, the Migration card in the main screen shows
0 of 1 VMs migrated

-- should the Migration card say 2 of 3 VMs migrated instead?

Item 2 -
When the Retry Migration completes, what should we show for Elapsed Time in the main Overview page?
Note that, in this scenario the Plan has completed in several attempts (> 1 Request), and the Requests themselves could have been ordered over a span of several minutes/hours/days.

priley86 · 2018-07-05T16:35:24Z

Does this also include the fix for the recently opened #469 @AparnaKarve ?

AllenBW · 2018-07-05T16:53:10Z

Item 1 - Sonds very reasonable to me! Migrated implies success, only two of them succeeded,unless we wanta negate the verbiage... 🤔

Item 2 - yeah this is tricky, the rub is that not all the VMs will be migraine at the same time some will have already been completed while others will still be needing to be completed... Maybe it should be total elapsed time.. Including the retry time..

AparnaKarve · 2018-07-05T17:07:37Z

@priley86 Yes, it includes a fix for #469 -- I will add it to the above comment.

AllenBW · 2018-07-05T18:49:49Z

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

+      transformation_host_name: task.options && task.options.transformation_host_name,
+      delivered_on: new Date(task.options.delivered_on),
+      updated_on: new Date(task.updated_on),
+      completed: task.message === 'VM Transformations completed' || task.message === 'VM Transformations failed',


Is this a string that might change with translation? 🤔

Somewhat related: we actually already have an issue about those... if we want to tackle that in a future PR (or in this one): #255

"VM Transformations completed" vs "Migrations completed"

Ideally, we can create some sort of dictionary to map them to the UI translation on the Redux end... I don't know if we tackle this now though...

Those strings are coming from the backend -- hopefully the backend has gettexted them.

But you are right, this should be -

completed: task.message === __('VM Transformations completed') || task.message === __('VM Transformations failed'),

We can't control the backend, but we can at least do our part.

I have also found a couple of incorrect sprintf usages - i.e. incorrect in the context of i18n.

I'm thinking of handling this string and a bunch of other strings in a separate PR for addressing all known i18n issues.

AllenBW · 2018-07-05T18:55:06Z

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

-      if (!task.diskSpaceCompletedGb) {
-        taskDetails.diskSpaceCompletedGb = '0';
-      }
+  vmTasks.forEach(task => {


any chance vmTasks could ever be null/undefined?

No, not in PlanReducer - i.e., we are always expected to have tasks in the Plan detail screen.
(PlanReducer is the reducer for Plan detail screen)

AparnaKarve · 2018-07-05T22:50:02Z

Per our discussion, I have kept the elapsed time in the Overview screen to reflect the elapsed time of the most recent request.

PlanB in the below screenshot in it's most recent request had 1 VM to migrate (Webserver-01). The migration for this VM took 4min:38sec

The Plan detail page shows the VM, Webserver-01 that took 4min:38sec -

/cc @vconzola

AllenBW

Awesome additions @AparnaKarve !

🏆 ⚓️ 👩‍🚀

priley86 · 2018-07-06T17:16:28Z

app/javascript/react/screens/App/Overview/components/Migrations/MigrationsInProgressCard.js

+  }
+  tasksOfPlan.forEach(task => {
+    tasks[task.source_id] = tasks[task.source_id] || {};
+    tasks[task.source_id].completed = task.status === 'Ok' && task.state === 'finished';


we still haven't extracted these task statuses to constants... since the number of places these are used is increasing, and we have pre/post playbook support upcoming, i think it would be nice to move those to constants in a future PR

priley86 · 2018-07-06T17:17:05Z

pulled this locally earlier today...works for me!

Once we have all requests with tasks for a given plan, reuse functions from `commonUtilitiesHelper` to retrieve the most recent tasks from the requests object based on the VM id's

AparnaKarve · 2018-07-09T21:54:20Z

@priley86 @AllenBW In the last commit, I have made the necessary adjustments for ManageIQ/manageiq#17674

Should be GTG now.

priley86 · 2018-07-09T21:56:49Z

thanks! Will be nice to get this one to QE sooner than later...merging...

AllenBW

OH snap, I though I had already reviewed the last version of this...

but also 👍 ❤️

AparnaKarve · 2018-07-09T22:55:45Z

@AllenBW Yeah the new structure options.config_info.actions.vm_id caught us all by surprise on Friday...since this PR was getting impacted with that change, I had to do some adjustments.

BTW, your old plans won't work anymore because of this new structure - so you would have to delete them all and create new ones.

simaishi · 2018-07-11T15:59:04Z

@AparnaKarve Please create a separate PR for Gaprindashvili due to conflicts...

$ git status
On branch gaprindashvili
Your branch is ahead of 'origin/gaprindashvili' by 3 commits.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit 2e91e28.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   app/javascript/components/index.js
	new file:   app/javascript/react/screens/App/Overview/components/Migrations/helpers/getMostRecentVMTasksFromRequests.js
	modified:   app/javascript/react/screens/App/Overview/overview.requestWithTasks.fixtures.js
	modified:   app/javascript/react/screens/App/Overview/overview.transformationPlans.fixtures.js
	modified:   app/javascript/react/screens/App/Plan/Plan.js
	modified:   app/javascript/react/screens/App/Plan/PlanActions.js
	modified:   app/javascript/react/screens/App/Plan/PlanConstants.js
	new file:   app/javascript/react/screens/App/common/commonUtilitiesHelper.js

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   app/javascript/react/screens/App/Overview/components/Migrations/MigrationsCompletedList.js
	both modified:   app/javascript/react/screens/App/Overview/components/Migrations/MigrationsInProgressCard.js
	both modified:   app/javascript/react/screens/App/Plan/PlanReducer.js

AparnaKarve · 2018-07-11T22:16:29Z

@simaishi Would it be possible to merge ManageIQ/manageiq#17627 and ManageIQ/manageiq#17674 first before I create a G specific PR for this?
Since the last commit in this PR (e6f6846) has adjustments that were made because the above PRs were merged.

If it's not possible to merge the above PRs at this time, I might have to exclude the last commit from this PR and then add it after those PRs are merged.

simaishi · 2018-07-11T22:23:17Z

@AparnaKarve I was holding off backporting those, as PRs/BZs aren't properly linked together and I can't tell which PRs are needed for pre/post feature... I hope to get them cleared out in the next few days, so let's hold off creating G-specific PR for this until next week.

AparnaKarve · 2018-07-11T22:25:45Z

@simaishi No problem - we can wait.
Thanks!!

simaishi · 2018-07-25T14:44:00Z

@AparnaKarve FYI, ManageIQ/manageiq#17627 and ManageIQ/manageiq#17674 are backported to Gaprindashvili branch now.

JPrause · 2018-07-26T19:11:34Z

@AparnaKarve was there further work needed to resolve the conflict.

AparnaKarve · 2018-07-26T22:38:11Z

Gaprindashvili specific PR - #521

simaishi · 2018-07-30T23:51:35Z

Backported to Gaprindashvili via #521

AllenBW added bug blocker gaprindashvili/yes labels Jul 4, 2018

AllenBW reviewed Jul 4, 2018

View reviewed changes

AllenBW changed the title ~~Resolve all Retry Migration issues~~ [#437] Resolve all Retry Migration issues Jul 5, 2018

AllenBW reviewed Jul 5, 2018

View reviewed changes

AparnaKarve force-pushed the show_progress_in_migration_card branch 2 times, most recently from 00ecba1 to 29844e8 Compare July 5, 2018 22:41

AllenBW approved these changes Jul 6, 2018

View reviewed changes

priley86 reviewed Jul 6, 2018

View reviewed changes

priley86 approved these changes Jul 6, 2018

View reviewed changes

himdel mentioned this pull request Jul 7, 2018

Webpack 4 ManageIQ/manageiq-ui-classic#3776

Merged

15 tasks

AparnaKarve added 8 commits July 9, 2018 13:54

Use mostRecentRequest to get the task details

8600e7b

request and task math to get previous task records

a2ef240

create a helper for reusing utils like groupBy and flatten array

822de91

API now uses a new url to get tasks for all requests of a given plan

dc3ea31

Once we have all requests with tasks for a given plan, reuse functions from `commonUtilitiesHelper` to retrieve the most recent tasks from the requests object based on the VM id's

fix tests

9cffc1f

Extract Task/Request logic in a helper (for reuse)

e00904f

Use the helper to get most recent VM tasks for Plan

f689f89

Use the helper in PlanReducer to get most recent VM tasks for Plan

a45a951

AparnaKarve added 2 commits July 9, 2018 13:54

PR feedback - rename flattenArray to flatten

8228673

Adjustments w.r.t the new structure of the vm_id array retrieval

e6f6846

AparnaKarve force-pushed the show_progress_in_migration_card branch from 29844e8 to e6f6846 Compare July 9, 2018 21:47

priley86 merged commit 2e91e28 into ManageIQ:master Jul 9, 2018

AllenBW reviewed Jul 9, 2018

View reviewed changes

AparnaKarve deleted the show_progress_in_migration_card branch July 9, 2018 22:56

This was referenced Jul 9, 2018

Migration Status string constants for i18n #476

Merged

Fix the Archived Plan view #479

Merged

AparnaKarve added the Sprint 10 label Jul 10, 2018

AparnaKarve mentioned this pull request Jul 10, 2018

Progress bar for Retried migration is not updating #469

Closed

simaishi added the gaprindashvili/conflict label Jul 11, 2018

AparnaKarve mentioned this pull request Jul 16, 2018

Upon retrying failed migration plans, how do we present plan details ? #374

Closed

AparnaKarve mentioned this pull request Jul 26, 2018

[GAPRINDASHVILI] Resolve all Retry Migration issues #521

Merged

simaishi added gaprindashvili/backported and removed gaprindashvili/conflict labels Jul 30, 2018

[#437] Resolve all Retry Migration issues #468

[#437] Resolve all Retry Migration issues #468

Conversation

AparnaKarve commented Jul 3, 2018 • edited by simaishi Loading

AparnaKarve commented Jul 3, 2018

AllenBW Jul 4, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Jul 5, 2018

priley86 commented Jul 5, 2018

AllenBW commented Jul 5, 2018 • edited Loading

AparnaKarve commented Jul 5, 2018

AllenBW Jul 5, 2018 • 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

AparnaKarve commented Jul 5, 2018

AllenBW left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priley86 commented Jul 6, 2018

AparnaKarve commented Jul 9, 2018

priley86 commented Jul 9, 2018

AllenBW left a comment • edited Loading

Choose a reason for hiding this comment

AparnaKarve commented Jul 9, 2018

simaishi commented Jul 11, 2018

AparnaKarve commented Jul 11, 2018

simaishi commented Jul 11, 2018

AparnaKarve commented Jul 11, 2018

simaishi commented Jul 25, 2018

JPrause commented Jul 26, 2018

AparnaKarve commented Jul 26, 2018

simaishi commented Jul 30, 2018

AparnaKarve commented Jul 3, 2018 •

edited by simaishi

Loading

AllenBW Jul 4, 2018 •

edited

Loading

AllenBW commented Jul 5, 2018 •

edited

Loading

AllenBW Jul 5, 2018 •

edited

Loading

AllenBW left a comment •

edited

Loading