Lazy marshaling for OpenAPI v2 spec #251

DangerOnTheRanger · 2021-09-28T17:04:43Z

This PR makes the marshaling process lazy for OpenAPI v2.

DangerOnTheRanger · 2021-09-28T17:07:30Z

/cc @apelisse @Jefftree

apelisse

I sent a few suggestion that I think make the code simpler and less error-prone.

pkg/handler/handler.go

DangerOnTheRanger · 2021-09-28T23:37:59Z

Computing the ETag once in cache.Get makes sense - fixed, thanks!

apelisse · 2021-09-29T16:04:29Z

The code looks good. Can we see how this impacts Kubernetes though? How impact is the first request to /openapi/v2? Does this impact "start time"? Can we have a sort of analysis for that? I'd rather make sure that we know what we're causing to happen here. Thanks! cc @Jefftree

DangerOnTheRanger · 2021-10-11T22:48:07Z

From some testing with pprof and good old Bash time, I see memory savings of about 35% prior to a request to the OpenAPI endpoint. The first request is understandably slower with this PR (~0.6s on average on my machine, up from ~0.3s on average without this PR), but subsequent requests that hit a warm cache are just as fast. Startup time of the apiserver seems to be unaffected as well.

pkg/handler/handler.go

apelisse · 2021-10-13T17:35:18Z

From some testing with pprof and good old Bash time, I see memory savings of about 35% prior to a request to the OpenAPI endpoint. The first request is understandably slower with this PR (~0.6s on average on my machine, up from ~0.3s on average without this PR), but subsequent requests that hit a warm cache are just as fast. Startup time of the apiserver seems to be unaffected as well.

Thank you for that Kermit!

@sttts, do you have an opinion on this change? If we account for openapi v2 deprecation and openapi v3 split across group version, I think the caching will have a very positive impact on performance.

pkg/handler/handler.go

apelisse · 2021-10-14T22:57:38Z

Looks good to me, I'd like someone else to sign-off. Also hoping @sttts can approve :-)

Jefftree · 2021-10-20T18:13:30Z

maybe a nit to squash the commits, but otherwise LGTM as well.

apelisse · 2021-10-21T17:08:46Z

@DangerOnTheRanger Can you squash and I'll approve/lgtm after. And give @sttts a few days to give his opinion.

pkg/handler/handler.go

apelisse · 2021-10-26T17:19:30Z

Let's fix the json thing, rebase and merge so that we can integrate that in Jeff's PR, thanks!

pkg/handler/handler.go

Updates to consume the aggregated OpenAPI spec lazy-marshaling behaviour introduced with: kubernetes/kube-openapi#251 Signed-off-by: Alper Rifat Ulucinar <[email protected]> Kubernetes-commit: 38f888bdd14b8eddb86ec8ca8461267fe7f8ded1

* kubectl proxy: append context host path to request path Signed-off-by: fabiankramm <[email protected]> Kubernetes-commit: b1a6f8cdf90c0a3861157fea24dcfa89c2aafcf9 * fix AsApproximateFloat64() for BinarySI Kubernetes-commit: 3d1076ebf310820a2e6163a48f1485e1ab2d670b * klog 2.20.0, logr v1.1.0, zapr v1.1.0 This replaces the experimental logr v0.4 with the stable v1.1.0 release. This is a breaking API change for some users because: - Comparing logr.Logger against nil is not possible anymore: it's now a struct instead of an interface. Code which allows a nil logger should switch to *logr.Logger as type. - Logger implementations must be updated in lockstep. Instead of updating the forked zapr code in json.go, directly using the original go-logr/zapr is simpler and avoids duplication of effort. The updated zapr supports logging of numeric verbosity. Error messages don't have a verbosity (= always get logged), so "v" is not getting added to them anymore. Source code logging for panic messages got fixed so that it references the code with the invalid log call, not the json.go implementation. Finally, zapr includes additional information in its panic messages ("zap field", "ignored key", "invalid key"). Kubernetes-commit: cb6a65377775110631bc865acc06c3f957592813 * apimachinery: remove unused ignoredConversions map and nameFunc in converter. Kubernetes-commit: 7db782ee039a6740c3abd2352dfff1ea74e40209 * generated: Run hack/lint-dependencies.sh and hack/update-vendor.sh Signed-off-by: Stephen Augustus <[email protected]> Kubernetes-commit: 0be115722bf30f42c7a954d5cdd4b48efd70ae77 * generated: Run hack/update-gofmt.sh Signed-off-by: Stephen Augustus <[email protected]> Kubernetes-commit: 481cf6fbe753b9eb2a47ced179211206b0a99540 * run hack/update-netparse-cve.sh Kubernetes-commit: 0cd75e8fec62a2531637e80bb950ac9983cac1b0 * update vendor Kubernetes-commit: 2c73d7834acb5ddf380441b9ef3260db4168557d * Merge pull request #104368 from aojea/ruleguard golang 1.17 fails to parse IPs with leading zeros Kubernetes-commit: b0bc8adbc2178e15872f9ef040355c51c45d04bb * [go1.17] Bump golang.org/x/... dependencies hack/pin-dependency.sh golang.org/x/crypto master hack/pin-dependency.sh golang.org/x/net master hack/pin-dependency.sh golang.org/x/oauth2 master hack/pin-dependency.sh golang.org/x/sync master hack/pin-dependency.sh golang.org/x/sys master hack/pin-dependency.sh golang.org/x/term master hack/pin-dependency.sh golang.org/x/time master hack/pin-dependency.sh golang.org/x/tools master Signed-off-by: Stephen Augustus <[email protected]> Kubernetes-commit: 0e9881a9dc9d06aaf93723b4dfc7f4e1cb92e215 * Merge pull request #103692 from justaugustus/go117 [go1.17] Update to go1.17 Kubernetes-commit: c1e69551be1a72f0f8db6778f20658199d3a686d * Object creation with generateName should return a proper error Signed-off-by: Vince Prignano <[email protected]> Kubernetes-commit: 8a9d61278f6c2177309f58bf2655f2269e8f6afd * Fix typo coersion -> coercion Signed-off-by: Mateusz Gozdek <[email protected]> Kubernetes-commit: 53892932973a3c400550c7854423e7fd5f2f9067 * Additional resource quantity testing Fractional binary SI quantities that cannot be represented as decimal internally were incorrectly calculated. Kubernetes-commit: 2d7a9160a678685fed7376ede218f3dc6dff4958 * Update the valid string from rand.go Kubernetes-commit: 117fb6a45b2fda8fd7fd4a10c19d5244e924b771 * Merge pull request #104699 from vincepri/generate-name-error Object creation with generateName should return AlreadyExists instead of a Timeout Kubernetes-commit: 85b11ad24e996e2db4aa00a99e16f066544b22b0 * CloseIdleConnections for wrapped Transport It iterates over the wrapped transports until it finds one that implements the CloseIdleConnections method and executes it. add test for closeidle http1 connections add test for http1.1 reconnect with inflight request add test to reuse connection request add test for request connect after timeout add test for client-go request concurrency Kubernetes-commit: b9d865a8185b62d83e9ff81b0e3499a26ac6960d * Updated vendor files and pinned versions Kubernetes-commit: c5b4e05834d8edceac94ab1a91c3153581534393 * vendor sigs.k8s.io/json Kubernetes-commit: 434ce4336ab06b3c34208822d558c0432ada3ad3 * Migrate to k8s.io/utils/clock in apimachinery Kubernetes-commit: adf82f050c94f844a7f7c2b65c467a8a7f8e923b * Compact pretty-printed compatibility fixtures when decoding Kubernetes-commit: 74ca1b953a875d5458e04008f4f5bdc535838415 * Merge pull request #104949 from Karthik-K-N/json-iterator-version-update Updated json-iterator version to 1.1.12 from 1.1.11 Kubernetes-commit: 6a49ed41eab79d745c53723ce7f134222279545e * Migrate to k8s.io/utils/clock in client-go Kubernetes-commit: bb7dac443a2039f97c822f610e78d4b65482c56d * Merge pull request #105095 from wojtek-t/migrate_clock_3 Unify towards k8s.io/utils/clock - part 3 Kubernetes-commit: 353f0a5eabe4bd8d31bb67275ee4beeb4655be3f * Make package paths referenced by import boss valid Kubernetes-commit: f6b831aeaca2ff1481074e05c6771050f2b40516 * Merge pull request #105330 from liggitt/importboss-doc Make package paths referenced by import boss valid Kubernetes-commit: d551560a78292e1d4cac1de2ae684c803ddea183 * run hack/{pind-dependency.sh, update-vendor.sh} Signed-off-by: Madhav Jivrajani <[email protected]> Kubernetes-commit: a43fca76ea7ff6fb08153c9081f7858cd4d06dd8 * Merge pull request #105372 from MadhavJivrajani/vendor-clock-utils Vendor in k8s.io/utils Kubernetes-commit: eebeff9f7e0fccf1d220ce809eaea7f7f9248ce0 * Use stdlib json encoder for yaml and pretty-json marshaling Kubernetes-commit: a166f887f607767fe9dba06e00f9fc72ff856f51 * Fix bug where attempting to use patch with deleteFromPrimitiveList on an empty or nonexistent list incorrectly adds the item to be removed. Kubernetes-commit: 8a72a54d7c33313a1676e9ef600be50c06995101 * Redirect proxy requests for only GET & HEAD methods - Extract the current redirect code into a function (proxyRedirectsforRootPath) and redirect for only http.MethodGet or http.MethodHead - Create a unit test that confirms that only GET & HEAD methods are redirected Kubernetes-commit: be65bc3f8643ea7a61ec223776141fc8d9e9b39f * Merge pull request #105466 from liggitt/json-stdlib-pretty Use json stdlib for pretty-printer encoding Kubernetes-commit: 7cce7eec116f4487c6f6a73d7751322c84e64830 * resource: support using Quantity as command line value The Quantity type itself cannot be used because the Set method has the wrong signature. Embedding Quantity inside a new QuantityValue type makes it possible to inherit most of the methods while overriding the Set method. Kubernetes-commit: 963d3c122dcaaea61afa49230ef46765f94f8781 * Merge pull request #104873 from pohly/json-output-stream JSON output streams Kubernetes-commit: fb82a0d7eb252acacd9ef8b7cea63cf1cff535b3 * Add missing json tag on internal unstructured list Kubernetes-commit: fd64f8d7efea05db30e1a011c0dffc52c37101ed * ResettableRESTMapper to make it possible to reset wrapped mappers Kubernetes-commit: de4598d0db5e2babe89dd334407b2ba8024ec9a1 * Fix strict json decoder test Kubernetes-commit: b4632c38f06583aa9b5d3bf6f47e7c781c3b6e60 * Test json/yaml decoding type coercion Kubernetes-commit: ffb2d12633cdc9a908d965a54ab6157ab52d60e8 * Merge pull request #105702 from liggitt/json-strict-test JSON decoder fixup Kubernetes-commit: 3f40906dd8a54fb91650553a6457496181f591bc * Switch from json-iterator to utiljson Kubernetes-commit: bba877d3a6d0e6498d5e43a54939d5e4e8baee1a * Merge pull request #105030 from liggitt/json-stdlib switch from json-iterator to forked stdlib json decoder Kubernetes-commit: cc25656b00baa33168b7a9bc574101a06788efea * klog 2.30.0, logr 1.2.0, zapr 1.2.0 The new releases fix logging of KObj in JSON output: klog implements the new logr.Marshaler interface and zapr uses it instead of Stringer when logging the ObjectRef created by KObj. Kubernetes-commit: 169e8b65a00b45ef8bbc7a14cd985df1c835953b * Merge pull request #104877 from pohly/json-kobj component-base: test and fix JSON output for KObj Kubernetes-commit: a5cd438b9fbf49e013453f4d6c9b2e935a78071c * upgrade github.com/evanphx/json-patch to v4.12.0 Fix partial negative indice support in json patch Kubernetes-commit: 20cc72344e653ab90c1a851816bb206b715fd231 * Merge pull request #105896 from zqzten/upgrade-json-patch upgrade json-patch to v4.12.0 Kubernetes-commit: 18cb34ebb2b64a7607057c7dea80427e2af387f3 * apierrors: Avoid spurious <nil> in invalid error message Kubernetes-commit: 57fdd167e4ecdf2af8d297919d87b56b7a5adcad * apierrors: optimize ToAggregate() for zero-length lists Kubernetes-commit: 091724a6d86eb8ce86ffd4aaca4e8d4fb07785ef * Merge pull request #105959 from liggitt/podsecurity-details PodSecurity: return namespace validation errors in standard field.ErrorList format Kubernetes-commit: 1d9d530ee1b672acb9f2ba089123b5350d64dc3b * generated: ./hack/update-vendor.sh Kubernetes-commit: a4f6152743af5201fdbb48bda6730797d3c8f572 * Merge pull request #105983 from jiahuif-forks/dep/bump-smd Upgrade sigs.k8s.io/structured-merge-diff/v4 to v4.2.0 Kubernetes-commit: 8e2d7a3d64976eb23e1a4fdc8c068f5210014da6 * Bump k8s.io/kube-openapi to commit ee342a809c29 Updates to consume the aggregated OpenAPI spec lazy-marshaling behaviour introduced with: kubernetes/kube-openapi#251 Signed-off-by: Alper Rifat Ulucinar <[email protected]> Kubernetes-commit: 38f888bdd14b8eddb86ec8ca8461267fe7f8ded1 * Pin new dependency: github.com/google/cel-go v0.9.0 Kubernetes-commit: d73403dc12ad1d9576d65b5c65e30a87d17ad314 * Merge pull request #106234 from jpbetz/cel-libs Add wired off code for Validation rules for Custom Resource Definitions using the CEL expression language Kubernetes-commit: 6b41d75794381487ef7204b016faa75e350a32b7 * generated: ./hack/update-vendor.sh Kubernetes-commit: 73ffb492032896c1c87edfa1d85de5fc74bb526c * fix SA4005: ineffective assignment to field PatchMeta.patchStrategies (staticcheck) Kubernetes-commit: fa3c4b953fb8192c60dcc3874bff8f9c12ffd54d * fix ineffectual assignment to base var Kubernetes-commit: 31d45d000b83f982476d175a4e8a13d97cf95def * nolint unused expectNoMatchDirect function Kubernetes-commit: c4080c2ad1e0af0dbb8fc6871f3310b3c18a7024 * nolint float64(-0.0), //nolint:staticcheck // SA4026: Kubernetes-commit: 35c05a3afa6fc9fee3ab202329ce988faf1dc651 * remove ineffectual assignment base var Kubernetes-commit: 98884f733a019ab991da29aaba3e42d89bf202ec * migrate nolint coments to golangci-lint Kubernetes-commit: d126b1483840b5ea7c0891d3e7a693bd50fae7f8 * Merge pull request #106448 from aojea/hlee/issue-103721/staticcheck use golangci-lint Kubernetes-commit: 1367cca8fd67b09606b01c0a9e46cef59aef3424 * Server Side Field Validation Implements server side field validation behind the `ServerSideFieldValidation` feature gate. With the feature enabled, any create/update/patch request with the `fieldValidation` query param set to "Strict" will error if the object in the request body have unknown fields. A value of "Warn" (also the default when the feautre is enabled) will succeed the request with a warning. When the feature is disabled (or the query param has a value of "Ignore"), the request will succeed as it previously had with no indications of any unknown or duplicate fields. Kubernetes-commit: e50e2bbc889eb274ad1463a54188a2805767bfde * Revert sigs.k8s.io/structured-merge-diff/v4 to v4.1.2 Kubernetes-commit: d148bbcee39e3c290f9d5663e848a398d402152d * Merge pull request #106661 from liggitt/automated-cherry-pick-of-#106660-upstream-release-1.23 Automated cherry pick of #106660: Revert sigs.k8s.io/structured-merge-diff/v4 to v4.1.2 Kubernetes-commit: 724289524084f6edbbe53e31d2c6e636343fdebb Co-authored-by: fabiankramm <[email protected]> Co-authored-by: TAGAMI Yukihiro <[email protected]> Co-authored-by: Patrick Ohly <[email protected]> Co-authored-by: j2gg0s <[email protected]> Co-authored-by: Stephen Augustus <[email protected]> Co-authored-by: Antonio Ojea <[email protected]> Co-authored-by: Kubernetes Publisher <[email protected]> Co-authored-by: Vince Prignano <[email protected]> Co-authored-by: Mateusz Gozdek <[email protected]> Co-authored-by: Clayton Coleman <[email protected]> Co-authored-by: Manjunath A Kumatagi <[email protected]> Co-authored-by: Karthik K N <[email protected]> Co-authored-by: Jordan Liggitt <[email protected]> Co-authored-by: wojtekt <[email protected]> Co-authored-by: Madhav Jivrajani <[email protected]> Co-authored-by: brianpursley <[email protected]> Co-authored-by: Stephen Heywood <[email protected]> Co-authored-by: Mikhail Mazurskiy <[email protected]> Co-authored-by: Zach Zhu <[email protected]> Co-authored-by: Jiahui Feng <[email protected]> Co-authored-by: Alper Rifat Ulucinar <[email protected]> Co-authored-by: Joe Betz <[email protected]> Co-authored-by: Kevin Delgado <[email protected]>

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 28, 2021

k8s-ci-robot requested review from philips and roycaihw September 28, 2021 17:05

k8s-ci-robot requested review from apelisse and Jefftree September 28, 2021 17:07

apelisse reviewed Sep 28, 2021

View reviewed changes

pkg/handler/handler.go Outdated Show resolved Hide resolved

pkg/handler/handler.go Outdated Show resolved Hide resolved

pkg/handler/handler.go Outdated Show resolved Hide resolved

pkg/handler/handler.go Outdated Show resolved Hide resolved

Jefftree reviewed Oct 13, 2021

View reviewed changes