From 89fd54ac550a55604b951d67015fa319445420ea Mon Sep 17 00:00:00 2001 From: Houston Putman Date: Wed, 15 Jan 2025 14:16:48 -0600 Subject: [PATCH] Add safe deletion of services --- controllers/controller_utils_test.go | 19 ++- controllers/solrcloud_controller.go | 104 ++++++++++--- .../solrcloud_controller_backup_test.go | 2 + .../solrcloud_controller_ingress_test.go | 87 +++++++++++ controllers/solrcloud_controller_test.go | 36 +---- controllers/util/common.go | 12 +- controllers/util/solr_util.go | 26 ++-- tests/e2e/resource_utils_test.go | 116 ++++++--------- tests/e2e/solrcloud_ingress_test.go | 138 ++++++++++++++++++ 9 files changed, 393 insertions(+), 147 deletions(-) create mode 100644 tests/e2e/solrcloud_ingress_test.go diff --git a/controllers/controller_utils_test.go b/controllers/controller_utils_test.go index 8b7fc427..6cdb14f1 100644 --- a/controllers/controller_utils_test.go +++ b/controllers/controller_utils_test.go @@ -241,7 +241,7 @@ func expectService(ctx context.Context, parentResource client.Object, serviceNam 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 { service := &corev1.Service{} EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { - Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist") + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist") g.Expect(service.Spec.Selector).To(Equal(selectorLables), "Service is not pointing to the correct Pods.") @@ -275,7 +275,7 @@ func expectServiceWithChecks(ctx context.Context, parentResource client.Object, 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 { service := &corev1.Service{} ConsistentlyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { - Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist") + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist") g.Expect(service.Spec.Selector).To(Equal(selectorLables), "Service is not pointing to the correct Pods.") @@ -299,10 +299,23 @@ func expectNoService(ctx context.Context, parentResource client.Object, serviceN }).Should(MatchError("services \""+serviceName+"\" not found"), message, "Service exists when it should not") } +func eventuallyExpectNoService(ctx context.Context, parentResource client.Object, serviceName string, message string, additionalOffset ...int) { + EventuallyWithOffset(resolveOffset(additionalOffset), func() error { + return k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{}) + }).Should(MatchError("services \""+serviceName+"\" not found"), message, "Service exists when it should not") +} + func expectNoServices(ctx context.Context, parentResource client.Object, message string, serviceNames []string, additionalOffset ...int) { ConsistentlyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { for _, serviceName := range serviceNames { - g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message) + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message, "service", serviceName) + } + }).Should(Succeed()) +} +func eventuallyExpectNoServices(ctx context.Context, parentResource client.Object, message string, serviceNames []string, additionalOffset ...int) { + EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { + for _, serviceName := range serviceNames { + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message, "service", serviceName) } }).Should(Succeed()) } diff --git a/controllers/solrcloud_controller.go b/controllers/solrcloud_controller.go index 7a1264e7..affc2202 100644 --- a/controllers/solrcloud_controller.go +++ b/controllers/solrcloud_controller.go @@ -173,22 +173,6 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } } - } else { - // Need no individual services per node, delete if found - for _, nodeName := range solrNodeNames { - serviceName := nodeName - foundService := &corev1.Service{} - err = r.Get(ctx, types.NamespacedName{Name: serviceName, Namespace: instance.Namespace}, foundService) - if err == nil { - err = r.Delete(ctx, foundService) - if err != nil { - return requeueOrNot, err - } - logger.Info("Deleted service for node", "node", serviceName) - } else if !errors.IsNotFound(err) { - return requeueOrNot, err - } - } } // Generate HeadlessService @@ -342,7 +326,6 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } var statefulSet *appsv1.StatefulSet - if !blockReconciliationOfStatefulSet { // Generate StatefulSet that should exist expectedStatefulSet := util.GenerateStatefulSet(instance, &newStatus, hostNameIpMap, reconcileConfigInfo, tls, security) @@ -396,12 +379,12 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } else { // If we are blocking the reconciliation of the statefulSet, we still want to find information about it. - err = r.Get(ctx, types.NamespacedName{Name: instance.StatefulSetName(), Namespace: instance.Namespace}, statefulSet) - if err != nil { - if !errors.IsNotFound(err) { - return requeueOrNot, err - } else { + statefulSet = &appsv1.StatefulSet{} + if e := r.Get(ctx, types.NamespacedName{Name: instance.StatefulSetName(), Namespace: instance.Namespace}, statefulSet); e != nil { + if errors.IsNotFound(e) { statefulSet = nil + } else { + err = e } } } @@ -665,6 +648,12 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } + // Remove unused services if necessary + err = r.cleanupUnconfiguredServices(ctx, instance, podList, logger) + if err != nil && !errors.IsNotFound(err) { + return requeueOrNot, err + } + if !reflect.DeepEqual(instance.Status, newStatus) { logger.Info("Updating SolrCloud Status", "status", newStatus) oldInstance := instance.DeepCopy() @@ -678,7 +667,76 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return requeueOrNot, err } -// InitializePods Ensure that all SolrCloud Pods are initialized +// cleanupUnconfiguredServices remove services that are no longer defined by the SolrCloud resource, and no longer in use by pods. +// This does not currently include removing per-node services that are no longer in use because the SolrCloud has been scaled down. +func (r *SolrCloudReconciler) cleanupUnconfiguredServices(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, podList []corev1.Pod, logger logr.Logger) (err error) { + var onlyServiceTypeInUse string + if solrCloud.UsesHeadlessService() { + onlyServiceTypeInUse = util.HeadlessServiceType + } else if solrCloud.UsesIndividualNodeServices() { + onlyServiceTypeInUse = util.PerNodeServiceType + } else { + // This should never happen, there are only 2 options + return + } + for _, pod := range podList { + 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 + } + } + + // If we are at this point, then we can assume we are completely transitioned and can delete the unused services + if solrCloud.UsesHeadlessService() { + err = r.deleteServicesOfType(ctx, solrCloud, util.PerNodeServiceType, logger) + } else if solrCloud.UsesIndividualNodeServices() { + err = r.deleteServicesOfType(ctx, solrCloud, util.HeadlessServiceType, logger) + } + return +} + +func (r *SolrCloudReconciler) deleteServicesOfType(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, serviceType string, logger logr.Logger) (err error) { + foundServices := &corev1.ServiceList{} + selectorLabels := solrCloud.SharedLabels() + selectorLabels[util.ServiceTypeAnnotation] = serviceType + + serviceSelector := labels.SelectorFromSet(selectorLabels) + listOps := &client.ListOptions{ + Namespace: solrCloud.Namespace, + LabelSelector: serviceSelector, + } + + if err = r.List(ctx, foundServices, listOps); err != nil { + logger.Error(err, "Error listing services for SolrCloud", "serviceType", serviceType) + return + } + + for _, headlessService := range foundServices.Items { + if e := r.deleteService(ctx, &headlessService, logger); e != nil { + // Don't break, just add the error for later + err = e + } + } + return +} + +func (r *SolrCloudReconciler) deleteService(ctx context.Context, service *corev1.Service, logger logr.Logger) (err error) { + logger.Info("Deleting Service for SolrCloud", "Service", service.Name) + err = r.Client.Delete(ctx, service) + if err != nil && !errors.IsNotFound(err) { + logger.Error(err, "Error deleting unused Service for SolrCloud", "Service", service.Name) + } + return +} + +// initializePods Ensure that all SolrCloud Pods are initialized func (r *SolrCloudReconciler) initializePods(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, statefulSet *appsv1.StatefulSet, logger logr.Logger) (podSelector labels.Selector, podList []corev1.Pod, err error) { foundPods := &corev1.PodList{} selectorLabels := solrCloud.SharedLabels() diff --git a/controllers/solrcloud_controller_backup_test.go b/controllers/solrcloud_controller_backup_test.go index a0be8d5a..7f223fce 100644 --- a/controllers/solrcloud_controller_backup_test.go +++ b/controllers/solrcloud_controller_backup_test.go @@ -98,6 +98,7 @@ var _ = FDescribe("SolrCloud controller - Backup Repositories", func() { Expect(statefulSet.Spec.Template.Annotations).To(Equal(util.MergeLabelsOrAnnotations(testPodAnnotations, map[string]string{ "solr.apache.org/solrXmlMd5": solrXmlMd5, util.SolrBackupRepositoriesAnnotation: "test-repo", + util.ServiceTypeAnnotation: util.HeadlessServiceType, })), "Incorrect pod annotations") // Env Variable Tests @@ -321,6 +322,7 @@ var _ = FDescribe("SolrCloud controller - Backup Repositories", func() { Expect(statefulSet.Spec.Template.Annotations).To(Equal(map[string]string{ "solr.apache.org/solrXmlMd5": fmt.Sprintf("%x", md5.Sum([]byte(configMap.Data["solr.xml"]))), util.SolrBackupRepositoriesAnnotation: "another,test-repo", + util.ServiceTypeAnnotation: util.HeadlessServiceType, }), "Incorrect pod annotations") }) }) diff --git a/controllers/solrcloud_controller_ingress_test.go b/controllers/solrcloud_controller_ingress_test.go index 8a616a9a..87fb57b6 100644 --- a/controllers/solrcloud_controller_ingress_test.go +++ b/controllers/solrcloud_controller_ingress_test.go @@ -163,6 +163,92 @@ var _ = FDescribe("SolrCloud controller - Ingress", func() { }) }) + FContext("Full Ingress - Switch off after creation", func() { + BeforeEach(func() { + solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Ingress, + UseExternalAddress: true, + DomainName: testDomain, + NodePortOverride: 100, + }, + PodPort: 3000, + CommonServicePort: 4000, + } + }) + FIt("has the correct resources", func(ctx context.Context) { + By("testing the Solr StatefulSet") + statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName()) + // Pod Annotations test + 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.") + + By("testing the Solr Common Service") + commonService := expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + Expect(commonService.Annotations).To(Equal(testCommonServiceAnnotations), "Incorrect common service annotations") + Expect(commonService.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service") + Expect(commonService.Spec.Ports[0].Port).To(Equal(int32(4000)), "Wrong port on common Service") + Expect(commonService.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on common Service") + + By("ensuring the Solr Headless Service does not exist") + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + + By("making sure the individual Solr Node Services exist and route correctly") + nodeNames := solrCloud.GetAllSolrPodNames() + Expect(nodeNames).To(HaveLen(replicas), "SolrCloud has incorrect number of nodeNames.") + for _, nodeName := range nodeNames { + service := expectService(ctx, solrCloud, nodeName, util.MergeLabelsOrAnnotations(statefulSet.Spec.Selector.MatchLabels, map[string]string{"statefulset.kubernetes.io/pod-name": nodeName}), false) + expectedNodeServiceLabels := util.MergeLabelsOrAnnotations(solrCloud.SharedLabelsWith(solrCloud.Labels), map[string]string{"service-type": "external"}) + Expect(service.Labels).To(Equal(util.MergeLabelsOrAnnotations(expectedNodeServiceLabels, testNodeServiceLabels)), "Incorrect node '"+nodeName+"' service labels") + Expect(service.Annotations).To(Equal(testNodeServiceAnnotations), "Incorrect node '"+nodeName+"' service annotations") + Expect(service.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service") + Expect(service.Spec.Ports[0].Port).To(Equal(int32(100)), "Wrong port on node Service") + Expect(service.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on node Service") + } + + By("making sure Ingress was created correctly") + ingress := expectIngress(ctx, solrCloud, solrCloud.CommonIngressName()) + Expect(ingress.Labels).To(Equal(util.MergeLabelsOrAnnotations(solrCloud.SharedLabelsWith(solrCloud.Labels), testIngressLabels)), "Incorrect ingress labels") + Expect(ingress.Annotations).To(Equal(ingressLabelsWithDefaults(testIngressAnnotations)), "Incorrect ingress annotations") + Expect(ingress.Spec.IngressClassName).To(Not(BeNil()), "Ingress class name should not be nil") + Expect(*ingress.Spec.IngressClassName).To(Equal(testIngressClass), "Incorrect ingress class name") + testIngressRules(solrCloud, ingress, true, replicas, 4000, 100, testDomain) + + By("making sure the node addresses in the Status are correct") + expectSolrCloudStatusWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloudStatus) { + g.Expect(found.InternalCommonAddress).To(Equal("http://"+solrCloud.CommonServiceName()+"."+solrCloud.Namespace+":4000"), "Wrong internal common address in status") + g.Expect(found.ExternalCommonAddress).To(Not(BeNil()), "External common address in status should not be nil") + g.Expect(*found.ExternalCommonAddress).To(Equal("http://"+solrCloud.Namespace+"-"+solrCloud.Name+"-solrcloud"+"."+testDomain), "Wrong external common address in status") + }) + + By("Turning off node external addressability and making sure the node services are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External.UseExternalAddress = false + found.Spec.SolrAddressability.External.HideNodes = true + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to not advertise the nodes externally.") + }) + + // Since node external addressability is off, but common external addressability is on, the ingress should exist, but the node services should not + eventuallyExpectNoServices(ctx, solrCloud, "Node services shouldn't exist after the update, but they do.", nodeNames) + + expectIngress(ctx, solrCloud, solrCloud.CommonIngressName()) + + headlessService := expectService(ctx, solrCloud, solrCloud.HeadlessServiceName(), statefulSet.Spec.Selector.MatchLabels, true) + Expect(headlessService.Annotations).To(Equal(testHeadlessServiceAnnotations), "Incorrect headless service annotations") + Expect(headlessService.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service") + Expect(headlessService.Spec.Ports[0].Port).To(Equal(int32(3000)), "Wrong port on headless Service") + Expect(headlessService.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on headless Service") + Expect(headlessService.Spec.Ports[0].Protocol).To(Equal(corev1.ProtocolTCP), "Wrong protocol on headless Service") + Expect(headlessService.Spec.Ports[0].AppProtocol).To(BeNil(), "AppProtocol on headless Service should be nil when not running with TLS") + + By("Turning off common external addressability and making sure the ingress is deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External = nil + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to remove external addressability") + }) + eventuallyExpectNoIngress(ctx, solrCloud, solrCloud.CommonIngressName()) + }) + }) + FContext("Hide Nodes from external connections - Using default ingress class", func() { ingressClass := &netv1.IngressClass{ ObjectMeta: metav1.ObjectMeta{ @@ -206,6 +292,7 @@ var _ = FDescribe("SolrCloud controller - Ingress", func() { By("testing the Solr StatefulSet") statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName()) + 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.") Expect(statefulSet.Spec.Template.Spec.Containers).To(HaveLen(1), "Solr StatefulSet requires a container.") diff --git a/controllers/solrcloud_controller_test.go b/controllers/solrcloud_controller_test.go index beaf13e0..5dca7f02 100644 --- a/controllers/solrcloud_controller_test.go +++ b/controllers/solrcloud_controller_test.go @@ -280,7 +280,7 @@ var _ = FDescribe("SolrCloud controller - General", func() { Expect(statefulSet.Spec.Template.ObjectMeta.Annotations).To(HaveKey(util.SolrScheduledRestartAnnotation), "Pod Template does not have scheduled restart annotation when it should") // Remove the annotation when we know that it exists, we don't know the exact value so we can't check it below. delete(statefulSet.Spec.Template.Annotations, util.SolrScheduledRestartAnnotation) - 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") + 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") Expect(statefulSet.Spec.Template.Spec.NodeSelector).To(Equal(testNodeSelectors), "Incorrect pod node selectors") Expect(statefulSet.Spec.Template.Spec.Containers[0].LivenessProbe, testProbeLivenessNonDefaults, "Incorrect Liveness Probe") @@ -759,38 +759,4 @@ var _ = FDescribe("SolrCloud controller - General", func() { expectNoConfigMap(ctx, solrCloud, fmt.Sprintf("%s-solrcloud-configmap", solrCloud.GetName())) }) }) - - FContext("SolrCloud with external addressability", func() { - BeforeEach(func() { - replicas := int32(1) - solrCloud.Spec = solrv1beta1.SolrCloudSpec{ - Replicas: &replicas, - ZookeeperRef: &solrv1beta1.ZookeeperRef{ - ConnectionInfo: &solrv1beta1.ZookeeperConnectionInfo{ - InternalConnectionString: "host:7271", - }, - }, - SolrAddressability: solrv1beta1.SolrAddressabilityOptions{ - External: &solrv1beta1.ExternalAddressability{ - Method: solrv1beta1.Ingress, - UseExternalAddress: true, - HideNodes: false, - DomainName: "test.solr.org", - }, - }, - } - }) - // TODO Move to solrcloud_controller_ingress_test, unless we add service coverage in this same section? - FIt("has the correct resources", func(ctx context.Context) { - By("testing the created ingress") - expectIngress(ctx, solrCloud, solrCloud.CommonIngressName()) - - By("ensuring the ingress disappears when externalAddressability settings change") - expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { - found.Spec.SolrAddressability.External = nil - g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Disable externalAddressability for the solrcloud") - }) - eventuallyExpectNoIngress(ctx, solrCloud, solrCloud.CommonIngressName()) - }) - }) }) diff --git a/controllers/util/common.go b/controllers/util/common.go index 64bf6549..cf943cdc 100644 --- a/controllers/util/common.go +++ b/controllers/util/common.go @@ -431,8 +431,9 @@ func CopyPodTemplates(from, to *corev1.PodTemplateSpec, basePath string, logger requireUpdate = CopyPodVolumes(&from.Spec.Volumes, &to.Spec.Volumes, basePath+"Spec.Volumes", logger) || requireUpdate if !DeepEqualWithNils(to.Spec.HostAliases, from.Spec.HostAliases) { - requireUpdate = true + hostAliasUpdate := false if to.Spec.HostAliases == nil { + hostAliasUpdate = true to.Spec.HostAliases = from.Spec.HostAliases } else { // Do not remove aliases that are no longer used. @@ -443,19 +444,22 @@ func CopyPodTemplates(from, to *corev1.PodTemplateSpec, basePath string, logger if fromAlias.Hostnames[0] == toAlias.Hostnames[0] { found = true if !DeepEqualWithNils(toAlias, fromAlias) { - requireUpdate = true + hostAliasUpdate = true to.Spec.HostAliases[i] = fromAlias break } } } if !found { - requireUpdate = true + hostAliasUpdate = true to.Spec.HostAliases = append(to.Spec.HostAliases, fromAlias) } } } - logger.Info("Update required because field changed", "field", basePath+"Spec.HostAliases", "from", to.Spec.HostAliases, "to", from.Spec.HostAliases) + if hostAliasUpdate { + requireUpdate = true + logger.Info("Update required because field changed", "field", basePath+"Spec.HostAliases", "from", to.Spec.HostAliases, "to", from.Spec.HostAliases) + } } if !DeepEqualWithNils(to.Spec.ImagePullSecrets, from.Spec.ImagePullSecrets) { diff --git a/controllers/util/solr_util.go b/controllers/util/solr_util.go index db4c4085..478145a5 100644 --- a/controllers/util/solr_util.go +++ b/controllers/util/solr_util.go @@ -52,6 +52,10 @@ const ( SolrXmlFile = "solr.xml" LogXmlMd5Annotation = "solr.apache.org/logXmlMd5" LogXmlFile = "log4j2.xml" + ServiceTypeAnnotation = "service-type" // "solr.apache.org/serviceType" + HeadlessServiceType = "headless" + PerNodeServiceType = "external" + CommonServiceType = "common" // Protected StatefulSet annotations // These are to be saved on a statefulSet update @@ -125,6 +129,16 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl } shareProcessNamespace = customPodOptions.ShareProcessNamespace } + if podAnnotations == nil { + podAnnotations = make(map[string]string, 1) + } + var serviceType string + if solrCloud.UsesHeadlessService() { + serviceType = HeadlessServiceType + } else if solrCloud.UsesIndividualNodeServices() { + serviceType = PerNodeServiceType + } + podAnnotations[ServiceTypeAnnotation] = serviceType // The isNotStopped readiness gate will always be used for managedUpdates podReadinessGates := []corev1.PodReadinessGate{ @@ -413,9 +427,6 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl // Did the user provide a custom log config? if reconcileConfigInfo[LogXmlFile] != "" { if reconcileConfigInfo[LogXmlMd5Annotation] != "" { - if podAnnotations == nil { - podAnnotations = make(map[string]string, 1) - } podAnnotations[LogXmlMd5Annotation] = reconcileConfigInfo[LogXmlMd5Annotation] } @@ -432,9 +443,6 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl // track the MD5 of the custom solr.xml in the pod spec annotations, // so we get a rolling restart when the configMap changes if reconcileConfigInfo[SolrXmlMd5Annotation] != "" { - if podAnnotations == nil { - podAnnotations = make(map[string]string, 1) - } podAnnotations[SolrXmlMd5Annotation] = reconcileConfigInfo[SolrXmlMd5Annotation] } @@ -912,7 +920,7 @@ func getAppProtocol(solrCloud *solr.SolrCloud) *string { // solrCloud: SolrCloud instance func GenerateCommonService(solrCloud *solr.SolrCloud) *corev1.Service { labels := solrCloud.SharedLabelsWith(solrCloud.GetLabels()) - labels["service-type"] = "common" + labels[ServiceTypeAnnotation] = CommonServiceType selectorLabels := solrCloud.SharedLabels() selectorLabels["technology"] = solr.SolrTechnologyLabel @@ -964,7 +972,7 @@ func GenerateCommonService(solrCloud *solr.SolrCloud) *corev1.Service { // solrCloud: SolrCloud instance func GenerateHeadlessService(solrCloud *solr.SolrCloud) *corev1.Service { labels := solrCloud.SharedLabelsWith(solrCloud.GetLabels()) - labels["service-type"] = "headless" + labels[ServiceTypeAnnotation] = HeadlessServiceType selectorLabels := solrCloud.SharedLabels() selectorLabels["technology"] = solr.SolrTechnologyLabel @@ -1019,7 +1027,7 @@ func GenerateHeadlessService(solrCloud *solr.SolrCloud) *corev1.Service { // nodeName: string node func GenerateNodeService(solrCloud *solr.SolrCloud, nodeName string) *corev1.Service { labels := solrCloud.SharedLabelsWith(solrCloud.GetLabels()) - labels["service-type"] = "external" + labels[ServiceTypeAnnotation] = PerNodeServiceType selectorLabels := solrCloud.SharedLabels() selectorLabels["technology"] = solr.SolrTechnologyLabel diff --git a/tests/e2e/resource_utils_test.go b/tests/e2e/resource_utils_test.go index a58904cb..31b88c63 100644 --- a/tests/e2e/resource_utils_test.go +++ b/tests/e2e/resource_utils_test.go @@ -346,6 +346,27 @@ func expectNoPodNow(ctx context.Context, parentResource client.Object, podName s ).To(MatchError("pods \""+podName+"\" not found"), "Pod exists when it should not") } +func expectPodNow(ctx context.Context, parentResource client.Object, podName string, additionalOffset ...int) *corev1.Pod { + pod := &corev1.Pod{} + ExpectWithOffset( + resolveOffset(additionalOffset), + k8sClient.Get(ctx, resourceKey(parentResource, podName), pod), + ).To(Succeed(), "Pod should exist", "name", podName, "namespace", parentResource.GetNamespace()) + return pod +} + +func expectPodWithChecks(ctx context.Context, parentResource client.Object, podName string, additionalChecks func(Gomega, *corev1.Pod), additionalOffset ...int) *corev1.Pod { + pod := &corev1.Pod{} + EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, podName), pod)).To(Succeed(), "Expected Pod does not exist") + + if additionalChecks != nil { + additionalChecks(g, pod) + } + }).Should(Succeed()) + return pod +} + func expectService(ctx context.Context, parentResource client.Object, serviceName string, selectorLables map[string]string, isHeadless bool, additionalOffset ...int) *corev1.Service { return expectServiceWithChecks(ctx, parentResource, serviceName, selectorLables, isHeadless, nil, resolveOffset(additionalOffset)) } @@ -353,7 +374,7 @@ func expectService(ctx context.Context, parentResource client.Object, serviceNam 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 { service := &corev1.Service{} EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { - Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist") + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist") g.Expect(service.Spec.Selector).To(Equal(selectorLables), "Service is not pointing to the correct Pods.") @@ -367,27 +388,13 @@ func expectServiceWithChecks(ctx context.Context, parentResource client.Object, additionalChecks(g, service) } }).Should(Succeed()) - - By("recreating the Service after it is deleted") - ExpectWithOffset(resolveOffset(additionalOffset), k8sClient.Delete(ctx, service)).To(Succeed()) - EventuallyWithOffset( - resolveOffset(additionalOffset), - func() (types.UID, error) { - newResource := &corev1.Service{} - err := k8sClient.Get(ctx, resourceKey(parentResource, serviceName), newResource) - if err != nil { - return "", err - } - return newResource.UID, nil - }).Should(And(Not(BeEmpty()), Not(Equal(service.UID))), "New Service, with new UID, not created.") - return service } 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 { service := &corev1.Service{} ConsistentlyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { - Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist") + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist") g.Expect(service.Spec.Selector).To(Equal(selectorLables), "Service is not pointing to the correct Pods.") @@ -411,10 +418,23 @@ func expectNoService(ctx context.Context, parentResource client.Object, serviceN }).Should(MatchError("services \""+serviceName+"\" not found"), message, "Service exists when it should not") } +func eventuallyExpectNoService(ctx context.Context, parentResource client.Object, serviceName string, message string, additionalOffset ...int) { + EventuallyWithOffset(resolveOffset(additionalOffset), func() error { + return k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{}) + }).Should(MatchError("services \""+serviceName+"\" not found"), message, "Service exists when it should not") +} + func expectNoServices(ctx context.Context, parentResource client.Object, message string, serviceNames []string, additionalOffset ...int) { ConsistentlyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { for _, serviceName := range serviceNames { - g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message) + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message, "service", serviceName) + } + }).Should(Succeed()) +} +func eventuallyExpectNoServices(ctx context.Context, parentResource client.Object, message string, serviceNames []string, additionalOffset ...int) { + EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) { + for _, serviceName := range serviceNames { + g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message, "service", serviceName) } }).Should(Succeed()) } @@ -432,20 +452,6 @@ func expectIngressWithChecks(ctx context.Context, parentResource client.Object, additionalChecks(g, ingress) } }).Should(Succeed()) - - By("recreating the Ingress after it is deleted") - ExpectWithOffset(resolveOffset(additionalOffset), k8sClient.Delete(ctx, ingress)).To(Succeed()) - EventuallyWithOffset( - resolveOffset(additionalOffset), - func() (types.UID, error) { - newResource := &netv1.Ingress{} - err := k8sClient.Get(ctx, resourceKey(parentResource, ingressName), newResource) - if err != nil { - return "", err - } - return newResource.UID, nil - }).Should(And(Not(BeEmpty()), Not(Equal(ingress.UID))), "New Ingress, with new UID, not created.") - return ingress } @@ -468,6 +474,12 @@ func expectNoIngress(ctx context.Context, parentResource client.Object, ingressN }).Should(MatchError("ingresses.networking.k8s.io \""+ingressName+"\" not found"), "Ingress exists when it should not") } +func eventuallyExpectNoIngress(ctx context.Context, parentResource client.Object, ingressName string, additionalOffset ...int) { + EventuallyWithOffset(resolveOffset(additionalOffset), func() error { + return k8sClient.Get(ctx, resourceKey(parentResource, ingressName), &netv1.Ingress{}) + }).Should(MatchError("ingresses.networking.k8s.io \""+ingressName+"\" not found"), "Ingress exists when it should not") +} + func expectPodDisruptionBudget(ctx context.Context, parentResource client.Object, podDisruptionBudgetName string, selector *metav1.LabelSelector, maxUnavailable intstr.IntOrString, additionalOffset ...int) *policyv1.PodDisruptionBudget { return expectPodDisruptionBudgetWithChecks(ctx, parentResource, podDisruptionBudgetName, selector, maxUnavailable, nil, resolveOffset(additionalOffset)) } @@ -485,20 +497,6 @@ func expectPodDisruptionBudgetWithChecks(ctx context.Context, parentResource cli additionalChecks(g, podDisruptionBudget) } }).Should(Succeed()) - - By("recreating the PodDisruptionBudget after it is deleted") - ExpectWithOffset(resolveOffset(additionalOffset), k8sClient.Delete(ctx, podDisruptionBudget)).To(Succeed()) - EventuallyWithOffset( - resolveOffset(additionalOffset), - func() (types.UID, error) { - newResource := &policyv1.PodDisruptionBudget{} - err := k8sClient.Get(ctx, resourceKey(parentResource, podDisruptionBudgetName), newResource) - if err != nil { - return "", err - } - return newResource.UID, nil - }).Should(And(Not(BeEmpty()), Not(Equal(podDisruptionBudget.UID))), "New PodDisruptionBudget, with new UID, not created.") - return podDisruptionBudget } @@ -518,20 +516,6 @@ func expectConfigMapWithChecks(ctx context.Context, parentResource client.Object additionalChecks(g, configMap) } }).Should(Succeed()) - - By("recreating the ConfigMap after it is deleted") - ExpectWithOffset(resolveOffset(additionalOffset), k8sClient.Delete(ctx, configMap)).To(Succeed()) - EventuallyWithOffset( - resolveOffset(additionalOffset), - func() (types.UID, error) { - newResource := &corev1.ConfigMap{} - err := k8sClient.Get(ctx, resourceKey(parentResource, configMapName), newResource) - if err != nil { - return "", err - } - return newResource.UID, nil - }).Should(And(Not(BeEmpty()), Not(Equal(configMap.UID))), "New ConfigMap, with new UID, not created.") - return configMap } @@ -574,20 +558,6 @@ func expectDeploymentWithChecks(ctx context.Context, parentResource client.Objec additionalChecks(g, deployment) } }).Should(Succeed()) - - By("recreating the Deployment after it is deleted") - ExpectWithOffset(resolveOffset(additionalOffset), k8sClient.Delete(ctx, deployment)).To(Succeed()) - EventuallyWithOffset( - resolveOffset(additionalOffset), - func() (types.UID, error) { - newResource := &appsv1.Deployment{} - err := k8sClient.Get(ctx, resourceKey(parentResource, deploymentName), newResource) - if err != nil { - return "", err - } - return newResource.UID, nil - }).Should(And(Not(BeEmpty()), Not(Equal(deployment.UID))), "New Deployment, with new UID, not created.") - return deployment } diff --git a/tests/e2e/solrcloud_ingress_test.go b/tests/e2e/solrcloud_ingress_test.go new file mode 100644 index 00000000..e9a45abb --- /dev/null +++ b/tests/e2e/solrcloud_ingress_test.go @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package e2e + +import ( + "context" + solrv1beta1 "github.com/apache/solr-operator/api/v1beta1" + "github.com/apache/solr-operator/controllers/util" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" +) + +var _ = FDescribe("E2E - SolrCloud - Ingress", func() { + var ( + solrCloud *solrv1beta1.SolrCloud + ) + + BeforeEach(func() { + solrCloud = generateBaseSolrCloud(1) + solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Ingress, + UseExternalAddress: true, + DomainName: testDomain, + }, + } + }) + + JustBeforeEach(func(ctx context.Context) { + By("creating the SolrCloud") + Expect(k8sClient.Create(ctx, solrCloud)).To(Succeed()) + + DeferCleanup(func(ctx context.Context) { + cleanupTest(ctx, solrCloud) + }) + + By("Waiting for the SolrCloud to come up healthy") + solrCloud = expectSolrCloudToBeReady(ctx, solrCloud) + + By("creating a first Solr Collection") + createAndQueryCollection(ctx, solrCloud, "basic", 1, 1) + }) + + FContext("Can Remove Ingress and Services when changing addressability", func() { + + FIt("Can adapt to changing needs", func(ctx context.Context) { + By("testing the Solr StatefulSet") + statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName()) + // Pod Annotations test + 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.") + + By("testing the Solr Common Service") + expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + + By("ensuring the Solr Headless Service does not exist") + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + + By("making sure the individual Solr Node Services exist and route correctly") + nodeNames := solrCloud.GetAllSolrPodNames() + Expect(nodeNames).To(HaveLen(1), "SolrCloud has incorrect number of nodeNames.") + for _, nodeName := range nodeNames { + expectService(ctx, solrCloud, nodeName, util.MergeLabelsOrAnnotations(statefulSet.Spec.Selector.MatchLabels, map[string]string{"statefulset.kubernetes.io/pod-name": nodeName}), false) + } + + By("making sure Ingress was created correctly") + expectIngress(ctx, solrCloud, solrCloud.CommonIngressName()) + + By("Turning off node external addressability and making sure the node services are deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External.UseExternalAddress = false + found.Spec.SolrAddressability.External.HideNodes = true + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to not advertise the nodes externally.") + }) + + // Since node external addressability is off, but common external addressability is on, the ingress should exist, but the node services should not + expectIngress(ctx, solrCloud, solrCloud.CommonIngressName()) + + expectService(ctx, solrCloud, solrCloud.HeadlessServiceName(), statefulSet.Spec.Selector.MatchLabels, true) + + eventuallyExpectNoServices(ctx, solrCloud, "Node services shouldn't exist after the update, but they do.", nodeNames) + pod := expectPodNow(ctx, solrCloud, solrCloud.GetSolrPodName(0)) + Expect(pod.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.HeadlessServiceType)) + + By("Turning off common external addressability and making sure the ingress is deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability.External = nil + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to remove external addressability") + }) + eventuallyExpectNoIngress(ctx, solrCloud, solrCloud.CommonIngressName()) + + By("Turning back on common external addressability and making sure the headless service is deleted") + expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) { + found.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{ + External: &solrv1beta1.ExternalAddressability{ + Method: solrv1beta1.Ingress, + UseExternalAddress: true, + DomainName: testDomain, + }, + } + g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to add external addressability") + }) + expectIngress(ctx, solrCloud, solrCloud.CommonIngressName()) + + By("testing the Solr Common Service") + expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false) + + By("making sure the individual Solr Node Services are created") + + Expect(nodeNames).To(HaveLen(1), "SolrCloud has incorrect number of nodeNames.") + for _, nodeName := range nodeNames { + expectService(ctx, solrCloud, nodeName, util.MergeLabelsOrAnnotations(statefulSet.Spec.Selector.MatchLabels, map[string]string{"statefulset.kubernetes.io/pod-name": nodeName}), false) + } + + By("ensuring the Solr Headless Service is deleted after the pod is updated") + expectPodWithChecks(ctx, solrCloud, solrCloud.GetSolrPodName(0), func(g Gomega, pod *corev1.Pod) { + g.Expect(pod.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.PerNodeServiceType)) + }) + expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.") + + }) + }) +})