TargetLoader + TargetReconciler architecture#52
Open
denyost wants to merge 92 commits into
Open
Conversation
Deploying gnmic-operator with
|
| Latest commit: |
3280229
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://84bbb1ef.gnmic-operator2.pages.dev |
| Branch Preview URL: | https://feature-limit-target-loading.gnmic-operator2.pages.dev |
…ic/operator into feature/limit-target-loading
…com/gnmic/operator into feature/limit-target-loading
Collaborator
Author
|
@karimra can you please review this PR and merge if ok. |
karimra
requested changes
May 13, 2026
| cleanup := func() { | ||
| cancel() | ||
| r.DiscoveryRegistry.Unregister(key) | ||
| close(targetChannel) |
Collaborator
There was a problem hiding this comment.
you should not close the channel here. cleanup is called by 2 different goroutines when they exist (that's panic closing of a closed channel) and if you enable push, any inflight webhook will send into a closed channel.
Collaborator
Author
There was a problem hiding this comment.
fix:
- removed
close(targetChannel)-> now only context cancellation is used - cleanup function is no longer called in the loader goroutine -> context cancellation and registry cleanup are now triggered only when the processor can no longer handle messages or when the loader configuration was invalid
| // Delete buffered events that will be current with new snapshot | ||
| m.deferredEvents = nil | ||
|
|
||
| m.collectSnapshot(chunk, logger) |
| ) | ||
|
|
||
| // NewLoader creates a loader by name | ||
| func NewLoader(cfg core.CommonLoaderConfig, spec *gnmicv1alpha1.TargetSourceSpec) (core.Loader, core.CommonLoaderConfig, error) { |
Collaborator
There was a problem hiding this comment.
this function doesn't need to return the config it received.
Collaborator
Author
There was a problem hiding this comment.
fix: resolved pointer and return smells
- In
NewLoader, the decision whether to accept webhook pushes has to be persisted in the registry - Since the registry lives in the targetsource_controller, a pointer is used to update its configuration directly instead of returning it
- This allows the API endpoint to check if pushes from a given TargetSource should be accepted
…ng the message processor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces per target channel buffering with options for a snapshot- or event-based discovery approach. It also introduces a generic registry that is being used to communicate between the apiserver (webhook) and the message processor.
Core
internal/controller/discovery/core/message_interface.go: Discovery messages such as DiscoveryEvent and DiscoverySnapshot become typed unions.internal/controller/discovery/core/types.go: Adjusted types to support the DiscoveryMessage abstraction (e.g., DiscoveredTarget, DiscoverySnapshot, DiscoveryEvent, EventAction).Loaders
internal/controller/discovery/loaders/utils: Implements helper functions specific to loaders (likesend.go).Discovery
internal/controller/discovery/const.go: holds labels used in context with Kubernetes.internal/controller/discovery/registry.go: A generic implementation of a registry providing functions like NewRegistry, Register and Unregisterinternal/controller/discovery/loaders.go: Creates the specific loader based on the configured Spec withinTargetSource.internal/controller/discovery/message_processor.go: Updated to use the new DiscoveryMessage interface; handles both DiscoverySnapshot and DiscoveryEvent via type assertion/dispatch.internal/controller/discovery/client.go, which interacts with Kubernetes.Controller wiring & refactor
internal/controller/targetsource_controller.go: Reconciler refactor to improve clarity.