Improve benchmark performance by using lightweight fake clientsets#9841
Improve benchmark performance by using lightweight fake clientsets#9841Choraden wants to merge 3 commits into
Conversation
k8s-ci-robot
added
release-note-none
|
This issue is currently awaiting triage. If SIG Autoscaling contributors determines this is a relevant issue, they will accept it by applying the The DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
area/cluster-autoscaler
k8s-ci-robot
added
size/S
After profiling the benchmark execution, it was found that the majority of execution time was spent building the cluster state due to slow schema validation in the standard fake clientset. Introducing and using NewSimpleFakeSet with the lightweight NewSimpleClientset avoids this validation overhead and significantly speeds up the benchmark execution.
k8s-ci-robot
added
size/M
Increasing the benchmark run count from 6 to 10 in the GitHub workflow provides more samples for benchstat, resulting in more reliable and statistically stable performance comparison results. This adjustment is made possible by the recent benchmark duration improvements.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Choraden The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
The scale-up benchmark setup is increased from 200 to 1000 nodes, and the scale-down benchmark setup is increased from 400 to 2000 nodes. The criteria was stability of the results and similar execution time for both scenarios. This larger scale represents a more realistic workload size for cluster-autoscaler performance testing, made feasible by the recent optimizations that significantly speed up fake clientset operations.
|
@GaetanoMar96 Please take a look at integration utils changes. Thanks! |
|
|
||
| options := config.ResolveOptions() | ||
| infra := integration.SetupInfrastructure(t) | ||
| infra := integration.SetupSimpleInfrastructure(t) |
There was a problem hiding this comment.
Why are we switching to the simple clientset here? Also, I was wondering if we can get rid of this duplicated pattern of non-simple/simple clientsets and infrastructures in the main test utils.
My suggestion is to create a utils.go under the bench directory that includes these "simple" functions. This would make it easier for people creating benchmarks to know how to correctly write them, as right now it is not obvious to a user which functions to use in which case.
There was a problem hiding this comment.
We are switching it here, cause it does not work with non-simple clientset.
Notice that previously NewFakeSet returned non-simple clientset and simple provreq clientset. To standardize it I introduced a clear distinction. NewFakeSet returns non-simple clients. NewSimpleFakeSet returns simple clients where possible.
There was a problem hiding this comment.
This would make it easier for people creating benchmarks to know how to correctly write them, as right now it is not obvious to a user which functions to use in which case.
This is why benchmark has internal newClusterFakes() function that defaults clientsets.
| @@ -200,7 +200,7 @@ func (s scenario) runIteration(b *testing.B, i int, fProf, fTrace *os.File) { | |||
|
|
|||
| // newClusterFakes initializes a fake cluster with predefined resource limits. | |||
| func newClusterFakes() *integration.FakeSet { | |||
There was a problem hiding this comment.
Since the overall benchmark suite currently lacks a README and detailed documentation, and knowing that other people will be working on this in the future, it would be really helpful to enhance it now.
Adding a quick README.md to the cluster-autoscaler/core/bench/ directory explaining how to properly write and run these benchmarks would be highly valuable. This also ties into my previous point: moving the benchmark-specific "simple" setup functions into a dedicated utils.go inside the bench directory, alongside a README, would make the setup much more obvious and maintainable for future contributors.
There was a problem hiding this comment.
I think I agree that we could migrate all clientset utils closer to benchmark itself. It also depends on the future of test/integration utils.
Unfortunately, I don't have much time to work on this refactor now, so let's park the idea.
Regarding README.md it is being worked on in separate PR.
|
/retest |
|
@Choraden: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
After profiling the benchmark execution, it was found that the majority of execution time was spent building the cluster state due to slow schema validation in the standard fake clientset.
Introducing and using NewSimpleFakeSet with the lightweight NewSimpleClientset avoids this validation overhead and significantly speeds up the benchmark execution.
Huge thanks to @tetianakh for asking the right questions that lead to this improvement!
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: