Copilot commented on code in PR #880:
URL: https://github.com/apache/solr-operator/pull/880#discussion_r4473998200
##########
controllers/solrcloud_controller.go:
##########
@@ -212,6 +234,10 @@ func (r *SolrCloudReconciler) Reconcile(ctx
context.Context, req ctrl.Request) (
}
}
+ if err = r.reconcilePodMonitor(ctx, instance, logger); err != nil {
+ return requeueOrNot, err
+ }
Review Comment:
A PodMonitor error here stops reconciliation before the ConfigMap, security
configuration and StatefulSet are managed. For example, an invalid value in
`podMonitor.labels` passes the SolrCloud schema but is rejected when creating
the PodMonitor, blocking cloud creation or scaling. Report monitoring failures
as warning events and schedule retries while continuing the rest of the cloud
reconciliation.
##########
controllers/solrcloud_controller.go:
##########
@@ -1001,6 +1027,82 @@ func (r *SolrCloudReconciler) reconcileNodeService(ctx
context.Context, logger l
return nil, ip
}
+
+// reconcilePodMonitor creates, updates or deletes the PodMonitor for the
cloud.
+func (r *SolrCloudReconciler) reconcilePodMonitor(ctx context.Context,
instance *solrv1beta1.SolrCloud, logger logr.Logger) error {
+ if !instance.UsesPodMonitor() {
+ return r.deletePodMonitor(ctx, instance, logger)
+ }
+
+ // Keep managing the rest of the SolrCloud: metrics are not worth
stalling a Solr cluster over.
+ if !usePrometheusCRD {
+ msg := "Cannot create a PodMonitor, as the Solr Operator is not
configured to use the Prometheus Operator CRDs. Install the Solr Operator with
`--prometheus-operator=true` to enable this."
+ logger.Error(nil, msg)
+ r.Recorder.Event(instance, corev1.EventTypeWarning,
"MissingPrometheusCRD", msg)
+ return nil
+ }
+
+ // TODO: Gate this on Solr 10, once #827 has landed and the operator
can parse the Solr version.
+ // Solr 9 serves no prometheus-format metrics, so scraping it only
produces errors. Deleting rather
+ // than skipping also covers a SolrCloud downgraded from Solr 10 back
to Solr 9.
+ //
+ // if major, err := instance.SolrMajorVersion(); err != nil {
+ // return err
+ // } else if major < 10 {
+ // msg := fmt.Sprintf("Not creating a PodMonitor: Solr %d
does not serve prometheus-format metrics at %s. Use a SolrPrometheusExporter
instead.", major, util.SolrMetricsPath)
+ // logger.Error(nil, msg)
+ // r.Recorder.Event(instance, corev1.EventTypeWarning,
"UnsupportedSolrVersion", msg)
+ // return r.deletePodMonitor(ctx, instance, logger)
+ // }
+
+ podMonitor := util.GeneratePodMonitor(instance)
+ podMonitorLogger := logger.WithValues("podMonitor", podMonitor.Name)
+
+ foundPodMonitor := &monitoringv1.PodMonitor{}
+ err := r.Get(ctx, types.NamespacedName{Name: podMonitor.Name,
Namespace: podMonitor.Namespace}, foundPodMonitor)
+ if err != nil && errors.IsNotFound(err) {
+ podMonitorLogger.Info("Creating PodMonitor")
+ if err = controllerutil.SetControllerReference(instance,
podMonitor, r.Scheme); err == nil {
+ err = r.Create(ctx, podMonitor)
+ }
+ } else if err == nil {
+ var needsUpdate bool
+ needsUpdate, err = util.OvertakeControllerRef(instance,
foundPodMonitor, r.Scheme)
+ needsUpdate = util.CopyPodMonitorFields(podMonitor,
foundPodMonitor, podMonitorLogger) || needsUpdate
+
+ // Update the found PodMonitor and write the result back if
there are any changes
+ if needsUpdate && err == nil {
+ podMonitorLogger.Info("Updating PodMonitor")
+ err = r.Update(ctx, foundPodMonitor)
+ }
+ }
+ return err
+}
+
+// deletePodMonitor removes a PodMonitor that the cloud no longer wants. The
operator cannot have
+// created one without the Prometheus Operator CRDs, so there is nothing to
look for in that case.
+func (r *SolrCloudReconciler) deletePodMonitor(ctx context.Context, instance
*solrv1beta1.SolrCloud, logger logr.Logger) error {
+ if !usePrometheusCRD {
+ return nil
+ }
+
+ podMonitor := &monitoringv1.PodMonitor{}
+ err := r.Get(ctx, types.NamespacedName{Name: instance.PodMonitorName(),
Namespace: instance.Namespace}, podMonitor)
+ if err != nil {
+ if errors.IsNotFound(err) {
+ return nil
+ }
+ return err
+ }
+
+ logger.Info("Deleting PodMonitor, as it is no longer enabled for the
SolrCloud", "podMonitor", podMonitor.Name)
+ err = r.Delete(ctx, podMonitor)
Review Comment:
This deletes any PodMonitor with the generated name, even if the SolrCloud
has never enabled PodMonitor management. Since Helm enables this integration
automatically when the CRDs exist, upgrading can delete a hand-managed
`<cloud>-solrcloud-metrics` PodMonitor without opting in. Check that the
PodMonitor is controlled by this SolrCloud before deleting it.
##########
controllers/util/solr_metrics_util.go:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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 util
+
+import (
+ "strconv"
+
+ monitoringv1
"github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
+ corev1 "k8s.io/api/core/v1"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/utils/ptr"
+
+ solr "github.com/apache/solr-operator/api/v1beta1"
+)
+
+const (
+ // SolrMetricsPath is the path of Solr's metrics API
+ SolrMetricsPath = "/solr/admin/metrics"
+
+ // Solr 10 answers in the Prometheus exposition format even without
this parameter, but asking for
+ // it explicitly means a change of that default cannot silently break
scraping.
+ SolrMetricsWtParam = "wt"
+ SolrMetricsWtPrometheus = "prometheus"
+
+ PrometheusScrapeAnnotation = "prometheus.io/scrape"
+ PrometheusSchemeAnnotation = "prometheus.io/scheme"
+ PrometheusPathAnnotation = "prometheus.io/path"
+ PrometheusPortAnnotation = "prometheus.io/port"
+ // The wt parameter needs an annotation of its own: Prometheus puts the
path annotation in
+ // __metrics_path__, where a "?" is escaped. A scrape config maps this
one to the __param_wt label.
+ PrometheusWtParamAnnotation = "prometheus.io/param-" +
SolrMetricsWtParam
+)
+
+// PrometheusScrapeAnnotations returns the prometheus.io/* annotations
describing how to scrape the
+// metrics API of a Solr pod. They are a convention, so only a scrape config
acting on them has effect.
+func PrometheusScrapeAnnotations(solrCloud *solr.SolrCloud) map[string]string {
+ return map[string]string{
+ PrometheusScrapeAnnotation: "true",
+ PrometheusSchemeAnnotation: solrCloud.UrlScheme(false),
+ PrometheusPathAnnotation: SolrMetricsPath,
+ PrometheusPortAnnotation:
strconv.Itoa(solrCloud.Spec.SolrAddressability.PodPort),
+ PrometheusWtParamAnnotation: SolrMetricsWtPrometheus,
+ }
+}
+
+// GeneratePodMonitor returns a new monitoringv1.PodMonitor for the metrics
API of the SolrCloud's
+// pods. One PodMonitor covers the whole cloud: the Prometheus Operator turns
it into a single scrape
+// job that discovers every Solr pod as its own target, following the cloud as
it scales.
+func GeneratePodMonitor(solrCloud *solr.SolrCloud) *monitoringv1.PodMonitor {
+ options := solrCloud.Spec.Prometheus.PodMonitor
+
+ endpoint := monitoringv1.PodMetricsEndpoint{
+ Port: ptr.To(SolrClientPortName),
+ Path: SolrMetricsPath,
+ Params: map[string][]string{SolrMetricsWtParam:
{SolrMetricsWtPrometheus}},
+ Scheme:
ptr.To(monitoringv1.Scheme(solrCloud.UrlScheme(false))),
+ Interval: monitoringv1.Duration(options.Interval),
+ ScrapeTimeout: monitoringv1.Duration(options.ScrapeTimeout),
+ }
+
+ // The secret of the SolrCloud is used unless another one is given, as
the bootstrapped
+ // security.json grants its user access to /admin/metrics.
+ if solrCloud.Spec.SolrSecurity != nil &&
solrCloud.Spec.SolrSecurity.AuthenticationType == solr.Basic {
+ basicAuthSecret := options.BasicAuthSecret
+ if basicAuthSecret == "" {
+ basicAuthSecret = solrCloud.BasicAuthSecretName()
+ }
+ endpoint.BasicAuth = &monitoringv1.BasicAuth{
+ Username: corev1.SecretKeySelector{
+ LocalObjectReference:
corev1.LocalObjectReference{Name: basicAuthSecret},
+ Key:
corev1.BasicAuthUsernameKey,
+ },
+ Password: corev1.SecretKeySelector{
+ LocalObjectReference:
corev1.LocalObjectReference{Name: basicAuthSecret},
+ Key:
corev1.BasicAuthPasswordKey,
+ },
+ }
+ }
+
+ if tlsConfig := options.TLSConfig; tlsConfig != nil {
+ endpoint.TLSConfig = &monitoringv1.SafeTLSConfig{
+ InsecureSkipVerify:
ptr.To(tlsConfig.InsecureSkipVerify),
+ }
Review Comment:
For a SolrCloud with `spec.solrTLS.clientAuth: Need`, this produces an HTTPS
scrape configuration without a client certificate or key, so scrapes fail
during the TLS handshake even when `caSecret` is correct. The new TLS options
cannot supply those credentials, and manually adding them to the PodMonitor is
undone by drift correction. Expose PEM client-certificate/key secret selectors
and populate `SafeTLSConfig.Cert` and `KeySecret` so PodMonitors can scrape
this supported Solr TLS configuration.
--
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]
