Converting the management cluster list and detail pages to be paginated #13170

codyrancher · 2025-01-21T16:08:46Z

Summary

Fixes #9548

Occurred changes and/or fixed issues

Reduces the number of findAll calls and rely on the PaginatedResourceTable to do the paginated calls.

Areas or cases that should be tested

The management cluster list and detail pages

Areas which could experience regressions

The management cluster list and detail pages

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

codyrancher · 2025-01-21T16:59:26Z

shell/detail/provisioning.cattle.io.cluster.vue

@@ -283,97 +178,8 @@ export default {
      return info;
    },

-    fakeMachines() {


I haven't seen the behavior described in the comment. It appears were able to scale up and down and the machines/machine pools sync properly without all of these extra hoops.

this is like the project/namespace list were we should show the group even if there are no rows. for instance if a pool has been scaled down to zero but we should still show the pool group. Does this still work with SSP off and on?

codyrancher · 2025-01-21T16:59:57Z

shell/detail/provisioning.cattle.io.cluster.vue

@@ -243,15 +138,15 @@ export default {
    };
  },

-  watch: {
-    showNodes(neu) {


I don't think we'll need this anymore especially with the new socket code.

codyrancher · 2025-01-21T17:00:39Z

shell/detail/provisioning.cattle.io.cluster.vue

-      if ( this.$store.getters['management/canList'](MANAGEMENT.NODE_TEMPLATE) ) {
-        fetchTwo.nodeTemplates = this.$store.dispatch('management/findAll', { type: MANAGEMENT.NODE_TEMPLATE });
-      }
+      this.clusterToken = await this.value.getOrCreateToken();


I can prolly await on both this and the hasLocalCluster to speed things up slightly.

codyrancher · 2025-01-21T17:02:16Z

shell/detail/provisioning.cattle.io.cluster.vue

- * This class allows us to fake a machine deployment - when created, we set additional properties (_cluster etc)
- * and use these in the getters.
- **/
-class EmptyCapiMachineDeployment extends CapiMachineDeployment {


No longer used without the machinepool finagling.

codyrancher · 2025-01-21T17:05:31Z

shell/detail/provisioning.cattle.io.cluster.vue

@@ -691,26 +543,6 @@ export default {
        return day(time).format(this.dateTimeFormatStr);
      }
    },
-
-    machineSortGenerationFn() {


This appears to only be necessary if we have the fake machines and nodes but neither appear to be necessary.

codyrancher · 2025-01-21T17:06:06Z

shell/detail/provisioning.cattle.io.cluster.vue

@@ -745,33 +577,22 @@ export default {
      @update:value="$emit('input', $event)"
    >
      <Tab
-        v-if="showMachines"
+        v-if="showMachines || true"


This will be removed. Accidentally left this in.

codyrancher · 2025-01-21T17:07:22Z

shell/list/provisioning.cattle.io.cluster.vue

-      return filterHiddenLocalCluster(this.rows, this.$store);
+    // We no longer make the request for rows the paginated table does. We still need to access these rows for things like harvester.
+    filteredRows() {
+      return this.$refs.paginatedTable?.filteredRows || [];


Any thoughts on a better way to do this? I don't quite thing getters get us there unless we want to duplicate the filtering.

We need to have a way to get the uses of rows and filteredRowes working in both local pagination and SSP.

SSP
rows & hiddenHarvesterCount - we can get row counts from the counts object, we might need to make a specific http request to get the other count though (pageSize=1&filter=xyz)
rows & tokenExpiredData - i think we'll need to remove this feature with SSP on.
filteredRows & nonStandardNamespaces - this would need to be another http request

local pagination
i think maybe we continue with the code in the PR

Do we actually need to change anything for hiddenHarvesterCount and tokenExpiredData?

Does harvester support SSP? Also, Is tokenExpiredData Harvester specific? I don't see the banner label keys in our translations. Will SSP for Harvester be supported before the Harvester specific code is removed from shell?

I disabled it for nonStandardNamespaces.

hiddenHarvesterCount - this is used to help guide the user to manage their harvester cluster in the harvester plugin (as harvester clusters are not shown in the cluster managment cluster list). we need total of all applicable clusters - total of kube clusters (remaining are harvester clusters). Given rows no longer represents all clusters we would need determine both totals abother way

tokenExpiredData - used to show a warning if any cluster has cluster creds associated which are expiring/expiring. we get this info from model data, so can't make an alternative request to fetch. we could either change it to warn given only the current page OR drop entirely for SSP world (would need to be a listed regression)

nonStandardNamespaces - this can stay, but we would need to make a http request with pageSize =1 and filters for not fleet-local or fleet-default namespaces. there will be a total in the response. we could do this just once on fetch. alternatively though, and just realised, we can also just look at the COUNTS resource which will have a summary of resources per namespace

both hiddenHarvesterCount and tokenExpiredData cases are currently for harvester clusters (though the second uses a generic mechanism that can be expanded to other cluster types). requests to support them are handled by steve (even if they are harvester resources) so are supported by SSP.

codyrancher · 2025-01-21T17:08:40Z

shell/utils/pagination-utils.ts

-      if (setting.resource === enabledFor.resource?.id) {
-        if (!!setting.context) {
-          return enabledFor.resource?.context ? setting.context.includes(enabledFor.resource.context) : false;


It's not obvious to me why this is necessary? It seems to cause problems if you want to use the PaginatedResourceTable on non-list pages.

This whole block needs to stay.

if (setting.resource === enabledFor.resource?.id) { - used to ensure the correct entry is found
if (!!setting.context) { - the context was used to explictely block pagination outside of supported areas. For instance the prov cluster was only allowed in the side nav and home page and not the cluster management list

codyrancher · 2025-01-21T17:14:39Z

shell/utils/settings.ts

+  r.serverPagination.stores.cluster.resources.enableSome.enabled.push('provisioning.cattle.io.cluster');
+  r.serverPagination.stores.cluster.resources.enableSome.enabled.push('cluster');
+  r.serverPagination.stores.cluster.resources.enableSome.enabled.push('rke.cattle.io.etcdsnapshot');


Temporary to allow paginated requests

in theory the three above shouldn't change anything as they're in the cluster store rather than management.

to get these without code changes, and fix the prov and mgmt cluster contexts (which are now not needed) shell/config/settings.ts serverPagination can be updated and then the new button Populate with latest pagination defaults can be used (and then applied). That process will soon be greatly updated (i have a PR, but it's in flux)

richard-cox

It doesn't look like the list currently makes SSP requests. I fixed this by comment in shell/utils/settings.ts file.

Once enabled there were failed http calls to secondary resources given the filter wasn't supported by certain type (this is expected, added inline comment). i commented those filters out and the list shows however as there's no configured pagination the columns come from the schema attribute columns rather than matching what was there before. there's a fair few examples on how the pagination headers can be setup in the explorer product

richard-cox · 2025-01-23T18:11:40Z

shell/utils/pagination-utils.ts

-      if (setting.resource === enabledFor.resource?.id) {
-        if (!!setting.context) {
-          return enabledFor.resource?.context ? setting.context.includes(enabledFor.resource.context) : false;


This whole block needs to stay.

if (setting.resource === enabledFor.resource?.id) { - used to ensure the correct entry is found
if (!!setting.context) { - the context was used to explictely block pagination outside of supported areas. For instance the prov cluster was only allowed in the side nav and home page and not the cluster management list

richard-cox · 2025-01-23T18:14:57Z

shell/utils/settings.ts

+  r.serverPagination.stores.cluster.resources.enableSome.enabled.push('provisioning.cattle.io.cluster');
+  r.serverPagination.stores.cluster.resources.enableSome.enabled.push('cluster');
+  r.serverPagination.stores.cluster.resources.enableSome.enabled.push('rke.cattle.io.etcdsnapshot');


in theory the three above shouldn't change anything as they're in the cluster store rather than management.

to get these without code changes, and fix the prov and mgmt cluster contexts (which are now not needed) shell/config/settings.ts serverPagination can be updated and then the new button Populate with latest pagination defaults can be used (and then applied). That process will soon be greatly updated (i have a PR, but it's in flux)

richard-cox · 2025-01-23T18:20:44Z

shell/detail/provisioning.cattle.io.cluster.vue

+
+        if (this.$store.getters[`management/paginationEnabled`]?.(args)) {
+          const templateOpt = PaginationParamFilter.createMultipleFields([{
+            field: 'spec.clusterName',


this reference, and snapshot and machine deployments need adding to StevePaginationUtils.VALID_FIELDS and then also to rancher/rancher#48603

Done for both

codyrancher · 2025-01-28T16:53:42Z

It doesn't look like the list currently makes SSP requests. I fixed this by comment in shell/utils/settings.ts file.
Once enabled there were failed http calls to secondary resources given the filter wasn't supported by certain type (this is expected, added inline comment). i commented those filters out and the list shows however as there's no configured pagination the columns come from the schema attribute columns rather than matching what was there before. there's a fair few examples on how the pagination headers can be setup in the explorer product

Yeah, I really don't know what happened there. Anyway, it should be working now.

codyrancher · 2025-01-28T16:55:55Z

This whole block needs to stay.
if (setting.resource === enabledFor.resource?.id) { - used to ensure the correct entry is found
if (!!setting.context) { - the context was used to explictely block pagination outside of supported areas. For instance the prov cluster was only allowed in the side nav and home page and not the cluster management list

I put this back in. I think it might be nice if we add dev warnings so it's a little more obvious why some resources aren't using the pagination API

codyrancher · 2025-01-28T16:57:00Z

shell/plugins/dashboard-store/actions.js

@@ -143,6 +143,21 @@ export default {
    }
  },

+  async findPageOrAll(ctx, {


Is there an existing alternative to this?

Interesting point. I've just used findPage when fetching secondary resources.

We can switch to this approach though

Lets go down this path.

I've just hit a similar scenario with findMatching. if pagination is on for type it would use findPage instead.

i was thinking of flipping the order, so we would having

findAll

findAllOrPage

findMatching

findMatchingOrPage

actually, wondering about this pattern.

we would need developers to understand the difference between findX and findPage. Open to suggestions.

for my uses case (findMatching or findPage) i've simplified it to a new findLabelSelector action

I mean... if we're going into the paginated world developers will have to understand the difference. Is there a way to get around that?

If we wanted to slow roll this and get devs slowly accustomed I think we've already gone down the wrong path. To slow roll I would've done the feature flag switches on a page level so we could keep the exact same logic on the existing pages and then switch to the paginated pages if the feature flag is set. In this scenario we would've had a two way door where we could turn the feature flag off (maybe a per page global setting) and fallen back on existing tested logic. Since we're switching on the component level we now have paginated and unpaginated code mixed throughout so I think the devs will have to recognize and understand it.

One other option instead of the findAllorPage method could just be dependency inversion with an interface like find(notPaginatedFindMethod, paginatedFindMethod). In this scenario the consumer has to understand what method's they're invoking and we're just doing the feature flag switching for them. It's a little clunky with the store though.

richard-cox

There's an error in console

Error in fetch(): TypeError: Cannot read properties of null (reading 'fields')
    at steve-pagination-utils.ts:433:1

one of the filter values is null. there'll be a bug somewhere, probably somewhere generic. let me know if you want me to track it down.

With latest head i'm seeing an error when a filter is used ("unrecognized operator: ",). Will check with the BE team

richard-cox · 2025-01-30T11:33:40Z

shell/config/settings.ts

@@ -278,8 +278,10 @@ export const DEFAULT_PERF_SETTING: PerfSettings = {
          enableAll:  false,
          enableSome: {
            enabled: [
-              { resource: CAPI.RANCHER_CLUSTER, context: ['home', 'side-bar'] },
+              { resource: CAPI.RANCHER_CLUSTER, context: ['home', 'side-bar', 'provisioning.cattle.io.clusters'] },


we should now be able to remove both home and side-bar contexts.

I removed it but I know I needed it for my current version of rancher. I haven't tried the latest since we had that sort issue you pointed out.

interesting. there's the filter issue in latest rancher. lets take a look at this once that's fixed (waiting on steve bump, though delayed a bit because i asked for rancher/rancher#48603 at same time)

regarding the latest update, i made a mistake.

leaving only the prov cluster context in will mean the side nav won't use pagination. which means there'll be no error given the filtering that the side nav uses is (backend) broken.

so the final version of this line would need to have no context

richard-cox · 2025-01-30T11:34:08Z

shell/config/settings.ts

              { resource: MANAGEMENT.CLUSTER, context: ['side-bar'] },
+              { resource: CAPI.MACHINE, context: ['provisioning.cattle.io.clusters', 'provisioning.cattle.io.cluster'] },


the two context entries here should be safe to remove (and from below as well)

I removed it but I know I needed it for my current version of rancher. I haven't tried the latest since we had that sort issue you pointed out.

shell/list/provisioning.cattle.io.cluster.vue

richard-cox · 2025-01-30T11:59:38Z

shell/list/provisioning.cattle.io.cluster.vue

-      return filterHiddenLocalCluster(this.rows, this.$store);
+    // We no longer make the request for rows the paginated table does. We still need to access these rows for things like harvester.
+    filteredRows() {
+      return this.$refs.paginatedTable?.filteredRows || [];


We need to have a way to get the uses of rows and filteredRowes working in both local pagination and SSP.

SSP
rows & hiddenHarvesterCount - we can get row counts from the counts object, we might need to make a specific http request to get the other count though (pageSize=1&filter=xyz)
rows & tokenExpiredData - i think we'll need to remove this feature with SSP on.
filteredRows & nonStandardNamespaces - this would need to be another http request

local pagination
i think maybe we continue with the code in the PR

richard-cox · 2025-01-30T12:05:00Z

shell/plugins/dashboard-store/actions.js

@@ -143,6 +143,21 @@ export default {
    }
  },

+  async findPageOrAll(ctx, {


Interesting point. I've just used findPage when fetching secondary resources.

We can switch to this approach though

richard-cox · 2025-01-30T12:05:55Z

shell/plugins/dashboard-store/actions.js

@@ -143,6 +143,21 @@ export default {
    }
  },

+  async findPageOrAll(ctx, {
+    type, context, paginatedOpt, findAllOpt


not that findAllOpts isn't supplied where this is used.

however we should be safe just chopping out the pagination object and using the paginatedOpts (though it should be harmless)

I'm not sure I'm understanding the ask here.

Are you just wanting me to remove the object de-structuring?

sorry, that should read note.

looking at where the findPageOrAll actions are dispatched there are only ever type, context and paginatedOpt properties, never findAllOpt.

i don't think findAllOpt is needed though. for the findAll case we could just chop out the pagination property from the paginatedOpt object

shell/utils/cluster.js

shell/list/provisioning.cattle.io.cluster.vue

codyrancher · 2025-01-31T04:05:40Z

There's an error in console

Error in fetch(): TypeError: Cannot read properties of null (reading 'fields')
at steve-pagination-utils.ts:433:1

one of the filter values is null. there'll be a bug somewhere, probably somewhere generic. let me know if you want me to track it down.

This one turned out to be my fault. I missed it because I had largely stopped verifying the hide-local-cluster setting and it was only surfacing when that was on.

shell/components/form/ResourceTabs/index.vue

fixes rancher#9548

codyrancher · 2025-02-03T16:56:29Z

@richard-cox except for the findPaginatedOrAll conversation there aren't anymore changes you want here are there? This is just backend blocked.

I just want to make sure there's nothing else that needs to be actioned upon before I move on.

richard-cox · 2025-02-04T11:54:57Z

shell/utils/cluster.js

@@ -63,6 +63,24 @@ export function paginationFilterHiddenLocalCluster(store, filterMgmtCluster = tr
  return PaginationParamFilter.createMultipleFields(filter);
 }

+/**
+ * Checks to see if the current user has access to the local store


minor typo - local store --> local cluster

richard-cox

I managed to miss the detail page, given it a once over code wise.

Functional wise we're still blocked on the BE filter bug

richard-cox · 2025-02-04T11:58:18Z

shell/detail/provisioning.cattle.io.cluster.vue

@@ -212,27 +114,24 @@ export default {
      this.logSocket.disconnect();
      this.logSocket = null;
    }
+
+    this.$store.dispatch('management/forgetType', CAPI.MACHINE);


how come these needed?

richard-cox · 2025-02-04T12:00:21Z

shell/detail/provisioning.cattle.io.cluster.vue

@@ -82,9 +62,6 @@ export default {
  },

  async fetch() {
-    await this.value.waitForProvisioner();


shouldn't this need to stay for non-ssp world?

richard-cox · 2025-02-04T12:00:55Z

shell/detail/provisioning.cattle.io.cluster.vue

@@ -103,102 +80,27 @@ export default {
      this.extCustomParams = { provider: this.value.machineProvider };
    }

-    // Support for a model extension


this section has been lost

richard-cox · 2025-02-04T12:01:35Z

shell/detail/provisioning.cattle.io.cluster.vue

-    }
-
-    const schema = this.$store.getters[`management/schemaFor`](CAPI.RANCHER_CLUSTER);
-    const fetchOne = { schemaDefinitions: schema.fetchResourceFields() };


schema.fetchResourceFields has been lost too

richard-cox · 2025-02-04T12:04:08Z

shell/detail/provisioning.cattle.io.cluster.vue

-      if ( this.$store.getters['management/canList'](MANAGEMENT.NODE_POOL) ) {
-        fetchTwo.allNodePools = this.$store.dispatch('management/findAll', { type: MANAGEMENT.NODE_POOL });
-      }
+    this.loadNodeTemplateForMasthead();


these should be awaited (asynchronously as part of promises object, as per below pattern, all all changed to use allHash method)

richard-cox · 2025-02-04T13:09:25Z

shell/detail/provisioning.cattle.io.cluster.vue

+          search: false
+        },
+        {
+          ...ROLES,


this comes from a label, so we should be able to sort/filter via ssp

(same for mgmt nodes)

richard-cox · 2025-02-04T13:10:10Z

shell/detail/provisioning.cattle.io.cluster.vue

+          search: false
+        },
+        {
+          ...MACHINE_NODE_OS,


we might be able to sort on this, though it would be a bit off with the empty value (which defaults to linux)

(same for mgmt nodes)

richard-cox · 2025-02-04T13:11:33Z

shell/detail/provisioning.cattle.io.cluster.vue

+      return [
+        {
+          ...STEVE_STATE_COL,
+          value:         'snapshotFile.status',


is this indexed? if not we need to add it

richard-cox · 2025-02-04T13:13:29Z

shell/detail/provisioning.cattle.io.cluster.vue

@@ -283,97 +178,8 @@ export default {
      return info;
    },

-    fakeMachines() {


this is like the project/namespace list were we should show the group even if there are no rows. for instance if a pool has been scaled down to zero but we should still show the pool group. Does this still work with SSP off and on?

richard-cox · 2025-02-04T13:17:14Z

shell/detail/provisioning.cattle.io.cluster.vue

+          context="provisioning.cattle.io.cluster"
+        >
+          <!-- Removing the group by namespace/list buttons in the middle header -->
+          <template #header-middle />


We specify a groupBy (backup location), i think that the controls should still show (given it's an annotation that can be indexed)

rancher-ui-project-bot bot assigned codyrancher Jan 21, 2025

codyrancher force-pushed the management-cluster branch from 4046f35 to eb5daa5 Compare January 21, 2025 16:11

codyrancher changed the title ~~Management cluster~~ Converting the management cluster list and detail pages to be paginated Jan 21, 2025

rancher-ui-project-bot bot added this to the v2.11.0 milestone Jan 21, 2025

codyrancher commented Jan 21, 2025

View reviewed changes

codyrancher requested a review from richard-cox January 21, 2025 17:14

codyrancher force-pushed the management-cluster branch 3 times, most recently from 25db9c0 to 827d555 Compare January 23, 2025 16:00

richard-cox requested changes Jan 23, 2025

View reviewed changes

codyrancher force-pushed the management-cluster branch 2 times, most recently from 46cc4bc to 752a239 Compare January 28, 2025 16:52

codyrancher commented Jan 28, 2025

View reviewed changes

codyrancher marked this pull request as ready for review January 28, 2025 17:33

codyrancher force-pushed the management-cluster branch from 752a239 to 3259fb7 Compare January 28, 2025 17:49

richard-cox requested changes Jan 30, 2025

View reviewed changes

codyrancher force-pushed the management-cluster branch from 3259fb7 to a756d9a Compare January 31, 2025 05:52

codyrancher commented Jan 31, 2025

View reviewed changes

shell/components/form/ResourceTabs/index.vue Outdated Show resolved Hide resolved

codyrancher requested a review from richard-cox January 31, 2025 05:58

codyrancher force-pushed the management-cluster branch from 26fd853 to d13ac57 Compare January 31, 2025 16:56

codyrancher and others added 2 commits January 31, 2025 15:52

Converting the management cluster list and detail pages to be paginated

1699f33

fixes rancher#9548

Tweak to events fix, improved existing valid fields comment

9f700e1

codyrancher force-pushed the management-cluster branch from d13ac57 to 9f700e1 Compare January 31, 2025 22:53

richard-cox reviewed Feb 4, 2025

View reviewed changes

richard-cox requested changes Feb 4, 2025

View reviewed changes

richard-cox mentioned this pull request Feb 4, 2025

RKE1 EOL UI Warnings #13284

Open

		{ resource: MANAGEMENT.CLUSTER, context: ['side-bar'] },
		{ resource: CAPI.MACHINE, context: ['provisioning.cattle.io.clusters', 'provisioning.cattle.io.cluster'] },

Converting the management cluster list and detail pages to be paginated #13170

Are you sure you want to change the base?

Converting the management cluster list and detail pages to be paginated #13170

Conversation

codyrancher commented Jan 21, 2025 • edited Loading

Summary

Occurred changes and/or fixed issues

Areas or cases that should be tested

Areas which could experience regressions

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard-cox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher commented Jan 28, 2025 • edited Loading

codyrancher commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard-cox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher commented Jan 31, 2025

codyrancher commented Feb 3, 2025

Choose a reason for hiding this comment

richard-cox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyrancher commented Jan 21, 2025 •

edited

Loading

codyrancher commented Jan 28, 2025 •

edited

Loading