feat: [M3-7257] - Invalidate VPC-related queries after deleting a linode by coliu-akamai · Pull Request #9814 · linode/manager

coliu-akamai · 2023-10-18T17:43:04Z

Description 📝

After deleting a linode, because we didn't invalidate any VPC related queries, the VPC pages would have incorrect information unless we refreshed the page. This change invalidates VPC related queries when deleting a linode so that the vpc pages are up to date.

Changes 🔄

List any change relevant to the reviewer.

Added utility function to get all vpcs a linode is assigned to with, based on its configs
Invalidate vpc-related queries for vpcs a linode is assigned to
Added some extra spacing underneath the 'Reboot linode banner' on a VPC details page -- seems like there was spacing missing, added more to make it align more with the mockups
updated VPC todo comments to be consistent - //@TODO VPC: ...todos...

Preview 📷

Before:
https://github.com/linode/manager/assets/139280159/eb4fae00-dbf0-49af-b54f-bb3ec449e307

After:
https://github.com/linode/manager/assets/139280159/24012745-b602-4aef-abd6-abe724c85c3c

How to test 🧪

Prerequisites

(How to setup test environment)
Use the dev environment, with VPC tags,

Have a linode and at least two vpc's created (each with at least one subnet)

Verification steps

(How to verify changes)
Verify that the VPC landing page and specific VPC detail pages are correctly updated after you delete a linode that is assigned to a VPC. Test the following cases:

Linode assigned to one vpc one subnet
Linode assigned to one vpc (multiple subnets within that VPC)
Linode assigned to two+ VPCs

Tests

yarn test LinodesLanding/utils.test.ts

As an Author I have considered 🤔

Check all that apply

coliu-akamai

One thing I noticed when working on this PR is that when we navigate back to the VPC landing and detail pages, we see the old information for a brief moment before it gets updated. I've tried looking having the landing/detail pages be up to date as soon as we navigate to those pages, but wasn't having a lot of luck. Wondering if this is just how React Query works / is it bc of active/inactive queries or something?

coliu-akamai · 2023-10-18T17:52:25Z

packages/manager/src/features/Linodes/LinodesLanding/DeleteLinodeDialog.tsx

+  const flags = useFlags();
+  const { data: account } = useAccount();
+
+  const enableVPCActions = isFeatureEnabled(
+    'VPCs',
+    Boolean(flags.vpc),
+    account?.capabilities ?? []
+  );


Adding this check to determine when to fire the useAllLinodeConfigsQuery (line 49) -- feature flagging the query, basically

coliu-akamai · 2023-10-18T17:57:40Z

packages/manager/src/features/Linodes/LinodesLanding/DeleteLinodeDialog.tsx

    linodeId !== undefined && open
  );

+  const { data: configs } = useAllLinodeConfigsQuery(


The PR to include a VPC column in the LInodes Landing page table caused a bunch of 429s due to the useAllLinodeConfigsQuery being fired for every single linode. (see prs #9485 and #9625 for context + their respective tickets)

I think that would be less of an issue here bc we only fire this query when we are deleting the linode (and also only if the user has VPC capabilities), not for every single linode, but please lmk if I'm mistaken!

I think this is reasonable 👍

Yeah, you are correct! This is good!

coliu-akamai · 2023-10-18T17:58:48Z

packages/manager/src/features/VPCs/VPCDetail/VPCDetail.tsx

      {numLinodes > 0 && (
        <DismissibleBanner
          preferenceKey={`reboot-linodes-warning-banner-${vpc.id}`}
+          sx={{ marginBottom: theme.spacing(2) }}


see PR description for reasoning behind this additional margin

abailly-akamai

Query invalidated and approach is solid to me ✅

abailly-akamai · 2023-10-19T13:45:00Z

packages/manager/src/features/Linodes/LinodesLanding/utils.test.ts

+      const vpcIds = getVPCsFromLinodeConfigs([config]);
+      expect(vpcIds).toEqual([2, 3]);
+    });
+  });


abailly-akamai · 2023-10-19T13:48:39Z

packages/manager/src/features/Linodes/LinodesLanding/DeleteLinodeDialog.tsx

    linodeId !== undefined && open
  );

+  const { data: configs } = useAllLinodeConfigsQuery(


I think this is reasonable 👍

packages/manager/src/features/Linodes/LinodesLanding/DeleteLinodeDialog.tsx

bnussman-akamai · 2023-10-19T14:24:08Z

packages/manager/src/features/Linodes/LinodesLanding/DeleteLinodeDialog.tsx

    linodeId !== undefined && open
  );

+  const { data: configs } = useAllLinodeConfigsQuery(


Yeah, you are correct! This is good!

bnussman-akamai · 2023-10-19T14:33:39Z

packages/manager/src/features/Linodes/LinodesLanding/DeleteLinodeDialog.tsx

+    const vpcIds = getVPCsFromLinodeConfigs(configs ?? []);
+    if (vpcIds.length > 0) {
+      queryClient.invalidateQueries([vpcQueryKey, 'paginated']);
+      // invalidate data for specific vpcs this linode is assigned to
+      vpcIds.forEach((vpcId) => {
+        queryClient.invalidateQueries([vpcQueryKey, 'vpc', vpcId]);
+        queryClient.invalidateQueries([
+          vpcQueryKey,
+          'vpc',
+          vpcId,
+          subnetQueryKey,
+        ]);
+      });
+    }


This logic looks good to me. Generally, I prefer to have invalidation logic inside of the useDeleteLinodeMutation's onSuccess in the packages/manager/src/queries/linodes/linodes.ts file but in situations like this, I think this is a fair approach.

The one flaw of this approach is that it is technically possible (but probably unlikely) for the Linode configs to not have loaded by the time we run this logic.

I know I told you that for invalidations, "it’s best to be as specific as possible", but I just looked at packages/manager/src/queries/linodes/events.ts and it looks like we handle volumes and firewalls there. Maybe we should just follow that pattern for now? The benefit of invalidating based on the event is that invalidation can happen across many instances of cloud manager that the user may have open.

We could go either way with it, but I like this more targeted invalidation approach

In that case will go with this more targeted approach for now. Maybe when we no longer need the feature flag/account capabilities check, we can return to this?

dwiley-akamai

VPC pages are correctly updated after Linode deletion for each of the test cases ✅
Code review ✅

dwiley-akamai · 2023-10-19T15:34:03Z

packages/manager/src/features/Linodes/LinodesLanding/DeleteLinodeDialog.tsx

+    const vpcIds = getVPCsFromLinodeConfigs(configs ?? []);
+    if (vpcIds.length > 0) {
+      queryClient.invalidateQueries([vpcQueryKey, 'paginated']);
+      // invalidate data for specific vpcs this linode is assigned to
+      vpcIds.forEach((vpcId) => {
+        queryClient.invalidateQueries([vpcQueryKey, 'vpc', vpcId]);
+        queryClient.invalidateQueries([
+          vpcQueryKey,
+          'vpc',
+          vpcId,
+          subnetQueryKey,
+        ]);
+      });
+    }


We could go either way with it, but I like this more targeted invalidation approach

… todo comments to be consistent

coliu-akamai · 2023-10-19T21:40:23Z

Merging as cypress test failures are not due to vpc issues/not from this PR!

coliu-akamai added 2 commits October 13, 2023 14:42

initial version -- invalidate vpc related queries

c303787

invalidate specific queries, write tests

dba97b9

coliu-akamai added the VPC Relating to VPC project label Oct 18, 2023

coliu-akamai self-assigned this Oct 18, 2023

feature flag query and add changeset

dfe0307

coliu-akamai commented Oct 18, 2023

View reviewed changes

coliu-akamai added the Ready for Review label Oct 18, 2023

coliu-akamai marked this pull request as ready for review October 18, 2023 18:02

coliu-akamai requested a review from a team as a code owner October 18, 2023 18:02

coliu-akamai requested review from bnussman-akamai, dwiley-akamai, hana-akamai and tyler-akamai and removed request for a team October 18, 2023 18:02

Merge branch 'develop' into feat-m3-7257

d511015

abailly-akamai approved these changes Oct 19, 2023

View reviewed changes

bnussman-akamai reviewed Oct 19, 2023

View reviewed changes

dwiley-akamai approved these changes Oct 19, 2023

View reviewed changes

coliu-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Oct 19, 2023

coliu-akamai requested a review from bnussman-akamai October 19, 2023 17:46

address feedback and add potential VPC todo comment, update other VPC…

964dad7

… todo comments to be consistent

bnussman-akamai approved these changes Oct 19, 2023

View reviewed changes

update comment

649ee8b

coliu-akamai merged commit 54b5f00 into linode:develop Oct 19, 2023

This was referenced Oct 26, 2023

Release v1.106.0 - release → staging #9842

Merged

Release v1.106.0 - staging → master #9850

Merged

coliu-akamai deleted the feat-m3-7257 branch November 6, 2023 19:44

Conversation

coliu-akamai commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Verification steps

Tests

As an Author I have considered 🤔

Uh oh!

coliu-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

coliu-akamai Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwiley-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coliu-akamai commented Oct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coliu-akamai commented Oct 18, 2023 •

edited

Loading

coliu-akamai Oct 18, 2023 •

edited

Loading