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

feat: [M3-7114] - Allow user to choose resize migration type when resizing Linode #9677

Merged
merged 31 commits into from
Oct 11, 2023

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Sep 14, 2023

Description 📝

This work will introduce warm migrations which combine the benefits of cold and live migrations to greatly reduce customer downtime when resizing Linodes.

Important
The notification and event copy/messaging for warm migrations will be updated with API work, but you test the UI flow, payload, feature flags and everything else. I will have to update the cypress tests later with actual copy (9/27)

Major Changes 🔄

  • Add feature flag unifiedMigrations

Resize Flow:

  • Add an optional migration_type POST field for the linode/instances/.../resize endpoint that should be warm or cold. Backend defaults to cold, but in the UI we are defaulting to warm unless the user chooses otherwise.
  • Add a radio button group for selecting warm or cold migrations in the LinodeResize flow.
    • If the Linode is powered off, we disable controls (even though warm is selected by default) and proceed with a cold migration.
  • Update & add cypress E2E tests for changes

Linode Landing:

  • As a user the flag icon is ambiguous and doesn't give much meaning, therefore we want to remove the icon altogether and provide and updated status where it makes sense. For resizes, we want to change the wording from running to migrating with a green dot icon

Other:

  • Fixed the animations for <Notice /> and fixed a bug in our fadeIn keyframes. Pesky colons!
    • In addition, the padding for the <Notice /> didn't seem right or match <DismissibleBanner /> which seemed to be the intention, so this has been fixed.

TODO:

  • Waiting on APIv4 dev work with event/notification messaging to update Cypress tests
  • Waiting on UX copy for messaging

Preview 📷

Before After
Screenshot 2023-09-13 at 9 30 01 PM Screenshot 2023-09-13 at 9 29 52 PM
Event Notification
Screenshot 2023-09-15 at 11 56 01 AM
Enabled Disabled
Screenshot 2023-09-28 at 9 29 14 AM Screenshot 2023-09-28 at 9 27 39 AM
Payload
Screenshot 2023-09-13 at 3 49 32 PM
Linode Landing
Screenshot 2023-09-27 at 10 34 27 PM Screenshot 2023-09-27 at 10 30 26 PM
Feature Flag
Screenshot 2023-09-25 at 7 45 22 PM

How to test 🧪

  1. Enable Unified Migrations feature flag via dev tools
  2. Go to Linodes landing page and resize a Linode from the action menu
  3. Observe you now have a radio group to choose warm or cold migrations with warm set as the default
  4. When pressing Resize Linode you should see a confirmation dialog appear before resize takes effect
  5. During resize we should see resize events as outlined in screenshot and notification (notice) on Linode detail page.
    1. Note: The copy for a warm resize has not been updating yet (awaiting API), but you should see the payload with the new migration_type: warm as shown in screenshot.
  6. A cold resize does NOT have any additional confirmation
  7. Confirm the confirmation dialog closes on error + the resize panel scrolls to the error for a warm migration; confirm that the resize panel scrolls to the error for a cold migration
  8. Confirm that errors do not persist upon closing/reopening the resize panel (for both the same linode and a different linode)
  9. Confirm that if a linode is offline, default is cold migration + the panel is disabled and a tooltip appears. Confirm that even if you close/reopen the resize panel, the default stays as the cold migration
  10. Confirm the feature flag works as expected
  11. Confirm the 'Migration Types' label in the migrations panel is not highlighted when choosing between the migration types

@mjac0bs
Copy link
Contributor

mjac0bs commented Sep 15, 2023

In addition, the padding for the didn't seem right or match which seemed to be the intention, so this has been fixed.

Thanks for fixing this! 🎉 🥇 I'd recently noticed that a few notices were looking weirdly skinny, seemingly randomly, but hadn't tracked it down or made a ticket.

@jaalah-akamai jaalah-akamai marked this pull request as ready for review September 26, 2023 01:52
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner September 26, 2023 01:52
@jaalah-akamai jaalah-akamai requested review from dwiley-akamai and coliu-akamai and removed request for a team September 26, 2023 01:52
@jaalah-akamai jaalah-akamai changed the title WIP - feat: [M3-7114] - Add Migration Types for Linode Resize feat: [M3-7114] - Add Migration Types for Linode Resize Sep 26, 2023
@jaalah-akamai jaalah-akamai changed the title feat: [M3-7114] - Add Migration Types for Linode Resize feat: [M3-7114] - Allow user to choose resize migration type when resizing Linode Sep 26, 2023
Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Hoping to circle back soon with a proper review, but just swinging in now to thank you for the Cypress test additions!

@jaalah-akamai jaalah-akamai added the @linode/api-v4 Changes are made to the @linode/api-v4 package label Sep 26, 2023
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

✅ Observe you now have a radio group to choose warm or cold migrations
✅ Warm is set as default
✅ When pressing Resize Linode you should see a confirmation dialog appear before resize takes effect
✅ During resize we should see resize events as outlined in screenshot and notification (notice) on Linode detail page. Note: The copy for a warm resize has not been updating yet (awaiting API), but you should see the payload with the new migration_type: warm as shown in screenshot.
✅ A cold resize does NOT have any additional confirmation (assuming this means no confirmation dialog)

Something interesting to note: when I clicked 'Continue' in the confirmation popup, if an error appears, the popup stays -- not sure if this is the behavior you want or not:
image

packages/api-v4/src/linodes/actions.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Nice! looks great overall.

A couple comments on the UI:

This tooltip should be wider. The copy is a bit lengthy and It's pretty hard to read currently.
Screenshot 2023-09-27 at 17 30 32


The logic for disabling the field if a linode is powered down is implemented, however there's no explanation as to why. Yes, it's logical we can't perform a warm migration on a powered down Linode, but I don't think we should assume a user will immediately figure out (it's a new feature). I think the UX is lacking here and I'd rather see the "Cold" selection enabled with no option to switch to the warm option and some helper text as to why.

Screenshot 2023-09-27 at 17 33 04

@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Sep 28, 2023

@abailly-akamai:

  • I finished updating our TooltipIcon to be more flexible and allow a width to be passed in for variable widths, and then I realized the latest mock excludes a tooltip altogether. Anyways I kept it in because I think it could be useful and added a storybook example.
  • I agree with your second concern and I sent it over to UX. We either need some explanation or we just set it to cold in the disabled state. I'm awaiting their reply! Hopefully it doesn't hold up this PR because it will need to be in next release. 🤞

Update: I addressed your second concern! Updated description

Screenshot 2023-09-27 at 9 20 21 PM

@coliu-akamai The dialog should close on error and scroll to the top still, good catch 👍

@jaalah-akamai
Copy link
Contributor Author

@abailly-akamai I also want to point out some additional changes for the Linodes Landing page (updated description):

@coliu-akamai coliu-akamai self-requested a review September 28, 2023 13:17
@coliu-akamai
Copy link
Contributor

coliu-akamai commented Sep 28, 2023

Will be checking these off as I go through them

  • Observe you now have a radio group to choose warm or cold migrations
  • Warm is set as default
  • When pressing Resize Linode you should see a confirmation dialog appear before resize takes effect
  • During resize we should see resize events as outlined in screenshot and notification (notice) on Linode detail page. Note: The copy for a warm resize has not been updating yet (awaiting API), but you should see the payload with the new migration_type: warm as shown in screenshot.
  • A cold resize does NOT have any additional confirmation (assuming this means no confirmation dialog)
  • dialog closes on error + scrolls
  • if linode is offline, default is cold migration + panel disabled
  • feature flag works as expected

@coliu-akamai
Copy link
Contributor

coliu-akamai commented Sep 28, 2023

Are errors supposed to be cleared if we close the panel and then reopen? The following error is persistent (which honestly I could see making sense for at least this specific error but maybe not other errors -- just wanted to double check):
image

edit: Other errors persistent as well

@abailly-akamai
Copy link
Contributor

@jaalah thx for the changes - looks much better 👍

I also noticed there's an issue with persisting state in the resize modal which I think is unrelated to this PR, but which this PR makes more evident. See video below, look at the persisting error banner and new modal across various resizes attempts

Screen.Recording.2023-09-28.at.09.52.57.mov

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

  • Feature flag functionality ✅
  • Radio button group for selecting warm or cold migration in the Linode Resize modal ✅
    • Radio button group disabled if Linode is powered off ✅
    • Confirmation modal displayed if “Warm resize” chosen ✅
  • Events/Notifications appear as expected ✅

When you click one of the radio buttons, the label gets highlighted:

Screen.Recording.2023-09-28.at.10.21.48.AM.mov

I'm also observing linodes not having their statuses updated properly after resizes in the Linodes landing table (e.g., resize one that was running --> the process completes and the linode shows as "Offline" in the table, although it's actually running; it doesn't update properly until a refresh), but that seems to be an existing issue?

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed that the issue with the error persisting is gone!
Holding off on approval while we're investigating this weird issue some more:

image

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Leaving my approval here tho it shouldn't count as one of the co-author.

Tested all scenari i could think of with a special focus on the dialog state management.

  • Resize modal content and UI behaviour for a powered down Linode ✅
  • Resize modal content UI behaviour for a powered Linode ✅
  • Copy ✅

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Pushed a quick fix for the one failing resize test! Looking good to me 👍

@jaalah-akamai
Copy link
Contributor Author

Screenshot 2023-10-10 at 9 29 04 AM

  • Moved (Recommended) outside of tooltip next to label text
  • Added Learn more. link to documentation for both cold/warm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge @linode/api-v4 Changes are made to the @linode/api-v4 package Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants