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

Use Kubernetes Pagination for Lists #5

Merged
merged 2 commits into from May 23, 2018

Conversation

alecmerdler
Copy link
Contributor

@alecmerdler alecmerdler commented May 14, 2018

Description

Modifies the watchK8sList Redux action to use basic k8s pagination to incrementally fetch the full list of k8s objects (set to ?limit=250). This achieves a reduction in the perceived load time for users with huge cluster workloads.

Note: Still needs react-virtualized components to address actual view performance.

Screenshots

Incremental loading (set limit=20 for demo purposes):
console-pagination

Addresses https://jira.coreos.com/browse/CONSOLE-377

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 14, 2018
@@ -55,7 +55,7 @@ const ConfigMapDetails = ({obj: configMap}) => {
};

const ConfigMaps = props => <List {...props} Header={ConfigMapHeader} Row={ConfigMapRow} />;
const ConfigMapsPage = props => <ListPage ListComponent={ConfigMaps} canCreate={true} {...props} />;
const ConfigMapsPage = props => <ListPage ListComponent={ConfigMaps} canCreate={true} limit={100} {...props} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if limit should be passed as a prop or always used by default.

@openshift openshift deleted a comment from coreos-ui May 14, 2018
@openshift openshift deleted a comment from coreos-ui May 14, 2018
@alecmerdler alecmerdler force-pushed the CONSOLE-377 branch 2 times, most recently from db6b225 to 8dbda07 Compare May 14, 2018 16:59
@alecmerdler
Copy link
Contributor Author

Open Questions:

  • Do we always use ?limit=100 for every list, or is it passed as a prop?
    • prefer default behavior for all Firehoses
    • prefer constant incremental loading using ?limit= vs 1 small request using ?limit= + the rest
  • Which Redux actions do we dispatch?
    • initialLoad Redux event
  • List items get re-ordered as they are fetched because they are alphabetized
    • punt to UX
  • Text filtering as new results are loaded
    • punt to UX

@jcaianirh
Copy link
Member

@openshift/team-ux-review

this could use some UX weigh-in. paginating requests means that a user will see say.... 50 pods in a list, then as they're about to click on one pod, the next XHR comes back and 50 more pods are inserted in the list and they click on the wrong link and are frustrated
alec, amruta, & I discussed possible solutions, but we didn't come across anything that seemed obviously correct

@serenamarie125
Copy link
Contributor

I'd suggest that the page size is settable by the user. We have a pattern for this in PatternFly today, although there is no suggested "sizes", reason being that the page size likely differs based on the product, technology and performance issues. I would suggest that we allow the user to choose between 25/50/100. @alecmerdler @jcaianirh thoughts?

FYI @beanh66 @cshinn

@alecmerdler
Copy link
Contributor Author

alecmerdler commented May 16, 2018

I'd suggest that the page size is settable by the user

@serenamarie125 The question is not about page size, as this PR does not introduce client-side pagination. We strongly advocate for keeping list views a single page with incrementally loading data:

  • Reduces complexity of our already complex list code
  • Supports the primary use-case (Go to list, type in text-filter, click desired item)
  • Causes more confusion: pages would be added as more data loads in (can't know the size of the list)
    We need a pattern that captures the user's current intent (searching/filtering) while supporting items being added to the list.

@serenamarie125
Copy link
Contributor

@alecmerdler i'm sorry, i misunderstood. Let's see if i get this right - you are incrementally loading the page ? What order is the information in? Can you request that it be ordered based on the current sort order of the page ?

@alecmerdler alecmerdler force-pushed the CONSOLE-377 branch 3 times, most recently from c66e0ce to 9050164 Compare May 17, 2018 18:06
@alecmerdler alecmerdler changed the title [WIP] Use Kubernetes Pagination for Lists Use Kubernetes Pagination for Lists May 17, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2018
@alecmerdler alecmerdler force-pushed the CONSOLE-377 branch 2 times, most recently from b748e4e to 39bf40c Compare May 17, 2018 19:13
const currentJS = current.toJSON();
currentJS.metadata.resourceVersion = nextJS.metadata.resourceVersion;
if (_.isEqual(currentJS, nextJS)) {
if (current.deleteIn(['metadata', 'resourceVersion']).equals(next.deleteIn(['metadata', 'resourceVersion']))) {
Copy link
Contributor

@kans kans May 17, 2018

Choose a reason for hiding this comment

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

I tried this once a while ago, and it didn't work. I must have used Map for my test instead offromJS - the former only works if the nested objects are ===. Nice.

@@ -28,6 +26,7 @@ const POLLs = {};
const REF_COUNTS = {};

const nop = () => {};
const paginationLimit = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about a higher number? A 10K cluster would require 100 requests to finish loading. For Pods at least, we show the current count which would tick up ... for a while. It looks like pods run about 7KB per item - nodes are closer to 12. A limit of 250 would land at ~2MB per response.

// 1. the WS closes abnormally
// 2. the WS can not establish a connection within $TIMEOUT
const incrementallyLoad = async(continueToken = '') => {
const response = await k8sList(k8skind, {...query, limit: paginationLimit, continue: continueToken}, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of things:

  1. It looks like incrementallyLoad will run to completion even if we drop the watch (stopK8sWatch). I didn't look too closely, but we may even continue to start up the rest of the machinery (this could be an existing race condition). We should probably check REF_COUNTS[id] on every iteration and throw() and do magic in the catch.

  2. I can imagine incrementallyLoading never finishing on a large cluster with a sad internet. The behavior in that case would be we start loading the list, a XHR drops, then we throw away the data and start over. What do you think about adding a try/catch here to retry any given continueToken N times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some basic retry logic (attempts <= 3) and a naïve attempt to cancel the incremental fetch if a stopK8sWatch is dispatched. I'm not sure the best way to catch the errors I throw, any suggestions @kans?

Copy link
Contributor

Choose a reason for hiding this comment

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

Errors should fall into the error handler on the then - (second function so we don't accidentally catch rendering errors) on 219. It needs to check for ¿both? refcounts and the strings you put in the errors or you could subclass Error and check for that for more flexibility/clarity.

BTW, I don't think throwing strings is cromulent anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kans I'm having trouble getting an intuition around these edge cases; would you be okay with merging what we have here and making follow-up PRs to address those? I plan to improve our load-testing tools (ServiceWorker, etc) once this is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the worst case behavior would be that we create WS that live forever. To fix the race, we'd need to check for REF_COUNTS[id] in three places (and throw a special Error):

  1. before #L160
  2. Before we create the WS on #L180.
  3. A new function in the promise.all because pollAndWatch is called recursively for error handling.

We catch all errors on #L213. If the error is special error, I think we just need to exit.

I'd be OK deferring this work to an immediate follow up since we are already subject to the same race.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 18, 2018
@kans
Copy link
Contributor

kans commented May 21, 2018

@serenamarie125

Since I don't see a reply:

Let's see if i get this right - you are incrementally loading the page ?

affirmative.

What order is the information in?

Whatever order the API server returns - I'm not exactly sure what it is, but it certainly isn't what we want.

Can you request that it be ordered based on the current sort order of the page?

No. Lists are sorted and filtered client side because of limited functionality in the API.

ggreer
ggreer previously requested changes May 22, 2018
const response = await k8sList(k8skind, {...query, limit: paginationLimit, ...(continueToken ? {continue: continueToken} : {})}, true);

if (response.metadata.continue) {
dispatch(actions.bulkAddToList(id, response.items));
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to check REF_COUNTS[id] in here and abort the loading if it's not > 0. That way we're not trying to load data long after a component has been unmounted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I'm also OK with fixing this in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think we should address it separately because it is an existing problem and not a regression from this PR. I made two TODO comments to remind us.

dispatch(actions.bulkAddToList(id, response.items));
return incrementallyLoad(response.metadata.continue);
}
dispatch(actions.loaded(id, response.items));
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to dispatch loaded with all the items, not just the ones in the last response? In my experimentation, this causes the list to only show the last bit of data it fetched. eg: set paginationLimit to 10 and try to load all pods. Here's a video of it happening: https://i.imgur.com/NQKiEqL.gifv

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!, but we'd need to change the behavior of the reducer to do that. Always dispatching loaded first would probably be easier and we'd incrementally render stuff, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, fixed this by dispatching the loaded event after the first k8sList.

@serenamarie125
Copy link
Contributor

@alecmerdler as we spoke the other day, I wasn't able to identify any other products which have this same situation. Is this something that we can collaborate on together during development from a design perspective?

@alecmerdler alecmerdler force-pushed the CONSOLE-377 branch 2 times, most recently from b98f081 to 7a8feae Compare May 22, 2018 22:55
@ggreer ggreer dismissed their stale review May 22, 2018 23:12

bug fixed

Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@alecmerdler looks good so far ... I have a couple of comments
1- is it possible to put a message similar to this above the first row of data ( under the header ) so that the user is informed that the table is still loading?
screen shot 2018-05-23 at 11 16 28 am
2- the filter component flashes quite a bit because the numbers are updating, I wonder if there's another alternative to that as well? Does it make sense not to show the filters til the table is completely full ?
3 - do we allow for interaction in the filter/search component in the top right when the table is still loading ?

Copy link
Contributor

@ggreer ggreer left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

@ggreer
Copy link
Contributor

ggreer commented May 23, 2018

1- is it possible to put a message similar to this above the first row of data ( under the header ) so that the user is informed that the table is still loading?

Yes, but unless it's positioned carefully, it will guarantee that the items will move up when they're done loading.

3 - do we allow for interaction in the filter/search component in the top right when the table is still loading ?

Yes. You can click on filters or start searching and the current results will be filtered. As new data comes in, anything that matches the filters and search will be shown.

Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

I'm ok with this @alecmerdler , and pursuing iterating on the design in a follow up

@alecmerdler alecmerdler merged commit 7fadf57 into openshift:master May 23, 2018
@alecmerdler alecmerdler deleted the CONSOLE-377 branch May 23, 2018 18:18
atiratree pushed a commit to atiratree/console that referenced this pull request Oct 19, 2018
The folder holds documentation, deployment configuration and various
tools supporting kubevirt-related development processes.

Within this patch, following is added
- README
- kubevirt-web-ui.yaml - first version of deployment configuration
- mergeUpstream.sh script - to merge openshift/console changes back to our fork
christianvogt referenced this pull request in christianvogt/console Feb 22, 2019
enable redux devtools in non-production
jeff-phillips-18 referenced this pull request in jeff-phillips-18/console Oct 10, 2019
jtomasek pushed a commit to jtomasek/console that referenced this pull request Feb 12, 2020
openshift-merge-robot pushed a commit that referenced this pull request Dec 2, 2020
Use python3 in Dockerfile.product
jhadvig pushed a commit to jhadvig/console that referenced this pull request Dec 10, 2020
TimothyAsirJeyasing pushed a commit to TimothyAsirJeyasing/console that referenced this pull request Aug 3, 2022
Use mock model for HorizontalNav to limit tabs from being exposed in Operator DetailsPage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants