Cluster Explorer: Restrict / Remove usages of findAll: traditional types #13025

richard-cox · 2025-01-09T16:57:09Z

Blocked on

Requires squash-merge

Summary

Fixes #13045

Does not cover others from #9965 or those related to #10417

Occurred changes and/or fixed issues

Converted to use ResourceLabeledSelect (LabelSelect with paging if SSP for type is enabled)
- Questions Resource type
  - To test load add helm repo https://github.com/richard-cox/rodeo, install chart rancher-demo
- Resource detail page's generic Events tab
Replace fetchAll with fetchAll within specific namespace (more risk than full pagination, but cleaner and benefits world where SSP is disabled as well)
- all workload types in the namespace detail page used to show workloads list
  - note that the workloads by state components (top bar and list) get their info from the counts endpoint rather than calculating from all resources
- services and certs (secrets of type tls) in ingress detail page used to show associated resources
  - same but for ingress edit page to show possible associated resources
  - these use a common helper file
CronJobs now find their associated jobs individually, rather than fetching all jobs and then filtering
Smaller changes
- When the LabelSelect is setup to show pagination controls... hide them if there's a single page
- Minor fixes to shell/components/form/ResourceLabeledSelect.vue
- Added comments that need to be reviewed as part of Application of selectors locally scales badly #10417
- Removed RelatedWorkloadsTable
  - this was added in Migrating the PersistentVolumeClaim pages #2342, but the usage was removed in Replacing the custom related workloads and events tabs on the PVC page #2436. Given it does a findAll on all workload types this seems like a good pr to remove it
- Usages of FetchSecondaryResources had regressed. Now
  - ensure the args are correct and acted on (aka canPaginate blocks code that don't need to run in non-SSP world)
  - ensure the response, whether promise or hash of promises, work correctly
Added comments where areas need addressing via Application of selectors locally scales badly #10417

Areas or cases that should be tested

Questions Resources type (see above)
A selection of various resource detail page's events tab
Cluster --> Project/Namespaces --> any namespace --> workloads list
Service Discovery --> Ingresses --> Create and Edit, specifically selecting certificates and rule service
Workloads --> CronJobs --> click on cron job --> Jobs tab --> correct Jobs are shown
Cluster --> Node (list) --> Displays correct secondary resource fields (including pod info)
Cluster --> Storage --> Persistent Volumn (list) --> Displays correct secondary resource fields (PVCs)

Areas which could experience regressions

as per above

Checklist

The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
The PR has a Milestone
The PR template has been filled out
The PR has been self reviewed
The PR has a reviewer assigned
The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

…nts list

- Functional Changes - SSP now works after vue3 bump - Home Page Clusters list now uses server-side pagination - Side Bar clusters list now uses server-side pagination - Wire in now supported sorting / filtering by id and name used for table columns - Allow pagination to be enabled given a specific context - Call findPage without persisting to store - New Pagination Tools - PaginatedResourceTable - Convenience Component, wraps ResourceTable with pagination specific props - PaginationWrapper - Convenience class to handle requests for resources and updates to them (avoiding store) - Regressions - Side Nav menu ready state was `mgmtCluster.isReady && !pCluster?.hasError`, now ???

…would incorrectly fetch all resources anyway - convert reminaing storage lists

- Functional Changes - SSP now works after vue3 bump - Home Page Clusters list now uses server-side pagination - Side Bar clusters list now uses server-side pagination - Wire in now supported sorting / filtering by id and name used for table columns - Allow pagination to be enabled given a specific context - Call findPage without persisting to store - New Pagination Tools - PaginatedResourceTable - Convenience Component, wraps ResourceTable with pagination specific props - PaginationWrapper - Convenience class to handle requests for resources and updates to them (avoiding store) - Regressions - Side Nav menu ready state was `mgmtCluster.isReady && !pCluster?.hasError`, now ???

Note - prov clusters is broken (only fetches local) due to blocking pr. breals - notPinned list

- remove from resource list, put in resource-fetch (used also by pag res table)

…ation-cluster-explorer

- remove from resource list, put in resource-fetch (used also by pag res table)

…ation-cluster-explorer

- changes namespaces kicked of side nav cluster requests (thought pinnedIds changed) - fix generic lists re-fetching given ns filter changes (they don't have namespaced arg)

…ation-cluster-explorer

…dall-2

…-findall-2

- opens up to event header e2e failures again...

shell/components/RelatedWorkloadsTable.vue

codyrancher · 2025-01-22T16:00:20Z

shell/components/PaginatedResourceTable.vue

-      promises.push(this.fetchSecondaryResources({ canPaginate: this.canPaginate }));
+      const res = this.fetchSecondaryResources({ canPaginate: this.canPaginate });
+
+      promises.push(Array.isArray(res) ? res : [res]);


By any chance do you mean to deconstruct the array before pushing?

promises.push(...(Array.isArray(res) ? res : [res]));

I don't think Promise.all() accepts nested arrays of promises.

Good catch. That's a bug. There was another as well, as map of promises was the alternative

I reworked this in the end and now the response to fetchSecondaryResources will always be a single promise (the map was pointless as it would be just mapped into an array of promises). This might affect your PR

shell/components/Questions/Reference.vue

codyrancher · 2025-01-22T16:07:31Z

shell/components/Questions/Reference.vue

+        :paginated-resource-settings="paginateResourceSetting"
+        :all-resources-settings="allResourceSettings"


By any chance do we want to keep the interface consistent with PaginatedResourceTable? i.e. api-filter and local-filter.

Or maybe update PaginatedResourceTable to use a settings style object.

when i stumbled over this again i had the same thought.

i'll update cover

prop names

make a common type to fetch / filter the resources

went full circle.

the prop names here better describe what the control is doing (showing all in a dropdown or showing pages in a dropdown), the function calls are also different in nature.

did tidy it up though and move the types into their own file, added jsdocs, etc c19aabe

codyrancher · 2025-01-22T18:12:02Z

shell/components/form/ResourceTabs/index.vue

+      if (!!existing) {
+        Object.assign(existing, required);
+      } else {


I think it might be nice to emit a warning or something in the console if this happens, I can imagine a case where things overwritten leading to an author having to dig to figure out why.

This use case covers the user hitting the refresh button and the same pagination settings come through again. It's also restricted to the events table, so the chance of a dev overriding it somewhere is low.

shell/detail/networking.k8s.io.ingress.vue

codyrancher

Just requesting changes based on my individual comments.

- fix and simplify FetchPageSecondaryResources - ensure resource detail page events list doesn't add ns filters (not needed)

…-findall-2

…mproving naming

richard-cox added 30 commits August 19, 2024 16:23

cluster list now uses resource table

25c0c70

New PaginatedResourceTable

eda38cf

WIP

8e120d0

add context to pag setting

203cdfd

tidying up

a0a6883

Add server-side pagination to cluster explorer events and general eve…

8b03d56

…nts list

Servier-side pagination for catalog apps, cluster repo and operation

dcd69ed

Servier-side pagination for Service --> HPA, inmgress, services

34e173b

Merge branch 'pagination-home-page' into pagination-cluster-explorer

bbf4bcd

fixes after merge. apply PagResTable to node list, vue3 fixes

6bfc45d

- Fix issue where list components containing PaginatedResourceTables …

1f61ce5

…would incorrectly fetch all resources anyway - convert reminaing storage lists

policy section

16d1a0f

fix async button in manual refresh mode

006debb

fgh

2454ad3

aaaaa

a94360e

Iteration

f1858b6

Note - prov clusters is broken (only fetches local) due to blocking pr. breals - notPinned list

Merge branch 'pagination-home-page' into pagination-cluster-explorer

475aaef

Fix dupe inStore

96b6b9a

- remove from resource list, put in resource-fetch (used also by pag res table)

Merge remote-tracking branch 'origin/pagination-home-page' into pagin…

c61ad5f

…ation-cluster-explorer

Fix dupe inStore

442d85c

- remove from resource list, put in resource-fetch (used also by pag res table)

Merge remote-tracking branch 'origin/pagination-home-page' into pagin…

fdc99ee

…ation-cluster-explorer

Two fixes

cb18993

- changes namespaces kicked of side nav cluster requests (thought pinnedIds changed) - fix generic lists re-fetching given ns filter changes (they don't have namespaced arg)

Merge remote-tracking branch 'origin/pagination-home-page' into pagin…

0054f40

…ation-cluster-explorer

fixes / updates

66f73da

testing/tidying

a6810c1

WIP - Rest of workload lists

6f56573

finish off workloads

d626da2

tidying up

850721b

richard-cox added 3 commits January 9, 2025 13:55

Tidying up ready to review

86ae345

Merge branch 'pagination-remove-findall-1' into pagination-remove-fin…

221e787

…dall-2

finish off functional stuff

0919441

richard-cox added this to the v2.11.0 milestone Jan 9, 2025

rancher-ui-project-bot bot assigned richard-cox Jan 9, 2025

richard-cox mentioned this pull request Jan 13, 2025

Remove usages of findAll for core resources in cluster explorer view - 1 #13006

Closed

7 tasks

richard-cox changed the title ~~Remove usages of findAll for core resources in cluster explorer view - 2~~ Remove usages of findAll for core resources in cluster explorer view Jan 13, 2025

richard-cox changed the title ~~Remove usages of findAll for core resources in cluster explorer view~~ Cluster Explorer: Restrict / Remove usages of findAll: traditional types Jan 13, 2025

richard-cox added 3 commits January 13, 2025 11:25

fix lint

7cb061b

Merge remote-tracking branch 'upstream/master' into pagination-remove…

1fe4b89

…-findall-2

Tidy up event headers

96e30e2

- opens up to event header e2e failures again...

richard-cox commented Jan 13, 2025

View reviewed changes

shell/components/RelatedWorkloadsTable.vue Show resolved Hide resolved

Fix lint and unit test

898b903

richard-cox requested a review from codyrancher January 13, 2025 15:13

richard-cox marked this pull request as ready for review January 13, 2025 15:14

codyrancher reviewed Jan 22, 2025

View reviewed changes

shell/components/Questions/Reference.vue Outdated Show resolved Hide resolved

codyrancher reviewed Jan 22, 2025

View reviewed changes

shell/detail/networking.k8s.io.ingress.vue Show resolved Hide resolved

codyrancher requested changes Jan 22, 2025

View reviewed changes

richard-cox added 5 commits January 23, 2025 15:56

Changes following review

972e85e

- fix and simplify FetchPageSecondaryResources - ensure resource detail page events list doesn't add ns filters (not needed)

Merge remote-tracking branch 'upstream/master' into pagination-remove…

51f21ab

…-findall-2

missed a param

bec8e7a

Move PaginatedResourceTable and ResourceLabeledSelect to type file, i…

c19aabe

…mproving naming

fix linting

8ff97aa

richard-cox requested a review from codyrancher January 23, 2025 18:08

codyrancher approved these changes Jan 23, 2025

View reviewed changes

richard-cox merged commit 9985ce0 into rancher:master Jan 24, 2025
32 checks passed

richard-cox deleted the pagination-remove-findall-2 branch January 24, 2025 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster Explorer: Restrict / Remove usages of findAll: traditional types #13025

Cluster Explorer: Restrict / Remove usages of findAll: traditional types #13025

richard-cox commented Jan 9, 2025 •

edited

Loading

codyrancher Jan 22, 2025

richard-cox Jan 23, 2025

codyrancher Jan 22, 2025 •

edited

Loading

richard-cox Jan 23, 2025

richard-cox Jan 23, 2025

codyrancher Jan 22, 2025

richard-cox Jan 23, 2025

codyrancher left a comment

		:paginated-resource-settings="paginateResourceSetting"
		:all-resources-settings="allResourceSettings"

Cluster Explorer: Restrict / Remove usages of findAll: traditional types #13025

Cluster Explorer: Restrict / Remove usages of findAll: traditional types #13025

Conversation

richard-cox commented Jan 9, 2025 • edited Loading

Summary

Occurred changes and/or fixed issues

Areas or cases that should be tested

Areas which could experience regressions

Checklist

codyrancher Jan 22, 2025

Choose a reason for hiding this comment

richard-cox Jan 23, 2025

Choose a reason for hiding this comment

codyrancher Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

richard-cox Jan 23, 2025

Choose a reason for hiding this comment

richard-cox Jan 23, 2025

Choose a reason for hiding this comment

codyrancher Jan 22, 2025

Choose a reason for hiding this comment

richard-cox Jan 23, 2025

Choose a reason for hiding this comment

codyrancher left a comment

Choose a reason for hiding this comment

richard-cox commented Jan 9, 2025 •

edited

Loading

codyrancher Jan 22, 2025 •

edited

Loading