kyaml/openapi: replace the embedded Kubernetes schema with precomputed patch metadata#6194
Closed
dims wants to merge 3 commits into
Closed
kyaml/openapi: replace the embedded Kubernetes schema with precomputed patch metadata#6194dims wants to merge 3 commits into
dims wants to merge 3 commits into
Conversation
The kyaml merge walker only reads two OpenAPI extensions from the embedded Kubernetes schema: x-kubernetes-patch-strategy and x-kubernetes-patch-merge-key. Parsing the whole 3.4 MB swagger document to answer those lookups costs tens of milliseconds per process, and embedding it costs about 340 KiB in every binary that links kyaml, including kubectl and kubeadm. This change precomputes that metadata into a generated table, the same approach precomputedIsNamespaceScoped already uses for namespace scoping. The table holds the 47 fields that carry a patch strategy in the builtin schema, plus the definition spine needed to reach them: about 63 KB of generated Go instead of a 4.6 MiB embedded document. A new function PatchMetaSchemaForResourceType serves builtin types from the table when the global schema is in its default state, and falls back to the full schema for unknown types or any customized state. The merge walker now uses it. SchemaForResourceType keeps its full contract, so callers that need descriptions or validation data see no change, and the openapi field in kustomization.yaml keeps working. A generative equivalence test walks every builtin resource root in lockstep against the parsed schema and asserts the table reports the same strategy and merge keys at every path, and that pruned subtrees contain no patch metadata. This includes reproducing an existing quirk: the schema lookup drops patch extensions on fields that resolve through a $ref, so the table does too. With this change a default kustomize build never parses the embedded swagger. The document itself is unchanged and still used by SchemaForResourceType; slimming or replacing it becomes a follow-up once the remaining full-schema consumers are accounted for (issue kubernetes-sigs#5016 tracks the broader schema question). Regenerate with: go run ./openapi/internal/patchmetagen openapi/kubernetesapi/v1_21_2/swagger.pb Signed-off-by: Davanum Srinivas <davanum@gmail.com>
The strategic-merge patch metadata for builtin types now comes entirely from the precomputed table, so the 4.6 MiB embedded schema document (kubernetesapi/v1_21_2, go-bindata plumbing, and the kind-based fetch scripts) can go. The table is generated from the published OpenAPI document of an exact kubernetes/kubernetes tag: the Makefile downloads api/openapi-spec/swagger.json for the pinned API_VERSION from raw.githubusercontent.com and regenerates zz_generated_patchmeta.go from it. No schema data is fetched from a live cluster anymore, and no schema document is embedded or committed. Generating from the published tag also fixes a fidelity gap: the old embedded document was fetched from a kind cluster with alpha APIs disabled, so every v1alpha1 type was missing from it. The table now covers them. What still works unchanged: - strategic-merge patches for builtin types (the precomputed table; equivalence to the source document is verified by make verify-patchmeta, which downloads the pinned tag and runs TestPrecomputedPatchMetaEquivalence) - the openapi field in kustomization.yaml with a path to a custom schema document, and AddSchema; supplied definitions override the table per type - the Kustomization schema (kustomizationapi), which stays embedded - namespace-scope checks (precomputedIsNamespaceScoped) What changes: - SchemaForResourceType returns nil for builtin types unless a schema document has been supplied; PatchMetaSchemaForResourceType serves merge metadata - the openapi version field accepts only the pinned default version; other versions error with a pointer to the path field - $ref field comments referencing builtin definitions, and schema-driven formatting of builtin types, need a supplied schema document; tests exercising those mechanisms now supply small schema fixtures Regenerate with: make zz_generated_patchmeta.go # in kyaml/openapi Verify with: make verify-patchmeta Signed-off-by: Davanum Srinivas <davanum@gmail.com>
Contributor
|
This PR has multiple commits, and the default merge method is: merge. 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. |
kubernetes-prow
Bot
added
the
kind/cleanup
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dims 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 |
kubernetes-prow
Bot
added
cncf-cla: yes
The kustomize openapi info command and one api test imported the deleted kubernetesapi package, which broke compilation of the api and kustomize modules and, through them, the Lint and module check jobs. - kustomize openapi info prints the same output format as before, built from openapi.GetSchemaVersion(). - The api test asserts the default version string directly, matching the other version assertions in the same file. - The openshift custom-schema test fixture referenced builtin definitions (PodTemplateSpec, ConfigMapVolumeSource) that it never defined. It only worked before because the old delayed-parse machinery merged the embedded builtin document underneath custom schemas, and only when a builtin lookup happened to run first. With no embedded document, custom schema documents must be self-contained; the fixture now defines everything it references. Signed-off-by: Davanum Srinivas <davanum@gmail.com>
Member
|
/assign @koba1t |
Member
Member
Author
|
@koba1t i was running an analysis and this came up again: hence my attempt in this PR. I am ok with whichever approach :) Happy to close this out, just let me know. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This removes the embedded Kubernetes OpenAPI schema document (the 4.6 MiB
kyaml/openapi/kubernetesapi/v1_21_2tree) and replaces the one thing the merge code actually reads from it with a small precomputed table.The kyaml merge walker reads exactly two extensions from the schema:
x-kubernetes-patch-strategyandx-kubernetes-patch-merge-key. In the whole 3.4 MB document, only 49 fields carry a patch strategy. The first commit generates a table of those fields plus the definition structure needed to reach them (about 69 KB of Go), addsPatchMetaSchemaForResourceTypeto serve it, and switches the merge walker to it. The second commit deletes the embedded document, the go-bindata plumbing, and the kind based fetch scripts, and changes the Makefile to downloadapi/openapi-spec/swagger.jsonfor an exact kubernetes/kubernetes tag (API_VERSION) and regenerate the table from that.Numbers:
kubectlandkubeadmeach carry about 340 KiB of linked schema bytes from this document today. The table is about 69 KB of source.kustomize buildno longer parses the document at all, which removes the parse cost that openapi parsing performance improvement with protobuffer #4568 reduced from 600ms to 39ms.Fidelity notes, both verified by a generative test:
make verify-patchmetadownloads the pinned tag's document and walks every resource root in lockstep against it, asserting identical strategies and merge keys at every path, and that every pruned subtree carries no patch metadata.v1alpha1type was missing from it. The published tag document includes them, so the table now covers types the embedded document never did.Relationship to existing work: #5016 tracks the schema version question and #6190 repackages the embedded document with a bundle compiler. This PR is a different point in that design space: it removes the document and keeps only the merge metadata, generated from an exact tag. Because the table costs kilobytes per version instead of megabytes, the union across releases discussed in #5016 becomes cheap: the generator already accepts multiple documents. Happy to rework this to sit on top of #6190 if that ordering is preferred.
Which issue(s) this PR fixes:
Related to #5016
Behavior changes to review carefully:
SchemaForResourceTypereturns nil for builtin types unless a schema document has been supplied. Callers that need descriptions or validation data must supply a document; merge consumers usePatchMetaSchemaForResourceType.openapifield'sversionselector accepts only the pinned default version and errors otherwise, with a message pointing at thepathfield. Thepathfield andAddSchemaare unchanged, and supplied definitions override the table for the types they define.$reffield comments that reference builtin definitions, and schema driven formatting of builtin types, need a supplied schema document now. The tests exercising those mechanisms supply small schema fixtures, so the mechanisms themselves remain covered.$refbuiltin definitions (for exampleio.k8s.api.core.v1.PodTemplateSpec) and the embedded document was merged underneath it, but only when a builtin lookup happened to run first, so the old behavior was order dependent. The openshift test fixture relied on this and now defines everything it references.$ref, soretainKeysonDeployment.spec.strategyhas never taken effect. The table reproduces this so merge output is unchanged. The equivalence test documents it.Special notes for your reviewer:
make zz_generated_patchmeta.goinkyaml/openapi; verify withmake verify-patchmeta.Does this PR introduce a user-facing change?