NO-JIRA: monitor: record observed update/recreation counts on the stored object - #31496
NO-JIRA: monitor: record observed update/recreation counts on the stored object#31496devapragadeesh wants to merge 1 commit into
Conversation
…red 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.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@devapragadeesh: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
ready-for-human-review
|
Hi @devapragadeesh. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Walkthrough
ChangesMonitor recorder updates
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: devapragadeesh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
RecorderWriter.RecordResourceis documented as:In practice neither
monitor.openshift.io/observed-update-countnormonitor.openshift.io/observed-recreation-countever reaches the stored object, so therecorded-resource artifacts carry no counts at all.
What's wrong
In
pkg/monitor/recorder.go,toStore := obj.DeepCopyObject()is taken before the annotations arecomputed, while
newMetadataismeta.Accessor(obj)— an accessor over the caller's object. Soevery
SetAnnotationscall writes into the argument and leavestoStoreuntouched.Two consequences:
reads an empty
observed-update-count,strconv.ParseIntfails, and the count is pinned at"1"forever — except it is never actually stored, so it reads back as
"".pkg/monitortestlibrary/monitoring_store.go(
AddFunc/UpdateFunc/DeleteFunc, lines 64/90/112) pass objects straight from an informercache. Writing annotations into them modifies shared cache state.
Separately, at line 110 the recreation count was read from the update-count key:
so on every repeat observation
observed-recreation-countwas overwritten with the update count.The fix
Take the metadata accessor over the stored copy, so the annotations land on the object that is
actually retained and the caller's object is left alone; and read the recreation count from its own
annotation.
The
if newMetadata == nilbranch is dropped:meta.Accessornever returns a nil accessor togetherwith a nil error, and the error case is already handled by the
panica few lines above, so thatbranch was unreachable.
How it regressed
git log -Spins this down precisely:debugging") introduced the feature with
newMetadata, _ := meta.Accessor(toStore)— the accessorwas correctly taken over the stored copy. The recreation count was read from the update-count key
from day one, though.
InstanceKey, so theaccessor was hoisted above the
DeepCopyObject()call and re-pointed atobj. That silently movedevery
SetAnnotationsoff the stored copy and onto the caller's object. The same commit put theUID into the key, which is what made the recreation-increment branch unreachable.
There is corroborating evidence checked into the repo: the fixture
pkg/monitortests/node/watchpods/podTest/simple/podData.jsonwas captured in March 2022, i.e. afterthe day-one recreation bug but before the May 2022 regression. It records
observed-update-count: "5"alongsideobserved-recreation-count: "4"— the recreation counttrailing the update count by exactly one, which is the precise signature of reading the recreation
count out of the update-count annotation.
Scope note
I deliberately did not change the UID-mismatch branch that increments the recreation count.
monitorapi.InstanceKeyincludes the UID, so a recreated object is stored under a different keyand the
existingMetadata.GetUID() != newMetadata.GetUID()comparison can never be true on asuccessful lookup. Making recreation counting actually work would mean changing the map key, which is
a design change rather than a bug fix — happy to follow up separately if maintainers want it.
Testing
Added
pkg/monitor/recorder_test.gowith table-driven cases covering the seeded counts, repeatedobservations incrementing the update count, a recreated pod being tracked under its own key, and that
RecordResourcedoes not mutate its argument.Against
mainthe new tests fail:With the fix applied, the whole unit suite is green:
go test ./pkg/...— all 58 packages passgo test -race ./pkg/monitor/...— passgo build ./...,go vet ./pkg/monitor/...,gofmt -l— cleanNo cluster is required for any of this.