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

Add benchmark test for parsing openapi in protobuf format #4396

Merged
merged 1 commit into from Jan 25, 2022

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Jan 19, 2022

The following is the benchmark results:

goos: darwin
goarch: amd64
pkg: sigs.k8s.io/kustomize/kyaml/openapi
cpu: Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
BenchmarkSwaggerUnmarshalJSON
BenchmarkSwaggerUnmarshalJSON-16      	       2	 609778913 ns/op
BenchmarkOpenAPIV2ParseDocument
BenchmarkOpenAPIV2ParseDocument-16    	       4	 260217958 ns/op
BenchmarkProtoUnmarshal
BenchmarkProtoUnmarshal-16            	      79	  15062865 ns/op

If we switch to github.com/googleapis/gnostic/openapiv2, but still parsing JSON, we can get 2.3X faster.
If we switch to github.com/googleapis/gnostic/openapiv2 and use proto completely, we can get 40.5X faster.

If switching to github.com/googleapis/gnostic/openapiv2, we will need to make some big change to kyaml/openapi.

Related issue: #3670

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 19, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mengqiy

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2022
@mengqiy
Copy link
Member Author

mengqiy commented Jan 19, 2022

/cc @droot @natasha41575 @KnVerey

@natasha41575
Copy link
Contributor

natasha41575 commented Jan 19, 2022

If we switch to github.com/googleapis/gnostic/openapiv2, but still parsing JSON, we can get 2.3X faster.
If we switch to github.com/googleapis/gnostic/openapiv2 and use proto completely, we can get 40.5X faster.

IMO we should start with still parsing JSON. Because users can provide their current openapi schema in JSON we will need to continue to support JSON parsing regardless. I can work on this.

Once that is working, we can take a look at using proto. While doing so we should make sure that users can still provide additional schema in JSON. We also should allow users to provide their input schema in proto form. We currently accept both JSON and yaml, so supporting a third type of format should be fine.

How did you retrieve the proto format of the openapi data? I'm thinking that if we start parsing proto schemas, we should support a kustomize openapi fetch --proto option, analogous to the kustomize openapi fetch --json and kustomize openapi fetch --yaml we currently have.

@natasha41575
Copy link
Contributor

Also, I did a check to see if introducing github.com/googleapis/gnostic will be a problem with kubectl integration. It looks like upstream kubernetes already has this dependency, so it should be fine.

@yuwenma
Copy link
Contributor

yuwenma commented Jan 21, 2022

This looks great! (Could you fix the presubmit?)

@mengqiy
Copy link
Member Author

mengqiy commented Jan 25, 2022

To retrieve the openapi in proto, see: https://kubernetes.io/docs/concepts/overview/kubernetes-api/#openapi-v2

For your convenience, I use the following:

To retrieve it from a GKE cluster using gcloud:
curl -k -H "Accept: application/com.github.proto-openapi.spec.v2@v1.0+protobuf" -H "Authorization: Bearer $(gcloud auth print-access-token)" https://<your-server-ip>/openapi/v2 > openapi.pb

To retrieve it using kubectl + curl:
Run kubectl proxy first, then run
curl -k -H "Accept: application/com.github.proto-openapi.spec.v2@v1.0+protobuf" http://localhost:8001/openapi/v2 > openapi.pb

I can't use kubectl get --raw to do achieve it, since it doesn't allow me to set the header.

@mengqiy
Copy link
Member Author

mengqiy commented Jan 25, 2022

We should support parsing both JSON and proto. Our baked-in openapi schema should be in proto, since it can be much faster. And users are more likely to provide JSON.
As we discussed earlier, we should allow user to provide a partial openapi schema that will be merged with the built-in schema.

@mengqiy
Copy link
Member Author

mengqiy commented Jan 25, 2022

I reverted the change in go.mod that upgrade the kube-openapi dependency. It should be passing this time.

@natasha41575
Copy link
Contributor

Related: #3248

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2022
@k8s-ci-robot k8s-ci-robot merged commit c754ead into kubernetes-sigs:master Jan 25, 2022
@mengqiy mengqiy deleted the oabench branch January 25, 2022 20:37
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.

None yet

4 participants