fix(opencost): make agent OpenCost connection detection reliable#489
fix(opencost): make agent OpenCost connection detection reliable#489RamanKharchee wants to merge 2 commits into
Conversation
The agent reports opencostConnection=true only when it has a non-empty OpenCost URL and /healthz returns 2xx. Three defects kept that false even when OpenCost was healthy: 1. URL resolved once at boot. opencostURL was discovered a single time at startup and captured by the telemetry closure, while every other datasource in that closure is re-probed per tick. If OpenCost was not deployed/ready at boot, the URL stayed empty forever and opencostConnection never recovered without a pod restart. Resolve it per tick instead (cached ~CacheTTL by the Discoverer, so no extra API load) so the agent self-heals. 2. Discovery picked the Service's first port. findOne hardcoded Ports[0]. OpenCost's Service can expose both the cost-model API (9003, serves /healthz) and a UI port (9090); if the UI port sorts first, /healthz was probed on it and a healthy OpenCost reported down. Add FindFirstPreferPort and resolve OpenCost preferring port 9003, falling back to the first port otherwise. 3. No endpoint override. The chart wired only OPENCOST_ENABLED, leaving the agent fully dependent on the two label selectors. Wire optional opencost.endpoint -> OPENCOST_ENDPOINT for non-standard installs (the binary already honoured the env). Adds unit tests for port preference (selectPort, FindFirstPreferPort) covering the multi-port and fallback cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📦 Image Tags Updated |
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the nudgebee-agent to resolve the OpenCost URL dynamically per telemetry tick rather than only at startup, allowing the agent to self-heal if OpenCost is deployed after the agent boots. It also introduces a port preference mechanism (FindFirstPreferPort) to prioritize the cost-model API port (9003) over the UI port (9090) during autodiscovery. Feedback points out a potential issue where transient Kubernetes API errors during service discovery could result in caching an empty URL for the duration of the cache TTL (1 hour), and suggests refactoring the discovery logic to avoid caching on API errors.
| func (d *Discoverer) findOne(ctx context.Context, selector string, preferredPorts []int32) string { | ||
| list, err := d.cs.CoreV1().Services("").List(ctx, metav1.ListOptions{LabelSelector: selector}) | ||
| if err != nil || len(list.Items) == 0 { | ||
| return "" |
There was a problem hiding this comment.
Transient API Error Caching Bug
In the current implementation, if d.cs.CoreV1().Services("").List fails due to a transient API error (such as a temporary network glitch, API server rate limiting, or context timeout/cancellation), findOne returns "" (an empty string).
Because findOne does not distinguish between a transient API error and a service actually not being found (both return ""), FindFirstPreferPort will cache this negative result ("") for the full CacheTTL (1 hour).
This means that if a single transient error occurs during a telemetry tick, the agent will report opencostConnection=false (and potentially other auto-discovered connections as down) for a whole hour, even if OpenCost is perfectly healthy and running. This can lead to false-negative alerts or server-side issues like double-writes.
Suggested Fix
Refactor findOne to return (string, error) so that FindFirstPreferPort can distinguish between a successful "not found" result and an API error. Only cache the result in d.cache if no API errors were encountered during the discovery loop:
func (d *Discoverer) FindFirstPreferPort(ctx context.Context, selectors []string, preferredPorts ...int32) string {
// ...
url := ""
var apiErr error
for _, sel := range selectors {
u, err := d.findOne(ctx, sel, preferredPorts)
if err != nil {
apiErr = err
}
if u != "" {
url = u
break
}
}
d.mu.Lock()
if apiErr == nil || url != "" {
d.cache[cacheKey] = cacheEntry{url: url, expires: now.Add(CacheTTL)}
}
d.mu.Unlock()
return url
}
func (d *Discoverer) findOne(ctx context.Context, selector string, preferredPorts []int32) (string, error) {
list, err := d.cs.CoreV1().Services("").List(ctx, metav1.ListOptions{LabelSelector: selector})
if err != nil {
return "", err
}
if len(list.Items) == 0 {
return "", nil
}
// ...
}1 participant
Summary
The agent reports
opencostConnection=trueonly when it has a non-empty OpenCost URL and/healthzreturns 2xx. Three defects kept thatfalseeven when OpenCost was healthy and running:URL resolved once at boot.
opencostURLwas discovered a single time at startup and captured by the telemetry closure, while every other datasource in that closure is re-probed per tick. If OpenCost wasn't deployed/ready at boot, the URL stayed empty forever andopencostConnectionnever recovered without a pod restart. Now resolved per tick — cached ~CacheTTLby theDiscoverer, so no extra API-server load — so the agent self-heals when OpenCost is deployed after the agent.Discovery picked the Service's first port.
findOnehardcodedPorts[0]. OpenCost's Service can expose both the cost-model API (9003, serves/healthz) and a UI port (9090); if the UI port sorts first,/healthzwas probed on it and a healthy OpenCost reported down. AddedFindFirstPreferPortand resolve OpenCost preferring port 9003, falling back to the first port otherwise. The genericFindFirst(Prometheus/Loki/AlertManager) is unchanged — it delegates with no preference.No endpoint override. The chart wired only
OPENCOST_ENABLED, leaving the agent fully dependent on the two label selectors (app=opencost,app.kubernetes.io/name=opencost). Wired optionalopencost.endpoint→OPENCOST_ENDPOINTfor non-standard installs (the binary already honoured the env).Type of change
Chart version
version/appVersioninChart.yamlare placeholders set by.github/workflows/release.yaml(see the comment above them).Test plan
go build ./...,go vet ./...passgo test ./pkg/svcdiscover/... ./pkg/telemetry/...pass, including new tests:TestFindFirstPreferPort_PicksAPIPortOverUI,TestFindFirstPreferPort_FallsBackToFirstPort,TestSelectPortgofmtcleanhelm lint/helm template— not run locally; chart subcharts needhelm dependency buildfirst. The added block mirrors the existing{{- if .Values.globalConfig.prometheus_url }}conditional verbatim.Review Notes → Risks & Counterarguments
FindFirstPreferPortreuses theDiscoverer's 1h cache (positive and negative), so per-tick calls are cache hits between refreshes — at most oneServices().Listper selector group perCacheTTL. Self-heal latency after OpenCost appears is therefore ≤CacheTTL.OPENCOST_ENABLED=falsestill wins: when OpenCost is disabled at the agent the closure skips resolution entirely, preserving the server-side-takeover behaviour (opencostConnectionstays absent/false by design).Related
Counterpart to nudgebee-enterprise#32611 / nudgebee-enterprise#32612: those false-negative
opencostConnectionreports flow into the server-side OpenCost spend-sync account selection and can trigger a double-write. This PR fixes the agent-side reporting; that PR hardens the server-side query.🤖 Generated with Claude Code