[WIP] Add Targets Config Map Reconciler #914
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ian-mi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ian-mi
force-pushed
the
targets-reconciler
branch
from
7f81d18 to
29e3d34
Compare
|
The following is the coverage report on the affected files.
|
| targetsName: targetsName, | ||
| targetsNS: targetsNS, | ||
| updateCh: make(chan updateReq), | ||
| configMaps: client.CoreV1().ConfigMaps(targetsNS), |
There was a problem hiding this comment.
Why use a real client rather than a configmap lister here?
There was a problem hiding this comment.
The reconciler needs to be able to update the configmap as well.
| return nil | ||
| } | ||
|
|
||
| func (r *targetsReconciler) reconcile(ctx context.Context) { |
There was a problem hiding this comment.
I'm finding this method difficult to read and understand without comments. Can you add some comments explaining the flow?
It's not clear to me how this approach differs from using a mutex (but probably it does):
// pseudocode
func UpdateBrokerConfig(b) error {
mtxx.Lock()
defer mtx.Unlock()
// TODO update targets
return writeTargets()
}There was a problem hiding this comment.
This approach uses a single goroutine to guard access to state. This achieves more or less the same thing as a mutex but has the advantage that code executed within this goroutine does not need to worry about locking. Since all communication happens over channels it easy for the main goroutine to select on a resync channel for example without worrying about locking. Channels also allow for cancellation where mutexes do not.
The end result is that this reconciler combines the communication of state changes and serialization of config updates that was provided by the old targets updater goroutine with the protection of mutable state provided by in memory targets data structure.
There was a problem hiding this comment.
I'm likely biased. But I really don't see a point of forcing channel adoption in this case. This approach here seems to leak a lot of internals of targets config while IMO the reconciler should only worry about reading/writing the configmap data. It would be much easier for me to agree with such a change if update logic can be better wrapper within the targets config lib.
| return r | ||
| } | ||
|
|
||
| func (r *targetsReconciler) UpdateBrokerConfig(ctx context.Context, b *config.Broker) error { |
There was a problem hiding this comment.
I think the complexity here is aimed at allowing multiple threads to call this method concurrently and get an error back (which the previous implementation didn't allow).
There was a problem hiding this comment.
Yes, this allows the serialization and batching of writes across multiple reconcile goroutines while providing a blocking interface to the caller.
|
I intend to add some polish before marking this PR as ready to merge but first wanted to make sure we agree on the high-level design. |
|
@ian-mi: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Closing as the BrokerCell reconciler should make this unnecessary barring possible scaling issues. |
Closes #830
Proposed Changes