Performance dev/pr4 - pagination #8507

ghost · 2023-03-23T14:23:23Z

Full Description of changes and bits and pieces to test upcoming.

Edit - Associated issue #6541 (won't fix, pr merges to performance-dev

richard-cox · 2023-03-27T14:41:55Z

pkg/harvester/list/harvesterhci.io.dashboard.vue

@@ -15,7 +15,7 @@ import {
 } from '@shell/config/types';
 import { HCI } from '../types';
 import ResourceSummary, { resourceCounts, colorToCountName } from '@shell/components/ResourceSummary';
-import { colorForState } from '@shell/plugins/dashboard-store/resource-class';
+import { colorForState } from '@shell/plugins/steve/resourceUtils/resource-class';


Anything from @shell/plugins/dashboard-store should still live within dashboard-store

richard-cox

To Do

Check comments
slow loading of secondary resource leads to eventual list automatically updated
force fetch primary / secondary resource and list automatically updates?
capture issue on duplication of computed/getter fields executions (done in cache and in ui thread)

To Test

build and load harvester plugin

richard-cox · 2023-03-27T14:49:05Z

shell/plugins/steve/caches/resource-class.js

+  }
+
+  /**
+   * Find the resource/s associated with the params in the cache. IF we don't have the resource/s for the params we'll fetch them


needs updating.

richard-cox · 2023-03-27T14:57:09Z

shell/plugins/steve/caches/resource-class.js

+    }
+
+    // spread the raw resource and the calculatedFields together for the filterCondition which doesn't care about the difference
+    const resources = Object.values(this.resources)


TODO: RC can this be combined with below

- cluster socket now works - url wasn't updated to contain url with cluster id - fixed on instance of mgmtById - added upstream fix of addSchemaIndexFields

richard-cox

To make it a little clearer model base classes (shell/plugins/steve/hybrid-class.js, norman-class and steve-class) should move in to a shell/plugins/steve/models. that should make it cleared there's three resource based buckets - models, caches and utils. at some point we may want to look at moving advanced worker specific files into a single folder

richard-cox · 2023-03-29T10:40:41Z

shell/mixins/resource-fetch-query.js

+
+          return [sortBy, sortDesc];
+        },
+        setGroup: (groupBy) => {


Is this wired in, looks like it's missing due to @group-value-change="group = $event"

richard-cox · 2023-03-29T10:44:28Z

shell/components/ExplorerProjectsNamespaces.vue

@@ -356,6 +356,10 @@ export default {
      :loading="loading"
      group-tooltip="resourceTable.groupBy.project"
      key-field="_key"
+      :set-page-fn="resourceQueryMethods.setPage"


There's a lot of plumbing to connect sortable table pagination changes to the resource-fetch-query. Could this be improved?

Could they be trimmed up into a pagination prop which receives an object of pagination setX fns, also covering listLength?

Could they be stored in vuex (pros and cons to this one though)?

richard-cox · 2023-03-29T10:46:23Z

shell/components/ResourceList/index.vue

+    advancedWorker() {
+      return !!this.perfConfig?.advancedWorker?.enabled;
+    },
+    listLength() {


listLength and totalLength needs some jsdocs. is list length the size of filtered collection, and total length the unfiltered size?

richard-cox · 2023-03-29T12:45:44Z

shell/components/ResourceTable.vue

-      if ( !this.isNamespaced || (isAll && !this.currentProduct?.hideSystemResources) || this.ignoreFilter) {
+      // If the resources isn't namespaced or we want ALL of them, there's nothing to do or if the resources are filtered in the fetched results.
+      // if ( !this.isNamespaced || (isAll && !this.currentProduct?.hideSystemResources) || this.ignoreFilter || this.setSearchFn) {
+      if ( this.setSearchFn || !this.isNamespaced || (isAll && !this.currentProduct?.hideSystemResources) || this.ignoreFilter) {


This feels a bit obfuscated, but can get away with tweak to the comment

// If the resources isn't namespaced or we want ALL of them, there's nothing to do or if the resources are filtered in the fetched results.

-->

// If the resources isn't namespaced or we want ALL of them, there's nothing to do or if the resources are filtered in the fetched results (setSearchFn will defined).

richard-cox · 2023-03-29T12:46:25Z

shell/components/ResourceTable.vue

-    <template
-      v-for="(_, slot) of $scopedSlots"
-      v-slot:[slot]="scope"
+  <span>


span shouldn't be needed?

richard-cox · 2023-04-05T15:49:16Z

shell/plugins/steve/caches/resource-class.js

+  /**
+   * Requests data from the cache's data source
+   */
+  async request(params = {}) {


Depending on comment within method, we should consider changing this to find (aka make http request if needed and return values from cache) to align with verbage from vuex side?

richard-cox · 2023-04-05T15:50:24Z

shell/plugins/steve/worker/web-worker.advanced.js

 /**
 * Errors sent over the thread boundary must be clonable. The response to `fetch` isn't, so cater for that before we send it over
 */
-const parseRequestError = (e) => {


This will need wiring in to a similar top level place (it handled both types of exception fetch throws)

richard-cox · 2023-04-05T15:50:57Z

shell/plugins/steve/worker/web-worker.advanced.js

-  updateCache: (type, payload, detail = false) => {
+  createCache: (type) => {
+    if (type && !caches[type]) {
+      caches[type] = resourceCache(type, getters, rootGetters, state.api, uiApi, workerActions.createCache);


resourceCache doesn't have this many args

richard-cox · 2023-04-05T16:00:10Z

shell/plugins/steve/caches/base-cache.js

+  }
+
+  /**
+   * Find the resource/s associated with the params in the cache. IF we don't have the resource/s for the params we'll fetch them


Given this method now doesn't make the http request

jsdoc needs updating

find name might be better renamed to get (to avoid confusion with the find = http request + get from cache concept)

richard-cox · 2023-04-05T16:00:54Z

shell/plugins/steve/caches/index.js

+import ReplicationcontrollerCache from '@/shell/plugins/steve/caches/replicationcontroller';
+import CronJobCache from '@/shell/plugins/steve/caches/batch.cronjob';
+import JobCache from '@/shell/plugins/steve/caches/batch.job';
+import WorkloadCombinedCache from '@shell/plugins/steve/caches/workload.combined';

 export default {


Needs updating and testing with all the resource cache's

Fixes mutation test, now includes listLength and totalLength properties

richard-cox

Blocking

Bugs

General
- Race condition on some systems means url used to connect to cluster web socket hasn't been changed from default (see updateWorker for potential fix)
User and Auth
- Cannot Assign Group Role
  - Setup github provider --> assign group role. & create user
    - Cannot read properties of undefined (reading 'description')
    - Cannot read properties of undefined (reading 'description')
      at _getDescription (steve-description-class.js:2:1)
- Cannot Create User
  - same as above
- Cannot create any role
  - Create global role --> white page error
    - Cannot read properties of null (reading 'detailPageHeaderActionOverride')
    - Error in fetch(): TypeError: Cannot read properties of undefined (reading 'description')
      at _getDescription (steve-description-class.js:2:1)
Cluster Explorer
- Nav to Projects/Namespace. Error in console
  - console.js:31 Error in fetch(): TypeError: this.namespaces is not a function
    at VueComponent.selectedNamespaces (resource-fetch-query.js:293:1)
  - set page size to 10, refresh on project/namespace list, list shows 10 but no pagination controls
  - set page size to 10, refresh on project/namespace list, search fails to do anything (works on pods)
    - 5a55fdea-26a5-4fea-86d8-148085bd705f:58435 Uncaught (in promise) TypeError: value.includes is not a function
      at 5a55fdea-26a5-4fea-86d8-148085bd705f:58435:70
  - pods page, create a pod, dashboard returns to pods list. pod appears... then disappears
  - pods page, create a pod in another window, pod does not appear in original window
  - pods page, changing from all namespaces to a single namespace does not change the list
  - pods page. sorting by IP, Ready, Restarts doesn't work (no visible change to list)
  - changing NS filter does not hit advanced worker requests
Fleet (continues delivery)
- Bundles, Workspaces sort by any field doesn't work. Git Repos works
Cluster Manager
- Clusters list with an rke2 instance coming up has error in console
  - console.js:31 [Vue warn]: Property or method "subMatches" is not defined on the instance but referenced during render. Make sure that this property is reactive, either in the data option, or for class-based components, by initializing the property. See: https://vuejs.org/v2/guide/reactivity.html#Declaring-Reactive-Properties.

General PR

Failing e2e tests

General PR/Comments

Lots of jsdoc / comments needed. some // need to
Lots of ToDos still
Any file that's resource cache related should be named with cache rather than class

Tests (once bugs are fixed and more code review comments resolved)

Pagination
- Pages (nex/prev, first/last page)
- Sorting
  - default-sort-by honoured
  - on normal + calc fields
- Filtering
  - on normal + calc fields
- intermedate loading / manual refresh / forced ns filtering
- Resources to check
  - Workloads, each type + pods
  - config maps
  - secrets
Finds
- params like load, force, etc are honoured
- re-check namespace vs namespaces
- error handling
  - caused by something we throw, connection errors, errors from backend (see parseRequestError)
- repeat finds with don't kick off http requests
  - with exact same params
  - with different page number
- repeat find do kick off http requests
  - force on
Re-pagination
- Change a secondary resource, primary resource's dependent field is updated, re-pagination happens, new page sent to ui thread
- Change a primary resource, primary resource's fields are updated and resource page is re-calculated, re-pagination happens, new page sent to ui thread
- Change primary or secondary resouce that does not change the current page means ui thread not updated
Secondary resources
- Only required secondary resources are passed back to ui thread, which are used by models to calc fields
  - only those required secondary resource are returned
  - how does ui thread recognise these lists, what if we go from deployment (requiring pods) to pods list?
Fetching non-cluster info from within worker thread
- how often, how caching is managed
Lists
- resource-fetch-query - how do we differentiate between local ns filtering and request ns filtering
- live reload columns work when sorting, filtering, re-paginated, etc
- cacluated fields that rely on l10n work correctly
Performance Setting
Disabling advanced worker ... everything works as expected
Description of advanced worker in perf settings still makes sense
podsByNamespace functionality still works fine
Deep dive in to mixins/resource-fetch-query
- does namespaceFilter fire at same time as resourceQuery
Error handling
- Errors caused by code

Pending In Depth Review

mixins/resource-fetch-query
- including pagination functions
podsByNamespace

Nice to have / not really blockers

Tidying up places where loose functions are exported
- resourceUtils would be converted to export default { calculatedFields, fields }
Move pagination settings into vuex store alongside list to reduce LOTS of plumbing from components through to advanced worker
Make advanced worker perf setting more granualar

Not Blocking

Improvements / Tech Debt

resource cache's calculated fields are calculated within worker (to paginate) but aren't passed by to ui thread. ui thread model then (re)calculated fields.
- This could be an issue if calc field iterates through another list, for example lots of finding deployment's pods. If this is optimised needs testing
We now pretty much mock the ctx / store within the advanced worker. That could be used in conjuction with the models (which receive ctx) to allow us to use the same models. Using models would mean no utils classes and much more simpler calculated fields (we could possibly use decorators on getters instead)
There's lots of boilerplate code in resource caches

richard-cox · 2023-04-14T13:01:36Z

shell/components/ResourceList/index.vue

+      return !!this.perfConfig?.advancedWorker?.enabled;
+    },
+    listLength() {
+      const { params: { resource: type } } = this.$route;


type is already available via this.resource

richard-cox · 2023-04-14T13:01:44Z

shell/components/ResourceList/index.vue

+      return this.$store.getters['cluster/listLength'](type);
+    },
+    totalLength() {
+      const { params: { resource: type } } = this.$route;


type is already available via this.resource

richard-cox · 2023-04-14T13:02:50Z

shell/components/ResourceList/index.vue

@@ -150,6 +164,11 @@ export default {
      if (neu && !this.hasFetch) {
        this.$fetchType(this.resource);
      }
+    },
+    resourceQuery(neu, old) { // TODO: RC test


this is similar to namespaceFilter, there's a couple of different use cases that had to cover (see comment). are the same needed use cases needed here?

richard-cox · 2023-04-14T13:03:49Z

shell/plugins/dashboard-store/actions.js

@@ -550,7 +569,9 @@ export default {
    return classify(ctx, data);
  },

-  clone(ctx, { resource } = {}) {
+  clone(ctx, payload) {
+    const { resource } = payload;


change not needed (and is less save)

richard-cox · 2023-04-14T13:04:29Z

shell/plugins/steve/resourceWatcher.js

@@ -68,6 +67,7 @@ const {
  WATCH_PENDING, WATCH_REQUESTED, WATCHING, STOPPED, REMOVE_PENDING, REQUESTED_REMOVE
 } = WATCH_STATUSES;

+// TODO: RC jsdocs
 export default class ResourceWatcher extends Socket {


jsdoc definitely needed for this one

richard-cox · 2023-04-14T13:17:15Z

shell/plugins/steve/caches/resource-class.js

+    });
+
+    this.__requests[cacheKey] = {
+      ...this.__requests[cacheKey],


why do we spread?

richard-cox · 2023-04-14T13:18:12Z

shell/plugins/steve/worker/web-worker.advanced.js

 import cacheClasses from '@shell/plugins/steve/caches';
 import ResourceRequest from '@shell/plugins/steve/api/resourceRequest';
 import Trace from '@shell/plugins/steve/trace';
+import { CACHE_STATES } from '@shell/plugins/steve/caches/base-cache';
+import { hashObj } from '~/shell/utils/crypto/browserHashUtils';


All (new) imports with ~/ need fixing

richard-cox · 2023-04-14T13:18:43Z

shell/plugins/steve/worker/web-worker.advanced.js

+
+    if (cacheAge > 500 && (uiRequests[cacheGetter] || []).length < 1) {
+      // TODO: RC question If the cache of non-cluster resources changes... do we ned to bump anything that depends on them?
+      self.postMessage({ get: cacheGetter });


Do we ever rely on content from outside the cluster store in any of the calculated fields?

richard-cox · 2023-04-14T13:19:47Z

shell/plugins/steve/worker/web-worker.advanced.js

@@ -394,15 +484,79 @@ const workerActions = {

    Object.values(caches).forEach(c => c.setDebug(cache));
  },
-  updateBatch(type, id, change) {
+  singleUpdateBatch(type, id, change) {
    if (!state.batchChanges[type]) {
      state.batchChanges[type] = {};
    }
    state.batchChanges[type][id] = change;
  },
  sendBatch(requestHash) {


v important code block, needs jsdoc and comments

richard-cox · 2023-04-14T13:20:03Z

shell/plugins/steve/worker/web-worker.advanced.js

@@ -394,15 +484,79 @@ const workerActions = {

    Object.values(caches).forEach(c => c.setDebug(cache));
  },
-  updateBatch(type, id, change) {
+  singleUpdateBatch(type, id, change) {
    if (!state.batchChanges[type]) {
      state.batchChanges[type] = {};
    }
    state.batchChanges[type][id] = change;
  },
  sendBatch(requestHash) {


this doesn't re-paginate (byPage) .. looks like this just pushes in-page resource updates

Merge branch 'performance_dev/pr3' into performance_dev/pr4

dc499c2

github-actions bot assigned ghost Mar 23, 2023

A lot of changes but most of them are just moving code to be shareable.

ae211e9

richard-cox reviewed Mar 27, 2023

View reviewed changes

richard-cox added this to the v2.7.next2 milestone Apr 4, 2023

Sean and others added 4 commits April 4, 2023 09:56

Order of operations seems sorted now, workloads added back in

df04c74

namespace issue when using namespace categories such as "userNamespaces"

b488e8d

RC TODOs and fixes

fe02cef

- cluster socket now works - url wasn't updated to contain url with cluster id - fixed on instance of mgmtById - added upstream fix of addSchemaIndexFields

More TODOs, move some to github review

b601b72

richard-cox requested changes Apr 5, 2023

View reviewed changes

Subscriptions work better now, listLength and totalLength work smoothly

8fe0cc3

ghost marked this pull request as ready for review April 12, 2023 08:06

Sean and others added 4 commits April 12, 2023 04:11

cache imports fixed, subscriptions only push resources on page to ui

6ee3275

Fixes mutation test, now includes listLength and totalLength properties

tightening up the rootgetters

079ec8b

Fixes login, detail requests, auth users, roles, and provider lists

e9cbb91

trace improvements, comments

437605f

richard-cox requested changes Apr 14, 2023

View reviewed changes

Move todos to more definitive list in pr

766d4e3

richard-cox mentioned this pull request Apr 21, 2023

Performance: Server-side-like pagination via web worker (step 4) #6541

Closed

ghost closed this May 1, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance dev/pr4 - pagination #8507

Performance dev/pr4 - pagination #8507

ghost commented Mar 23, 2023 •

edited by richard-cox

Loading

richard-cox Mar 27, 2023

richard-cox left a comment

richard-cox Mar 27, 2023

richard-cox Mar 27, 2023

richard-cox left a comment

richard-cox Mar 29, 2023

richard-cox Mar 29, 2023

richard-cox Mar 29, 2023

richard-cox Mar 29, 2023

richard-cox Mar 29, 2023

richard-cox Apr 5, 2023

richard-cox Apr 5, 2023

richard-cox Apr 5, 2023

richard-cox Apr 5, 2023

richard-cox Apr 5, 2023

richard-cox left a comment

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

richard-cox Apr 14, 2023

Performance dev/pr4 - pagination #8507

Performance dev/pr4 - pagination #8507

Conversation

ghost commented Mar 23, 2023 • edited by richard-cox Loading

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

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

richard-cox left a comment

Choose a reason for hiding this comment

Blocking

Bugs

General PR

General PR/Comments

Tests (once bugs are fixed and more code review comments resolved)

Nice to have / not really blockers

Not Blocking

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

ghost commented Mar 23, 2023 •

edited by richard-cox

Loading