-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
openapi parsing performance improvement with protobuffer #4568
openapi parsing performance improvement with protobuffer #4568
Conversation
Skipping CI for Draft Pull Request. |
edfe675
to
9de2908
Compare
1f5db6d
to
7b84dcf
Compare
f679c5c
to
bf635a8
Compare
@natasha41575: This PR has multiple commits, and the default merge method is: merge. Instructions 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/test-infra repository. |
7b37903
to
be3d39f
Compare
kyaml/openapi/openapi.go
Outdated
type format string | ||
|
||
const ( | ||
JSON format = "json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the implementation below, it seems we can handle json or yaml when the format is json
.
Maybe we can call it JsonOrYaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Changed to JsonOrYaml
"k8s.io/kube-openapi/pkg/validation/spec" | ||
"sigs.k8s.io/kustomize/kyaml/openapi/kubernetesapi" | ||
"sigs.k8s.io/kustomize/kyaml/openapi/kubernetesapi/v1218pb" | ||
v1212 "sigs.k8s.io/kustomize/kyaml/openapi/kubernetesapi/v1212" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should use a newer version of the schema. Maybe v1.22?
It can be done in a followup PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yeah we were thinking about bumping the schema since we are pretty far behind. I filed #4583, let's discuss in a followup.
6979861
to
fc3c59a
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mengqiy, natasha41575 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 |
One question, but not a blocker. /lgtm |
|
||
const ( | ||
JsonOrYaml format = "jsonOrYaml" | ||
Proto format = "proto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these constants public but their type private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see a purpose in making the type public, but I can see a potential use case in the future of passing in the format from another package once we start supporting user-provided proto schemas, hence why I made the const public.
I'm happy to change them to both being public or both being private if that makes more sense, I don't have a strong preference either way.
/hold cancel |
Thanks everyone for the quick reviews! |
Thanks so much for this fix. It was very nice to see these results this morning... Test_kustomize/kust-issue-2767/kustomize/branch-master (0.15s) Test_kustomize/kust-issue-2808/kustomize/branch-master (0.13s) Test_kustomize/kust-issue-2907/kustomize/branch-master (0.14s) Test_kustomize/kust-issue-2987/kustomize/branch-master (0.13s) Test_kustomize/kust-issue-3157/kustomize/branch-master (0.14s) Test_kustomize/kust-issue-3419/kustomize/branch-master (0.14s) |
Thanks @natasha41575! |
Fixes #2987
Fixes #4100
Fixes #3670
Related: #4569
The performance improvement changes the format we use to parse our builtin openapi from JSON to proto. See kubernetes/kube-openapi#283, it reduces the parsing time from
~628ms
to~39ms
.You can see the performance improvement when running the example provided on this issue:
Previously:
With this branch:
You can also see about a half second improvement in performance when running certain tests, e.g.
On master:
On this branch:
There is a separate PR to update the openapi scripts here: #4582