fix(metrics): resolve pods to their Deployment, not their ReplicaSet - #298
Merged
Merged
Conversation
Relationship metrics (container_http_requests_*, container_net_tcp_*, container_net_latency_seconds and all L7 protocol families) published two identities for the same workload: the Deployment name, and the ReplicaSet name with its pod-template-hash. On dev-gke 62% of the series for container_http_requests_duration_seconds_total_bucket carried kind=ReplicaSet, across 77 distinct ReplicaSet names. Every rollout minted a fresh set of series that never collapsed back, and alert rules that aggregate by destination_workload_name fired once per variant. resolvePodDescriptor climbs Pod -> ReplicaSet -> Deployment, but two defects made it stop at the ReplicaSet and stay there: 1. The climb is skipped whenever getControllerOfOwner returns an error, which it does when the owner is absent from the informer snapshot - either because informers have not synced yet, or because the ReplicaSet was pruned by revisionHistoryLimit while its pods still run. 2. The result was cached anyway. The `if owner, err := ...` used `:=`, which declared a new err scoped to the if statement, so the outer err was never assigned and the trailing `if err == nil` was always true. A transient informer miss was therefore memoized under the pod UID and returned for the rest of the pod's life, which is why this never self-healed. Distinguish transient from terminal failures with errOwnerNotCached and errUnsupportedOwnerKind: an owner missing from the cache leaves the descriptor uncached so the next call retries, while an untracked kind (Argo Rollouts, Elasticsearch, OpenTelemetryCollector, VMAgent - all present in this cluster) is terminal and still cached to avoid re-walking the chain on every scrape. When a ReplicaSet genuinely cannot be resolved, derive the Deployment name from its pod-template-hash suffix. stripPodTemplateHash only strips a final segment matching Kubernetes' vowel-free hash alphabet, so a bare ReplicaSet created without a Deployment is never renamed to one that does not exist. Also fixes two smaller identity splits: - NewDestinationKey overwrote a workload's Name with the DNS FQDN whenever the actual destination looked external, while keeping the k8s-resolved Namespace and Kind. That produced mixed-provenance labels such as name=temporal-frontend.nudgebee.svc.cluster.local + namespace=nudgebee + kind=Deployment (173 series). The FQDN is now substituted only when there is no in-cluster identity; it remains available via the destination label. Gating on Kind rather than name keeps genuinely external destinations intact, which the external-service discovery queries rely on. - ResolvePodOwner's API-server fallback returned Kind "Pod" while resolvePodDescriptor returns "pod", so an unowned pod got a different kind depending on which path resolved it. Aligned to lowercase, matching the convention that non-owner sentinels are lowercase and real k8s Kinds keep their casing. Label values are otherwise left alone on purpose: "external" is pinned by 11 production discovery queries, so normalising the casing would silently break external service detection. Fixes new series only. Pods already running keep their cached identity until they are replaced.
There was a problem hiding this comment.
Code Review
This pull request improves Kubernetes workload identity resolution by ensuring that transient owner-resolution failures (e.g., when an owner is missing from the informer cache) are not cached, preventing pods from being permanently pinned to intermediate identities like ReplicaSets. It also standardizes pod kinds to lowercase, introduces a fallback to strip pod-template hashes from ReplicaSet names, and ensures that FQDNs do not overwrite fully-resolved Kubernetes workload names in destination keys. The reviewer suggested adding a depth limit to the ownership climb loop to prevent potential infinite loops from circular owner references.
blue4209211
approved these changes
Jul 28, 2026
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
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.
Problem
Relationship metrics publish two identities for the same workload — the Deployment name, and the ReplicaSet name with its pod-template-hash:
llm-serverllm-server-779f867dc9llm-server-78c94f7dfdMeasured on dev-gke: 62% of
container_http_requests_duration_seconds_total_bucketseries carrykind=ReplicaSet(13,620 viasrc_workload_kind, 1,236 viadestination_workload_kind), across 77 distinct ReplicaSet names.Two consequences:
destination_workload_namefire once per variant. This surfaced as duplicateHigh P95 latency for llm-server/...for llm-server-779f867dc9events at the same second.Affects
container_net_latency_seconds, allcontainer_net_tcp_*, and all 15 L7 protocol families (src / destination / actual_destination).Root cause
resolvePodDescriptorintends to climbPod -> ReplicaSet -> Deployment, but:1. The climb is skipped on any error.
getControllerOfOwnererrors when the owner is absent from the informer snapshot — informers not yet synced, or the ReplicaSet pruned byrevisionHistoryLimitwhile its pods still run.namethen stays as the ReplicaSet.2. The wrong answer was cached forever.
:=declares a newerrscoped to theif. The outererrwas never assigned, so the trailingif err == nil { PodDescriptors.Store(...) }was always true. A transient informer miss got memoized under the pod UID and returned for the rest of the pod's life — which is why this never self-healed and why both names coexist steadily.Changes
errwith an explicitcacheableflag; only memoize a fully-resolved chain.errOwnerNotCached(transient -> don't cache, retry next scrape) fromerrUnsupportedOwnerKind(terminal -> still cache). This matters:Elasticsearch,OpenTelemetryCollector,VMAgent,VMAlertmanagerandRunnerall appear as owner kinds in this cluster and would otherwise re-walk the chain on every scrape.stripPodTemplateHashfallback for a genuinely unresolvable ReplicaSet. Conservative by design — only strips a final segment matching Kubernetes' vowel-free hash alphabet[bcdfghjklmnpqrstvwxz2456789]{6,11}, so a bare ReplicaSet (replicaset1,my-app-frontend) is never renamed into a Deployment that doesn't exist.NewDestinationKeyno longer overwrites a k8s-resolved workload'sNamewith the DNS FQDN. That produced mixed-provenance labels (FQDN name + real namespace + real kind), 173 series. Gated onKindrather than name so genuinely external destinations still get the FQDN — which the 11 external-service discovery queries inapi-server/services/application/discovery.godepend on.ResolvePodOwner's API-server fallback returnedKind: "Pod"whileresolvePodDescriptorreturns"pod". Aligned to lowercase.Deliberately not changed
kindcasing is inconsistent (pod/node/externallowercase vsDeployment/ReplicaSetPascalCase), but"external"is pinned by 11 production queries. Normalising it would silently break external-service discovery for postgres, mysql, mongo, clickhouse, cassandra, zookeeper, redis, memcached, rabbitmq, kafka and nats. The lowercase values are sentinels meaning "not a k8s owner kind" — a real distinction, not an accident.Tests
New
common/ip_resolver_workload_identity_test.goandcommon/net_workload_identity_test.go— 11 test functions, 19 table cases:Full
./common/suite passes, including pre-existingTestDestinationKeyand"pod controlled by replicaset resolve to replicaset".go vet,gofmt, andGOOS=linux GOARCH=amd64build all clean.Rollout note
This fixes new series only. Pods already running keep their cached identity until replaced — natural churn on spot nodes, or
kubectl rollout restartto clear immediately. Historical series age out with retention.