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

fix: [M3-7630] - Fix Kubernetes Upgrade Flow #10057

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Jan 10, 2024

Description 📝

  • Fixes the Kubernetes Upgrade Flow
  • This bug was not caught by our automated testing because we did not have full coverage for the upgrade flow

The Bug 🐛

  • On the Kubernetes Upgrade flow on the Kubernetes Details page, users were not able to reach step 2 of the upgrade flow. The Upgrade dialog would close after submitting step 1.
    • This was caused by us rendering null in our upgrade component when we still need to render step 2
  • This bug only applies to the details page
    • This was the case because on the details page, we passed the version of the cluster to the upgrade dialog from the React Query store, which caused the dialog to close immediately when the version updated. The reason this did not happen to the landing page is because the cluster's current version was stored in an intermediary state that was not reactive to the React Query cache.

The Solution 🔧

  • Don't render null if the Kubernetes version is up to date. We must continue to render the component so that the user can reach step 2, which is recycling the nodes.

Preview 📷

Before After
Screen.Recording.2024-01-10.at.6.12.29.PM.mov
Screen.Recording.2024-01-10.at.5.41.44.PM.mov

How to test 🧪

Reproduction steps

  • Create a Kubernetes Cluster with version 1.27
  • Go to the details page for that Kubernetes cluster
  • Try to upgrade the Kubernetes cluster to 1.28
  • Observe that when you click "Upgrade Version", the dialog closes and no step 2 shows up ☹️

Verification steps

  • Create a Kubernetes Cluster with version 1.27
  • Go to the details page for that Kubernetes cluster
  • Try to upgrade the Kubernetes cluster to 1.28
  • Verify you can complete both step 1 and step 2 successfully 🎉

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@bnussman-akamai bnussman-akamai added the Bug Fixes for regressions or bugs label Jan 10, 2024
@bnussman-akamai bnussman-akamai self-assigned this Jan 10, 2024
@bnussman-akamai bnussman-akamai requested review from a team as code owners January 10, 2024 23:13
@bnussman-akamai bnussman-akamai requested review from jdamore-linode, dwiley-akamai and hana-akamai and removed request for a team January 10, 2024 23:13
@bnussman-akamai bnussman-akamai added the LKE Related to Linode Kubernetes Engine offerings label Jan 10, 2024
Comment on lines -52 to -54
if (nextVersion === null) {
return null;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Because the React Query cache updates immediately when the PUT request happens to update the Cluster's version, it caused nextVersion to become null, which is expected behavior. This caused the dialog to stop rendering. We need the dialog to remain open even though the version update was successful so that we can show them step 2 of recycling their nodes.

I'm sure there are other ways to fix this (for example: restructuring the Dialogs), but I took the most simple approach for the time being

Comment on lines +53 to +56
if (!nextVersion) {
setError('Your Kubernetes Cluster is already on the latest version.');
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than using a non-null assertion, we'll show this error. This case should never really happen

Copy link

github-actions bot commented Jan 10, 2024

Coverage Report:
Base Coverage: 79.83%
Current Coverage: 79.86%

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.

Thanks Banks! Thanks for fixing this test too 🤦‍♂️

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this 🔧

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Jan 11, 2024
@bnussman-akamai bnussman-akamai merged commit a4ed4cf into linode:develop Jan 11, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Bug Fixes for regressions or bugs LKE Related to Linode Kubernetes Engine offerings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants