perf: limit initSchema calls from openapi.IsNamespaceScoped #5076

ephesused · 2023-03-06T17:19:02Z

Avoid calling initSchema from openapi.IsNamespaceScoped when possible. Work done in #4152 introduced a precomputed namespace scope map based on the default built-in schema. This commit extends that work by avoiding calls to initSchema when a resource is not found in the precomputed map and the default built-in schema is in use. In those cases, there is no benefit to calling initSchema since the precomputed map is exactly what will be calculated by parsing the default built-in schema.

This PR resolves some of the concern in #4569, but the PR does not fix the issue completely. Specifically, this PR does not address this earlier comment from @KnVerey:

We should add a --proto option to kustomize openapi fetch and also allow proto-formatted schemas in the openapi field.

The PR is split into two commits, one for a new benchmark and one for the performance improvement. Using the benchmark for analysis...

Before:

$ go test ./kyaml/openapi -bench BenchmarkPrecomputed -benchmem -run '^$'
goos: windows
goarch: amd64
pkg: sigs.k8s.io/kustomize/kyaml/openapi
cpu: Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz
BenchmarkPrecomputedIsNamespaceScoped/unknown_resource-8                      22          52757164 ns/op        46676575 B/op     339637 allocs/op
BenchmarkPrecomputedIsNamespaceScoped/namespace_scoped-8                22427852                52.83 ns/op            0 B/op          0 allocs/op
BenchmarkPrecomputedIsNamespaceScoped/cluster_scoped-8                  21984786                54.13 ns/op            0 B/op          0 allocs/op
PASS
ok      sigs.k8s.io/kustomize/kyaml/openapi     5.260s

After:

$ go test ./kyaml/openapi -bench BenchmarkPrecomputed -benchmem -run '^$'
goos: windows
goarch: amd64
pkg: sigs.k8s.io/kustomize/kyaml/openapi
cpu: Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz
BenchmarkPrecomputedIsNamespaceScoped/cluster_scoped-8          18558757                53.92 ns/op            0 B/op          0 allocs/op
BenchmarkPrecomputedIsNamespaceScoped/unknown_resource-8        15135080                76.36 ns/op            0 B/op          0 allocs/op
BenchmarkPrecomputedIsNamespaceScoped/namespace_scoped-8        22635700                52.51 ns/op            0 B/op          0 allocs/op
PASS
ok      sigs.k8s.io/kustomize/kyaml/openapi     4.102s

Add a benchmark test for IsNamespaceScoped performance when the default schema is in use.

Avoid calling initSchema from openapi.IsNamespaceScoped when possible. Work done in kubernetes-sigs#4152 introduced a precomputed namespace scope map based on the default built-in schema. This commit extends that work by avoiding calls to initSchema when a resource is not found in the precomputed map and the default built-in schema is in use. In those cases, there is no benefit to calling initSchema since the precomputed map is exactly what will be calculated by parsing the default built-in schema.

k8s-ci-robot · 2023-03-06T17:19:04Z

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

k8s-ci-robot · 2023-03-06T17:19:12Z

Hi @ephesused. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

natasha41575

LGTM. Just one nit

natasha41575 · 2023-03-10T17:15:51Z

kyaml/openapi/openapi_benchmark_test.go

@@ -5,6 +5,7 @@ package openapi

 import (
 	"path/filepath"
+	"sigs.k8s.io/kustomize/kyaml/yaml"


nit: move this import down with the second group of imports

Done - thanks for the catch.

kyaml/openapi/openapi_benchmark_test.go

natasha41575 · 2023-03-10T17:17:45Z

/ok-to-test

ephesused · 2023-03-14T16:54:04Z

It turns out there's more complexity in the processing, and I want to take a little time to think about this one before coding further.

Thanks to the test coverage - particularly api/krusty's TestCustomOpenApiFieldFromComponentWithOverlays - it's clear this optimization introduces a bug. What's not clear is how to retain the current behavior and provide the optimization.

At first, it would seem simple - just delay the initSchema loads until the point where the schema truly is needed. The problem is that by the time the schema truly is needed, the earlier state has been lost. In effect, the question centers around what the initSchema calls would have done for earlier calls to NewGvk when the initSchema invocations were skipped. That's certainly possible, but it's more complex than I had thought it would be.

Explanation...

For the code at master, the TestCustomOpenApiFieldFromComponentWithOverlays flow does something like so:

(api/krusty/kustomizer.go) Kustomizer.Run calls openapi.SetSchema
(api/internal/target/kusttarget.go) KustTarget.accumulateDirectory calls openapi.SetSchema

Later, during accumulation:

(api/resmap.factory.go) Factory.FromFile chains down to...
(kyaml/resid/gvk.go) NewGvk calls...
(kyaml/openapi/openapi.go) IsCertainlyClusterScoped calls...
(kyaml/openapi/openapi.go) initSchema - at this point that's against the default

Later, still during accumulation:

(api/internal/target/kusttarget.go) KustTarget.accumulateDirectory calls openapi.SetSchema based on the defined openapi path

And a little later:

(kyaml/resid/gvk.go) NewGvk leads to...
(kyaml/openapi/openapi.go) initSchema - at this point that's against the defined openapi path

The result is that the stored data now has both the default and the defined openapi path.

The change I proposed eliminates the initSchema call from certain NewGvk calls. As a result, the default schema might not load. That absence leads to the testcase failure. Currently, NewGvk has a side effect of loading the default schema when the Gvk is not known. It turns out that the default schema load is a requirement for certain cases - and TestCustomOpenApiFieldFromComponentWithOverlays is one of them.

I believe the reason TestCustomOpenApiFieldFromComponentWithOverlays is affected is that com.github.openshift.api.apps.v1.DeploymentConfigSpec is defined via a reference to io.k8s.api.core.v1.PodTemplateSpec. In the absence of the default schema load, the reference is not present. In turn, that affects the results of the patch.

When namespace scope can be determined by the precomputed map but the type is not present in the precomputed map, delay the parsing of the default built-in schema. If the schema to be initialized is the default built-in schema and the type is not in the precomputed map, then the type will not be found in the default built-in schema. There is no need to parse the default built-in schema for that answer; its parsing may be delayed until it is needed for some other purpose. In cases where the schema is used solely for namespace scope checks, the schema might not ever be parsed. Skipping the parsing reduces both execution time and memory use.

ephesused · 2023-05-02T15:04:29Z

For my previous attempt, the only schema whose parsing was affected was the default built-in schema. That means I don't have to address a general solution - I can craft a simple one targeted at the one affected schema. That's addressed in the latest commit.

irvifa · 2023-08-29T05:19:35Z

/lgtm
/assign @natasha41575

natasha41575 · 2023-08-30T15:45:22Z

@ephesused I'm not sure why the tests aren't triggering (and I can't manually trigger them) - could you try pushing a dummy commit and see if it runs the tests?

ephesused · 2023-08-30T17:08:59Z

@ephesused I'm not sure why the tests aren't triggering (and I can't manually trigger them) - could you try pushing a dummy commit and see if it runs the tests?

I simply merged to make this PR current. I see the workflows waiting to run, pending approval from a maintainer. Thanks!

k8s-ci-robot · 2023-09-01T15:51:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ephesused, 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:

~~OWNERS~~ [natasha41575]

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

natasha41575 · 2023-09-01T16:07:21Z

Looks like a test failure with TestCustomOpenApiFieldFromComponentWithOverlays.

Happy to re-approve once that is fixed.

ephesused · 2023-09-01T16:09:25Z

Looks like a test failure with TestCustomOpenApiFieldFromComponentWithOverlays.

Thanks - I'll try to take a look at that later today.

openapiData initializes with defaultBuiltInSchemaParseStatus set to 0, so schemaNotParsed should have 0 as its value.

ephesused · 2023-09-01T22:16:56Z

That was a bit embarrassing - I don't know how I managed to mess that up four months ago.

@natasha41575, I believe the last commit should resolve the issue. I'm never able to run all the tests cleanly in my environment, but the ones I checked look good.

For example:

$ git log --pretty=oneline | head -1
7e104198440938beca50a1fa17b8ccad5479fd96 fix: correct openapi.go's schemaNotParsed value
$ go test ./api/krusty -run TestCustomOpenApi -count 1
ok      sigs.k8s.io/kustomize/api/krusty        0.709s
$ go test ./kyaml/openapi/... -count 1
?       sigs.k8s.io/kustomize/kyaml/openapi/kubernetesapi       [no test files]
?       sigs.k8s.io/kustomize/kyaml/openapi/kubernetesapi/v1_21_2       [no test files]
?       sigs.k8s.io/kustomize/kyaml/openapi/kustomizationapi    [no test files]
ok      sigs.k8s.io/kustomize/kyaml/openapi     0.241s
$

natasha41575 · 2023-09-08T19:11:14Z

/lgtm

ephesused added 2 commits March 6, 2023 10:23

test: add openapi.IsNamespaceScoped benchmark

9377afe

Add a benchmark test for IsNamespaceScoped performance when the default schema is in use.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 6, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 6, 2023

k8s-ci-robot requested review from mortent and phanimarupaka March 6, 2023 17:19

natasha41575 reviewed Mar 10, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 10, 2023

natasha41575 added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Mar 22, 2023

ephesused mentioned this pull request Apr 11, 2023

Openapi parsing performance improvements #4569

Open

ephesused added 2 commits May 2, 2023 10:22

Merge remote-tracking branch 'origin/master' into issue4569

3c758eb

k8s-ci-robot assigned natasha41575 and irvifa Aug 29, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2023

Merge branch 'master' into issue4569

c1d5c53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2023

natasha41575 approved these changes Sep 1, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2023

fix: correct openapi.go's schemaNotParsed value

7e10419

openapiData initializes with defaultBuiltInSchemaParseStatus set to 0, so schemaNotParsed should have 0 as its value.

natasha41575 added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 8, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2023

k8s-ci-robot merged commit 985835f into kubernetes-sigs:master Sep 8, 2023
9 checks passed

shapirus mentioned this pull request Oct 14, 2023

Performance degradation starting with v3.8.6 #4100

Closed

shapirus mentioned this pull request Oct 27, 2023

Researching performance degradation (and improvement) over a number of releases #5422

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: limit initSchema calls from openapi.IsNamespaceScoped #5076

perf: limit initSchema calls from openapi.IsNamespaceScoped #5076

ephesused commented Mar 6, 2023

k8s-ci-robot commented Mar 6, 2023

k8s-ci-robot commented Mar 6, 2023

natasha41575 left a comment

natasha41575 Mar 10, 2023

ephesused May 2, 2023

natasha41575 commented Mar 10, 2023

ephesused commented Mar 14, 2023

ephesused commented May 2, 2023

irvifa commented Aug 29, 2023

natasha41575 commented Aug 30, 2023

ephesused commented Aug 30, 2023

k8s-ci-robot commented Sep 1, 2023

natasha41575 commented Sep 1, 2023

ephesused commented Sep 1, 2023

ephesused commented Sep 1, 2023

natasha41575 commented Sep 8, 2023

perf: limit initSchema calls from openapi.IsNamespaceScoped #5076

perf: limit initSchema calls from openapi.IsNamespaceScoped #5076

Conversation

ephesused commented Mar 6, 2023

k8s-ci-robot commented Mar 6, 2023

k8s-ci-robot commented Mar 6, 2023

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 Mar 10, 2023

Choose a reason for hiding this comment

ephesused May 2, 2023

Choose a reason for hiding this comment

natasha41575 commented Mar 10, 2023

ephesused commented Mar 14, 2023

ephesused commented May 2, 2023

irvifa commented Aug 29, 2023

natasha41575 commented Aug 30, 2023

ephesused commented Aug 30, 2023

k8s-ci-robot commented Sep 1, 2023

natasha41575 commented Sep 1, 2023

ephesused commented Sep 1, 2023

ephesused commented Sep 1, 2023

natasha41575 commented Sep 8, 2023