Repository navigation
Conversation
A container's log streamer shares one tags map between the stdout and stderr destinations, every WithSource child and the log rate limiter. Each destination guards it with its own processLock and the rate limiter metrics goroutine uses no lock at all, while UpdateTags mutated the map in place. Concurrent flushes on another destination therefore raced with UpdateTags and rep crashed with "fatal error: concurrent map read and map write" / "concurrent map iteration and map write" in SendAppLog. Replace the shared map with a copy-on-write tagStore: UpdateTags publishes a new map atomically and readers use an immutable snapshot. The regression test uses an ingress client that reads the tags map like diego-logging-client/go-loggregator do; the counterfeiter fake only stores the reference, which is why the existing race test did not detect this. Regression from 83f58c5 ("Support update of metric tags for running LRPs").
…ning
storeNode.logStreamer is only set in Run and gardenContainer only in
Create, so an UpdateLRPInstance with metric tags for an instance that was
claimed but not yet running dereferenced nil ("http: panic serving ...
invalid memory address or nil pointer dereference" at storenode.go).
Skip the log streamer and garden property updates when they do not exist
yet. Create and Run hold the same opLock and build from n.info.LogConfig,
which Update has already changed, so the new tags still take effect.
Regression from 83f58c5 ("Support update of metric tags for running LRPs").
cf-foundation-community-automation
Bot
moved this to Inbox
in Application Runtime Platform Working Group
ameowlia
left a comment
ameowlia
left a comment
There was a problem hiding this comment.
My AI has two suggestions for further testing. But other than that it all looks good to me.
[MINOR] — executor/depot/containerstore/containerstore_test.go, storenode.go:759 / No test for Update before Create (gardenContainer nil) — Test Coverage
The new spec covers only "created, not run" (logStreamer nil, gardenContainer set). Update in the Reserved/Initialized state (gardenContainer nil) is never tested, and nothing checks that Create sends the updated log_config property. By reading the code it does: Create copies n.info at :202 under opLock. Fix: add a spec that calls Update before Create, then asserts no panic and that the log_config property sent to garden at Create contains the new tags.
[MINOR] — executor/depot/log_streamer/tag_store_test.go / tagStore isolation paths not tested — Test Coverage
Two paths have no test: Store(nil), and a caller mutating its map after Store. The second is the isolation that defensive copying is meant to give. Fix: add unit specs for Store(nil) and for mutating the map after Store, asserting that Load is unaffected.
…tion - containerstore: Update in the Reserved and Initialized states does not panic, and Create sends the updated tags to garden in the log_config property. - tagStore: Store(nil) results in an empty map, and changing the caller's map after Store or storing new tags does not change what was stored or already loaded.
|
Done |
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Application Runtime Platform Working Group
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Application Runtime Platform Working Group
Summary
Fixes two rep crashes in the metric tag update path for running LRPs. Both were introduced in 83f58c5 (#662).
fatal error: concurrent map read and map write/concurrent map iteration and map writeA container's log streamer shares one tags map between the stdout and stderr destinations, every
WithSourcechild (CELL, HEALTH, PROXY, ...) and the log rate limiter.UpdateTagschanged that map in place while holding only one destination'sprocessLock. The other destinations read it under their own locks, and the rate limiter's metrics goroutine reads it with no lock. rep crashes inSendAppLog/WithEnvelopeTags, called fromstreamDestination.flush.The tags now live in a copy-on-write
tagStore(atomic.Pointer[map[string]string]) shared by all of them.UpdateTagsswaps in a new map, and readers get one that is never changed afterwards.nil pointer dereferenceinstoreNode.UpdatelogStreameris only set inRunandgardenContaineronly inCreate, so updating the metric tags of a container that isn't running yet panics. net/http recovers the panic, but the update request fails.Updatenow skips both calls when the field is nil.CreateandRunpick up the new tags fromn.info.LogConfig.Tests
Both specs fail on
develop(data race / panic) and pass with this change../scripts/test-in-docker.bashpasses for MySQL and Postgres.Backward Compatibility
Breaking Change? No
No API, config or behaviour changes. Tag updates still reach every log source of the container.