From 11249283c22a07f0850417c11f12220bf7c9e38b Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Tue, 24 Dec 2024 10:46:43 +0100 Subject: [PATCH] chore: reduce code duplication in agent controller Signed-off-by: Simon Pasquier --- Documentation/api.md | 36 ++++- bundle.yaml | 1 - ...onitoring.coreos.com_prometheusagents.yaml | 1 - ...onitoring.coreos.com_prometheusagents.yaml | 1 - .../prometheusagents-crd.json | 2 +- .../v1alpha1/prometheusagent_types.go | 15 +- .../v1alpha1/zz_generated.deepcopy.go | 2 +- .../v1alpha1/prometheusagentspec.go | 5 +- pkg/prometheus/agent/operator.go | 138 ++++++++---------- pkg/prometheus/server/operator.go | 18 ++- test/e2e/prometheusagent_test.go | 5 +- test/framework/prometheusagent.go | 2 +- 12 files changed, 118 insertions(+), 108 deletions(-) diff --git a/Documentation/api.md b/Documentation/api.md index e511840c2..6546066fb 100644 --- a/Documentation/api.md +++ b/Documentation/api.md @@ -18402,13 +18402,14 @@ PrometheusAgentSpec mode
-string + +PrometheusAgentMode + (Optional) -

Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s). -For now this field has no effect.

+

Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s).

(Alpha) Using this field requires the PrometheusAgentDaemonSet feature gate to be enabled.

@@ -26317,6 +26318,28 @@ int +

PrometheusAgentMode +(string alias)

+

+(Appears on:PrometheusAgentSpec) +

+
+
+ + + + + + + + + + + + +
ValueDescription

"DaemonSet"

Deploys PrometheusAgent as DaemonSet.

+

"StatefulSet"

Deploys PrometheusAgent as StatefulSet.

+

PrometheusAgentSpec

@@ -26338,13 +26361,14 @@ int mode
-string + +PrometheusAgentMode + (Optional) -

Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s). -For now this field has no effect.

+

Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s).

(Alpha) Using this field requires the PrometheusAgentDaemonSet feature gate to be enabled.

diff --git a/bundle.yaml b/bundle.yaml index ba9f917fa..1a03b60e5 100644 --- a/bundle.yaml +++ b/bundle.yaml @@ -25885,7 +25885,6 @@ spec: mode: description: |- Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s). - For now this field has no effect. (Alpha) Using this field requires the `PrometheusAgentDaemonSet` feature gate to be enabled. enum: diff --git a/example/prometheus-operator-crd-full/monitoring.coreos.com_prometheusagents.yaml b/example/prometheus-operator-crd-full/monitoring.coreos.com_prometheusagents.yaml index 519a3aaef..005d3cb46 100644 --- a/example/prometheus-operator-crd-full/monitoring.coreos.com_prometheusagents.yaml +++ b/example/prometheus-operator-crd-full/monitoring.coreos.com_prometheusagents.yaml @@ -4727,7 +4727,6 @@ spec: mode: description: |- Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s). - For now this field has no effect. (Alpha) Using this field requires the `PrometheusAgentDaemonSet` feature gate to be enabled. enum: diff --git a/example/prometheus-operator-crd/monitoring.coreos.com_prometheusagents.yaml b/example/prometheus-operator-crd/monitoring.coreos.com_prometheusagents.yaml index e5b3d2470..cc2c4e2aa 100644 --- a/example/prometheus-operator-crd/monitoring.coreos.com_prometheusagents.yaml +++ b/example/prometheus-operator-crd/monitoring.coreos.com_prometheusagents.yaml @@ -4728,7 +4728,6 @@ spec: mode: description: |- Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s). - For now this field has no effect. (Alpha) Using this field requires the `PrometheusAgentDaemonSet` feature gate to be enabled. enum: diff --git a/jsonnet/prometheus-operator/prometheusagents-crd.json b/jsonnet/prometheus-operator/prometheusagents-crd.json index 60e82fa46..3065e3abd 100644 --- a/jsonnet/prometheus-operator/prometheusagents-crd.json +++ b/jsonnet/prometheus-operator/prometheusagents-crd.json @@ -4016,7 +4016,7 @@ "type": "integer" }, "mode": { - "description": "Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s).\nFor now this field has no effect.\n\n(Alpha) Using this field requires the `PrometheusAgentDaemonSet` feature gate to be enabled.", + "description": "Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s).\n\n(Alpha) Using this field requires the `PrometheusAgentDaemonSet` feature gate to be enabled.", "enum": [ "StatefulSet", "DaemonSet" diff --git a/pkg/apis/monitoring/v1alpha1/prometheusagent_types.go b/pkg/apis/monitoring/v1alpha1/prometheusagent_types.go index 62b6c4a5a..ff095b663 100644 --- a/pkg/apis/monitoring/v1alpha1/prometheusagent_types.go +++ b/pkg/apis/monitoring/v1alpha1/prometheusagent_types.go @@ -94,13 +94,22 @@ func (l *PrometheusAgentList) DeepCopyObject() runtime.Object { // +k8s:openapi-gen=true type PrometheusAgentSpec struct { // Mode defines how the Prometheus operator deploys the PrometheusAgent pod(s). - // For now this field has no effect. // // (Alpha) Using this field requires the `PrometheusAgentDaemonSet` feature gate to be enabled. // - // +kubebuilder:validation:Enum=StatefulSet;DaemonSet // +optional - Mode *string `json:"mode,omitempty"` + Mode *PrometheusAgentMode `json:"mode,omitempty"` monitoringv1.CommonPrometheusFields `json:",inline"` } + +// +kubebuilder:validation:Enum=StatefulSet;DaemonSet +type PrometheusAgentMode string + +const ( + // Deploys PrometheusAgent as DaemonSet. + DaemonSetPrometheusAgentMode PrometheusAgentMode = "DaemonSet" + + // Deploys PrometheusAgent as StatefulSet. + StatefulSetPrometheusAgentMode PrometheusAgentMode = "StatefulSet" +) diff --git a/pkg/apis/monitoring/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/monitoring/v1alpha1/zz_generated.deepcopy.go index c4f09c542..84de8b144 100644 --- a/pkg/apis/monitoring/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/monitoring/v1alpha1/zz_generated.deepcopy.go @@ -1881,7 +1881,7 @@ func (in *PrometheusAgentSpec) DeepCopyInto(out *PrometheusAgentSpec) { *out = *in if in.Mode != nil { in, out := &in.Mode, &out.Mode - *out = new(string) + *out = new(PrometheusAgentMode) **out = **in } in.CommonPrometheusFields.DeepCopyInto(&out.CommonPrometheusFields) diff --git a/pkg/client/applyconfiguration/monitoring/v1alpha1/prometheusagentspec.go b/pkg/client/applyconfiguration/monitoring/v1alpha1/prometheusagentspec.go index 3ee632630..c43e3e9d9 100644 --- a/pkg/client/applyconfiguration/monitoring/v1alpha1/prometheusagentspec.go +++ b/pkg/client/applyconfiguration/monitoring/v1alpha1/prometheusagentspec.go @@ -18,6 +18,7 @@ package v1alpha1 import ( monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + v1alpha1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1alpha1" v1 "github.com/prometheus-operator/prometheus-operator/pkg/client/applyconfiguration/monitoring/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -27,7 +28,7 @@ import ( // PrometheusAgentSpecApplyConfiguration represents a declarative configuration of the PrometheusAgentSpec type for use // with apply. type PrometheusAgentSpecApplyConfiguration struct { - Mode *string `json:"mode,omitempty"` + Mode *v1alpha1.PrometheusAgentMode `json:"mode,omitempty"` v1.CommonPrometheusFieldsApplyConfiguration `json:",inline"` } @@ -40,7 +41,7 @@ func PrometheusAgentSpec() *PrometheusAgentSpecApplyConfiguration { // WithMode sets the Mode field in the declarative configuration to the given value // and returns the receiver, so that objects can be built by chaining "With" function invocations. // If called multiple times, the Mode field is set to the value of the last call. -func (b *PrometheusAgentSpecApplyConfiguration) WithMode(value string) *PrometheusAgentSpecApplyConfiguration { +func (b *PrometheusAgentSpecApplyConfiguration) WithMode(value v1alpha1.PrometheusAgentMode) *PrometheusAgentSpecApplyConfiguration { b.Mode = &value return b } diff --git a/pkg/prometheus/agent/operator.go b/pkg/prometheus/agent/operator.go index edd8258a0..b8c9fdf41 100644 --- a/pkg/prometheus/agent/operator.go +++ b/pkg/prometheus/agent/operator.go @@ -37,6 +37,7 @@ import ( "k8s.io/utils/ptr" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1alpha1" monitoringv1alpha1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1alpha1" "github.com/prometheus-operator/prometheus-operator/pkg/assets" monitoringclient "github.com/prometheus-operator/prometheus-operator/pkg/client/versioned" @@ -542,8 +543,14 @@ func (c *Operator) addHandlers() { } // Sync implements the operator.Syncer interface. -// TODO: Consider refactoring the common code between syncDaemonSet() and syncStatefulSet(). func (c *Operator) Sync(ctx context.Context, key string) error { + err := c.sync(ctx, key) + c.reconciliations.SetStatus(key, err) + + return err +} + +func (c *Operator) sync(ctx context.Context, key string) error { pobj, err := c.promInfs.Get(key) if apierrors.IsNotFound(err) { @@ -551,26 +558,13 @@ func (c *Operator) Sync(ctx context.Context, key string) error { // Dependent resources are cleaned up by K8s via OwnerReferences return nil } + if err != nil { return err } p := pobj.(*monitoringv1alpha1.PrometheusAgent) p = p.DeepCopy() - if ptr.Deref(p.Spec.Mode, "StatefulSet") == "DaemonSet" { - err = c.syncDaemonSet(ctx, key, p) - } else { - err = c.syncStatefulSet(ctx, key, p) - } - c.reconciliations.SetStatus(key, err) - return err -} - -func (c *Operator) syncDaemonSet(ctx context.Context, key string, p *monitoringv1alpha1.PrometheusAgent) error { - if !c.daemonSetFeatureGateEnabled { - return fmt.Errorf("feature gate for Prometheus Agent's DaemonSet mode is not enabled") - } - if err := k8sutil.AddTypeInformationToObject(p); err != nil { return fmt.Errorf("failed to set Prometheus type information: %w", err) } @@ -587,18 +581,29 @@ func (c *Operator) syncDaemonSet(ctx context.Context, key string, p *monitoringv return nil } - logger.Info("sync prometheus") + logger.Info("sync prometheusagent") - opts := []prompkg.ConfigGeneratorOption{prompkg.WithDaemonSet()} + if ptr.Deref(p.Spec.Mode, "") == v1alpha1.DaemonSetPrometheusAgentMode && !c.daemonSetFeatureGateEnabled { + return fmt.Errorf("feature gate for Prometheus Agent's DaemonSet mode is not enabled") + } + + // Generate the configuration data. + var ( + assetStore = assets.NewStoreBuilder(c.kclient.CoreV1(), c.kclient.CoreV1()) + opts = []prompkg.ConfigGeneratorOption{} + ) if c.endpointSliceSupported { opts = append(opts, prompkg.WithEndpointSliceSupport()) } - cg, err := prompkg.NewConfigGenerator(c.logger, p, opts...) + if ptr.Deref(p.Spec.Mode, "") == v1alpha1.DaemonSetPrometheusAgentMode { + opts = append(opts, prompkg.WithDaemonSet()) + } + + cg, err := prompkg.NewConfigGenerator(logger, p, opts...) if err != nil { return err } - assetStore := assets.NewStoreBuilder(c.kclient.CoreV1(), c.kclient.CoreV1()) if err := c.createOrUpdateConfigurationSecret(ctx, p, cg, assetStore); err != nil { return fmt.Errorf("creating config failed: %w", err) } @@ -612,14 +617,31 @@ func (c *Operator) syncDaemonSet(ctx context.Context, key string, p *monitoringv return fmt.Errorf("synchronizing web config secret failed: %w", err) } + switch ptr.Deref(p.Spec.Mode, "") { + case v1alpha1.DaemonSetPrometheusAgentMode: + err = c.syncDaemonSet(ctx, key, p, cg, tlsAssets) + default: + if err := operator.CheckStorageClass(ctx, c.canReadStorageClass, c.kclient, p.Spec.Storage); err != nil { + return err + } + + err = c.syncStatefulSet(ctx, key, p, cg, tlsAssets) + } + + return err +} + +func (c *Operator) syncDaemonSet(ctx context.Context, key string, p *monitoringv1alpha1.PrometheusAgent, cg *prompkg.ConfigGenerator, tlsAssets *operator.ShardedSecret) error { + logger := c.logger.With("key", key) + dsetClient := c.kclient.AppsV1().DaemonSets(p.Namespace) - logger.Debug("reconciling daemonset") - - _, err = c.dsetInfs.Get(keyToDaemonSetKey(p, key)) - exists := !apierrors.IsNotFound(err) - if err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("retrieving daemonset failed: %w", err) + var notFound bool + if _, err := c.dsetInfs.Get(keyToDaemonSetKey(p, key)); err != nil { + notFound = apierrors.IsNotFound(err) + if !notFound { + return fmt.Errorf("retrieving daemonset failed: %w", err) + } } dset, err := makeDaemonSet( @@ -631,8 +653,7 @@ func (c *Operator) syncDaemonSet(ctx context.Context, key string, p *monitoringv return fmt.Errorf("making daemonset failed: %w", err) } - if !exists { - logger.Debug("no current daemonset found") + if notFound { logger.Debug("creating daemonset") if _, err := dsetClient.Create(ctx, dset, metav1.CreateOptions{}); err != nil { return fmt.Errorf("creating daemonset failed: %w", err) @@ -668,52 +689,9 @@ func (c *Operator) syncDaemonSet(ctx context.Context, key string, p *monitoringv return nil } -func (c *Operator) syncStatefulSet(ctx context.Context, key string, p *monitoringv1alpha1.PrometheusAgent) error { - if err := k8sutil.AddTypeInformationToObject(p); err != nil { - return fmt.Errorf("failed to set Prometheus type information: %w", err) - } - +func (c *Operator) syncStatefulSet(ctx context.Context, key string, p *monitoringv1alpha1.PrometheusAgent, cg *prompkg.ConfigGenerator, tlsAssets *operator.ShardedSecret) error { logger := c.logger.With("key", key) - // Check if the Agent instance is marked for deletion. - if c.rr.DeletionInProgress(p) { - return nil - } - - if p.Spec.Paused { - logger.Info("the resource is paused, not reconciling") - return nil - } - - logger.Info("sync prometheus") - - if err := operator.CheckStorageClass(ctx, c.canReadStorageClass, c.kclient, p.Spec.Storage); err != nil { - return err - } - - opts := []prompkg.ConfigGeneratorOption{} - if c.endpointSliceSupported { - opts = append(opts, prompkg.WithEndpointSliceSupport()) - } - cg, err := prompkg.NewConfigGenerator(c.logger, p, opts...) - if err != nil { - return err - } - - assetStore := assets.NewStoreBuilder(c.kclient.CoreV1(), c.kclient.CoreV1()) - if err := c.createOrUpdateConfigurationSecret(ctx, p, cg, assetStore); err != nil { - return fmt.Errorf("creating config failed: %w", err) - } - - tlsAssets, err := operator.ReconcileShardedSecret(ctx, assetStore.TLSAssets(), c.kclient, prompkg.NewTLSAssetSecret(p, c.config)) - if err != nil { - return fmt.Errorf("failed to reconcile the TLS secrets: %w", err) - } - - if err := c.createOrUpdateWebConfigSecret(ctx, p); err != nil { - return fmt.Errorf("synchronizing web config secret failed: %w", err) - } - // Reconcile the governing service. svc := prompkg.BuildStatefulSetService( governingServiceName, @@ -733,10 +711,13 @@ func (c *Operator) syncStatefulSet(ctx context.Context, key string, p *monitorin logger := logger.With("statefulset", ssetName, "shard", fmt.Sprintf("%d", shard)) logger.Debug("reconciling statefulset") + var notFound bool obj, err := c.ssetInfs.Get(prompkg.KeyToStatefulSetKey(p, key, shard)) - exists := !apierrors.IsNotFound(err) - if err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("retrieving statefulset failed: %w", err) + if err != nil { + notFound = apierrors.IsNotFound(err) + if !notFound { + return fmt.Errorf("retrieving statefulset failed: %w", err) + } } existingStatefulSet := &appsv1.StatefulSet{} @@ -772,8 +753,7 @@ func (c *Operator) syncStatefulSet(ctx context.Context, key string, p *monitorin } operator.SanitizeSTS(sset) - if !exists { - logger.Debug("no current statefulset found") + if notFound { logger.Debug("creating statefulset") if _, err := ssetClient.Create(ctx, sset, metav1.CreateOptions{}); err != nil { return fmt.Errorf("creating statefulset failed: %w", err) @@ -822,7 +802,7 @@ func (c *Operator) syncStatefulSet(ctx context.Context, key string, p *monitorin ssets[ssetName] = struct{}{} } - err = c.ssetInfs.ListAllByNamespace(p.Namespace, labels.SelectorFromSet(labels.Set{prompkg.PrometheusNameLabelName: p.Name, prompkg.PrometheusModeLabeLName: prometheusMode}), func(obj interface{}) { + err := c.ssetInfs.ListAllByNamespace(p.Namespace, labels.SelectorFromSet(labels.Set{prompkg.PrometheusNameLabelName: p.Name, prompkg.PrometheusModeLabeLName: prometheusMode}), func(obj interface{}) { s := obj.(*appsv1.StatefulSet) if _, ok := ssets[s.Name]; ok { @@ -1034,7 +1014,7 @@ func (c *Operator) enqueueForMonitorNamespace(nsName string) { // enqueueForNamespace enqueues all Prometheus object keys that belong to the // given namespace or select objects in the given namespace. func (c *Operator) enqueueForNamespace(store cache.Store, nsName string) { - nsObject, exists, err := store.GetByKey(nsName) + nsObject, found, err := store.GetByKey(nsName) if err != nil { c.logger.Error( "get namespace to enqueue Prometheus instances failed", @@ -1042,7 +1022,7 @@ func (c *Operator) enqueueForNamespace(store cache.Store, nsName string) { ) return } - if !exists { + if !found { c.logger.Error(fmt.Sprintf("get namespace to enqueue Prometheus instances failed: namespace %q does not exist", nsName)) return } diff --git a/pkg/prometheus/server/operator.go b/pkg/prometheus/server/operator.go index 88932d62d..89e988cdd 100644 --- a/pkg/prometheus/server/operator.go +++ b/pkg/prometheus/server/operator.go @@ -557,7 +557,7 @@ func (c *Operator) enqueueForMonitorNamespace(nsName string) { // enqueueForNamespace enqueues all Prometheus object keys that belong to the // given namespace or select objects in the given namespace. func (c *Operator) enqueueForNamespace(store cache.Store, nsName string) { - nsObject, exists, err := store.GetByKey(nsName) + nsObject, found, err := store.GetByKey(nsName) if err != nil { c.logger.Error( "get namespace to enqueue Prometheus instances failed", @@ -565,7 +565,7 @@ func (c *Operator) enqueueForNamespace(store cache.Store, nsName string) { ) return } - if !exists { + if !found { c.logger.Error( fmt.Sprintf("get namespace to enqueue Prometheus instances failed: namespace %q does not exist", nsName), ) @@ -775,7 +775,7 @@ func (c *Operator) sync(ctx context.Context, key string) error { if c.endpointSliceSupported { opts = append(opts, prompkg.WithEndpointSliceSupport()) } - cg, err := prompkg.NewConfigGenerator(c.logger, p, opts...) + cg, err := prompkg.NewConfigGenerator(logger, p, opts...) if err != nil { return err } @@ -825,10 +825,13 @@ func (c *Operator) sync(ctx context.Context, key string) error { logger := logger.With("statefulset", ssetName, "shard", fmt.Sprintf("%d", shard)) logger.Debug("reconciling statefulset") + var notFound bool obj, err := c.ssetInfs.Get(prompkg.KeyToStatefulSetKey(p, key, shard)) - exists := !apierrors.IsNotFound(err) - if err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("retrieving statefulset failed: %w", err) + if err != nil { + notFound = apierrors.IsNotFound(err) + if !notFound { + return fmt.Errorf("retrieving statefulset failed: %w", err) + } } existingStatefulSet := &appsv1.StatefulSet{} @@ -865,8 +868,7 @@ func (c *Operator) sync(ctx context.Context, key string) error { } operator.SanitizeSTS(sset) - if !exists { - logger.Debug("no current statefulset found") + if notFound { logger.Debug("creating statefulset") if _, err := ssetClient.Create(ctx, sset, metav1.CreateOptions{}); err != nil { return fmt.Errorf("creating statefulset failed: %w", err) diff --git a/test/e2e/prometheusagent_test.go b/test/e2e/prometheusagent_test.go index cc5571d50..37cf22e68 100644 --- a/test/e2e/prometheusagent_test.go +++ b/test/e2e/prometheusagent_test.go @@ -125,12 +125,10 @@ func testAgentCheckStorageClass(t *testing.T) { name := "test" prometheusAgentCRD := framework.MakeBasicPrometheusAgent(ns, name, name, 1) - prometheusAgentCRD, err := framework.CreatePrometheusAgentAndWaitUntilReady(ctx, ns, prometheusAgentCRD) require.NoError(t, err) // Invalid storageclass e2e test - _, err = framework.PatchPrometheusAgent( context.Background(), prometheusAgentCRD.Name, @@ -153,6 +151,7 @@ func testAgentCheckStorageClass(t *testing.T) { }, ) require.NoError(t, err) + var loopError error err = wait.PollUntilContextTimeout(ctx, 5*time.Second, framework.DefaultTimeout, true, func(ctx context.Context) (bool, error) { current, err := framework.MonClientV1alpha1.PrometheusAgents(ns).Get(ctx, name, metav1.GetOptions{}) @@ -167,7 +166,6 @@ func testAgentCheckStorageClass(t *testing.T) { return false, nil }) - require.NoError(t, err, "%v: %v", err, loopError) } @@ -235,7 +233,6 @@ func testPromAgentDaemonSetResourceUpdate(t *testing.T) { p.Name, ns, monitoringv1alpha1.PrometheusAgentSpec{ - Mode: ptr.To("DaemonSet"), CommonPrometheusFields: monitoringv1.CommonPrometheusFields{ Resources: v1.ResourceRequirements{ Requests: v1.ResourceList{ diff --git a/test/framework/prometheusagent.go b/test/framework/prometheusagent.go index 87f635e12..bdbada29f 100644 --- a/test/framework/prometheusagent.go +++ b/test/framework/prometheusagent.go @@ -75,7 +75,7 @@ func (f *Framework) MakeBasicPrometheusAgentDaemonSet(ns, name string) *monitori Annotations: map[string]string{}, }, Spec: monitoringv1alpha1.PrometheusAgentSpec{ - Mode: ptr.To("DaemonSet"), + Mode: ptr.To(monitoringv1alpha1.DaemonSetPrometheusAgentMode), CommonPrometheusFields: monitoringv1.CommonPrometheusFields{ Version: operator.DefaultPrometheusVersion, ServiceAccountName: "prometheus",