remote-write: fix race condition between ApplyConfig and Notify #13135
Conversation
machine424
requested review from
csmarchbanks,
cstyan,
bwplotka and
tomwilkie
as code owners
between Storage.Notify() and Storage.ApplyConfig() see prometheus#12747 Signed-off-by: machine424 <ayoubmrini424@gmail.com>
|
fix pushed, see test failing without https://github.com/prometheus/prometheus/actions/runs/6849888288/job/18622973182?pr=13135 |
storage/remote/storage.go
Outdated
| @@ -77,6 +77,9 @@ func NewStorage(l log.Logger, reg prometheus.Registerer, stCallback startTimeCal | |||
| } | |||
|
|
|||
| func (s *Storage) Notify() { | |||
| s.mtx.Lock() | |||
There was a problem hiding this comment.
s.rws.mtx.Lock is sufficient as s.rws.ApplyConfig called from ApplyConfig is the one that really conflicts with Notify, but I didn't see any use of inner locks like that.
There was a problem hiding this comment.
I think it's a better idea if it's locked inside rws. I don't see any other cases where inner fields of rws are touched by Storage.
So the loop for Notify, including lock, should be in a method on WriteStorage, and this method just forwards to that.
…with Storage.ApplyConfig() Signed-off-by: machine424 <ayoubmrini424@gmail.com>
bboreham
changed the title
|
@machine424 thank you! |
|
Are there plans to add this patch to the 2.45 LTS version? We've been dealing with sporadic crashes hoping this would eventually get included. |
|
I'm not aware of how the LTS releases are being updated, @roidelapluie @jesusvazquez is there a backport process? |
|
Gentle bump on this @roidelapluie @jesusvazquez |
|
Prometheus LTS is described here: https://prometheus.io/docs/introduction/release-cycle/ The process to backport is the same as for a regular release. See for instance #13450, #13779. |
…etheus#13135 Signed-off-by: machine424 <ayoubmrini424@gmail.com>
…etheus#13135 Signed-off-by: machine424 <ayoubmrini424@gmail.com>
between Storage.Notify() and Storage.ApplyConfig()
see #12747
I'll push a fix, waiting for the test to fail in CI: DONE
Ready for review.
fixes #12747