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

main_test.go: Introduce overarching benchmark test #513

Merged
merged 1 commit into from
Aug 23, 2018

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Aug 15, 2018

What this PR does / why we need it:

This patch adds a simple go benchmark test, injecting Kubernetes objects
and simulating scrape requests. It uses the Kubernetes client-go fake
client. Alongside comes some refactoring of each collectors structure
using informer factories to be compatible with the fake client.

The patch lays the groundwork to make future performance optimizations
comparable with past versions.

How to run test:
go test -race -bench . -memprofile=mem.out -cpuprofile=cpu.out

Relates to #498

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 15, 2018
@k8s-ci-robot k8s-ci-robot requested review from brancz and zouyee August 15, 2018 12:11
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 15, 2018
main_test.go Outdated
func injectFixtures(client *fake.Clientset) error {
creators := []func(*fake.Clientset, int) error{
configMap,
pod,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to add more fixtures here, but would prefer general feedback first to prevent lost work.

@brancz
Copy link
Member

brancz commented Aug 15, 2018

This looks fantastic, and although the Pod and ConfigMap objects used for the test are not particularly representative of real world data they produce pretty much exactly the same profiles as production data does.

Once the release phase is over I'm happy with merging this.

/lgtm

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 15, 2018
@brancz
Copy link
Member

brancz commented Aug 15, 2018

/hold

main_test.go Outdated
}

for _, c := range creators {
for i := 0; i < 1000; i++ {
Copy link
Member

Choose a reason for hiding this comment

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

It is a meaningful job, may it be better to make the number of tests configurable?

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 introduced multiplier to injectFixtures. Let me know what you think. This test will be further adjusted along side the future performance adjustments. This patch only lays down the skeleton to ensure we can backport the same test case to older versions to compare performance improvements.

This patch adds a simple go benchmark test, injecting Kubernetes objects
and simulating scrape requests. It uses the Kubernetes client-go fake
client. Alongside comes some refactoring of each collectors structure
using informer factories to be compatible with the fake client.

The patch lays the groundwork to make future performance optimizations
comparable with past versions.

How to run test:
`go test -race -bench  . -memprofile=mem.out -cpuprofile=cpu.out`
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2018
@andyxning
Copy link
Member

andyxning commented Aug 20, 2018

Maybe we need to add a e2e_perf_test Makefile target to add more detailed e2e performance test.

@mxinden
Copy link
Contributor Author

mxinden commented Aug 21, 2018

Maybe we need to add a e2e_perf_test Makefile target to add more detailed e2e performance test.

@andyxning would you mind elaborating further here? For me this is not an end-to-end test. It does not actually put kube-state-metrics in a production-like environment.

I think in the long run we should have go test -race -bench . -memprofile=mem.out -cpuprofile=cpu.out behind a benchmark make target. Does that sounds reasonable?

@brancz
Copy link
Member

brancz commented Aug 23, 2018

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2018
@brancz
Copy link
Member

brancz commented Aug 23, 2018

The v1.4.0 release is out and therefore feature freeze has been lifted. Let's move forward with this.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, mxinden

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5799520 into kubernetes:master Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants