crossplane / crossplane/crossplane-runtime

MRMetricRecorder keys its observation maps by name only, dropping metrics for same-named MRs in different namespaces

Open
#1,140 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
198
Forks
161
Avg merge
1d 11h
Merged PRs (30d)
9

Description

### What happened?

`MRMetricRecorder` tracks in-flight managed resources in two `sync.Map`s keyed by **`managed.GetName()`** alone, with no namespace component. Now that managed resources are namespaced (v2), two MRs with the same name in different namespaces collide, and metric observations are silently lost.

From [`pkg/reconciler/managed/metrics.go`](https://github.com/crossplane/crossplane-runtime/blob/main/pkg/reconciler/managed/metrics.go) (`main` @ `4e7ed23`):

```go
func (r *MRMetricRecorder) recordFirstTimeReconciled(managed resource.Managed) {
if managed.GetCondition(xpv2.TypeSynced).Status == corev1.ConditionUnknown {
r.mrDetected.With(getLabels(managed)).Observe(...)
r.firstObservation.Store(managed.GetName(), time.Now()) // name only
}
}

func (r *MRMetricRecorder) recordFirstTimeReady(managed resource.Managed) {
if managed.GetCondition(xpv2.TypeReady).Status == corev1.ConditionTrue {
_, ok := r.firstObservation.Load(managed.GetName()) // name only
if !ok {
return // <-- observation dropped
}
r.mrFirstTimeReady.With(getLabels(managed)).Observe(...)
r.firstObservation.Delete(managed.GetName())
}
}
```

Sequence for two MRs sharing a name across namespaces:

1. `ns-a/collide` first reconcile → `Store("collide")`.
2. `ns-b/collide` first reconcile → `Store("collide")`, overwriting the first entry.
3. `ns-a/collide` becomes Ready → `Load` hits, observes, then `Delete("collide")`.
4. `ns-b/collide` becomes Ready → `Load` misses → `return`, **observation lost**.

`managed_resource_first_time_to_readiness_seconds` therefore undercounts.

The same defect affects `lastObservation` and `managed_resource_drift_seconds`. That path is worse, because `recordUnchanged` only ever receives a bare name — the namespace isn't available at the call site at all ([`reconciler.go#L1512`](https://github.com/crossplane/crossplane-runtime/blob/main/pkg/reconciler/managed/reconciler.go)):

```go
r.metricRecorder.recordUnchanged(managed.GetName())
```

### How can we reproduce it?

Crossplane v2.4.0 + `provider-nop` v0.5.0, provider running with `--poll=10s` so a delayed `conditionAfter` is actually applied.

**Test — identical names in two namespaces:**

```yaml
apiVersion: nop.crossplane.io/v1alpha1
kind: NopResource
metadata:
name: collide # same name in ns coll-a and coll-b
namespace: coll-a # and again with namespace: coll-b
spec:
forProvider:
conditionAfter:
- time: 30s # non-zero, so both MRs sit in the map concurrently
conditionType: Ready
conditionStatus: "True"
conditionReason: Available
```

Both reach `Ready=True`:

```console
$ kubectl get nopresources -A -o custom-columns='NS:.metadata.namespace,NAME:.metadata.name,READY:.status.conditions[?(@.type=="Ready")].status' --no-headers
coll-a collide True
coll-b collide True
```

But only one was counted:

```console
$ curl -s localhost:8080/metrics | grep first_time_to_readiness_seconds_count
crossplane_managed_resource_first_time_to_readiness_seconds_count{gvk="nop.crossplane.io/v1alpha1, Kind=NopResource"} 1
```

**Control — distinct names, same two namespaces, same timing:** adding `coll-a/unique-a` and `coll-b/unique-b` moves the counter from 1 to 3, i.e. `+2` as expected. So the loss is attributable to the name collision and not to the timing.

Summary: 4 MRs `Ready`, counter reads 3.

### Suggested fix

Key both maps by namespace + name. Something like:

```go
func mrKey(mg resource.Managed) string {
return mg.GetNamespace() + "/" + mg.GetName() // or client.ObjectKeyFromObject(mg).String()
}
```

This requires changing the `recordUnchanged(name string)` signature on the `MetricRecorder` interface to take the `resource.Managed` (or a `client.ObjectKey`) so the namespace is available — `recordDrift` already receives the full object, so only `recordUnchanged` and its single call site need adjusting.

Happy to put up a PR if you agree with the approach.

### How much impact is this issue causing?

Low to medium — metrics-only, no reconcile-behaviour impact. It matters most where the same MR names recur across many namespaces, which is common for templated or per-tenant workloads and for anything using namespaces as the unit of replication. It surfaced while using `provider-nop` to load-test a control plane with identically-named MRs sharded across namespaces, where readiness counts silently disagreed with reality.

### What environment did it happen in?

- crossplane-runtime: `main` @ `4e7ed23` (also present in v2.0.0 and the v2.x line)
- Crossplane: v2.4.0
- provider-nop: v0.5.0
- Kubernetes: v1.31 (kind)

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in pkg/reconciler/managed/metrics.go, then inspect the recordUnchanged call at pkg/reconciler/managed/reconciler.go#L1512 and the MetricRecorder interface. Trace firstObservation and lastObservation through first-time readiness, drift, and unchanged recording; done means same-named managed resources in different namespaces retain separate metric observations without changing reconcile behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
observability-sre
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.