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

[2.5.11] Added typer to store #402

Merged
merged 1 commit into from
Oct 29, 2021
Merged

Conversation

paynejacob
Copy link
Contributor

@paynejacob paynejacob commented Sep 21, 2021

rancher/rancher#34721
rancher/rancher#30718
rancher/rancher#25487

Problem

For a deep dive on this problem see here. TLDR; Norman is slow listing large resources because the Kubernetes dynamic client parses the response from the Kubernetes API multiple times when the type is unstructured.Unstructured. This can lead to high memory consumption and excessive processing time deseralizing responses.

Solution

Stores can be optionally passed a typer. If the typer recognizes the resource associated with the given store it will return the Golang stuct for the given resource. This allows the client to deseralize the response more efficiently.

Testing

For a testing environment I used a k3d cluster with rancher running locally. All requests were done with curl piping the output to /dev/null. To test this I generated 200 configmaps with 8KB of data each. I then listed them with the application/json content type three times and averaged the response time. With no changes it took an average of 13,341 ms to list configmaps. Applying these changes but not registering ConfigMapList with the typer the response time averaged 12,719 ms. Registering the ConfigMapList with the typer brought it down to 7,764 ms.

Notes:

  • The time to first byte from the Kubernetes API averaged ~700 ms. This was not measured with every test but the request to the API is not affected.
  • All these test are done with all components talking over the local network. The real world performance gains main be greater when network transfers are factored in.
  • The 8KB data size was selected because response times were noticeably slower after 8KB. This threshold is dependent on a multitude of factors including CPU clockspeed, memory speed, memory size, etc. Clusters provisioned with more resources may not realize performance gains with these changes as quickly as smaller clusters. The performance will always be at a minimum the same as before these changes.
  • These tests do not account for how list transformers are interacting with the data, the performance gains here may not be consistent across all resources.
  • In testing I added Accept: application/protobuf, combined with these changes I was able to get responses as low as 2,000 ms. Protobuf is only available if the typer recognizes the resources and significantly limits our ability to debug calls between Rancher and Kubernetes. With these limitations I don't think it is worth adding but could be worth exploring in the future as a further optimization.

@paynejacob paynejacob force-pushed the fix/list-perf branch 2 times, most recently from ff60e98 to 994701d Compare September 22, 2021 00:29
@paynejacob paynejacob changed the title added typer to store WIP: added typer to store Sep 22, 2021
@paynejacob paynejacob changed the title WIP: added typer to store Added typer to store Oct 1, 2021
@paynejacob paynejacob force-pushed the fix/list-perf branch 2 times, most recently from 965d33f to d7812fc Compare October 1, 2021 22:54
@paynejacob paynejacob requested review from thedadams and removed request for StrongMonkey October 1, 2021 22:59
store/proxy/proxy_store.go Outdated Show resolved Hide resolved
store/proxy/proxy_store.go Outdated Show resolved Hide resolved
store/proxy/proxy_store.go Outdated Show resolved Hide resolved
@paynejacob paynejacob marked this pull request as draft October 5, 2021 20:31
@paynejacob
Copy link
Contributor Author

marking as draft until 2.5.11

@paynejacob paynejacob marked this pull request as ready for review October 11, 2021 20:46
@paynejacob paynejacob marked this pull request as draft October 11, 2021 20:46
@paynejacob paynejacob marked this pull request as ready for review October 14, 2021 23:30
thedadams
thedadams previously approved these changes Oct 20, 2021
store/proxy/proxy_store.go Outdated Show resolved Hide resolved
store/proxy/proxy_store.go Outdated Show resolved Hide resolved
store/proxy/proxy_store_test.go Outdated Show resolved Hide resolved
@paynejacob paynejacob dismissed stale reviews from thedadams via 924b3b0 October 22, 2021 18:33
@paynejacob paynejacob force-pushed the fix/list-perf branch 2 times, most recently from aaca61a to 4d90494 Compare October 22, 2021 18:40
@snasovich
Copy link
Contributor

@paynejacob , did we decide to drop _drop part? If that's the case, we want the description updated to reflect that. Let me know if I'm missing where this is implemented.

@dramich dramich removed their request for review October 22, 2021 20:50
@paynejacob
Copy link
Contributor Author

@snasovich yes we removed that functionality because it wasn't going to benefit the UI. ill update the description.

Copy link
Contributor

@snasovich snasovich left a comment

Choose a reason for hiding this comment

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

LGTM, kudos for adding a unit test for this

@paynejacob paynejacob changed the title Added typer to store [2.5.11] Added typer to store Oct 25, 2021
@paynejacob paynejacob requested a review from cbron October 26, 2021 17:12
@cbron cbron requested a review from ibuildthecloud October 28, 2021 19:47
Copy link
Contributor

@cbron cbron left a comment

Choose a reason for hiding this comment

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

lgtm, not a norman expert but the approach seems correct.

@cbron cbron merged commit 85ac7aa into rancher:master Oct 29, 2021
@snasovich
Copy link
Contributor

Note that this PR is actually for 2.6 release. The name is confusing due to how Norman was versioned and timeline of Rancher releases. The one for 2.5.x is this one: #405

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants