Consolidate webhooks #14082

skonto · 2023-06-07T11:27:15Z

Part of the work for #13965

Proposed Changes

As discussed in Merge domain mapping logic into the controller #14024 this is a quick win for webhooks and then a feature doc will be provided to describe how we move next with different deployment models.
Merges the logic for the domain mapping and serving webhooks
This requires Defaulting Controller options for all kind of webhooks pkg#2738 to pass the queue name nicely.

Release Note

Domain mapping controller logic is now merged with the Serving controller. Domainmapping webhook is merged with the Serving webhook.

codecov · 2023-06-07T12:08:07Z

Codecov Report

Patch coverage has no change and project coverage change: +0.04 🎉

Comparison is base (bde2f42) 86.23% compared to head (40a9e9f) 86.27%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14082      +/-   ##
==========================================
+ Coverage   86.23%   86.27%   +0.04%     
==========================================
  Files         199      199              
  Lines       14795    14795              
==========================================
+ Hits        12759    12765       +6     
+ Misses       1733     1730       -3     
+ Partials      303      300       -3

Impacted Files	Coverage Δ
cmd/controller/main.go	`0.00% <ø> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

nak3 · 2023-06-08T00:45:05Z

cmd/webhook/main.go

@@ -19,6 +19,10 @@ package main
 import (
 	"context"

+	servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"


domainmapping v1alpha1 is deprecated #14059 and will be removed after v1.11 cut #14060.
We should add v1alpha1 if this PR will be merged before v1.11 cut but it will be dropped very soon 😅

dprotaso · 2023-06-15T15:53:30Z

cmd/webhook/main.go

@@ -167,7 +242,9 @@ func main() {
 	sharedmain.WebhookMainWithContext(ctx, "webhook",
 		certificates.NewController,
 		newDefaultingAdmissionController,


You can just add domain mapping types to the newDefaultingAdmissionController and newValidationAdmissionController and you don't need to create new ones

@dprotaso I thought about that. My idea was to keep a separate controller per concern for now, to be able to have some independent configuration in the future eg. number of workers per queue, logging etc.
If we merge then should not we increase the concurrency for the common controller (default is 2)?

If we merge then should not we increase the concurrency for the common controller (default is 2)?

There really isn't an advantange here - this admission controller is reconciling a single resource

Yes but is not the case that the number of reconciled objects can be equal to the number of services, since a domain mapping can be set per service? So the load for the domain reconciler is not low as far as I can tell eg. during a restart. Should we merge the controllers and keep the concurrency defaults? Anyway I will do a quick test.

I'm confused - this controller just mutates this one resource here https://github.com/knative/serving/blob/main/config/core/webhooks/defaulting.yaml

So the load for the domain reconciler is not low as far as I can tell eg. during a restart. Should we merge the controllers and keep the concurrency defaults? Anyway I will do a quick test.

Increasing the # of controllers will bump the total QPS & Burst (it's code in the sharedmain) for the process. As for the reconciler there are two go routines each - so this doesn't change when moving a controller/reconciler from one go cmd to another.

As for the reconciler there are two go routines each - so this doesn't change when moving a controller/reconciler from one go cmd to another.

I was referring to the reconciliation work done and the #of workqueue workers, not the rate with which we hit the api server (the latter is fine as you said). Check here: https://github.com/knative/pkg/blob/main/controller/controller.go#L209. The default concurrency there is 2.
When a controller is started that value is passed to it https://github.com/knative/pkg/blob/main/controller/controller.go#L446. So if we move the domain mapping types for defaulting to the default controller then we will still have 2 routines but with more types to handle in case of reconciliation, no?
In detail, controller will do reconciliation in c.processNextWorkItem here: https://github.com/knative/pkg/blob/main/controller/controller.go#L487-L494, that function is called in a separate thread and total threads equals concurrency:

for i := 0; i < threadiness; i++ { sg.Add(1) go func() { defer sg.Done() for c.processNextWorkItem() { } }() }

That function will call reconcile at some point: https://github.com/knative/pkg/blob/main/controller/controller.go#L542.
In other words, let's say we initially have two controllers, let's say: newDefaultingAdmissionController,
newDefaultingDomainMappingAdmissionController, each has 2 routines for its reconciler by default and a separate work queue for each.
If we merge these controllers then we have 2 routines by default but with potentially more keys enqueued and more work for the common work queue, no?

So if we move the domain mapping types for defaulting to the default controller then we will still have 2 routines but with more types to handle in case of reconciliation, no?

More types in the webhook != more resources that need reconciliation. We should only have one defaulting webhook and one validation webhook.

It's this one here

serving/config/core/webhooks/defaulting.yaml

Lines 34 to 53 in 921daf8

- apiGroups:

- autoscaling.internal.knative.dev

- networking.internal.knative.dev

- serving.knative.dev

apiVersions:

- "*"

operations:

- CREATE

- UPDATE

scope: "*"

resources:

- metrics

- podautoscalers

- certificates

- ingresses

- serverlessservices

- configurations

- revisions

- routes

- services

Actually we should be moving these values from this webhook over to the new one and just exclude the /status

serving/config/core/webhooks/domainmapping-defaulting.yaml

Lines 42 to 44 in 921daf8

resources:

- domainmappings

- domainmappings/status

If we merge these controllers then we have 2 routines by default but with potentially more keys enqueued and more work for the common work queue, no?

Generally we haven't cared about this but it's not an issue for the webhook controllers since it's only reconciling a single resource. There are better gains to make elsewhere - for example the revision controller still has a concurrency of two - maybe the serverlessservice (since it deals with endpoint updates) could be bumped higher.

But I'd say that's out of scope for this PR.

Ok let's postpone this discussion for another PR.

I guess to clarify I'm referring to the 'threadiness' per controller out of scope for this PR. But I still think it makes sense to consolidate domain mapping into the default webhook

dprotaso · 2023-06-15T15:54:17Z

config/core/webhooks/domainmapping-validation.yaml

@@ -24,7 +24,7 @@ webhooks:
 - admissionReviewVersions: ["v1", "v1beta1"]


We can delete these webhooks since we can merge them into the default validation and defaulting ones

Yes if we move everything to the default ones then we can delete it, also there will be no need for the special queue name. Should we? Still thinking about it. Check my comment above.

skonto · 2023-07-06T21:16:45Z

@dprotaso gentle ping. I updated the PR, now all webhooks are served by one controller.

dprotaso · 2023-07-07T19:49:13Z

/lgtm
/approve

thanks @skonto

knative-prow · 2023-07-07T19:49:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, skonto

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:

~~OWNERS~~ [dprotaso,skonto]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* consolidate webhooks * pass the queue name update * merge types, clean up webhook configs * remove unused

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/API API objects and controllers labels Jun 7, 2023

knative-prow bot requested review from jsanin-vmw and mgencur June 7, 2023 11:27

knative-prow bot added area/test-and-release It flags unit/e2e/conformance/perf test issues for product features approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 7, 2023

skonto requested review from dprotaso and removed request for mgencur and jsanin-vmw June 7, 2023 11:31

skonto requested a review from nak3 June 7, 2023 14:01

nak3 reviewed Jun 8, 2023

View reviewed changes

skonto force-pushed the consolidate_webhooks branch from 9471a09 to 57cb96a Compare June 14, 2023 14:07

skonto changed the title ~~[wip] Consolidate webhooks~~ Consolidate webhooks Jun 14, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2023

dprotaso reviewed Jun 15, 2023

View reviewed changes

skonto added 3 commits July 6, 2023 15:17

consolidate webhooks

3a34ed6

pass the queue name update

6ce5b70

merge types, clean up webhook configs

d573012

skonto force-pushed the consolidate_webhooks branch from 57cb96a to d573012 Compare July 6, 2023 21:12

remove unused

40a9e9f

knative-prow bot assigned dprotaso Jul 7, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2023

knative-prow bot merged commit fc166ac into knative:main Jul 7, 2023

skonto added a commit to skonto/serving that referenced this pull request Oct 5, 2023

Consolidate webhooks (knative#14082)

cc9a098

* consolidate webhooks * pass the queue name update * merge types, clean up webhook configs * remove unused

dprotaso mentioned this pull request May 24, 2024

Need a post-install job to clean up domain mapping webhook resources #15247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate webhooks #14082

Consolidate webhooks #14082

skonto commented Jun 7, 2023

codecov bot commented Jun 7, 2023 •

edited

Loading

nak3 Jun 8, 2023

dprotaso Jun 15, 2023

skonto Jun 19, 2023 •

edited

Loading

dprotaso Jun 19, 2023

skonto Jun 19, 2023 •

edited

Loading

dprotaso Jun 19, 2023 •

edited

Loading

skonto Jun 20, 2023 •

edited

Loading

dprotaso Jun 20, 2023

skonto Jun 20, 2023

dprotaso Jun 21, 2023

dprotaso Jun 15, 2023

skonto Jun 19, 2023 •

edited

Loading

skonto commented Jul 6, 2023

dprotaso commented Jul 7, 2023

knative-prow bot commented Jul 7, 2023

	- apiGroups:
	- autoscaling.internal.knative.dev
	- networking.internal.knative.dev
	- serving.knative.dev
	apiVersions:
	- "*"
	operations:
	- CREATE
	- UPDATE
	scope: "*"
	resources:
	- metrics
	- podautoscalers
	- certificates
	- ingresses
	- serverlessservices
	- configurations
	- revisions
	- routes
	- services

		@@ -24,7 +24,7 @@ webhooks:
		- admissionReviewVersions: ["v1", "v1beta1"]

Consolidate webhooks #14082

Consolidate webhooks #14082

Conversation

skonto commented Jun 7, 2023

Proposed Changes

codecov bot commented Jun 7, 2023 • edited Loading

Codecov Report

nak3 Jun 8, 2023

Choose a reason for hiding this comment

dprotaso Jun 15, 2023

Choose a reason for hiding this comment

skonto Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

dprotaso Jun 19, 2023

Choose a reason for hiding this comment

skonto Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

dprotaso Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

skonto Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

dprotaso Jun 20, 2023

Choose a reason for hiding this comment

skonto Jun 20, 2023

Choose a reason for hiding this comment

dprotaso Jun 21, 2023

Choose a reason for hiding this comment

dprotaso Jun 15, 2023

Choose a reason for hiding this comment

skonto Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

skonto commented Jul 6, 2023

dprotaso commented Jul 7, 2023

knative-prow bot commented Jul 7, 2023

codecov bot commented Jun 7, 2023 •

edited

Loading

skonto Jun 19, 2023 •

edited

Loading

skonto Jun 19, 2023 •

edited

Loading

dprotaso Jun 19, 2023 •

edited

Loading

skonto Jun 20, 2023 •

edited

Loading

skonto Jun 19, 2023 •

edited

Loading