Refactor Registry internals to make concurrent access safe #1115
Conversation
Yes, this is correct. Main idea of the
Entities inside |
Perfect, I think that's compatible with the approach taken in this PR in that case. |
|
Meta: I understand that the project prefers commits squashed. I tend to incrementally commit to address PR feedback, so that the feedback-inspired changes are visible at the commit level while the PR is under review, and then squash to one commit before merging. Does that work, or do you prefer squashing aggressively during the review process? Thanks! |
|
BTW, I see that the project seems to rely on end to end behavioral tests. I was wondering about dropping in some very simple go tests to at least exercise the race detector over simple operations on these types that I'm adding synchronization to. What are your thoughts on that? It seems like it might be the very first such golang test, which gives me some pause as I assume I'd have to make some changes to the CI configuration to ensure that said golang tests get executed. Not trying to add scope creep here, but wanted to at least make explicit that I thought about this and don't mind adding such test coverage if you all are on-board. Edit: I went ahead and pushed an example of the kind of test I'm describing, in which very basic operations are tested through n>1 (=2) goroutines with some synchronization that ensures they actually begin their workloads in parallel. I didn't do anything to "wire it in" to the repo's CI, but it is at least executable in GoLand (and passing). Hope that is a helpful supplement to my lukewarm proposal to add such a test, but again I don't feel strongly about it if it's way too out of line with the rest of the repo's testing story (in which case, no problem if you all want it dropped from the PR). |
No need to squash immediately, this can be done once at the very end |
The intention is to cover the whole functionality (or at least as much as possible) with e2e tests. Tests are typically added along with new functionality. However, in case any strange / corner case behavior is found later, tests are modified to cover it. Generally speaking, successful run of the test suite gives good hope that the operator can do most of the typical use-case scenarios.
Yes, sure. Tests covering any functionality (new and already existing, and especially complex scenarios) are always welcome. |
| // NB: These tests mostly exist to exercise synchronization and detect regressions related to them via the | ||
| // Golang race detector. See: https://go.dev/blog/race-detector | ||
| // In short, add -race to the go test flags when running this. | ||
| func Test_Registry_BasicOperations_ConcurrencyTest(t *testing.T) { |
There was a problem hiding this comment.
@sunsingerus here's the minimal (not assertion-heavy) go test I mentioned. its purpose is to exercise the synchronization primitives and detect any regressions related to concurrency (i.e., via -race)
|
Squashing this as I await feedback and get started on a third that actually touches the reconciliation logic |
Background: #1109 Signed-off-by: Zach Cross <zcross@chronosphere.io>
Background: #1109
Related PR(s):
This adds simple synchronization (
sync.RWMutex) to accesses of the mutable state withinRegistry.It also restructures the data to use a map per entity type, rather than a slice, replacing some linear searches of the data.
As recommended by @sunsingerus , I'm starting here with the Registry and planning on moving onto CHI status (and probably the announcer, at least with a slight change to its builder pattern for chaining log line inputs).
For me, the most pressing open question is whether my read of the code using Registry is semi-accurate: it seems that it is used for internal book keeping, and that individual
ObjectMetainstances are read in theWalk...functions without direct mutation. As I write that, I'm considering whether that was part of the intent of the original code passing these structs by value rather than by pointer.Meta: I ran into some issues executing the e2e tests locally (have not fixed yet) and some other simpler (fixed) problems developing this branch on an M1 Mac. I don't want to pull the fixes I made locally into the discussion here, but I won't be shocked if the e2e tests reveal some issue I'm not yet aware of due to this dev environment issue.
Important items to consider before making a Pull Request
Please check items PR complies to:
next-releasebranch, not intomasterbranch1. More info