Create SQSInformer from UnstructuredDataProduct reconcile loop#132
Create SQSInformer from UnstructuredDataProduct reconcile loop#132concaf wants to merge 1 commit intoredhat-data-and-ai:mainfrom
Conversation
SQSInformer CR is now created/updated by the UnstructuredDataProduct reconciler instead of requiring manual creation. An optional sqsInformerConfig field (with queueURL) is added to the UDP spec. Owner reference is set on the SQSInformer so it is automatically garbage-collected when the parent UDP is deleted. If the config is later removed from the spec, the existing SQSInformer is cleaned up. Closes redhat-data-and-ai#110
![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 SQSInformerConfig to the UnstructuredDataProduct API, enabling the controller to automatically manage the lifecycle of SQSInformer resources. The controller now handles the creation, update, and deletion of these resources based on the specification, and E2E tests have been updated accordingly. Feedback includes a suggestion to simplify the resource cleanup logic by removing an unnecessary Get call and a recommendation to address inconsistencies in owner references for other sub-resources in a follow-up PR.
| sqsInformerCR.Spec = operatorv1alpha1.SQSInformerSpec{ | ||
| QueueURL: unstructuredDataProductCR.Spec.SQSInformerConfig.QueueURL, | ||
| } | ||
| return controllerutil.SetControllerReference(unstructuredDataProductCR, sqsInformerCR, r.Scheme) |
There was a problem hiding this comment.
The PR description states that setting the owner reference follows the same pattern as DocumentProcessor, ChunksGenerator, and VectorEmbeddingsGenerator. However, those resources currently lack owner references in the reconciler (see lines 119-174). While adding it here for SQSInformer is correct and recommended for proper garbage collection, this creates an inconsistency. It is highly recommended to also add owner references to the other sub-resources in a follow-up PR to prevent resource leaks when an UnstructuredDataProduct is deleted.
| existingSQSInformer := &operatorv1alpha1.SQSInformer{} | ||
| sqsInformerKey := client.ObjectKey{ | ||
| Name: dataProductName, | ||
| Namespace: unstructuredDataProductCR.Namespace, | ||
| } | ||
| if err := r.Get(ctx, sqsInformerKey, existingSQSInformer); err == nil { | ||
| if err := r.Delete(ctx, existingSQSInformer); err != nil { | ||
| logger.Error(err, "failed to delete SQSInformer CR") | ||
| return r.handleError(ctx, unstructuredDataProductCR, err) | ||
| } | ||
| logger.Info("SQSInformer CR deleted (config removed)") | ||
| } else if !apierrors.IsNotFound(err) { | ||
| logger.Error(err, "failed to check for existing SQSInformer CR") | ||
| return r.handleError(ctx, unstructuredDataProductCR, err) | ||
| } |
There was a problem hiding this comment.
The manual cleanup of the SQSInformer can be simplified by calling Delete directly with a pre-populated object and checking for NotFound errors. This avoids an unnecessary Get request to the API server.
existingSQSInformer := &operatorv1alpha1.SQSInformer{
ObjectMeta: metav1.ObjectMeta{
Name: dataProductName,
Namespace: unstructuredDataProductCR.Namespace,
},
}
if err := r.Delete(ctx, existingSQSInformer); err != nil {
if !apierrors.IsNotFound(err) {
logger.Error(err, "failed to delete SQSInformer CR")
return r.handleError(ctx, unstructuredDataProductCR, err)
}
} else {
logger.Info("SQSInformer CR deleted (config removed)")
}1 participant
Summary
sqsInformerConfig(withqueueURL) to theUnstructuredDataProductspecSQSInformerCR when the config is provided, following the same pattern as DocumentProcessor, ChunksGenerator, and VectorEmbeddingsGeneratorSetControllerReference) on the SQSInformer so it is garbage-collected when the parent UDP is deletedCloses #110
Test plan
sqsInformerConfigcreates the SQSInformer CRsqsInformerConfig.queueURLupdates the SQSInformer CRsqsInformerConfigfrom the spec deletes the SQSInformersqsInformerConfigdoes not create an SQSInformer