Increase controller concurrency for parallel reconciliation - #273
Conversation
Move per-reconcile state (fileStore, inputPath, outputPath, doclingConfig) from reconciler struct fields to local variables and function parameters, eliminating data races when multiple reconcile goroutines run concurrently. Set MaxConcurrentReconciles to 10 (configurable via --max-concurrent-reconciles) for all controllers except ControllerConfig (which manages singleton global state and stays at 1). This allows up to 10 different CRs of the same kind to reconcile in parallel — controller-runtime guarantees that the same object is never reconciled concurrently regardless of this setting.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the performance and scalability of the operator by enabling parallel reconciliation for most controllers. By removing shared state from controller structs and passing it as function parameters, the reconcilers are now thread-safe, allowing multiple resources to be processed simultaneously without data races. The change also introduces a configurable concurrency limit, providing flexibility for different deployment environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces concurrent reconciliation across multiple controllers by adding a new max-concurrent-reconciles CLI flag and configuring the controller options accordingly. To support safe concurrent execution, the reconciler structs have been refactored to be stateless by removing shared fields (such as fileStore, inputPath, outputPath, and doclingConfig) and passing them as local variables through method arguments instead. Feedback on these changes highlights a potential data race on package-level global variables in the SourceCrawler controller when concurrent reconciliation is enabled. Additionally, it is recommended to use CRD-level validation markers (like MinItems=1 or CEL) to prevent potential out-of-bounds panics when accessing DependsOn[0] in the ChunksGenerator, DocumentProcessor, and VectorEmbeddingsGenerator controllers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func (r *SourceCrawlerReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| func (r *SourceCrawlerReconciler) SetupWithManager(mgr ctrl.Manager, maxConcurrentReconciles int) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles}). |
There was a problem hiding this comment.
Enabling concurrent reconciliation (MaxConcurrentReconciles > 1) introduces a potential data race on package-level global variables such as GoogleDriveControllerCfg, LDAPClient, CacheClient, cacheDirectory, and dataStorageBucket. These globals are read concurrently in Reconcile and helper functions (like buildGDriveSource), but can be mutated dynamically by the ControllerConfigReconciler in a separate goroutine. To ensure complete thread safety, consider wrapping these shared configurations in a thread-safe registry/provider or protecting them with a sync primitive (e.g., sync.RWMutex).
| r.inputPath = unstructured.StagePath(pipelineName, chunksGeneratorCR.Spec.DependsOn[0].Name) | ||
| r.outputPath = unstructured.StagePath(pipelineName, chunksGeneratorCR.Spec.StageName) | ||
| filePaths, err := r.fileStore.ListFilesInPath(ctx, r.inputPath) | ||
| inputPath := unstructured.StagePath(pipelineName, chunksGeneratorCR.Spec.DependsOn[0].Name) |
There was a problem hiding this comment.
Instead of implementing manual validation in the controller to check if DependsOn is empty, rely on Kubernetes API validation markers (such as +kubebuilder:validation:MinItems=1 or CEL validation markers) at the CRD level. This allows the API server to reject invalid configurations before they reach the controller, preventing potential out-of-range panics when accessing DependsOn[0].
References
- Rely on Kubernetes API validation markers to enforce constraints on configuration fields, rather than implementing redundant manual checks within the controller logic.
- Use CEL validation markers at the CRD level to enforce type-configuration pairings and prevent nil pointer dereferences.
| if err != nil { | ||
| return r.handleError(ctx, documentProcessorCR, err) | ||
| } | ||
| inputPath := unstructured.StagePath(pipelineName, documentProcessorCR.Spec.DependsOn[0].Name) |
There was a problem hiding this comment.
Instead of implementing manual validation in the controller to check if DependsOn is empty, rely on Kubernetes API validation markers (such as +kubebuilder:validation:MinItems=1 or CEL validation markers) at the CRD level. This allows the API server to reject invalid configurations before they reach the controller, preventing potential out-of-range panics when accessing DependsOn[0].
References
- Rely on Kubernetes API validation markers to enforce constraints on configuration fields, rather than implementing redundant manual checks within the controller logic.
- Use CEL validation markers at the CRD level to enforce type-configuration pairings and prevent nil pointer dereferences.
| r.inputPath = unstructured.StagePath(pipelineName, vectorEmbeddingsGeneratorCR.Spec.DependsOn[0].Name) | ||
| r.outputPath = unstructured.StagePath(pipelineName, vectorEmbeddingsGeneratorCR.Spec.StageName) | ||
| filePaths, err := r.fileStore.ListFilesInPath(ctx, r.inputPath) | ||
| inputPath := unstructured.StagePath(pipelineName, vectorEmbeddingsGeneratorCR.Spec.DependsOn[0].Name) |
There was a problem hiding this comment.
Instead of implementing manual validation in the controller to check if DependsOn is empty, rely on Kubernetes API validation markers (such as +kubebuilder:validation:MinItems=1 or CEL validation markers) at the CRD level. This allows the API server to reject invalid configurations before they reach the controller, preventing potential out-of-range panics when accessing DependsOn[0].
References
- Rely on Kubernetes API validation markers to enforce constraints on configuration fields, rather than implementing redundant manual checks within the controller logic.
- Use CEL validation markers at the CRD level to enforce type-configuration pairings and prevent nil pointer dereferences.
| var probeAddr string | ||
| var secureMetrics bool | ||
| var watchNamespace string | ||
| var maxConcurrentReconciles int |
There was a problem hiding this comment.
figure out exponential backoff for all services
1 participant
Summary
fileStore,inputPath,outputPath,doclingConfig) from struct fields to local variables and function parameters, eliminating data races when multiple goroutines reconcile concurrentlyMaxConcurrentReconcilesto 10 (default) for all controllers except ControllerConfig (stays at 1 since it manages singleton global state)--max-concurrent-reconcilesCLI flag to tune concurrency without rebuildingWhy this is safe
MaxConcurrentReconciles— the workqueue deduplicates by namespace/name keyStatusPatch), minimizing conflict surface across different objectsWhat changed per controller
MaxConcurrentReconcilesdoclingConfig,fileStore,inputPath,outputPathfileStorefileStore,inputPath,outputPathfileStore,inputPath,outputPathfileStoreTest plan
go build ./...passesgo test ./...passes--max-concurrent-reconciles=10and verify multiple CRs of the same kind reconcile in parallel--max-concurrent-reconciles=1and verify behavior matches current production (no regressions)