From 7d9c17bf8f5b9b37217161c730e602f023986398 Mon Sep 17 00:00:00 2001 From: devapragadeesh Date: Sun, 9 Aug 2026 15:18:29 +0530 Subject: [PATCH] NO-JIRA: monitor: record observed update/recreation counts on the stored object RecordResource is documented to annotate each recorded resource with observed-update-count and observed-recreation-count, but neither annotation ever reaches the stored object. The deep copy that gets stored is taken before the annotations are computed, while the metadata accessor used to write them is built from the caller's object. Every SetAnnotations call therefore mutates the argument and leaves the stored copy untouched. Because the stored copy carries no annotations, the next observation reads an empty update-count, ParseInt fails, and the count resets to "1" forever. Mutating the argument is a problem in its own right: the callers in monitortestlibrary/monitoring_store.go hand in objects straight from an informer cache, which must not be modified in place. Separately, the recreation count was read from the update-count annotation, so on every repeat observation the recreation count was overwritten with the update count. Take the accessor over the stored copy so the annotations land on it and the caller's object is left alone, and read the recreation count from its own annotation. The nil-metadata branch is dropped because meta.Accessor never returns a nil accessor with a nil error; the error is already handled by the panic above. Adds unit tests covering the seeded counts, repeated observations, and that the recorded object is not mutated. --- pkg/monitor/recorder.go | 24 ++++----- pkg/monitor/recorder_test.go | 98 ++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 12 deletions(-) create mode 100644 pkg/monitor/recorder_test.go diff --git a/pkg/monitor/recorder.go b/pkg/monitor/recorder.go index 09752e01abf5..01028ca3a1b1 100644 --- a/pkg/monitor/recorder.go +++ b/pkg/monitor/recorder.go @@ -66,24 +66,24 @@ func (m *recorder) RecordResource(resourceType string, obj runtime.Object) { UID: fmt.Sprintf("%v", newMetadata.GetUID()), } + // annotations are set on the stored copy so that the caller's object, which may come straight + // from an informer cache, is never mutated. toStore := obj.DeepCopyObject() - // without metadata, just stomp in the new value, we can't add annotations - if newMetadata == nil { - recordedResource[key] = toStore - return + storedMetadata, err := meta.Accessor(toStore) + if err != nil { + // coding error + panic(err) } - newAnnotations := newMetadata.GetAnnotations() + newAnnotations := storedMetadata.GetAnnotations() if newAnnotations == nil { newAnnotations = map[string]string{} } existingResource, ok := recordedResource[key] if !ok { - if newMetadata != nil { - newAnnotations[monitorapi.ObservedUpdateCountAnnotation] = "1" - newAnnotations[monitorapi.ObservedRecreationCountAnnotation] = "0" - newMetadata.SetAnnotations(newAnnotations) - } + newAnnotations[monitorapi.ObservedUpdateCountAnnotation] = "1" + newAnnotations[monitorapi.ObservedRecreationCountAnnotation] = "0" + storedMetadata.SetAnnotations(newAnnotations) recordedResource[key] = toStore return } @@ -107,7 +107,7 @@ func (m *recorder) RecordResource(resourceType string, obj runtime.Object) { } // set the recreate count. increment if the UIDs don't match - existingRecreateCountStr := existingAnnotations[monitorapi.ObservedUpdateCountAnnotation] + existingRecreateCountStr := existingAnnotations[monitorapi.ObservedRecreationCountAnnotation] if existingMetadata.GetUID() != newMetadata.GetUID() { if existingRecreateCount, err := strconv.ParseInt(existingRecreateCountStr, 10, 32); err != nil { newAnnotations[monitorapi.ObservedRecreationCountAnnotation] = existingRecreateCountStr @@ -118,7 +118,7 @@ func (m *recorder) RecordResource(resourceType string, obj runtime.Object) { newAnnotations[monitorapi.ObservedRecreationCountAnnotation] = existingRecreateCountStr } - newMetadata.SetAnnotations(newAnnotations) + storedMetadata.SetAnnotations(newAnnotations) recordedResource[key] = toStore return } diff --git a/pkg/monitor/recorder_test.go b/pkg/monitor/recorder_test.go new file mode 100644 index 000000000000..bab9dfef5f2e --- /dev/null +++ b/pkg/monitor/recorder_test.go @@ -0,0 +1,98 @@ +package monitor + +import ( + "testing" + + "github.com/openshift/origin/pkg/monitor/monitorapi" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +func pod(uid string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-pod", + UID: types.UID(uid), + }, + } +} + +func TestRecordResourceObservedCounts(t *testing.T) { + tests := []struct { + name string + recorded []*corev1.Pod + expectedUpdateCount string + expectedRecreateCount string + }{ + { + name: "first observation seeds the counts", + recorded: []*corev1.Pod{pod("uid-1")}, + expectedUpdateCount: "1", + expectedRecreateCount: "0", + }, + { + name: "repeated observations increment the update count", + recorded: []*corev1.Pod{pod("uid-1"), pod("uid-1"), pod("uid-1")}, + expectedUpdateCount: "3", + expectedRecreateCount: "0", + }, + { + // the UID is part of monitorapi.InstanceKey, so a recreated pod is tracked under its + // own key and starts counting again from scratch + name: "a recreated pod is tracked separately from its predecessor", + recorded: []*corev1.Pod{pod("uid-1"), pod("uid-1"), pod("uid-2")}, + expectedUpdateCount: "1", + expectedRecreateCount: "0", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + recorder := NewRecorder() + for _, p := range test.recorded { + recorder.RecordResource("pods", p) + } + + // the key is derived from the most recently recorded pod, since the UID is part of it + last := test.recorded[len(test.recorded)-1] + key := monitorapi.InstanceKey{ + Namespace: last.Namespace, + Name: last.Name, + UID: string(last.UID), + } + + stored, ok := recorder.CurrentResourceState()["pods"][key] + if !ok { + t.Fatalf("no resource recorded for %v", key) + } + annotations := stored.(*corev1.Pod).Annotations + + if actual := annotations[monitorapi.ObservedUpdateCountAnnotation]; actual != test.expectedUpdateCount { + t.Errorf("expected update count %q, got %q", test.expectedUpdateCount, actual) + } + if actual := annotations[monitorapi.ObservedRecreationCountAnnotation]; actual != test.expectedRecreateCount { + t.Errorf("expected recreation count %q, got %q", test.expectedRecreateCount, actual) + } + }) + } +} + +// TestRecordResourceDoesNotMutateInput ensures the caller's object is left alone. Callers hand in +// objects straight from an informer cache, so annotating them in place would corrupt the cache. +func TestRecordResourceDoesNotMutateInput(t *testing.T) { + recorder := NewRecorder() + + first := pod("uid-1") + recorder.RecordResource("pods", first) + if first.Annotations != nil { + t.Errorf("expected the recorded object to be unmodified, got annotations %v", first.Annotations) + } + + second := pod("uid-1") + recorder.RecordResource("pods", second) + if second.Annotations != nil { + t.Errorf("expected the recorded object to be unmodified, got annotations %v", second.Annotations) + } +}