Copilot commented on code in PR #888:
URL: https://github.com/apache/solr-operator/pull/888#discussion_r4503280315
##########
controllers/solrcloud_controller.go:
##########
@@ -1206,6 +1212,34 @@ func (r *SolrCloudReconciler)
storageClassAllowsExpansion(ctx context.Context, c
return allowed, className, nil
}
+// reconcileDeletion handles a SolrCloud that is being deleted, by running the
storage finalizer logic.
+func (r *SolrCloudReconciler) reconcileDeletion(ctx context.Context, cloud
*solrv1beta1.SolrCloud, logger logr.Logger) (ctrl.Result, error) {
+ if !util.ContainsString(cloud.ObjectMeta.Finalizers,
util.SolrStorageFinalizer) {
+ return reconcile.Result{}, nil
+ }
+
+ statefulSet := &appsv1.StatefulSet{}
+ err := r.Get(ctx, types.NamespacedName{Name: cloud.StatefulSetName(),
Namespace: cloud.Namespace}, statefulSet)
+ if errors.IsNotFound(err) {
+ // The StatefulSet may already be gone, e.g. with foreground
deletion.
+ // Its PVCs still carry the selector labels the operator always
generates.
+ statefulSet.Spec.Selector = &metav1.LabelSelector{MatchLabels:
cloud.SharedLabels()}
+ } else if err != nil {
+ return reconcile.Result{}, err
+ }
+
+ // Do not reconcile the storage finalizer unless we have PVC Labels
that we know the Solr data PVCs are using.
+ // Otherwise it will delete all PVCs possibly
+ if statefulSet.Spec.Selector == nil ||
len(statefulSet.Spec.Selector.MatchLabels) == 0 {
+ return reconcile.Result{}, nil
+ }
+ if err = r.reconcileStorageFinalizer(ctx, cloud, statefulSet, logger);
err != nil {
Review Comment:
The new regression spec creates no PVCs, and envtest does not run the
StatefulSet controller to create them, so it only tests cleanup with an empty
claim list. Add explicit PVC fixtures for both an existing and an absent
StatefulSet, asserting that deletion with the ConfigMap missing removes the
cloud's data claims while retaining unrelated claims, including one with only
the `solr-cloud` label.
##########
controllers/solrcloud_controller.go:
##########
@@ -1206,6 +1212,34 @@ func (r *SolrCloudReconciler)
storageClassAllowsExpansion(ctx context.Context, c
return allowed, className, nil
}
+// reconcileDeletion handles a SolrCloud that is being deleted, by running the
storage finalizer logic.
+func (r *SolrCloudReconciler) reconcileDeletion(ctx context.Context, cloud
*solrv1beta1.SolrCloud, logger logr.Logger) (ctrl.Result, error) {
+ if !util.ContainsString(cloud.ObjectMeta.Finalizers,
util.SolrStorageFinalizer) {
+ return reconcile.Result{}, nil
+ }
+
+ statefulSet := &appsv1.StatefulSet{}
+ err := r.Get(ctx, types.NamespacedName{Name: cloud.StatefulSetName(),
Namespace: cloud.Namespace}, statefulSet)
+ if errors.IsNotFound(err) {
+ // The StatefulSet may already be gone, e.g. with foreground
deletion.
+ // Its PVCs still carry the selector labels the operator always
generates.
+ statefulSet.Spec.Selector = &metav1.LabelSelector{MatchLabels:
cloud.SharedLabels()}
Review Comment:
This fallback is broader than the generated StatefulSet selector:
`GenerateStatefulSet` also adds `technology=solr-cloud`
(`controllers/util/solr_util.go:107–110`), whereas `SharedLabels()` only
supplies `solr-cloud=<name>`. Once the StatefulSet is gone, cleanup will
therefore delete non-Solr PVCs carrying the same cloud label that the
live-selector path would exclude. Include the technology label in the fallback
to preserve the existing deletion scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
