Fix/reduced decision log locking #6859
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
There seems to be a race issue here. Will dig into it on Monday.
|
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
This reverts commit c502cf9. Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
johanfylling
force-pushed
the
fix/decision_log_locking_2
branch
from
e449f5f to
58b83b5
Compare
| // Make a local copy of the plugins's status. This is needed as locking the status for | ||
| // the status upload duration will block policy evaluation and result in | ||
| // increased latency for OPA clients | ||
| p.mtx.Lock() |
There was a problem hiding this comment.
This comment is still valid, isn't it?
There was a problem hiding this comment.
The status update is no longer protected by the same mutex that is used for logging, so I don't think policy evaluation is affected anymore.
| @@ -892,12 +890,9 @@ func (p *Plugin) oneShot(ctx context.Context) (ok bool, err error) { | |||
| continue | |||
| } | |||
|
|
|||
| p.mtx.Lock() | |||
| for _, event := range events { | |||
| p.encodeAndBufferEvent(event) | |||
There was a problem hiding this comment.
encodeAndBufferEvent is called in the Log method as well.
There was a problem hiding this comment.
Correct. encodeAndBufferEvent now does it's own locking, which is why we shouldn't also lock here.
There was a problem hiding this comment.
@johanfylling this seems fine. The thing we need to be sure about is previously encodeAndBufferEvent was an atomic operation and now it's not. We just need to ensure this doesn't have any unintended consequences and also the benefits outweigh any potential risk.
|
I think it should be fine that All tests pass, and I've done a fair bit of stress testing without seeing anything amiss. Do you have any specific additional tests we can run, or possible side effects to look out for in mind? |
Thanks! Nothing particular. Good to know about the testing that's gone behind these changes. |
Reducing amount of work performed inside global lock in decision log plugin.
Local tests show a marginal improvement (~3 percentage points) in contention time during load (~500req/s).