fix(target-allocator): tolerate missing ServiceMonitor/PodMonitor CRDs#394
Open
wenegiemepraise wants to merge 5 commits into
Open
fix(target-allocator): tolerate missing ServiceMonitor/PodMonitor CRDs#394wenegiemepraise wants to merge 5 commits into
wenegiemepraise wants to merge 5 commits into
Conversation
…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.
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.
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.
Start each monitor informer lazily, driven by a watch on the ServiceMonitor/PodMonitor CustomResourceDefinitions, instead of building both informers eagerly and blocking on cache sync at startup. The Target Allocator now starts and stays healthy whether or not the CRDs are installed, and begins (or stops) watching each type automatically as its CRD is created or deleted -- no restart required. - NewPrometheusCRWatcher no longer builds informers; it adds an apiextensions client and empty per-type informer/stop-channel maps. - startMonitorInformer/stopMonitorInformer manage each type independently (fresh factory per start so a deleted-then-recreated CRD restarts cleanly). - watchCRDs starts/stops informers on CRD add/delete; Watch never fails just because a CRD is absent. - LoadConfig guards against absent informers under a read lock. - Close stops all per-type informers. Implicitly gated on prometheusCR.enabled (the watcher is only constructed when that is set), so behavior is unchanged when CR watching is off. Promotes k8s.io/apiextensions-apiserver to a direct dependency.
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
2 participants
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.
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.