[WIP] Use generated defaulters instead of webhook defaulters#1262
[WIP] Use generated defaulters instead of webhook defaulters#1262grantr wants to merge 9 commits intoknative:masterfrom
Conversation
generate-groups.sh won't generate defaulters, contrary to what the usage docs say. Until it does, this command will generate defaulters for the only package that wants to use them, serving/v1alpha1.
Defaulters are a feature of runtime.Scheme that can set default values for any runtime.Object. We can use kubernetes/code-generator's defaulter-gen to generate defaulters for use with CRDs like Revision. The k8s:defaulter-gen=TypeMeta comment tells defaulter-gen to generate defaulters for every struct with a TypeMeta field. In this case, we want to set the ServingState of a RevisionSpec to Active by default, so we need a defaulter for Revision since RevisionSpec doesn't embed TypeMeta. The actual defaulting function is SetDefault_Revision. The generated code makes it easy to hook this function into the Scheme. To set defaults on an object, import the clientset's generated scheme package and call scheme.Scheme.Default(object). The test can't do this because of an import cycle, so it creates a new scheme.
These were copied from the defaulters in pkg/webhook. The webhook defaulters are still used; these new ones remain unused. Moved defaulter tests to a new file and made them table-driven.
Both of these defaulters were setting defaults on nested revision templates. I think it's better to let the defaults be bound when the templated object is created instead of when the template is created.
Before reconciling a Revision, Configuration, Route, or Service, set its defaults using the scheme.
Defaults are now set by controllers when objects with defaults are reconciled. As a consequence of this, setting defaults in templates, as in the Service and Configuration specs, is no longer necessary.
google-prow-robot
added
area/API
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: grantr Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
google-prow-robot
added
the
size/L
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
grantr
changed the title
google-prow-robot
added
the
do-not-merge/work-in-progress
|
Marking this WIP waiting for tests to increase coverage. |
|
/test pull-knative-serving-go-coverage |
|
The following is the coverage report on pkg/. Say
*TestCoverage feature is being tested, do not rely on any info here yet |
|
@grantr Did you want to pursue this, or should we close? I would like to see us start to use this, if it makes sense. |
|
I do think we should use defaulters, but defaults should be set in webhooks, not controllers (to avoid modifying specs while reconciling objects). See also https://github.com/grantr/serving/pull/3#issuecomment-401177037. |
|
@grantr Yeah, I think in general I agree. I think the one place we're doing this sort of thing in the controller today is effectively scoped to thanks again for putting this together :) |
5 participants
/area api
/kind cleanup
This PR contains both #1236 and https://github.com/grantr/serving/pull/3. The following description is copied from https://github.com/grantr/serving/pull/3.
This replaces the defaulting code in the webhook with the generated scheme defaults from #1236. I recommend reviewing #1236 first, then when that's merged I'll rebase this PR onto master.
Reconciled resources (Service, Configuration, Route, and Revision in their respective controllers) are now always defaulted before reconciliation. This ensures that any previously created objects have default values during a reconcile. Currently, only Revision has a defaulter, but all controllers call
Scheme.Default()anyway for consistency. I'm happy to remove the no-op calls if desired.Controlled resources (Revision controlled by a Configuration, and Configuration controlled by a Service) are defaulted during their own reconciles, so they don't need to be defaulted during creation. For example, a revision created with a blank ConcurrencyModel will be defaulted to
ConcurrencyModel: Multibefore its first reconcile, so it's not necessary for the Configuration controller to default the Revision at creation time. This keeps the defaulting for each resource scoped to its own controller.There's no longer a need to bind revision defaults in Service and Configuration templates, so the Service and Configuration defaults in the webhook were removed without replacement.
/cc @mattmoor @dprotaso