Repository navigation
fix flaky query evictor tests - #7817
SungJin1212 wants to merge 1 commit into
Conversation
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
sandy2008
left a comment
sandy2008
left a comment
There was a problem hiding this comment.
Thanks for chasing this flake. I don't think polling fixes it, though; details inline.
|
|
||
| assert.Equal(t, float64(1), promtest.ToFloat64(evictor.evictionsTotal.WithLabelValues(string(resource.CPU)))) | ||
| // The eviction callback runs before the counter is incremented, so poll for it. | ||
| test.Poll(t, time.Second, float64(1), func() any { |
There was a problem hiding this comment.
This removes the "Not equal" failure, but the same tests still fail at about the same rate, now with panic: close of closed channel from registerTestQuery's callback (evictor_test.go:67): 3 panics in ~530 -race -count=100 runs here, vs 0 panics and 4 "Not equal" failures in 600 runs at the merge-base. In the racy runs the first poll sees 0 and sleeps d/100 = 10ms, exactly one CheckInterval; with cooldown 0 the evictor ticks again, FindHeaviest returns the same query because nothing deregistered it, and Cancel() closes evicted a second time. Same at line 214.
To remove the race, the evicted query has to stop being a candidate: deregister it in the callback the way engine_wrapper.go does in production, or have the evictor retire victims before cancelling them (that's what my #7616 does in evictor.go, along with moving the Inc() before Cancel()). A sync.Once around the close alone would just turn the panic into a counter of 2 or 4.
What this PR does:
This PR fixes flaky query evictor tests by adding polling.
The evictor could close the
evictedchannel before incrementingevictionsTotal, sowaitEvictedcan return while the counter is still 0.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags