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.

Sign up for GitHub

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

Jump to bottom

fix: [M3-8726] - Users unable to upgrade Kubernetes version from landing page #11056

Merged
merged 3 commits into from
Oct 8, 2024
Merged

fix: [M3-8726] - Users unable to upgrade Kubernetes version from landing page #11056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
edb4a12
fix dialog unmounting during upgrade and add cypress test
bnussman Oct 7, 2024
1d66657
fix cypress test
bnussman Oct 8, 2024
bf99d87
add changeset
bnussman Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-11056-fixed-1728428139880.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Fixed
---

Users unable to upgrade Kubernetes version from landing page ([#11056](https://github.com/linode/manager/pull/11056))
59 changes: 58 additions & 1 deletion packages/manager/cypress/e2e/core/kubernetes/lke-update.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
mockRecycleAllNodes,
mockGetDashboardUrl,
mockGetApiEndpoints,
mockGetClusters,
} from 'support/intercepts/lke';
import {
mockGetLinodeType,
Expand Down Expand Up @@ -113,7 +114,7 @@ describe('LKE cluster updates', () => {
* - Confirms that Kubernetes upgrade prompt is shown when not up-to-date.
* - Confirms that Kubernetes upgrade prompt is hidden when up-to-date.
*/
it('can upgrade Kubernetes engine version', () => {
it('can upgrade kubernetes version from the details page', () => {
const oldVersion = '1.25';
const newVersion = '1.26';

Expand Down Expand Up @@ -215,6 +216,62 @@ describe('LKE cluster updates', () => {
ui.toast.findByMessage('Recycle started successfully.');
});

it('can upgrade the kubernetes version from the landing page', () => {
const oldVersion = '1.25';
const newVersion = '1.26';

const cluster = kubernetesClusterFactory.build({
k8s_version: oldVersion,
});

const updatedCluster = { ...cluster, k8s_version: newVersion };

mockGetClusters([cluster]).as('getClusters');
mockGetKubernetesVersions([newVersion, oldVersion]).as('getVersions');
mockUpdateCluster(cluster.id, updatedCluster).as('updateCluster');
mockRecycleAllNodes(cluster.id).as('recycleAllNodes');

cy.visitWithLogin(`/kubernetes/clusters`);

cy.wait(['@getClusters', '@getVersions']);

cy.findByText(oldVersion).should('be.visible');

cy.findByText('UPGRADE').should('be.visible').should('be.enabled').click();

ui.dialog
.findByTitle(
`Step 1: Upgrade ${cluster.label} to Kubernetes ${newVersion}`
)
.should('be.visible');

mockGetClusters([updatedCluster]).as('getClusters');

ui.button
.findByTitle('Upgrade Version')
.should('be.visible')
.should('be.enabled')
.click();

cy.wait(['@updateCluster', '@getClusters']);

ui.dialog
.findByTitle('Step 2: Recycle All Cluster Nodes')
.should('be.visible');

ui.button
.findByTitle('Recycle All Nodes')
.should('be.visible')
.should('be.enabled')
.click();

cy.wait('@recycleAllNodes');

ui.toast.assertMessage('Recycle started successfully.');

cy.findByText(newVersion).should('be.visible');
});

/*
* - Confirms node, node pool, and cluster recycling UI flow using mocked API data.
* - Confirms that user is warned that recycling recreates nodes and may take a while.
Expand Down
4 changes: 2 additions & 2 deletions packages/manager/src/features/Kubernetes/KubernetesLanding/KubernetesLanding.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export const KubernetesLanding = () => {

const isRestricted = profile?.restricted ?? false;

const { data, error, isFetching } = useKubernetesClustersQuery(
Copy link
Member Author

Choose a reason for hiding this comment

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

For context

isLoading is true whenever the first fetch for the query is in-flight
isFetching is true whenever any fetch for the query is in-flight

Explanation

In the upgrade dialog, useKubernetesClusterMutation is called, which invalidates the paginated list of Kubernets clusters, which causes useKubernetesClustersQuery's isFetching to become true, which unmounted the upgrade dialog because of the if statement below. Switching to isLoading allows the Upgrade dialog to stay mounted during the upgrade, which fixes the bug

There are probably other ways to fix this bug. For example, the UpgradeVersionModal component could probably be made more robust. I decided to go with this fix because I think isLoading is the desired loading state here rather than isFetching anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the context. Agreed that the dialog is not in the best shape but that is out of scope.

const { data, error, isLoading } = useKubernetesClustersQuery(
{
page: pagination.page,
page_size: pagination.pageSize,
Expand Down Expand Up @@ -157,7 +157,7 @@ export const KubernetesLanding = () => {
);
}

if (isFetching) {
if (isLoading) {
return <CircleProgress />;
}

Expand Down