Replace force-reconcile labels with Watches, Owns and status predicates#205
Replace force-reconcile labels with Watches, Owns and status predicates#205gshikhar2021 wants to merge 1 commit into
Conversation
Replace the force-reconcile label pattern with Watches(), Owns() relationships and custom status field predicates for controller chain triggering. - Add custom predicates for IsNewFilesDetected/Processed/Chunked/Embedded - Use Watches() with EnqueueRequestsFromMapFunc to watch upstream controllers - Add early-exit checks to prevent duplicate processing from Owns() events - Skip early-exit when force-reconcile label present (SQS) or jobs pending - Move predicates to For() to avoid filtering Owns() events - Fix NotFound error logging order
![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 implements a more efficient reconciliation trigger system by adding status flags (e.g., IsNewFilesDetected) to track file processing progress and using custom predicates with controller watches. This replaces the manual force-reconcile label logic and aims to prevent redundant processing. Feedback focused on the 'early exit' logic, which currently risks blocking legitimate specification updates because it doesn't account for generation changes. Reviewers also recommended refactoring the complex Reconcile functions to improve maintainability and ensuring the controller continues to poll when external dependencies are unhealthy.
| return ctrl.Result{ | ||
| RequeueAfter: 10 * time.Second, | ||
| }, nil | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
Removing the RequeueAfter here will cause the controller to stop reconciling if the configuration is not yet healthy. Since IsConfigCRHealthy depends on external state that might not trigger an event for this specific resource, the controller should continue polling until the configuration is ready.
| return ctrl.Result{}, nil | |
| return ctrl.Result{RequeueAfter: 10 * time.Second}, nil |
| isNewFileProcessed := false | ||
|
|
||
| // Skip early-exit checks if we have pending jobs to process | ||
| if len(documentProcessorCR.Status.Jobs) == 0 && len(documentProcessorCR.Status.Conditions) > 0 { |
There was a problem hiding this comment.
The early exit logic should only apply to external triggers (like Watches). If the resource's Generation has increased, it means the user has updated the spec, and the controller must proceed with reconciliation regardless of the upstream status to ensure the new desired state is applied.
| if len(documentProcessorCR.Status.Jobs) == 0 && len(documentProcessorCR.Status.Conditions) > 0 { | |
| if documentProcessorCR.Generation == documentProcessorCR.Status.LastAppliedGeneration && len(documentProcessorCR.Status.Jobs) == 0 && len(documentProcessorCR.Status.Conditions) > 0 { |
| isNewFileChunked := false | ||
|
|
||
| // Only do early-exit checks if this CR already has status initialized to ensure initial status set | ||
| if len(chunksGeneratorCR.Status.Conditions) > 0 { |
There was a problem hiding this comment.
This early exit logic blocks reconciliation even when the resource spec has changed. It should be restricted to cases where the local generation matches the last applied generation, ensuring that user-initiated updates are always processed.
| if len(chunksGeneratorCR.Status.Conditions) > 0 { | |
| if chunksGeneratorCR.Generation == chunksGeneratorCR.Status.LastAppliedGeneration && len(chunksGeneratorCR.Status.Conditions) > 0 { |
| embedded := false | ||
|
|
||
| // Only do early-exit checks if this CR already has status initialized to ensure initial status set | ||
| if len(vectorEmbeddingsGeneratorCR.Status.Conditions) > 0 { |
There was a problem hiding this comment.
The early exit check prevents reconciliation of spec changes if the upstream ChunksGenerator is not in a specific state. Adding a check for Generation == LastAppliedGeneration ensures that the controller remains responsive to its own configuration changes.
| if len(vectorEmbeddingsGeneratorCR.Status.Conditions) > 0 { | |
| if vectorEmbeddingsGeneratorCR.Generation == vectorEmbeddingsGeneratorCR.Status.LastAppliedGeneration && len(vectorEmbeddingsGeneratorCR.Status.Conditions) > 0 { |
| isNewFileDetected := false | ||
| hasForceReconcileLabel := controllerutils.HasForceReconcileLabel(unstructuredDataProductCR) | ||
|
|
||
| if !hasForceReconcileLabel { |
There was a problem hiding this comment.
The early exit logic here should include a check for generation changes to avoid blocking user updates. Additionally, this block is becoming quite large and repetitive; consider refactoring the child status checks into a helper function to improve readability.
| if !hasForceReconcileLabel { | |
| if !hasForceReconcileLabel && unstructuredDataProductCR.Generation == unstructuredDataProductCR.Status.LastAppliedGeneration { |
| // +kubebuilder:rbac:groups=operator.dataverse.redhat.com,namespace=unstructured-controller-namespace,resources=documentprocessors/status,verbs=get;update;patch | ||
| // +kubebuilder:rbac:groups=operator.dataverse.redhat.com,namespace=unstructured-controller-namespace,resources=documentprocessors/finalizers,verbs=update | ||
|
|
||
| //nolint:gocyclo |
There was a problem hiding this comment.
Adding nolint:gocyclo suggests that the Reconcile function has become too complex. Instead of suppressing the linter, consider refactoring the logic (e.g., the early exit checks and job reconciliation) into separate helper methods to improve maintainability.
| // +kubebuilder:rbac:groups=operator.dataverse.redhat.com,namespace=unstructured-controller-namespace,resources=unstructureddataproducts/status,verbs=get;update;patch | ||
| // +kubebuilder:rbac:groups=operator.dataverse.redhat.com,namespace=unstructured-controller-namespace,resources=unstructureddataproducts/finalizers,verbs=update | ||
|
|
||
| //nolint:gocyclo |
There was a problem hiding this comment.
Instead of suppressing the gocyclo linter, consider refactoring the Reconcile function. The newly added early exit logic and the multiple CreateOrUpdate calls can be moved to helper methods, which would improve maintainability and testability.
2 participants
Replace the force-reconcile label pattern with Watches(), Owns() relationships
and custom status field predicates for controller chain triggering.