Skip to content
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

Prune internal clients from CRD apiserver #84005

Merged

Conversation

@yue9944882
Copy link
Member

yue9944882 commented Oct 16, 2019

sorted commits:

  1. rm -rf staging/src/k8s.io/apiextensions-apiserver/pkg/client
  2. switch to v1 crd in crd registration controller
  3. switch api helper functions to v1 CRD api (this is necessary because we have to switch the old helpers from internal types to v1)
  4. switch v1 CRD for apiserver internal
  5. switch to v1 CRD for internal controllers
  6. santize codegen scripts
  7. api storage/validation changes
  8. - generated nonsense -
  9. - generated nonsense -

note that this PR also involves non-trival changes in non-structural-violation controller. the old controller's condition is calculated based on internal types but now it's v1 type, the difference is that v1 doesn't have .spec.validation path.

NONE
@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Oct 17, 2019

/assign @sttts

@yue9944882 yue9944882 force-pushed the yue9944882:chore/crd-internal-client-prune branch 3 times, most recently from 2f03661 to b290a8b Oct 30, 2019
@yue9944882 yue9944882 force-pushed the yue9944882:chore/crd-internal-client-prune branch 2 times, most recently from f3b042c to 8f4758a Nov 8, 2019
@yue9944882 yue9944882 changed the title [WIP] Prune internal clients from CRD apiserver Prune internal clients from CRD apiserver Nov 8, 2019
@yue9944882 yue9944882 force-pushed the yue9944882:chore/crd-internal-client-prune branch from 0008776 to b42ed0b Nov 29, 2019
var swaggerMetadataDescriptions = metav1.ObjectMeta{}.SwaggerDoc()

// SetCRDCondition sets the status condition. It either overwrites the existing one or creates a new one.
func SetCRDCondition(crd *apiextensions.CustomResourceDefinition, newCondition apiextensions.CustomResourceDefinitionCondition) {

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

aren't the equivalent internal helpers deleted or at least pruned?

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Nov 29, 2019

Author Member

the internal helpers stays where they were, they're not pruned under k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/helpers.go

my IDE tooling tells me that we can cleanup some of those, do you think we should clean them up as well?

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

yes please

// NOTE: the newly logically-defaulted columns is not pointing to the original CRD object.
// One cannot mutate the original CRD columns using the logically-defaulted columns. Please iterate through
// the original CRD object instead.
func GetColumnsForVersion(crd *apiextensions.CustomResourceDefinition, version string) ([]apiextensions.CustomResourceColumnDefinition, error) {

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

is this used?

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

Defaulting and external types sounds suspicious.

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Nov 29, 2019

Author Member

columns, err := apiextensions.GetColumnsForVersion(crd, v.Name)

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Nov 29, 2019

Author Member
// NOTE: the newly logically-defaulted columns is not pointing to the original CRD object.
// One cannot mutate the original CRD columns using the logically-defaulted columns. Please iterate through
// the original CRD object instead.

i think it's saying that we should not modify the CRD columns b/c it's an active shared entry in the lister's cache?

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

could we move these as private funcs into pkg/apiserver? Helpers which do defaulting are usually not a good idea. Would prefer to avoid them spreading.

I have no idea what the pre-existing comment wants to tell me :)

@@ -84,7 +84,6 @@ func (m *CRConverterFactory) NewConverter(crd *apiextensions.CustomResourceDefin
// Determine whether we should expect to be asked to "convert" autoscaling/v1 Scale types
convertScale := false
if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) {
convertScale = crd.Spec.Subresources != nil && crd.Spec.Subresources.Scale != nil

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

accidental removal?

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Nov 29, 2019

Author Member

v1 crd doesn't have subresource in the top, instead we only need to check the per-version subresource now

@@ -86,7 +86,7 @@ func webhookClientConfigForCRD(crd *internal.CustomResourceDefinition) *webhook.
ret.Service = &webhook.ClientConfigService{
Name: apiConfig.Service.Name,
Namespace: apiConfig.Service.Namespace,
Port: apiConfig.Service.Port,
Port: *apiConfig.Service.Port,

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

can this be nil?

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Nov 29, 2019

Author Member

the defaulting prevents nil pointer for us

	if obj.Port == nil {
		obj.Port = utilpointer.Int32Ptr(443)
	}
}

var statusSpec *apiextensions.CustomResourceSubresourceStatus
var statusSpec = &apiextensionsinternal.CustomResourceSubresourceStatus{}

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

this now is always non-nil. Has that side-effects?

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Nov 29, 2019

Author Member

good catch! i believe it indeed has side-effects, we're installing their storages if they're non-nil. curiously why the tests are all happy tho.. we have no tests ensuring disablity of CRD subresources

@@ -718,14 +727,16 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
}
}

var scaleSpec *apiextensions.CustomResourceSubresourceScale
var scaleSpec = &apiextensionsinternal.CustomResourceSubresourceScale{}

This comment has been minimized.

Copy link
@sttts

sttts Nov 29, 2019

Contributor

this now is always non-nil. Has that side-effects?

This comment has been minimized.

Copy link
@yue9944882
@yue9944882 yue9944882 force-pushed the yue9944882:chore/crd-internal-client-prune branch 4 times, most recently from 3a63dc3 to 800fa60 Nov 29, 2019
yue9944882 added 4 commits Nov 8, 2019
switch api helper functions to v1 CRD api

switch v1 CRD for apiserver internal

switch to v1 CRD for internal controllers

api storage/validation related changes

move local-defaulting utils private to prevent spreading

boilerplate

keep the subresource status/scale spec nil unless it's enabled

clean up empty space
rename alias import

fmt
[generated] bazels

bazel
@yue9944882 yue9944882 force-pushed the yue9944882:chore/crd-internal-client-prune branch from 0617640 to 81471c3 Dec 3, 2019
@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Dec 3, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 3, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 3, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts, yue9944882

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Dec 3, 2019

/retest

1 similar comment
@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Dec 3, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit eff703d into kubernetes:master Dec 3, 2019
13 of 15 checks passed
13 of 15 checks passed
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
tide Not mergeable. Retesting: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation yue9944882 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 3, 2019

@yue9944882: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-kubemark-e2e-gce-big 81471c3 link /test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.