prevent startup deadlock when watching many CRDs - #290
Open
RezaMash wants to merge 14 commits into
Open
Conversation
…eta1 to policy/v1
On clusters with a large number of CRDs (e.g. ~477 on GDCH), sloop would
hang during startup and never bind its webserver on :8080. Liveness and
startup probes against /healthz then got "connection refused" forever,
putting the pod into a permanent CrashLoopBackOff.
Root cause was a lock-ordering deadlock during the initial informer sync:
- writeToOutChan() sent to the bounded kubeWatchChan (buffer 1000) while
holding i.protection. The author had already flagged this line as
dangerous.
- The single processing goroutine drains that channel doing several
synchronous, fsync'd (SyncWrites=true) BadgerDB transactions per event,
so it could not keep up with the initial-sync burst from the CRD
informers plus the core resources.
- Once the channel filled, informer handlers blocked on the send while
holding i.protection. Meanwhile the main goroutine was still in
startCustomInformers() iterating over every CRD, and each informer
setup also needs i.protection. It blocked, so NewKubeWatcherSource()
never returned and webserver.Run() was never reached.
Two changes:
1. ingress/kubewatcher.go: do not hold i.protection across the channel
send. Take the lock only to read `stopped`, release it, then send via a
select on i.outchan / i.stopChan. The send no longer blocks the lock,
and it unblocks promptly on shutdown instead of blocking forever on a
full channel. This removes the deadlock.
2. server/server.go: construct the kube watcher in a background goroutine
so the main goroutine reaches webserver.Run() and binds /healthz
immediately, regardless of how long the initial CRD sync takes. This is
defense in depth: even a merely slow (not deadlocked) watcher no longer
causes probes to kill the pod during startup. Shutdown reads
kubeWatcherSource under a mutex since it is now set from a goroutine.
With these changes the pod becomes Ready in seconds with watchCrds=true,
and CRD-backed resources (e.g. *.dbadmin.gdc.goog Instances) are recorded.
Verified: go build ./pkg/..., go test ./pkg/sloop/ingress/...
./pkg/sloop/server/..., and go test -race ./pkg/sloop/ingress/... all pass.
|
Thanks for the contribution! Before we can merge this, we need @RezaMash to sign the Salesforce Inc. Contributor License Agreement. |
Author
|
I signed the CLA. |
Add a GetResDescribe query that renders stored payloads via k8s.io/kubectl describers backed by a fake clientset seeded with the payload and its Events, surfacing fields like Image ID that the raw JSON views bury. Kinds without constructible describers and CRDs fall back to a generic field-tree rendering. Adds a Describe pane to the resource detail page.
This comment was marked as outdated.
This comment was marked as outdated.
Return the effective query window in view_options and use it for the timeline axis and end-time display. Default to an empty end_time so the server anchors to the newest data (the backup time when browsing a restore); replace the Now button with Latest.
getEndOfTime now uses the newest resource-summary lastSeen instead of the hour-aligned partition end, removing the dead zone at the end of restored-backup views. The Latest button now fits its label and resubmits immediately instead of leaving the end-time box empty.
Add the optional remoteKubeconfigSecret value: when set the statefulset mounts that Secret at /remote-kube read-only and points KUBECONFIG at it, so the instance records the cluster described by the kubeconfig instead of the cluster it runs on. Needed to watch node-less clusters (e.g. the GDC L1 management plane) from a cluster that has nodes. The default render is unchanged from 0.2.0.
MakeKubernetesClient logged config.Host before checking the error from ClientConfig, so any config problem (missing kubeconfig, unresolvable context) dereferenced a nil *rest.Config and killed the process instead of returning an error the caller could act on. Observed as a CrashLoopBackOff when a mounted kubeconfig was absent.
The watcher goroutine logged one line and returned on the first error, so a transient API-server failure at startup left the process running with zero watchers, an empty database and a green /healthz for the rest of its life. Retry every minute until initialization succeeds.
The ticker that retries CRD discovery was created after the call it would retry, so a single transient CustomResourceDefinitions().List() error left CRD watching off for the life of the process while the well-known informers kept running and /healthz stayed green. Arm the ticker first and let the refresh loop retry discovery; the refresh goroutine still starts after the first attempt returns so two startCustomInformers calls cannot race over the informer map.
Every entry of crd.Spec.Versions got its own informer. An unserved version has no endpoint and 404-loops forever while counting as running, and the apiserver returns the same objects through each served version, so multi-version CRDs recorded every object once per version (observed: 220 watch events for 110 Projects on a v1+v1alpha1 CRD). Pick the served storage version, falling back to the first served one, and skip CRDs that serve nothing.
Sloop exports Go memstats but no profiles, so a process that grows into its memory limit can only be diagnosed by inference. In CI both the local and the remote-watching instances were OOM-killed repeatedly at a 4Gi limit with stable informer and goroutine counts, and nothing could attribute the heap to a retainer. The handlers sit on their own mux behind StripPrefix: pprof.Index picks the profile out of the path by trimming the literal "/debug/pprof/", so under the context prefix every profile request would quietly render the index page instead. The test covers that case rather than mere registration.
Each informer holds a decoded copy of every object it watches, and sloop reads none of them: there is no Lister or Store access anywhere, only event handlers. The caches are therefore pure overhead on a process that has been OOM-killed repeatedly in CI, and managedFields is a large administrative slice of them - 11% of recorded payload bytes on a GDC management plane. A SetTransform on every informer (well-known and CRD alike) drops it on the way in, which shrinks the informer caches, the badger store and the snapshots together. Mutating in place is safe here because the informer owns the object, and avoids a deep copy per event. Tombstones from a relist after a watch gap are unwrapped so their payload is stripped too, and anything that is not a Kubernetes object passes through untouched rather than being dropped.
The Go runtime does not see the container memory limit, so it grows its heap until the kernel OOM-kills the process - repeatedly, in CI, usually mid-backup so the snapshot upload is truncated too. A soft limit makes the GC work harder as it nears the ceiling, trading throughput for staying alive. The value is explicit rather than derived from the downward API: GOMEMLIMIT should sit below resources.limits.memory to leave room for non-heap memory, and the downward API cannot do that arithmetic. Empty by default, so the rendered output is unchanged apart from the chart version.
…strip The transform is not called once per object. DeltaFIFO applies it to every delta, including the Sync deltas the periodic resync produces and the tombstones a post-watch-gap relist produces, and both of those carry the pointer already held in the indexer - the same one the handler goroutine may be marshaling. The unconditional write was therefore a data race, confirmed by the race detector. It is not a benign one: for unstructured objects, which is every CRD and the bulk of what is watched on a management plane, SetManagedFields(nil) is a map delete, and a map write concurrent with the json.Marshal walking that map is a fatal runtime abort that recover cannot catch. Sloop already had a habit of dying; this would have added a new way. Guard the write, so every pass after the first is a pure read, and hand tombstones back untouched - client-go has already run their inner object through the transform on its way into the store. The test drives a real informer through two resync ticks with a handler marshaling the delivered object, and fails under -race without the guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On clusters with a large number of CRDs (e.g. 500), sloop would
hang during startup and never bind its webserver on :8080. Liveness and
startup probes against /healthz then got "connection refused" forever,
putting the pod into a permanent CrashLoopBackOff.
Root cause was a lock-ordering deadlock during the initial informer sync:
holding i.protection. The author had already flagged this line as
dangerous.
synchronous, fsync'd (SyncWrites=true) BadgerDB transactions per event,
so it could not keep up with the initial-sync burst from the CRD
informers plus the core resources.
holding i.protection. Meanwhile the main goroutine was still in
startCustomInformers() iterating over every CRD, and each informer
setup also needs i.protection. It blocked, so NewKubeWatcherSource()
never returned and webserver.Run() was never reached.
Two changes:
ingress/kubewatcher.go: do not hold i.protection across the channel
send. Take the lock only to read
stopped, release it, then send via aselect on i.outchan / i.stopChan. The send no longer blocks the lock,
and it unblocks promptly on shutdown instead of blocking forever on a
full channel. This removes the deadlock.
server/server.go: construct the kube watcher in a background goroutine
so the main goroutine reaches webserver.Run() and binds /healthz
immediately, regardless of how long the initial CRD sync takes. This is
defense in depth: even a merely slow (not deadlocked) watcher no longer
causes probes to kill the pod during startup. Shutdown reads
kubeWatcherSource under a mutex since it is now set from a goroutine.
With these changes the pod becomes Ready in seconds with watchCrds=true,
and CRD-backed resources are recorded.
Verified: go build ./pkg/..., go test ./pkg/sloop/ingress/...
./pkg/sloop/server/..., and go test -race ./pkg/sloop/ingress/... all pass.