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

Add Anti-Affinity to the UI: Phase 1 — read-only #2760

Merged
merged 61 commits into from
Mar 27, 2025

Conversation

charliepark
Copy link
Contributor

@charliepark charliepark commented Mar 21, 2025

This PR is based off of this Omicron PR — oxidecomputer/omicron#7658 — so won't be mergeable quite yet. But this gets the UI ready for Anti-Affinity. (oxidecomputer/omicron#7858 is also relevant)

This PR is read-only, but we'll build off of it for Create/Update/Delete in subsequent PRs.
Screenshot 2025-03-20 at 5 16 54 PM
Screenshot 2025-03-20 at 5 17 09 PM

@benjaminleonard
Copy link
Contributor

Small detail, but its missing the dividing line. Aesthetically it does a similar job to the tab bar, like on the instance page where it separates the page content from the page header.

image

})
})
return null
}
Copy link
Collaborator

@david-crespo david-crespo Mar 26, 2025

Choose a reason for hiding this comment

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

This is not quite right because the fetchQuery is not being awaited. This explains why you were (I'm guessing) stuck using useApiQuery in the component — the fetches are kicked off by the loader, but the loader doesn't wait for them to be done. The way to make this wait would be:

const groups = await queryClient.fetchQuery(
  antiAffinityGroupList({ project }).optionsFn()
)
await Promise.all(
  groups.items.map(({ name }) =>
    queryClient.prefetchQuery(memberList({ antiAffinityGroup: name, project }))
  )
)

I prefer await instead of the then here because there's only one fetch to wait for before kicking off the requests that depend on it — in the other spots where we use then in a loader it's because there's a top-level Promise.all and I'm trying to jam everything in it.

However, with 16 instances in a group, it would probably make the pageload time too long if we wait for them all. A fun thing about this implementation is that you can turn off waiting by just removing the await from the Promise.all. Putting a little timer in there confirms that we're talking a few hundred ms without waiting for the members vs. double that if there are just a few members (fewer than 6 would mean they all run in parallel without any queuing by the browser).

export async function clientLoader({ params }: LoaderFunctionArgs) {
  const start = performance.now()
  const { project } = getProjectSelector(params)
  const groups = await queryClient.fetchQuery(
    antiAffinityGroupList({ project }).optionsFn()
  )
  /* await */ Promise.all(
    groups.items.map(({ name }) =>
      queryClient.prefetchQuery(memberList({ antiAffinityGroup: name, project }))
    )
  )
  console.log(performance.now() - start, 'ms')
  return null
}

The downside to not waiting is that the members column pops in a bit and the whole table shifts. I have a slight preference for a faster initial load, but I could see the argument the other way. We could also get sneaky and decide whether to wait or not based on the number of members. Like if there are under 6 we could wait, but if there are more, don't bother:

// kick off the fetches but only conditionally wait for them
const memberFetches = groups.items.map(({ name }) =>
  queryClient.prefetchQuery(memberList({ antiAffinityGroup: name, project }))
)
if (groups.items.length < 6) await Promise.all(memberFetches)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, one more thing. Generally when we are going to let something load async, we don't bother doing anything in the loader for it. The VPC list is one example, where we fetch the firewall rules for each VPC async as a regular useQuery.

const FirewallRuleCount = ({ project, vpc }: PP.Vpc) => {
const { data } = useQuery(apiq('vpcFirewallRulesView', { query: { project, vpc } }))
if (!data) return <SkeletonCell /> // loading
return <LinkCell to={pb.vpc({ project, vpc })}>{data.rules.length}</LinkCell>
}
const colHelper = createColumnHelper<Vpc>()
// just as in the vpcList call for the quick actions menu, include limit to make
// sure it matches the call in the QueryTable
export async function clientLoader({ params }: LoaderFunctionArgs) {
const { project } = getProjectSelector(params)
await queryClient.prefetchQuery(vpcList(project).optionsFn())
return null
}

However! Kicking off a fetch in the loader but not waiting for is actually a cool idea because it starts the fetch earlier. How much earlier? When I stick console.time() and console.timeLog() to see how long it is between the loader ending and the render happening:

diff --git a/app/pages/project/affinity/AffinityPage.tsx b/app/pages/project/affinity/AffinityPage.tsx
index 11d4bb76cc..df76942ad3 100644
--- a/app/pages/project/affinity/AffinityPage.tsx
+++ b/app/pages/project/affinity/AffinityPage.tsx
@@ -48,14 +48,7 @@
   const { project } = getProjectSelector(params)
   await queryClient
     .fetchQuery(antiAffinityGroupList({ project }).optionsFn())
-    .then((data) => {
-      // Preload the anti-affinity group details
-      data.items.forEach((antiAffinityGroup) => {
-        queryClient.fetchQuery(
-          memberList({ antiAffinityGroup: antiAffinityGroup.name, project })
-        )
-      })
-    })
+  console.time()
   return null
 }
 
@@ -136,6 +129,7 @@
   antiAffinityGroup: string
 }) => {
   const { project } = useProjectSelector()
+  console.timeLog()
   const { data: members } = useApiQuery('antiAffinityGroupMemberList', {
     path: { antiAffinityGroup },
     query: { project, limit: 2 },

...the results are interesting! If I turn off the latency randomization in the mock API, I consistently get 50-70ms from the end of the loader to the render, so that's the time we would save by kicking off the fetches a bit earlier. Not nothing but also not a ton. However, if I leave the randomization on and even make it a bit wider of a range, we sometimes have a much bigger gap because other loaders running in parallel to this one might take longer than this one, giving us more time in which nothing is happening, in which we could have been fetching the groups. So overall I think kicking off the fetches in the loader even when we're not waiting for them is probably a good idea and unlikely to have downsides.

That is to say, I think this one is the way to go:

// kick off the fetches but only conditionally wait for them
const memberFetches = groups.items.map(({ name }) =>
  queryClient.prefetchQuery(memberList({ antiAffinityGroup: name, project }))
)
if (groups.items.length < 6) await Promise.all(memberFetches)

@david-crespo david-crespo marked this pull request as ready for review March 26, 2025 19:41
Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Let's party. We'll do the e2e tests once we have new/edit.

@david-crespo david-crespo merged commit 5538906 into main Mar 27, 2025
7 checks passed
@david-crespo david-crespo deleted the affinity-1-read-only branch March 27, 2025 17:06
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Apr 1, 2025
oxidecomputer/console@72e2f0c...f52f50c

* [f52f50c7](oxidecomputer/console@f52f50c7) react 19.1
* [d0999667](oxidecomputer/console@d0999667) oxidecomputer/console#2773
* [e7bfb236](oxidecomputer/console@e7bfb236) oxidecomputer/console#2767
* [59e69764](oxidecomputer/console@59e69764) oxidecomputer/console#2768
* [44157459](oxidecomputer/console@44157459) oxidecomputer/console#2770
* [5a90b430](oxidecomputer/console@5a90b430) oxidecomputer/console#2769
* [c12696c0](oxidecomputer/console@c12696c0) make api-diff use latest generator
* [5538906c](oxidecomputer/console@5538906c) oxidecomputer/console#2760
* [653ba4cb](oxidecomputer/console@653ba4cb) bump omicron for instance affinity groups list
* [179f347f](oxidecomputer/console@179f347f) oxidecomputer/console#2766
* [dbaeb3d3](oxidecomputer/console@dbaeb3d3) oxidecomputer/console#2765
* [549c266e](oxidecomputer/console@549c266e) oxidecomputer/console#2762
* [d41ef17f](oxidecomputer/console@d41ef17f) oxidecomputer/console#2763
* [a5804f8f](oxidecomputer/console@a5804f8f) oxidecomputer/console#2761
* [fb2fdbec](oxidecomputer/console@fb2fdbec) oxidecomputer/console#2759
* [9de391c8](oxidecomputer/console@9de391c8) chore: playwright 1.51.1 came out like an hour later
* [b49f70c0](oxidecomputer/console@b49f70c0) oxidecomputer/console#2757
* [3eb24d2d](oxidecomputer/console@3eb24d2d) chore: bump playwright to 1.51
* [f29e4ea9](oxidecomputer/console@f29e4ea9) chore: bump vite + plugins, vitest, msw
* [0f3408d0](oxidecomputer/console@0f3408d0) chore: bump oxlint to 0.16
* [f9931e03](oxidecomputer/console@f9931e03) tools: probably fix ff test flake
* [1015a805](oxidecomputer/console@1015a805) tools: bump client generator, turn on noUnusedParameters in tsconfig
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Apr 1, 2025
oxidecomputer/console@72e2f0c...f52f50c

* [f52f50c7](oxidecomputer/console@f52f50c7)
react 19.1
* [d0999667](oxidecomputer/console@d0999667)
oxidecomputer/console#2773
* [e7bfb236](oxidecomputer/console@e7bfb236)
oxidecomputer/console#2767
* [59e69764](oxidecomputer/console@59e69764)
oxidecomputer/console#2768
* [44157459](oxidecomputer/console@44157459)
oxidecomputer/console#2770
* [5a90b430](oxidecomputer/console@5a90b430)
oxidecomputer/console#2769
* [c12696c0](oxidecomputer/console@c12696c0)
make api-diff use latest generator
* [5538906c](oxidecomputer/console@5538906c)
oxidecomputer/console#2760
* [653ba4cb](oxidecomputer/console@653ba4cb)
bump omicron for instance affinity groups list
* [179f347f](oxidecomputer/console@179f347f)
oxidecomputer/console#2766
* [dbaeb3d3](oxidecomputer/console@dbaeb3d3)
oxidecomputer/console#2765
* [549c266e](oxidecomputer/console@549c266e)
oxidecomputer/console#2762
* [d41ef17f](oxidecomputer/console@d41ef17f)
oxidecomputer/console#2763
* [a5804f8f](oxidecomputer/console@a5804f8f)
oxidecomputer/console#2761
* [fb2fdbec](oxidecomputer/console@fb2fdbec)
oxidecomputer/console#2759
* [9de391c8](oxidecomputer/console@9de391c8)
chore: playwright 1.51.1 came out like an hour later
* [b49f70c0](oxidecomputer/console@b49f70c0)
oxidecomputer/console#2757
* [3eb24d2d](oxidecomputer/console@3eb24d2d)
chore: bump playwright to 1.51
* [f29e4ea9](oxidecomputer/console@f29e4ea9)
chore: bump vite + plugins, vitest, msw
* [0f3408d0](oxidecomputer/console@0f3408d0)
chore: bump oxlint to 0.16
* [f9931e03](oxidecomputer/console@f9931e03)
tools: probably fix ff test flake
* [1015a805](oxidecomputer/console@1015a805)
tools: bump client generator, turn on noUnusedParameters in tsconfig
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants