fix(target-allocator): tolerate missing ServiceMonitor/PodMonitor CRDs - #394
wenegiemepraise wants to merge 4 commits into
Conversation
| }, nil | ||
| stopCh := make(chan struct{}) | ||
| informer.Start(stopCh) | ||
| if ok := cache.WaitForNamedCacheSync(resourceName, stopCh, informer.HasSynced); !ok { |
There was a problem hiding this comment.
startMonitorInformer holds informersMtx across Start and WaitForNamedCacheSync, and stopCh isn't recorded in informerStopChannels yet, so a slow apiserver could block the other start and stop paths and keep Close() from interrupting the sync. Could we run the sync outside the lock and select on w.stopChannel?
There was a problem hiding this comment.
You're right i didnt catch this. I reproduced and fixed the issue. The sync ran under informersMtx and only watched a local stop channel, so a slow/rejected LIST hung Close(). Now Start/WaitForNamedCacheSync run outside the lock and select on w.stopChannel, and Close() signals shutdown before draining. Added a test that blocks the LIST and asserts Close() returns (hangs before, passes after, race-clean).
| // existing CRDs on its initial sync, so CRDs present at startup are handled here | ||
| // too (startMonitorInformer is idempotent). | ||
| func (w *PrometheusCRWatcher) watchCRDs(notifyEvents chan struct{}) { | ||
| factory := apiextensionsinformers.NewSharedInformerFactory(w.crdClient, allocatorconfig.DefaultResyncTime) |
There was a problem hiding this comment.
watchCRDs informs on every CRD and caches each full object with its OpenAPI schema, even though only the two names in crdNameToResource matter, so memory could balloon on clusters with lots of CRDs. Could a PartialObjectMetadata informer or a metadata.name selector on those two names work instead?
| // Start informers for CRDs that already exist. Absent CRDs are simply | ||
| // skipped here; the CRD watch below starts them if/when they appear. | ||
| for crdName, resourceName := range crdNameToResource { | ||
| exists, err := w.crdExists(context.Background(), crdName) |
There was a problem hiding this comment.
crdExists in the Watch startup loop calls Get with context.Background() and no deadline, so a wedged apiserver could block the loop forever. Could we pass a context with a timeout?
| func (w *PrometheusCRWatcher) Close() error { | ||
| // Signal shutdown first so any in-flight startMonitorInformer aborts its cache | ||
| // sync and will not register a new informer after this point. | ||
| close(w.stopChannel) |
There was a problem hiding this comment.
Close() will panic if it's ever called twice since it closes w.stopChannel unconditionally. Only called once today, but a sync.Once would be cheap insurance.
| func (w *PrometheusCRWatcher) watchCRDs(notifyEvents chan struct{}) { | ||
| factory := apiextensionsinformers.NewSharedInformerFactory(w.crdClient, allocatorconfig.DefaultResyncTime) | ||
| crdInformer := factory.Apiextensions().V1().CustomResourceDefinitions().Informer() | ||
| _, _ = crdInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ |
There was a problem hiding this comment.
The AddEventHandler error is dropped here and in startMonitorInformer, so a failed registration would silently wire up no handler. Could we at least log it so a dead watch is visible?
| // LoadConfig no longer emits its targets), and a reload is signalled. This is | ||
| // the logic invoked by the CRD watch's DeleteFunc; it is exercised directly so | ||
| // the assertion does not depend on fake-clientset delete-watch delivery. | ||
| func TestStopMonitorInformerDropsType(t *testing.T) { |
There was a problem hiding this comment.
The Delete path from crdObjectName to stopMonitorInformer on the CRD informer's DeleteFunc is the one bit not covered end to end. Could we add a test that deletes the CRD via the fake client and asserts the informer stops, or confirm the E2E suite already hits it?
|
Checklist posted for the PR author to move into the PR description under a PR Checklist
|
3dadfd7 to
5f3d79d
Compare
…er startup The target-allocator declared the enable-prometheus-cr-watcher flag name as a constant but never registered it on the flag set, while the operator passes --enable-prometheus-cr-watcher whenever PrometheusCR.enabled is true. Because args are parsed with pflag.ExitOnError, the unregistered flag caused the binary to print 'unknown flag' and exit(2), putting the target-allocator pod into CrashLoopBackOff. This change registers the flag and ORs it with the YAML prometheus_cr.enabled setting, then fixes three latent defects that were previously unreachable because the binary crashed first: - promOperator: set a non-empty Namespace on the synthetic Prometheus object so the prometheus-operator config generator no longer panics with 'namespace can't be empty' in store.ForNamespace. - promOperator: set EvaluationInterval so the generated config does not render an empty global.evaluation_interval, which the prometheus config parser rejects with 'empty duration string'. - main: create and register service-discovery metrics and pass them to discovery.NewManager; passing a nil sdMetrics map makes every SD provider fail to register, yielding zero discovered targets. RELEASE_NOTES updated. (cherry picked from commit 1376451)
Add a regression test asserting that loading a Target Allocator config whose static scrape job omits scrape_protocols still yields a non-empty ScrapeProtocols on every loaded scrape config. This is defaulted by the pinned Prometheus library during yaml.UnmarshalStrict into the prometheus Config type, so the distributed /scrape_configs payload is never empty and the agent's prometheus-receiver validation passes. The test fails fast if a future dependency or load-path change drops this defaulting. (cherry picked from commit 0405f4d)
The pod-template restart-trigger sha256 was computed from Spec.Config only, so a change to Spec.Prometheus (rendered into a separate ConfigMap) left the pod template byte-identical and the workload controller did not roll the pods. Fold the serialized Spec.Prometheus (PrometheusConfig.Yaml()) into the hash input when it is non-empty, so a Prometheus-only change bumps the pod-template annotation and triggers a rolling restart, matching agent-config behavior. When no Prometheus config is set the hash input is byte-identical to the agent config alone, leaving non-Prometheus agents unaffected. (cherry picked from commit d61d693)
Makes the Target Allocator start and run healthily whether or not the community monitoring.coreos.com ServiceMonitor/PodMonitor CRDs are installed, and start or stop watching each type as its CRD appears or disappears with no restart. Previously Watch() built both informers unconditionally and blocked on WaitForNamedCacheSync, so an absent CRD meant the apiserver rejected LIST/WATCH, the informer never synced, and Watch() returned 'failed to sync cache'. Informers are now built lazily per type once their CRD is observed, and a metadata-only CustomResourceDefinition informer starts or stops each one at runtime. Requires the chart to grant get/list/watch on customresourcedefinitions (aws-observability/helm-charts#327); without it the CRD check fails and no ServiceMonitor/PodMonitor targets are discovered while the Target Allocator still reports healthy. Verified: go build ./... clean; make impi and make checklicense pass; unit tests pass across the target-allocator and manifests packages. (cherry picked from commit 5f3d79d)
5f3d79d to
13524bb
Compare
This branch has not been deployed
3 participants
Summary
Make the Target Allocator (TA) start and run healthily whether or not the community
monitoring.coreos.comServiceMonitor/PodMonitor CRDs are installed, and begin/stopwatching each type automatically as its CRD appears/disappears — with no restart, ever.
Built on top of this PR: #386
Problem
The TA consumes the SM/PM CRDs but doesn't require them to exist. Today
Watch()builds the ServiceMonitor and PodMonitor informers unconditionally atstartup and blocks on
WaitForNamedCacheSync. If a CRD is absent, the apiserverrejects LIST/WATCH for that GVR, the informer never syncs, and
Watch()returnsfailed to sync cache. Because that setup runs once, installing the CRD laterrequires a TA restart.
Fix
Make the watcher event-driven and per-type instead of one-shot and all-or-nothing:
crdExists— check whether a given CRD is installed before building its informer.startMonitorInformer/stopMonitorInformer— build/tear down a single monitorinformer on demand (idempotent); stopping drops its targets and signals a reload.
watchCRDs— an informer overCustomResourceDefinitionobjects; on Add starts thecorresponding monitor informer, on Delete stops it.
Watch()rewritten — start informers only for CRDs present at startup; absent onesare skipped (TA stays healthy) and started later by the CRD watch. Never returns
failed to sync cachedue to a missing CRD.Testing
1. Unit (this PR):
TestWatchStartsHealthyWithoutCRDs— no CRDs → Watch healthy, no error, no jobsTestWatchStartsInformerWhenCRDCreated— CRD created at runtime → informer starts, no restartTestWatchPerTypeIndependence— SM present, PM absent → only SM informer startsTestStopMonitorInformerDropsType— CRD removed → informer stopped, targets dropped, reload signalledTestCRDObjectName— CRD-name extraction incl. delete tombstone2. Before/after reproduction (A/B): same condition (CRDs absent) —
pre-fix
Watch()returnsfailed to sync cache(output above); post-fixTestWatchStartsHealthyWithoutCRDspasses (healthy) andTestWatchStartsInformerWhenCRDCreatedself-heals when the CRD appears.