OpenAPIResourcesGetter allows lazy-loading OpenAPI V2 #116565

seans3 · 2023-03-14T03:31:37Z

Creates OpenAPIResourcesGetter interface, and uses it within the schemaValidation struct to lazy-load the OpenAPI V2 specifications for validation (using the Validator from the factory).
Depends on the lazy-load mechanism within the OpenAPISchema() implementation in factory_client_access.go.
Can reduce memory consumption, since there are only some validation cases in which the OpenAPI V2 is needed.
Helps effort to move from OpenAPI V2 -> V3.

/kind cleanup

NONE

seans3 · 2023-03-14T03:33:08Z

/priority important-soon

seans3 · 2023-03-14T03:33:19Z

/cc @apelisse

seans3 · 2023-03-14T04:28:09Z

/retest

staging/src/k8s.io/kubectl/pkg/validation/validation.go

dims · 2023-04-24T03:03:28Z

/assign @apelisse

dims · 2023-06-06T16:09:16Z

@seans3 Can anyone else review this?

apelisse · 2023-06-06T16:11:03Z

I'm happy reviewing this, and did, but my comment is unanswered. I think @seans3 has switched to working on websocket and put that on the backburner. I'll try to re-open it when I find a chance.

dims · 2023-06-15T17:20:27Z

thanks @apelisse

staging/src/k8s.io/kubectl/pkg/validation/validation.go

seans3 · 2023-10-17T22:55:48Z

staging/src/k8s.io/kubectl/pkg/cmd/util/factory_client_access.go

@@ -154,13 +154,8 @@ func (f *factoryImpl) Validator(validationDirective string) (validation.Schema,
 		return validation.NullSchema{}, nil
 	}

-	resources, err := f.OpenAPISchema()


Creation of the factory does not download the Open API V2 now; the Open API V2 resources are only downloaded when they are actually used.

apelisse · 2023-10-18T17:52:50Z

staging/src/k8s.io/kubectl/pkg/validation/validation_test.go

 		Expect(err).ToNot(HaveOccurred())
-		validator = NewSchemaValidation(resources)
+		validator = NewSchemaValidation(&fakeResourcesGetter{doc: doc})


That test does nothing though. Can we add a test that shows the fakeResourceGetter isn't called at all, maybe with a PanicingResourceGetter?

we would have to create a validator for that, that might work.

Jefftree · 2023-10-24T16:55:33Z

@seans3 I noticed some instances of OpenAPISchema() have not been updated to be loaded lazily, eg: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go#L108. Does this PR plan to address all the invocations as well?

I see three other instances of f.OpenAPISchema(), one in apply.go, one in diff.go, and one in explain.go.

seans3 · 2023-10-26T22:26:59Z

Sean Sullivan I noticed some instances of OpenAPISchema() have not been updated to be loaded lazily, eg: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go#L108. Does this PR plan to address all the invocations as well?

I see three other instances of f.OpenAPISchema(), one in apply.go, one in diff.go, and one in explain.go.

This PR does not affect those naked calls returning openapi resources. This PR will only address lazy-loading the openapi resources within the Validator.

seans3 · 2023-10-26T22:29:19Z

/cc @Jefftree

apelisse · 2023-10-26T22:44:27Z

Currently, the only way to run apply without downloading the OpenAPI v2 is to do:

kubectl apply -f some.yaml --validate=false --server-side=true

That is because the validation greedily wants the OpenAPI v2, and so does strategic-merge-patch. This change is specifically about the validation. Jeff, I think it's fine if you add the other part to your pr (#120707)

apelisse · 2023-10-26T23:12:39Z

staging/src/k8s.io/kubectl/pkg/validation/validation_test.go

-		resources, err := openapi.NewOpenAPIData(s)
-		Expect(err).ToNot(HaveOccurred())
-		validator = NewSchemaValidation(resources)
+		validator = NewSchemaValidation(&fakeResourcesGetter{})


Yeah, I think if the fakeResourceGetter would fail on OpenAPISchema(), then this test is probably just fine.

seans3 · 2023-10-26T23:56:11Z

/sig cli

seans3 · 2023-10-26T23:56:33Z

/triage accepted

Jefftree · 2023-10-27T01:11:59Z

staging/src/k8s.io/kubectl/pkg/validation/validation.go

+// OpenAPIResourcesGetter represents a function to return
+// OpenAPI V2 resource specifications. Used for lazy-loading
+// these resource specifications.
+type OpenAPIResourcesGetter interface {


Can we put this in a common package so smp can also use it?

This interface now lives in the k8s.io/kubectl/pkg/util/openapi package (same as Resources).

Jefftree · 2023-10-27T01:19:57Z

Currently, the only way to run apply without downloading the OpenAPI v2 is to do:
kubectl apply -f some.yaml --validate=false --server-side=true
That is because the validation greedily wants the OpenAPI v2, and so does strategic-merge-patch. This change is specifically about the validation. Jeff, I think it's fine if you add the other part to your pr (#120707)

Why not bundle the invocation changes together? If we're introducing the interface for lazy fetching it would make sense to have all invocations of it be updated? This affects not just apply, but diff and explain as well. The changes would be almost identical to how the validator was updated https://github.com/kubernetes/kubernetes/pull/116565/files#diff-7cdf779b345ced699424395b0bf817e34af2a9fe68f9f6cd2c6e021d1de1d33eR56-R66

staging/src/k8s.io/kubectl/pkg/util/openapi/openapi.go

apelisse · 2023-10-27T20:43:12Z

staging/src/k8s.io/kubectl/pkg/validation/validation.go

 	}
 }

+// retrieveResources gets (once) and caches the openapi.Resources, and
+// the error condition for the retrieval.
+func (v *schemaValidation) retrieveResources() (openapi.Resources, error) {


The code in the factory is not great, but I think this is actually already done in the factory, no?
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/util/openapi/openapi_getter.go#L56-L63

Yes. You're right. We don't need to cache the Resources, since they're cached already in the OpenAPIParser (which is used for the OpenAPISchema() call in the factory. We can remove this, and do a simple call on the OpenAPIResourcesGetter#OpenAPISchema().

...the entire lazy-load mechanism is dependent on the implementation of the OpenAPISchema() within factory_client_access.go. Resources are cached here in OpenAPIParser:

type CachedOpenAPIParser struct { openAPIClient discovery.OpenAPISchemaInterface // Cached results sync.Once openAPIResources Resources err error }

seans3 · 2023-10-27T21:06:57Z

/retest

seans3 · 2023-10-27T21:07:26Z

/retest

apelisse · 2023-10-30T16:01:17Z

/approve
/lgtm

k8s-ci-robot · 2023-10-30T16:01:26Z

LGTM label has been added.

Git tree hash: 1f610b9b74fe9f5d2784b26cfdbec8f7235f17b9

k8s-ci-robot · 2023-10-30T16:01:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, seans3

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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [apelisse,seans3]

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 requested review from ardaguclu and KnVerey March 14, 2023 03:32

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 14, 2023

k8s-ci-robot requested a review from apelisse March 14, 2023 03:33

apelisse reviewed Mar 16, 2023

View reviewed changes

staging/src/k8s.io/kubectl/pkg/validation/validation.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned apelisse Apr 24, 2023

Jefftree mentioned this pull request Sep 19, 2023

Use OpenAPI V3 for client side SMP #120707

Merged

apelisse reviewed Oct 13, 2023

View reviewed changes

staging/src/k8s.io/kubectl/pkg/validation/validation.go Outdated Show resolved Hide resolved

apelisse reviewed Oct 13, 2023

View reviewed changes

staging/src/k8s.io/kubectl/pkg/validation/validation.go Outdated Show resolved Hide resolved

seans3 commented Oct 17, 2023

View reviewed changes

apelisse reviewed Oct 18, 2023

View reviewed changes

seans3 force-pushed the lazy-load-openapi branch from 6e0d3bd to 983ca55 Compare October 26, 2023 22:27

k8s-ci-robot requested a review from Jefftree October 26, 2023 22:29

seans3 force-pushed the lazy-load-openapi branch from 983ca55 to fe3ab71 Compare October 26, 2023 22:53

apelisse reviewed Oct 26, 2023

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 26, 2023

Jefftree reviewed Oct 27, 2023

View reviewed changes

seans3 force-pushed the lazy-load-openapi branch from fe3ab71 to c1f3be2 Compare October 27, 2023 17:41

apelisse reviewed Oct 27, 2023

View reviewed changes

staging/src/k8s.io/kubectl/pkg/util/openapi/openapi.go Show resolved Hide resolved

apelisse reviewed Oct 27, 2023

View reviewed changes

OpenAPIResourcesGetter allows lazy-loading OpenAPI V2

6614a29

seans3 force-pushed the lazy-load-openapi branch from c1f3be2 to 6614a29 Compare October 27, 2023 21:40

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

k8s-ci-robot merged commit ef9587c into kubernetes:master Oct 30, 2023
14 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenAPIResourcesGetter allows lazy-loading OpenAPI V2 #116565

OpenAPIResourcesGetter allows lazy-loading OpenAPI V2 #116565

seans3 commented Mar 14, 2023 •

edited

seans3 commented Mar 14, 2023

seans3 commented Mar 14, 2023

seans3 commented Mar 14, 2023

dims commented Apr 24, 2023

dims commented Jun 6, 2023

apelisse commented Jun 6, 2023

dims commented Jun 15, 2023

seans3 Oct 17, 2023 •

edited

apelisse Oct 18, 2023

apelisse Oct 18, 2023

Jefftree commented Oct 24, 2023 •

edited

seans3 commented Oct 26, 2023

seans3 commented Oct 26, 2023

apelisse commented Oct 26, 2023 •

edited

apelisse Oct 26, 2023

seans3 commented Oct 26, 2023

seans3 commented Oct 26, 2023

Jefftree Oct 27, 2023

seans3 Oct 27, 2023

Jefftree commented Oct 27, 2023

apelisse Oct 27, 2023

seans3 Oct 27, 2023

seans3 Oct 27, 2023 •

edited

seans3 commented Oct 27, 2023

seans3 commented Oct 27, 2023

apelisse commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

OpenAPIResourcesGetter allows lazy-loading OpenAPI V2 #116565

OpenAPIResourcesGetter allows lazy-loading OpenAPI V2 #116565

Conversation

seans3 commented Mar 14, 2023 • edited

seans3 commented Mar 14, 2023

seans3 commented Mar 14, 2023

seans3 commented Mar 14, 2023

dims commented Apr 24, 2023

dims commented Jun 6, 2023

apelisse commented Jun 6, 2023

dims commented Jun 15, 2023

seans3 Oct 17, 2023 • edited

Choose a reason for hiding this comment

apelisse Oct 18, 2023

Choose a reason for hiding this comment

apelisse Oct 18, 2023

Choose a reason for hiding this comment

Jefftree commented Oct 24, 2023 • edited

seans3 commented Oct 26, 2023

seans3 commented Oct 26, 2023

apelisse commented Oct 26, 2023 • edited

apelisse Oct 26, 2023

Choose a reason for hiding this comment

seans3 commented Oct 26, 2023

seans3 commented Oct 26, 2023

Jefftree Oct 27, 2023

Choose a reason for hiding this comment

seans3 Oct 27, 2023

Choose a reason for hiding this comment

Jefftree commented Oct 27, 2023

apelisse Oct 27, 2023

Choose a reason for hiding this comment

seans3 Oct 27, 2023

Choose a reason for hiding this comment

seans3 Oct 27, 2023 • edited

Choose a reason for hiding this comment

seans3 commented Oct 27, 2023

seans3 commented Oct 27, 2023

apelisse commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

seans3 commented Mar 14, 2023 •

edited

seans3 Oct 17, 2023 •

edited

Jefftree commented Oct 24, 2023 •

edited

apelisse commented Oct 26, 2023 •

edited

seans3 Oct 27, 2023 •

edited