Conversation
There was a problem hiding this comment.
Pull request overview
Fixes two logic bugs reported in #822 in the SolrCloud controller reconciliation flow and cluster replica-balancing utility, improving correctness for service cleanup gating and replica balancing edge cases.
Changes:
- Fix
cleanupUnconfiguredServices()annotation gating to avoid nil annotation map access and align behavior with the function’s intent. - Align
BalanceReplicasForCluster()behavior with its comment by skipping balancing when replicas are<= 1.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| controllers/util/solr_scale_util.go | Adjusts replica-count condition to treat 1-pod clusters as already “balanced”. |
| controllers/solrcloud_controller.go | Corrects service cleanup condition to properly handle missing/nil pod annotations. |
Comments suppressed due to low confidence (1)
controllers/solrcloud_controller.go:692
- Please add a regression test for cleanupUnconfiguredServices covering pods with nil/missing service-type annotations, since this bug fix changes the gating condition that decides whether service cleanup can proceed. Without a test, future refactors could reintroduce the nil-annotation logic issue.
if pod.Annotations != nil && pod.Annotations[util.ServiceTypeAnnotation] != "" {
if onlyServiceTypeInUse != pod.Annotations[util.ServiceTypeAnnotation] {
// Only remove services if all pods are using the same, and configured, type of service.
// Otherwise, we are in transition between service types and need to wait to delete anything.
return
}
} else {
// If we have a pod missing this annotation, then it has not been fully updated to a supported Operator version.
// We don't have the information, so assume both serviceTypes are in use, and don't remove anything.
return
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If the Cloud has 1 or zero pods, there is no reason to balance replicas. | ||
| if statefulSet.Spec.Replicas == nil || *statefulSet.Spec.Replicas < 1 { | ||
| if statefulSet.Spec.Replicas == nil || *statefulSet.Spec.Replicas <= 1 { | ||
| balanceComplete = true |
There was a problem hiding this comment.
Consider adding a focused unit/integration test that covers the new <= 1 replicas early-exit path (e.g., Spec.Replicas=1 returns balanceComplete=true and does not attempt any async request). This is a behavior change that could regress silently since this util file currently has no direct test coverage.
Fixes #822