* Rework how we GlobalResync.
This consolidates the logic for how we do GlobalResyncs in a few ways:
1. Remove SendGlobalUpdates and replace it with FilteredGlobalResync.
2. Have GlobalResync go through the FilteredGlobalResync with a tautological predicate.
3. Change the way we enqueue things during a global resync to avoid flooding the system
by using EnqueueAfter and wait.Jitter to stagger how things are queued.
The background for this is that when I started to benchmark reconciliation of a large
number of resources we'd see random large stalls like [this](https://mako.dev/run?run_key=4882560844824576&~dl=1&~cl=1&~rl=1&~rvl=1&~il=1&~sksl=1&~pal=1)
fairly consistently. Looking at the logs, these stalls coincide with incredibly deep
work queues due to global resyncs triggers by configmap notifications. When I disabled
global resyncs the spikes [went away](https://mako.dev/run?run_key=4975897060835328&~dl=1&~cl=1&~rl=1&~rvl=1&~il=1&~sksl=1&~pal=1).
To mitigate the huge pile-up due to the global resync we use `wait.Jitter` to stagger how
things are enqueued with a `maxFactor` of the length of the store. This also seems to
[keep things flowing](https://mako.dev/run?run_key=5701802099998720&~dl=1&~cl=1&~rl=1&~rvl=1&~il=1&~sksl=1&~pal=1),
although we will possibly need to tune things further.
* Update comment to mention the delay.
A while back we added a "StatsReporter" argument to `controller.NewImpl`,
but in serving every callsite of this is passing:
```
controller.NewImpl(r, logger, "my-string", MustNewStatsReporter("my-string", logger))
```
Where `MustNewStatsReporter` is just a form of knative/pkg's `controller.NewStatsReporter`
that logs fatally when an error is returned. It is notable that Serving's logic has been
duplicated to both Build and Eventing.
There are a handful of changes here:
1. Move MustNewStatsReporter into knative/pkg
2. Expose the current interface as NewImplWithStats
3. Drop the StatsReporter from NewImpl and default to `MustNewStatsReporter()`
This is a breaking change for downstream repositories, but should make their callsites universally simpler.
In serving we have `reconciler.Handler` that wraps a handler function
(e.g. `Enqueue`) in the `cache.ResourceEventHandlerFuncs`. This pattern
was becoming pervasive, so this simpler handler dramatically reduced our
boilerplate.
This is needed for the positive handoff, so that we can enqueue
the KPA after we received the positive answer from the activator
but still want to have buffer time to make sure the network
changes propagate everywhere.
* Use different reporting periods based on the metrics backend. Original implementation was setting this value to 1 min to accommodate the minimum allowed value for stackdriver, but that is a sub optimal behavior for Prometheus. The change also allows operator to override the default values.
* Fix the units of reconciler latency from nano seconds to milliseconds.
* Address PR comments.
* Add better logging for reconciliation.
* Remove unnecessary lock in the UT.
* Address PR comments.
I was hoping to switch from a simple string key to a struct to include additional metadata into our workqueue item (in particular the time at which it was queued), but the great unit tests here reminded me that this would not properly deduplicate keys (they would have different queue times!), so I'm settling for simplifying the loop in a few ways:
1. Keys can only be strings, so remove some error checking and a test.
1. We were wrapping a block in a `func` because it contained `defer`, but the outer function had nothing in it.
I'd still like to try and find a way to track queue times for keys, so that we can surface two interesting metrics:
1. How long are things spending queued?
1. How long between events being observed and us responding? (above + reconcile latency)
This promotes the private deletion-handling helper wrapping `meta.Accessor` into `kmeta`. The wordy name is based on the wordier `cache.DeletionHandlingMetaNamespaceKeyFunc`, since it serves a similar purpose.
* Prepare for global resync on ConfigMap changes
- Add GlobalResync(cache.SharedInformer) method to *controller.Impl
- Move UntypedStore from serving/pkg/config to pkg/configmap
- Add onAfterStore callbacks to UntypedStore
- Add TypeFilter to enable composable construction of such callbacks
* Address review
- Unit test GlobalResync()
- Reimpl GlobalResync() using ListKeys()
* Update comments
* Add test for onAfterStore
* Update godoc comment for Logger
This adds a logkey.Key for attaching the key being reconciled to a logger we embed into the context passed to Reconcile. The point of this is to enable us to eliminate [this](928d580756/pkg/reconciler/v1alpha1/service/service.go (L101-L104)) boilerplate from each of the Reconcilers.
These aren't identical, this has the form:
knative.dev/key: foo/bar
What it's replacing has the form:
knative.dev/namespace: foo
serving.knative.dev/service: bar
However, the type information present in the latter is redundant with the controller type that [should already be embedded](928d580756/pkg/reconciler/reconciler.go (L80)) in the logger.
The `controller.go` is from: https://github.com/knative/serving/pull/1770, however, this adds 100% coverage of `controller.go`, which we have been missing in `knative/serving`.
This also adds a `context.Context` to the `Reconcile` signature, to enable better sharing of logger setup across controllers.
Fixes: https://github.com/knative/pkg/issues/8
Matt Moore
Victor Agababov
mattmoor-sockpuppet
leo james
Ville Aikas
Brad Hoekstra
Aldo Culquicondor
Mustafa Demirhan
lichuqiang
mattmoor-sockpuppet
Ivan Mikushin