Do not N^2 loading webhook configurations #114794

lavalamp · 2023-01-04T00:38:34Z

This adds a processing delay to deduplicate notifications to reload webhook configurations. Solves N^2 behavior on startup and prevents useless work when webhooks change in rapid succession.

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

Fixes N^2 behavior I noticed while working on #113985.

Does this PR introduce a user-facing change?

kube-apiserver: removed N^2 behavior loading webhook configurations.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

lavalamp · 2023-01-04T00:39:12Z

I'll add corresponding changes to the mutating version of this file once someone looks at this.

lavalamp · 2023-01-04T00:39:21Z

/sig api-machinery

lavalamp · 2023-01-04T18:45:00Z

The test fail is real, the goroutine doesn't get cleaned up.

apelisse · 2023-01-05T22:41:28Z

staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go

 )

 // validatingWebhookConfigurationManager collects the validating webhook objects so that they can be called.
 type validatingWebhookConfigurationManager struct {
 	configuration *atomic.Value
 	lister        admissionregistrationlisters.ValidatingWebhookConfigurationLister
-	hasSynced     func() bool
+	delayer       *synctrack.Delayer


Why did you make this a pointer?

For consistency with the configuration member above, which also doesn't need to be a pointer...

apelisse · 2023-01-05T22:42:55Z

staging/src/k8s.io/client-go/tools/cache/synctrack/delayer_test.go

+	go func() {
+		defer wg.Done()
+		for i := 0; i < adds; i++ {
+			d.Add()
+		}
+	}()


I don't think it makes a difference, but you're documenting that Add may be called in parallel., so maybe you could have called them in parallel?

deads2k · 2023-01-06T15:50:48Z

having updateConfiguration handle the complete list of validating or mutating admission webhooks ensures

order of updates to admission webhooks will always be honored. It is impossible to update-2 for webhook/B to take effect before update-1 for webhook/A to take effect.

But the only known failure mode is actually the lister.List call itself: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go#L77-L84

With the new ability to ensure processing of the initial list has happened (your previous hasSync'd fix) and the synchronous processing done in the add/update/delete func, is there still value in having the List versus having special handling for delete and performing a running adding/remove, perhaps with a periodic sanity check for a diff with a lock and panic on failure?

lavalamp · 2023-01-06T17:07:37Z

With the new ability to ensure processing of the initial list has happened (your previous hasSync'd fix) and the synchronous processing done in the add/update/delete func, is there still value in having the List versus having special handling for delete and performing a running adding/remove, perhaps with a periodic sanity check for a diff with a lock and panic on failure?

I think it was probably just done this way for simplicity? But yes, it could be changed to not list. I think that would be more invasive? This change is pretty small. And waiting and then listing once is probably more efficient on startup than doing N inserts (not that it matters much for this code -- it's about the precedent for me).

deads2k · 2023-01-06T18:37:58Z

And waiting and then listing once is probably more efficient on startup than doing N inserts (not that it matters much for this code -- it's about the precedent for me)

Which precedent do we want? Prior to your PR that tracks "have all event handlers handled", this was the state of the art. After your PR, it is possible to build reliable tracking without the list and I think that combined with a periodic consistency checker, is likely how I would suggest people handle it. Would you suggest using a list plus a delay instead?

staging/src/k8s.io/client-go/tools/cache/synctrack/lazy.go

logicalhan · 2023-01-12T17:37:52Z

/remove-sig instrumentation

apelisse · 2023-01-11T23:58:12Z

staging/src/k8s.io/client-go/tools/cache/synctrack/lazy_test.go

+limitations under the License.
+*/
+
+package synctrack


Consider making the test external to make it clear that you're not using anything internal for the test

staging/src/k8s.io/client-go/tools/cache/synctrack/lazy.go

apelisse · 2023-01-12T20:32:06Z

staging/src/k8s.io/client-go/tools/cache/synctrack/lazy.go

+func (z *Lazy[T]) Get() (T, error) {
+	e := z.cache.Load()
+	if e == nil {
+		// Since we don't force a constructor, nil is a possible value.


Good catche! (typo intended 😂 )

apelisse · 2023-01-12T20:34:24Z

/lgtm
/approve
If you want to make the test external, I don't really care:
/hold

k8s-ci-robot · 2023-01-12T20:34:32Z

LGTM label has been added.

Git tree hash: c71b82421fff3463a7870a49e2f3c4cbb662f5bd

k8s-ci-robot · 2023-01-12T20:34:48Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/apiserver/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/tools/cache/OWNERS~~ [lavalamp]

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

Add a "lazy" type to track when an update is needed. It uses a nested locking technique to avoid extra evaluation calls.

apelisse · 2023-01-12T21:01:30Z

/lgtm

k8s-ci-robot · 2023-01-12T21:01:36Z

LGTM label has been added.

Git tree hash: 2d1ecb82975186122e94b77b4c2d206724b92e7c

lavalamp · 2023-01-12T21:03:42Z

/hold cancel

k8s-ci-robot requested review from deads2k and jpbetz January 4, 2023 00:38

lavalamp force-pushed the improved-has-synced branch 3 times, most recently from 7b76004 to 23ee5d9 Compare January 5, 2023 05:34

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 5, 2023

lavalamp force-pushed the improved-has-synced branch 3 times, most recently from 0ca4937 to 5d16e01 Compare January 5, 2023 21:36

apelisse reviewed Jan 5, 2023

View reviewed changes

lavalamp force-pushed the improved-has-synced branch from 5d16e01 to bfb4ce2 Compare January 5, 2023 23:20

lavalamp force-pushed the improved-has-synced branch from bfb4ce2 to f7e985b Compare January 9, 2023 23:30

k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Jan 9, 2023

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 Jan 10, 2023

lavalamp force-pushed the improved-has-synced branch from 0c47f53 to 60f134b Compare January 11, 2023 05:08

apelisse reviewed Jan 11, 2023

View reviewed changes

staging/src/k8s.io/client-go/tools/cache/synctrack/lazy.go Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 12, 2023

lavalamp force-pushed the improved-has-synced branch from fbb0c23 to 0e8dfe4 Compare January 12, 2023 00:39

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2023

k8s-ci-robot removed the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Jan 12, 2023

lavalamp force-pushed the improved-has-synced branch 2 times, most recently from ac3d387 to 3a0d86f Compare January 12, 2023 19:11

apelisse reviewed Jan 12, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2023

k8s-ci-robot assigned apelisse Jan 12, 2023

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

Fix N^2 startup for webhook configurations

5a1091d

Add a "lazy" type to track when an update is needed. It uses a nested locking technique to avoid extra evaluation calls.

lavalamp force-pushed the improved-has-synced branch from 3a0d86f to 5a1091d Compare January 12, 2023 21:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2023

k8s-ci-robot requested a review from apelisse January 12, 2023 21:01

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2023

k8s-ci-robot merged commit cc9cc4d into kubernetes:master Jan 13, 2023

k8s-ci-robot added this to the v1.27 milestone Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not N^2 loading webhook configurations #114794

Do not N^2 loading webhook configurations #114794

lavalamp commented Jan 4, 2023 •

edited

lavalamp commented Jan 4, 2023

lavalamp commented Jan 4, 2023

lavalamp commented Jan 4, 2023

apelisse Jan 5, 2023

lavalamp Jan 5, 2023

apelisse Jan 5, 2023

lavalamp Jan 5, 2023

lavalamp Jan 5, 2023

deads2k commented Jan 6, 2023 •

edited

lavalamp commented Jan 6, 2023

deads2k commented Jan 6, 2023

logicalhan commented Jan 12, 2023

apelisse Jan 11, 2023

lavalamp Jan 12, 2023

apelisse Jan 12, 2023

apelisse commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

apelisse commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

lavalamp commented Jan 12, 2023

Do not N^2 loading webhook configurations #114794

Do not N^2 loading webhook configurations #114794

Conversation

lavalamp commented Jan 4, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

lavalamp commented Jan 4, 2023

lavalamp commented Jan 4, 2023

lavalamp commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jan 6, 2023 • edited

lavalamp commented Jan 6, 2023

deads2k commented Jan 6, 2023

logicalhan commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

apelisse commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

lavalamp commented Jan 12, 2023

lavalamp commented Jan 4, 2023 •

edited

deads2k commented Jan 6, 2023 •

edited