Modify policy handler mechanism #1102
Conversation
…cy from the source interface
Juanadelacuesta
force-pushed
the
core-monitoring-2
branch
from
5dcc39f to
b965e2d
Compare
Juanadelacuesta
changed the title
Juanadelacuesta
force-pushed
the
core-monitoring-2
branch
3 times, most recently
from
8160551 to
5be5f65
Compare
Juanadelacuesta
force-pushed
the
core-monitoring-2
branch
3 times, most recently
from
2e43312 to
9ff8605
Compare
Juanadelacuesta
force-pushed
the
core-monitoring-2
branch
from
9ff8605 to
e88317f
Compare
There was a problem hiding this comment.
This looks great @Juanadelacuesta! I've left a few comments but only the one about the read lock is really blocking.
policy/manager.go
Outdated
| } | ||
| } | ||
|
|
||
| if pht := m.handlers[policyID]; pht != nil { |
There was a problem hiding this comment.
I think we need to hold a read lock on the m.handlers here in case of a concurrent ReloadSources call.
There was a problem hiding this comment.
I was thinking, do we even need the lock? Only the monitor will be modifying the handlers.
There was a problem hiding this comment.
The EnforceCooldown and ReloadSources methods (which I think get called from outside the monitor loop?) reads from the handlers. Mutating the map isn't atomic, so if you read it without the lock you can get weird stuff like a valid key but no value (which the runtime avoids by panicking with the message "fatal error: concurrent map read and map write").
Co-authored-by: Tim Gross <[email protected]>
Co-authored-by: Tim Gross <[email protected]>
Juanadelacuesta
force-pushed
the
core-monitoring-2
branch
from
d16d16d to
f30bebb
Compare
policy/manager.go
Outdated
|
|
||
| // Mark policy as must-keep so it doesn't get removed. | ||
| m.keep[policyID] = true | ||
| m.handlersLock.Lock() |
policy/manager.go
Outdated
| m.log.Trace("handler already exists", | ||
| "policy_id", policyID, "policy_source", policyIDs.Source) | ||
| pht.updates <- updatedPolicy | ||
| m.handlersLock.Unlock() |
There was a problem hiding this comment.
Do we need to wait until we've sent the update to unlock, or can we unlock immediately after the read? If we can unlock right after pht := m.handlers[policyID] there'd be less contention.
policy/manager.go
Outdated
| m.handlersLock.Lock() | ||
| defer m.handlersLock.Unlock() |
There was a problem hiding this comment.
Now that we're not sending the reload to the handlers in this method, do we still need this lock?
It looks like policySources never gets mutated once we create the Manager so we can use it without the lock. But if I've missed it somewhere then we'd probably need the lock on all the other policySources reads too.
There was a problem hiding this comment.
I think this lock was a left over of the previous implementation, lets remove it
This PR is a continuation of PR-1100
In that one, the source interface and all its implementations where updated to include a new non blocking query to get the latest version of a policy.
In this PR, we change the mechanism for monitoring and executing policies and move from using 2 blocking queries, one in the monitor for the overall number of policies and on in the policy handler to monitor changes on each specific policy, to having the manager monitor the total number of policies with a blocking query, and when it signals any changes, use the newly added function to get the latest version of any updated policy and feed id to the corresponding handler, that way the policy monitor is in charge of reading and updating any policy changes, while the policy handler just executes it.