Skip to content

Commit 1fbc6aa

Browse files
janhoygerlowskijaHoustonPutman
authored
Remove unused ingress and services during reconcile (#674)
Co-authored-by: Jan Høydahl <[email protected]> Co-authored-by: Jason Gerlowski <[email protected]> Co-authored-by: Houston Putman <[email protected]>
1 parent 741a6c8 commit 1fbc6aa

10 files changed

+417
-97
lines changed

‎controllers/controller_utils_test.go

+22-3
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ func expectService(ctx context.Context, parentResource client.Object, serviceNam
241241
func expectServiceWithChecks(ctx context.Context, parentResource client.Object, serviceName string, selectorLables map[string]string, isHeadless bool, additionalChecks func(Gomega, *corev1.Service), additionalOffset ...int) *corev1.Service {
242242
service := &corev1.Service{}
243243
EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) {
244-
Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist")
244+
g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist")
245245

246246
g.Expect(service.Spec.Selector).To(Equal(selectorLables), "Service is not pointing to the correct Pods.")
247247

@@ -275,7 +275,7 @@ func expectServiceWithChecks(ctx context.Context, parentResource client.Object,
275275
func expectServiceWithConsistentChecks(ctx context.Context, parentResource client.Object, serviceName string, selectorLables map[string]string, isHeadless bool, additionalChecks func(Gomega, *corev1.Service), additionalOffset ...int) *corev1.Service {
276276
service := &corev1.Service{}
277277
ConsistentlyWithOffset(resolveOffset(additionalOffset), func(g Gomega) {
278-
Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist")
278+
g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist")
279279

280280
g.Expect(service.Spec.Selector).To(Equal(selectorLables), "Service is not pointing to the correct Pods.")
281281

@@ -299,10 +299,23 @@ func expectNoService(ctx context.Context, parentResource client.Object, serviceN
299299
}).Should(MatchError("services \""+serviceName+"\" not found"), message, "Service exists when it should not")
300300
}
301301

302+
func eventuallyExpectNoService(ctx context.Context, parentResource client.Object, serviceName string, message string, additionalOffset ...int) {
303+
EventuallyWithOffset(resolveOffset(additionalOffset), func() error {
304+
return k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})
305+
}).Should(MatchError("services \""+serviceName+"\" not found"), message, "Service exists when it should not")
306+
}
307+
302308
func expectNoServices(ctx context.Context, parentResource client.Object, message string, serviceNames []string, additionalOffset ...int) {
303309
ConsistentlyWithOffset(resolveOffset(additionalOffset), func(g Gomega) {
304310
for _, serviceName := range serviceNames {
305-
g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message)
311+
g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message, "service", serviceName)
312+
}
313+
}).Should(Succeed())
314+
}
315+
func eventuallyExpectNoServices(ctx context.Context, parentResource client.Object, message string, serviceNames []string, additionalOffset ...int) {
316+
EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) {
317+
for _, serviceName := range serviceNames {
318+
g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message, "service", serviceName)
306319
}
307320
}).Should(Succeed())
308321
}
@@ -356,6 +369,12 @@ func expectNoIngress(ctx context.Context, parentResource client.Object, ingressN
356369
}).Should(MatchError("ingresses.networking.k8s.io \""+ingressName+"\" not found"), "Ingress exists when it should not")
357370
}
358371

372+
func eventuallyExpectNoIngress(ctx context.Context, parentResource client.Object, ingressName string, additionalOffset ...int) {
373+
EventuallyWithOffset(resolveOffset(additionalOffset), func() error {
374+
return k8sClient.Get(ctx, resourceKey(parentResource, ingressName), &netv1.Ingress{})
375+
}).Should(MatchError("ingresses.networking.k8s.io \""+ingressName+"\" not found"), "Ingress exists when it should not")
376+
}
377+
359378
func expectPodDisruptionBudget(ctx context.Context, parentResource client.Object, podDisruptionBudgetName string, selector *metav1.LabelSelector, maxUnavailable intstr.IntOrString, additionalOffset ...int) *policyv1.PodDisruptionBudget {
360379
return expectPodDisruptionBudgetWithChecks(ctx, parentResource, podDisruptionBudgetName, selector, maxUnavailable, nil, resolveOffset(additionalOffset))
361380
}

‎controllers/solrcloud_controller.go

+92-7
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,6 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
326326
}
327327

328328
var statefulSet *appsv1.StatefulSet
329-
330329
if !blockReconciliationOfStatefulSet {
331330
// Generate StatefulSet that should exist
332331
expectedStatefulSet := util.GenerateStatefulSet(instance, &newStatus, hostNameIpMap, reconcileConfigInfo, tls, security)
@@ -380,12 +379,12 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
380379
}
381380
} else {
382381
// If we are blocking the reconciliation of the statefulSet, we still want to find information about it.
383-
err = r.Get(ctx, types.NamespacedName{Name: instance.StatefulSetName(), Namespace: instance.Namespace}, statefulSet)
384-
if err != nil {
385-
if !errors.IsNotFound(err) {
386-
return requeueOrNot, err
387-
} else {
382+
statefulSet = &appsv1.StatefulSet{}
383+
if e := r.Get(ctx, types.NamespacedName{Name: instance.StatefulSetName(), Namespace: instance.Namespace}, statefulSet); e != nil {
384+
if errors.IsNotFound(e) {
388385
statefulSet = nil
386+
} else {
387+
err = e
389388
}
390389
}
391390
}
@@ -424,6 +423,17 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
424423
if err != nil {
425424
return requeueOrNot, err
426425
}
426+
} else {
427+
// If ingress exists, delete it
428+
foundIngress := &netv1.Ingress{}
429+
err = r.Get(ctx, types.NamespacedName{Name: instance.CommonIngressName(), Namespace: instance.GetNamespace()}, foundIngress)
430+
if err == nil {
431+
err = r.Delete(ctx, foundIngress)
432+
if err != nil {
433+
return requeueOrNot, err
434+
}
435+
logger.Info("Deleted Ingress")
436+
}
427437
}
428438

429439
// *********************************************************
@@ -638,6 +648,12 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
638648
}
639649
}
640650

651+
// Remove unused services if necessary
652+
err = r.cleanupUnconfiguredServices(ctx, instance, podList, logger)
653+
if err != nil && !errors.IsNotFound(err) {
654+
return requeueOrNot, err
655+
}
656+
641657
if !reflect.DeepEqual(instance.Status, newStatus) {
642658
logger.Info("Updating SolrCloud Status", "status", newStatus)
643659
oldInstance := instance.DeepCopy()
@@ -651,7 +667,76 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
651667
return requeueOrNot, err
652668
}
653669

654-
// InitializePods Ensure that all SolrCloud Pods are initialized
670+
// cleanupUnconfiguredServices remove services that are no longer defined by the SolrCloud resource, and no longer in use by pods.
671+
// This does not currently include removing per-node services that are no longer in use because the SolrCloud has been scaled down.
672+
func (r *SolrCloudReconciler) cleanupUnconfiguredServices(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, podList []corev1.Pod, logger logr.Logger) (err error) {
673+
var onlyServiceTypeInUse string
674+
if solrCloud.UsesHeadlessService() {
675+
onlyServiceTypeInUse = util.HeadlessServiceType
676+
} else if solrCloud.UsesIndividualNodeServices() {
677+
onlyServiceTypeInUse = util.PerNodeServiceType
678+
} else {
679+
// This should never happen, there are only 2 options
680+
return
681+
}
682+
for _, pod := range podList {
683+
if pod.Annotations == nil && pod.Annotations[util.ServiceTypeAnnotation] != "" {
684+
if onlyServiceTypeInUse != pod.Annotations[util.ServiceTypeAnnotation] {
685+
// Only remove services if all pods are using the same, and configured, type of service.
686+
// Otherwise, we are in transition between service types and need to wait to delete anything.
687+
return
688+
}
689+
} else {
690+
// If we have a pod missing this annotation, then it has not been fully updated to a supported Operator version.
691+
// We don't have the information, so assume both serviceTypes are in use, and don't remove anything.
692+
return
693+
}
694+
}
695+
696+
// If we are at this point, then we can assume we are completely transitioned and can delete the unused services
697+
if solrCloud.UsesHeadlessService() {
698+
err = r.deleteServicesOfType(ctx, solrCloud, util.PerNodeServiceType, logger)
699+
} else if solrCloud.UsesIndividualNodeServices() {
700+
err = r.deleteServicesOfType(ctx, solrCloud, util.HeadlessServiceType, logger)
701+
}
702+
return
703+
}
704+
705+
func (r *SolrCloudReconciler) deleteServicesOfType(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, serviceType string, logger logr.Logger) (err error) {
706+
foundServices := &corev1.ServiceList{}
707+
selectorLabels := solrCloud.SharedLabels()
708+
selectorLabels[util.ServiceTypeAnnotation] = serviceType
709+
710+
serviceSelector := labels.SelectorFromSet(selectorLabels)
711+
listOps := &client.ListOptions{
712+
Namespace: solrCloud.Namespace,
713+
LabelSelector: serviceSelector,
714+
}
715+
716+
if err = r.List(ctx, foundServices, listOps); err != nil {
717+
logger.Error(err, "Error listing services for SolrCloud", "serviceType", serviceType)
718+
return
719+
}
720+
721+
for _, headlessService := range foundServices.Items {
722+
if e := r.deleteService(ctx, &headlessService, logger); e != nil {
723+
// Don't break, just add the error for later
724+
err = e
725+
}
726+
}
727+
return
728+
}
729+
730+
func (r *SolrCloudReconciler) deleteService(ctx context.Context, service *corev1.Service, logger logr.Logger) (err error) {
731+
logger.Info("Deleting Service for SolrCloud", "Service", service.Name)
732+
err = r.Client.Delete(ctx, service)
733+
if err != nil && !errors.IsNotFound(err) {
734+
logger.Error(err, "Error deleting unused Service for SolrCloud", "Service", service.Name)
735+
}
736+
return
737+
}
738+
739+
// initializePods Ensure that all SolrCloud Pods are initialized
655740
func (r *SolrCloudReconciler) initializePods(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, statefulSet *appsv1.StatefulSet, logger logr.Logger) (podSelector labels.Selector, podList []corev1.Pod, err error) {
656741
foundPods := &corev1.PodList{}
657742
selectorLabels := solrCloud.SharedLabels()

‎controllers/solrcloud_controller_backup_test.go

+2
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ var _ = FDescribe("SolrCloud controller - Backup Repositories", func() {
9898
Expect(statefulSet.Spec.Template.Annotations).To(Equal(util.MergeLabelsOrAnnotations(testPodAnnotations, map[string]string{
9999
"solr.apache.org/solrXmlMd5": solrXmlMd5,
100100
util.SolrBackupRepositoriesAnnotation: "test-repo",
101+
util.ServiceTypeAnnotation: util.HeadlessServiceType,
101102
})), "Incorrect pod annotations")
102103

103104
// Env Variable Tests
@@ -321,6 +322,7 @@ var _ = FDescribe("SolrCloud controller - Backup Repositories", func() {
321322
Expect(statefulSet.Spec.Template.Annotations).To(Equal(map[string]string{
322323
"solr.apache.org/solrXmlMd5": fmt.Sprintf("%x", md5.Sum([]byte(configMap.Data["solr.xml"]))),
323324
util.SolrBackupRepositoriesAnnotation: "another,test-repo",
325+
util.ServiceTypeAnnotation: util.HeadlessServiceType,
324326
}), "Incorrect pod annotations")
325327
})
326328
})

‎controllers/solrcloud_controller_ingress_test.go

+87
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,92 @@ var _ = FDescribe("SolrCloud controller - Ingress", func() {
163163
})
164164
})
165165

166+
FContext("Full Ingress - Switch off after creation", func() {
167+
BeforeEach(func() {
168+
solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{
169+
External: &solrv1beta1.ExternalAddressability{
170+
Method: solrv1beta1.Ingress,
171+
UseExternalAddress: true,
172+
DomainName: testDomain,
173+
NodePortOverride: 100,
174+
},
175+
PodPort: 3000,
176+
CommonServicePort: 4000,
177+
}
178+
})
179+
FIt("has the correct resources", func(ctx context.Context) {
180+
By("testing the Solr StatefulSet")
181+
statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName())
182+
// Pod Annotations test
183+
Expect(statefulSet.Spec.Template.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.PerNodeServiceType), "Since external address is used for advertising, the perNode service should be specified in the pod annotations.")
184+
185+
By("testing the Solr Common Service")
186+
commonService := expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false)
187+
Expect(commonService.Annotations).To(Equal(testCommonServiceAnnotations), "Incorrect common service annotations")
188+
Expect(commonService.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service")
189+
Expect(commonService.Spec.Ports[0].Port).To(Equal(int32(4000)), "Wrong port on common Service")
190+
Expect(commonService.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on common Service")
191+
192+
By("ensuring the Solr Headless Service does not exist")
193+
expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.")
194+
195+
By("making sure the individual Solr Node Services exist and route correctly")
196+
nodeNames := solrCloud.GetAllSolrPodNames()
197+
Expect(nodeNames).To(HaveLen(replicas), "SolrCloud has incorrect number of nodeNames.")
198+
for _, nodeName := range nodeNames {
199+
service := expectService(ctx, solrCloud, nodeName, util.MergeLabelsOrAnnotations(statefulSet.Spec.Selector.MatchLabels, map[string]string{"statefulset.kubernetes.io/pod-name": nodeName}), false)
200+
expectedNodeServiceLabels := util.MergeLabelsOrAnnotations(solrCloud.SharedLabelsWith(solrCloud.Labels), map[string]string{"service-type": "external"})
201+
Expect(service.Labels).To(Equal(util.MergeLabelsOrAnnotations(expectedNodeServiceLabels, testNodeServiceLabels)), "Incorrect node '"+nodeName+"' service labels")
202+
Expect(service.Annotations).To(Equal(testNodeServiceAnnotations), "Incorrect node '"+nodeName+"' service annotations")
203+
Expect(service.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service")
204+
Expect(service.Spec.Ports[0].Port).To(Equal(int32(100)), "Wrong port on node Service")
205+
Expect(service.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on node Service")
206+
}
207+
208+
By("making sure Ingress was created correctly")
209+
ingress := expectIngress(ctx, solrCloud, solrCloud.CommonIngressName())
210+
Expect(ingress.Labels).To(Equal(util.MergeLabelsOrAnnotations(solrCloud.SharedLabelsWith(solrCloud.Labels), testIngressLabels)), "Incorrect ingress labels")
211+
Expect(ingress.Annotations).To(Equal(ingressLabelsWithDefaults(testIngressAnnotations)), "Incorrect ingress annotations")
212+
Expect(ingress.Spec.IngressClassName).To(Not(BeNil()), "Ingress class name should not be nil")
213+
Expect(*ingress.Spec.IngressClassName).To(Equal(testIngressClass), "Incorrect ingress class name")
214+
testIngressRules(solrCloud, ingress, true, replicas, 4000, 100, testDomain)
215+
216+
By("making sure the node addresses in the Status are correct")
217+
expectSolrCloudStatusWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloudStatus) {
218+
g.Expect(found.InternalCommonAddress).To(Equal("http://"+solrCloud.CommonServiceName()+"."+solrCloud.Namespace+":4000"), "Wrong internal common address in status")
219+
g.Expect(found.ExternalCommonAddress).To(Not(BeNil()), "External common address in status should not be nil")
220+
g.Expect(*found.ExternalCommonAddress).To(Equal("http://"+solrCloud.Namespace+"-"+solrCloud.Name+"-solrcloud"+"."+testDomain), "Wrong external common address in status")
221+
})
222+
223+
By("Turning off node external addressability and making sure the node services are deleted")
224+
expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) {
225+
found.Spec.SolrAddressability.External.UseExternalAddress = false
226+
found.Spec.SolrAddressability.External.HideNodes = true
227+
g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to not advertise the nodes externally.")
228+
})
229+
230+
// Since node external addressability is off, but common external addressability is on, the ingress should exist, but the node services should not
231+
eventuallyExpectNoServices(ctx, solrCloud, "Node services shouldn't exist after the update, but they do.", nodeNames)
232+
233+
expectIngress(ctx, solrCloud, solrCloud.CommonIngressName())
234+
235+
headlessService := expectService(ctx, solrCloud, solrCloud.HeadlessServiceName(), statefulSet.Spec.Selector.MatchLabels, true)
236+
Expect(headlessService.Annotations).To(Equal(testHeadlessServiceAnnotations), "Incorrect headless service annotations")
237+
Expect(headlessService.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service")
238+
Expect(headlessService.Spec.Ports[0].Port).To(Equal(int32(3000)), "Wrong port on headless Service")
239+
Expect(headlessService.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on headless Service")
240+
Expect(headlessService.Spec.Ports[0].Protocol).To(Equal(corev1.ProtocolTCP), "Wrong protocol on headless Service")
241+
Expect(headlessService.Spec.Ports[0].AppProtocol).To(BeNil(), "AppProtocol on headless Service should be nil when not running with TLS")
242+
243+
By("Turning off common external addressability and making sure the ingress is deleted")
244+
expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) {
245+
found.Spec.SolrAddressability.External = nil
246+
g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to remove external addressability")
247+
})
248+
eventuallyExpectNoIngress(ctx, solrCloud, solrCloud.CommonIngressName())
249+
})
250+
})
251+
166252
FContext("Hide Nodes from external connections - Using default ingress class", func() {
167253
ingressClass := &netv1.IngressClass{
168254
ObjectMeta: metav1.ObjectMeta{
@@ -206,6 +292,7 @@ var _ = FDescribe("SolrCloud controller - Ingress", func() {
206292

207293
By("testing the Solr StatefulSet")
208294
statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName())
295+
Expect(statefulSet.Spec.Template.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.HeadlessServiceType), "Since external address is not used for advertising, the headless service should be specified in the pod annotations.")
209296

210297
Expect(statefulSet.Spec.Template.Spec.Containers).To(HaveLen(1), "Solr StatefulSet requires a container.")
211298

‎controllers/solrcloud_controller_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ var _ = FDescribe("SolrCloud controller - General", func() {
280280
Expect(statefulSet.Spec.Template.ObjectMeta.Annotations).To(HaveKey(util.SolrScheduledRestartAnnotation), "Pod Template does not have scheduled restart annotation when it should")
281281
// Remove the annotation when we know that it exists, we don't know the exact value so we can't check it below.
282282
delete(statefulSet.Spec.Template.Annotations, util.SolrScheduledRestartAnnotation)
283-
Expect(statefulSet.Spec.Template.Annotations).To(Equal(util.MergeLabelsOrAnnotations(map[string]string{"solr.apache.org/solrXmlMd5": fmt.Sprintf("%x", md5.Sum([]byte(configMap.Data["solr.xml"])))}, testPodAnnotations)), "Incorrect pod annotations")
283+
Expect(statefulSet.Spec.Template.Annotations).To(Equal(util.MergeLabelsOrAnnotations(map[string]string{util.ServiceTypeAnnotation: util.HeadlessServiceType, "solr.apache.org/solrXmlMd5": fmt.Sprintf("%x", md5.Sum([]byte(configMap.Data["solr.xml"])))}, testPodAnnotations)), "Incorrect pod annotations")
284284
Expect(statefulSet.Spec.Template.Spec.NodeSelector).To(Equal(testNodeSelectors), "Incorrect pod node selectors")
285285

286286
Expect(statefulSet.Spec.Template.Spec.Containers[0].LivenessProbe, testProbeLivenessNonDefaults, "Incorrect Liveness Probe")

0 commit comments

Comments
 (0)