fix: reduce the number of api calls from Workspace Create Modal #9735

salonig23 · 2024-07-26T11:29:06Z

Ticket

Description

Workspace Create Modal was calling 3 API's multiple times - which was slowing down the webUi severely. Reduced the number of calls and made the code more efficient.

Test Plan

Check for both Agent and Kubernetes RM for both admin/non admin users:

Open the workspace list page as a non admin user and check that there are no failing API calls for getKubernetesResourceManagers, ListNamespaceBindings and GetK8sResourceQuotas.
Open the workspace create/Edit modal from multiple places and verify it renders correctly and none of these 3 API calls fail

Verify that Namespace Bindings and Resource Quotas can only be set by Cluster Admins in the webui

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

netlify · 2024-07-26T11:29:24Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`023f582`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66a7e3a467ae5900088c03a4
😎 Deploy Preview	https://deploy-preview-9735--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-07-26T11:29:37Z

Codecov Report

Attention: Patch coverage is 19.85816% with 113 lines in your changes missing coverage. Please review.

Project coverage is 53.35%. Comparing base (4aa6ffa) to head (023f582).
Report is 13 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9735   +/-   ##
=======================================
  Coverage   53.34%   53.35%           
=======================================
  Files        1257     1257           
  Lines      154493   154522   +29     
  Branches     3298     3307    +9     
=======================================
+ Hits        82422    82451   +29     
  Misses      71921    71921           
  Partials      150      150

Flag	Coverage Δ
backend	`44.87% <20.00%> (+<0.01%)`	⬆️
harness	`72.69% <ø> (ø)`
web	`51.73% <19.84%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/api_master.go	`2.35% <ø> (+0.08%)`	⬆️
master/internal/workspace/authz_basic_impl.go	`1.66% <ø> (ø)`
webui/react/src/hooks/usePermissions.ts	`76.82% <100.00%> (+1.82%)`	⬆️
master/internal/api_workspace.go	`64.14% <50.00%> (+0.20%)`	⬆️
master/internal/workspace/authz_rbac.go	`0.37% <0.00%> (ø)`
master/internal/workspace/authz_permissive.go	`1.63% <0.00%> (ø)`
webui/react/src/components/NavigationTabbar.tsx	`0.00% <0.00%> (ø)`
webui/react/src/components/NavigationSideBar.tsx	`0.00% <0.00%> (ø)`
webui/react/src/pages/WorkspaceList.tsx	`0.00% <0.00%> (ø)`
webui/react/src/stores/cluster.tsx	`37.08% <28.57%> (-0.79%)`	⬇️
... and 2 more

... and 5 files with indirect coverage changes

johnkim-det

fixes working great! added some comments especially around improvements to the approach I proposed given that you're now making the getKubernetesResourceManagers call in the cluster store.

also one concern is that the code coverage numbers for this PR are low, and we're trying to improve unit testing/line coverage for web. I don't necessarily think we need to block this fix/release on it, but could you create a separate ticket for going back and adding unit tests for this?

webui/react/src/components/NavigationSideBar.tsx

webui/react/src/components/NavigationTabbar.tsx

webui/react/src/components/WorkspaceCreateModal.tsx

webui/react/src/pages/WorkspaceList.tsx

webui/react/src/pages/WorkspaceList/WorkspaceActionDropdown.tsx

webui/react/src/components/WorkspaceCreateModal.tsx

webui/react/src/stores/cluster.tsx

webui/react/src/components/WorkspaceCreateModal.tsx

ashtonG

if namespaceBindingsList and resourceQuotasList need to be fresh per open, i strongly suggest keeping the useAsync calls and instead adding the open and resourceManagers checks to the workspaceId checks. As is, manually calling the callbacks introduces edge cases around stale information overwriting new information.

keita-determined · 2024-07-26T18:36:24Z

webui/react/src/components/WorkspaceCreateModal.tsx

  const resourceQuotasList = useAsync(
    async (canceller) => {
-      if (workspaceId === undefined) {
+      if (workspaceId === undefined || Loadable.getOrElse([], resourceManagers).length <= 0) {


nit: array length cant be negative

Suggested change

if (workspaceId === undefined || Loadable.getOrElse([], resourceManagers).length <= 0) {

if (workspaceId === undefined || Loadable.getOrElse([], resourceManagers).length === 0) {

questions: i see workspaceId === undefined || Loadable.getOrElse([], resourceManagers).length <= 0 prevent APIs in useAsync to be called many times. Just wondering if its safe enough. are there any situations that pass through the conditions and end up calling many API calls?

this is a good point -- i do think we need to keep whether or not the modal is open as a prop because once resourceManagers returns a non-zero length, all the other modals in the sidebars will begin to make requests.

basically my suggestion was to use the workspaceId check to function the same as the open prop check -- it's only defined when the user opens an edit modal, then goes back to undefined onClose. always undefined on create modals.

nit: in addition to changing length check, we could consolidate all of these to one reused var, something like const resourceManagers = Loadable.getOrElse([], loadableResourceManagers)

webui/react/src/components/WorkspaceCreateModal.tsx

webui/react/src/stores/cluster.tsx

webui/react/src/pages/WorkspaceList/WorkspaceActionDropdown.tsx

maxrussell

Backend LGTM; had a question, suggestion, or whatever for some frontend code

webui/react/src/components/NavigationSideBar.tsx

webui/react/src/components/NavigationTabbar.tsx

johnkim-det

thanks for finding the BE fixes for those errors! please note nit comment and test coverage concern, but other than that web LGTM.

johnkim-det · 2024-07-29T14:31:43Z

webui/react/src/components/WorkspaceCreateModal.tsx

  const resourceQuotasList = useAsync(
    async (canceller) => {
-      if (workspaceId === undefined) {
+      if (workspaceId === undefined || Loadable.getOrElse([], resourceManagers).length <= 0) {


nit: in addition to changing length check, we could consolidate all of these to one reused var, something like const resourceManagers = Loadable.getOrElse([], loadableResourceManagers)

salonig23 · 2024-07-29T17:53:43Z

created https://hpe-aiatscale.atlassian.net/browse/DET-10445 for writing tests @johnkim-det

(cherry picked from commit 20ed126)

fix: reduce the number of api Calls from Workspace Create Modal

610c22d

cla-bot bot added the cla-signed label Jul 26, 2024

salonig23 requested review from johnkim-det and keita-determined July 26, 2024 11:29

salonig23 marked this pull request as ready for review July 26, 2024 11:29

salonig23 requested a review from a team as a code owner July 26, 2024 11:29

fixed glitch in the modal

779a5d7

salonig23 force-pushed the fix-too-many-api-calls branch from cc0159c to 779a5d7 Compare July 26, 2024 11:32

carolinaecalderon added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Jul 26, 2024

johnkim-det reviewed Jul 26, 2024

View reviewed changes

ashtonG reviewed Jul 26, 2024

View reviewed changes

salonig23 force-pushed the fix-too-many-api-calls branch from e23340e to 62ad3f2 Compare July 26, 2024 18:17

address comments

6c18698

salonig23 force-pushed the fix-too-many-api-calls branch from 62ad3f2 to 6c18698 Compare July 26, 2024 18:29

salonig23 requested review from ashtonG and johnkim-det July 26, 2024 18:30

keita-determined reviewed Jul 26, 2024

View reviewed changes

johnkim-det reviewed Jul 26, 2024

View reviewed changes

salonig23 added 2 commits July 26, 2024 13:09

handle closing of edit modal

d6d8e55

improved permission checks

1dcd22d

salonig23 requested a review from a team as a code owner July 26, 2024 22:21

salonig23 requested review from ShreyaLnuHpe, maxrussell, johnkim-det and keita-determined and removed request for ShreyaLnuHpe July 26, 2024 22:21

maxrussell approved these changes Jul 26, 2024

View reviewed changes

webui/react/src/components/NavigationSideBar.tsx Show resolved Hide resolved

webui/react/src/components/NavigationTabbar.tsx Show resolved Hide resolved

johnkim-det approved these changes Jul 29, 2024

View reviewed changes

ashtonG approved these changes Jul 29, 2024

View reviewed changes

final fixes

56710bc

ashtonG mentioned this pull request Jul 29, 2024

fix: getkubernetesResourceManagers popup #9722

Closed

5 tasks

fix path for get k8s resource managers And resource quotas APIs

023f582

salonig23 force-pushed the fix-too-many-api-calls branch from 2f1fb54 to 023f582 Compare July 29, 2024 18:46

salonig23 merged commit 20ed126 into main Jul 29, 2024
80 of 95 checks passed

salonig23 deleted the fix-too-many-api-calls branch July 29, 2024 19:12

github-actions bot pushed a commit that referenced this pull request Jul 29, 2024

fix: reduce the number of api calls from Workspace Create Modal (#9735)

ed47fb0

(cherry picked from commit 20ed126)

salonig23 mentioned this pull request Jul 29, 2024

chore: undo test skip after fix was merged #9754

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: reduce the number of api calls from Workspace Create Modal #9735

fix: reduce the number of api calls from Workspace Create Modal #9735

salonig23 commented Jul 26, 2024 •

edited

Loading

netlify bot commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

johnkim-det left a comment

ashtonG left a comment

keita-determined Jul 26, 2024 •

edited

Loading

ashtonG Jul 26, 2024

johnkim-det Jul 26, 2024

johnkim-det Jul 29, 2024 •

edited

Loading

maxrussell left a comment

johnkim-det left a comment

johnkim-det Jul 29, 2024 •

edited

Loading

salonig23 commented Jul 29, 2024

	if (workspaceId === undefined \|\| Loadable.getOrElse([], resourceManagers).length <= 0) {
	if (workspaceId === undefined \|\| Loadable.getOrElse([], resourceManagers).length === 0) {

fix: reduce the number of api calls from Workspace Create Modal #9735

fix: reduce the number of api calls from Workspace Create Modal #9735

Conversation

salonig23 commented Jul 26, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Jul 26, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Jul 26, 2024 • edited Loading

Codecov Report

johnkim-det left a comment

Choose a reason for hiding this comment

ashtonG left a comment

Choose a reason for hiding this comment

keita-determined Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

ashtonG Jul 26, 2024

Choose a reason for hiding this comment

johnkim-det Jul 26, 2024

Choose a reason for hiding this comment

johnkim-det Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

maxrussell left a comment

Choose a reason for hiding this comment

johnkim-det left a comment

Choose a reason for hiding this comment

johnkim-det Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

salonig23 commented Jul 29, 2024

salonig23 commented Jul 26, 2024 •

edited

Loading

netlify bot commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

keita-determined Jul 26, 2024 •

edited

Loading

johnkim-det Jul 29, 2024 •

edited

Loading

johnkim-det Jul 29, 2024 •

edited

Loading