enhancement: allow toggling custom config deprecation behaviour #6955

rexagod · 2024-09-22T12:55:58Z

Description

Allow toggling custom config deprecation behaviour; helpful in scenarios where a controller disables all service discovery methods.

Type of change

CHANGE (fix or feature that would cause existing functionality to not work as expected)
FEATURE (non-breaking change which adds functionality)
BUGFIX (non-breaking change which fixes an issue)
ENHANCEMENT (non-breaking change which improves existing functionality)
NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Users can toggle Prometheus controller's custom configuration deprecation behavior using the "--deprecate-custom-configuration" flag.

simonpasquier · 2024-09-25T10:00:55Z

Documentation/operator.md

@@ -49,6 +49,8 @@ Usage of ./operator:
    	Value used by the operator to filter Alertmanager, Prometheus, PrometheusAgent and ThanosRuler objects that it should reconcile. If the value isn't empty, the operator only reconciles objects with an operator.prometheus.io/controller-id annotation of the same value. Otherwise the operator reconciles all objects without the annotation or with an empty annotation value.
  -deny-namespaces value
    	Namespaces not to scope the interaction of the Prometheus Operator (deny list). This is mutually exclusive with --namespaces.
+  -deprecate-custom-configuration


I'd suggest to name the flag -disable-unmanaged-prometheus-configuration and default to false.

mviswanathsai · 2024-09-30T18:15:19Z

Not sure I understand what the flag is for, can you elaborate a bit? I lack the context behind this.

simonpasquier · 2024-10-03T13:26:22Z

To provide more context, there's a legacy behavior which instructs the operator to not manage the Prometheus configuration when all selectors (ruleSelector, serviceMonitorSelector, ...) are nil. In this case, the user is supposed to build by hand the secret holding the (gzipped) configuration. It can be a pain to debug if you run into this mode by accident because the operator doesn't complain but the Prometheus statefulset will never spin up pods if the secret is missing.
This is very much deprecated now since we have additionalScrapeConfigs + ScrapeConfig CRD but removing it would break users. Adding a flag to say "don't support unamanaged configuration mode" improves the UX.

rexagod · 2024-10-07T15:08:59Z

This PR is rebased on #6992 to address an issue that crashed the make generate target.

Allow toggling custom config deprecation behaviour; helpful in scenarios where a controller disables all service discovery methods. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

simonpasquier

As a follow-up change, I suggest to enhance the Reconciled status condition so that when all selectors are nil, it gets reflected in the reconciled message. E.g. it's not an error but it can be hard to find out what you missed. Something like:

status:
  availableReplicas: 2
  conditions:
  - lastTransitionTime: "2024-10-21T14:45:40Z"
    message: ""
    observedGeneration: 2
    reason: ""
    status: "True"
    type: Available
  - lastTransitionTime: "2024-10-21T14:45:40Z"
    message: "None of the scrape resource selectors are specified."
    observedGeneration: 2
    reason: "NoScrapeResourceSelected"
    status: "True"
    type: Reconciled

simonpasquier · 2024-10-21T14:43:44Z

cmd/operator/main.go

@@ -183,7 +185,7 @@ func parseFlags(fs *flag.FlagSet) {
 	fs.Var(&cfg.SecretListWatchLabelSelector, "secret-label-selector", "Label selector to filter Secrets to watch")

 	fs.Float64Var(&memlimitRatio, "auto-gomemlimit-ratio", defaultMemlimitRatio, "The ratio of reserved GOMEMLIMIT memory to the detected maximum container or system memory. The value should be greater than 0.0 and less than 1.0. Default: 0.0 (disabled).")
-
+	fs.BoolVar(&disableUnmanagedPrometheusConfiguration, "disable-unmanaged-prometheus-configuration", false, "Toggle the legacy deprecation behaviour for unmanaged Prometheus configuration. Default: false.")


Suggested change

fs.BoolVar(&disableUnmanagedPrometheusConfiguration, "disable-unmanaged-prometheus-configuration", false, "Toggle the legacy deprecation behaviour for unmanaged Prometheus configuration. Default: false.")

fs.BoolVar(&disableUnmanagedPrometheusConfiguration, "disable-unmanaged-prometheus-configuration", false, "Disable support for unmanaged Prometheus configuration when all resource selectors are nil. Default: false.")

simonpasquier · 2024-10-21T14:44:54Z

pkg/prometheus/server/operator.go

@@ -117,6 +119,14 @@ func WithStorageClassValidation() ControllerOption {
 	}
 }

+// WithDisableUnmanagedPrometheusConfiguration tells that the controller should deprecate
+// the custom configuration.
+func WithDisableUnmanagedPrometheusConfiguration() ControllerOption {


Suggested change

func WithDisableUnmanagedPrometheusConfiguration() ControllerOption {

func WithUnmanagedConfigurationDisabled() ControllerOption {

simonpasquier · 2024-10-21T14:45:08Z

pkg/prometheus/server/operator.go

@@ -91,6 +91,8 @@ type Operator struct {
 	canReadStorageClass    bool

 	eventRecorder record.EventRecorder
+
+	disableUnmanagedPrometheusConfiguration bool


Suggested change

disableUnmanagedPrometheusConfiguration bool

disableUnmanagedConfiguration bool

simonpasquier · 2024-10-21T14:46:26Z

pkg/prometheus/server/operator.go

@@ -954,7 +964,7 @@ func (c *Operator) UpdateStatus(ctx context.Context, key string) error {
 	return nil
 }

-func logDeprecatedFields(logger *slog.Logger, p *monitoringv1.Prometheus) {
+func logDeprecatedFields(logger *slog.Logger, p *monitoringv1.Prometheus, disableUnmanagedPrometheusConfiguration bool) {


(suggestion) let's make it a pointer receiver function.

Suggested change

func logDeprecatedFields(logger *slog.Logger, p *monitoringv1.Prometheus, disableUnmanagedPrometheusConfiguration bool) {

func (c *Operator) logDeprecatedFields(logger *slog.Logger, p *monitoringv1.Prometheus) {

simonpasquier · 2024-10-21T14:48:47Z

cmd/operator/main.go

@@ -274,6 +276,9 @@ func run(fs *flag.FlagSet) int {
 		promControllerOptions         = []prometheuscontroller.ControllerOption{}
 		thanosControllerOptions       = []thanoscontroller.ControllerOption{}
 	)
+	if disableUnmanagedPrometheusConfiguration {
+		promControllerOptions = append(promControllerOptions, prometheuscontroller.WithDisableUnmanagedPrometheusConfiguration())


it might be good to log an info message saying that unmanaged configuration is disabled.

Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

simonpasquier

I've got just a few last nits otherwise LGTM.

simonpasquier · 2024-10-23T09:04:56Z

Documentation/operator.md

@@ -49,6 +49,8 @@ Usage of ./operator:
    	Value used by the operator to filter Alertmanager, Prometheus, PrometheusAgent and ThanosRuler objects that it should reconcile. If the value isn't empty, the operator only reconciles objects with an operator.prometheus.io/controller-id annotation of the same value. Otherwise the operator reconciles all objects without the annotation or with an empty annotation value.
  -deny-namespaces value
    	Namespaces not to scope the interaction of the Prometheus Operator (deny list). This is mutually exclusive with --namespaces.
+  -disable-unmanaged-prometheus-configuration
+    	Disable support for unmanaged Prometheus configuration when all resource selectors are nil. Default: false.


(nit) let's provide more context why you might want to use the flag.

Suggested change

Disable support for unmanaged Prometheus configuration when all resource selectors are nil. Default: false.

Disable support for unmanaged Prometheus configuration when all resource selectors are nil. As stated in the API documentation, unmanaged Prometheus configuration is a deprecated feature which can be avoided with "additionalScrapeConfigs` or the ScrapeConfig CRD. Default: false.

simonpasquier · 2024-10-23T09:06:10Z

pkg/prometheus/server/operator.go

@@ -91,6 +91,8 @@ type Operator struct {
 	canReadStorageClass    bool

 	eventRecorder record.EventRecorder
+
+	disableUnmanagedConfiguration bool


(nit) can we move it next to the other bool fields (e.g. at L92)?

simonpasquier · 2024-10-23T09:06:36Z

pkg/prometheus/server/operator.go

@@ -117,6 +119,14 @@ func WithStorageClassValidation() ControllerOption {
 	}
 }

+// WithUnmanagedConfigurationDisabled tells that the controller should deprecate
+// the custom configuration.
+func WithUnmanagedConfigurationDisabled() ControllerOption {


(nit)

Suggested change

func WithUnmanagedConfigurationDisabled() ControllerOption {

func WithoutUnmanagedConfiguration() ControllerOption {

…ehaviour

…ation behaviour

simonpasquier

A few nits but lgtm

simonpasquier · 2024-11-06T16:22:02Z

Documentation/operator.md

@@ -49,6 +49,8 @@ Usage of ./operator:
    	Value used by the operator to filter Alertmanager, Prometheus, PrometheusAgent and ThanosRuler objects that it should reconcile. If the value isn't empty, the operator only reconciles objects with an operator.prometheus.io/controller-id annotation of the same value. Otherwise the operator reconciles all objects without the annotation or with an empty annotation value.
  -deny-namespaces value
    	Namespaces not to scope the interaction of the Prometheus Operator (deny list). This is mutually exclusive with --namespaces.
+  -disable-unmanaged-prometheus-configuration additionalScrapeConfigs


hmm, not sure why we have additionalScrapeConfigs here but it looks like removing the backticks in the help text should fix it 🤯

simonpasquier · 2024-11-06T16:25:56Z

cmd/operator/main.go

@@ -183,7 +185,7 @@ func parseFlags(fs *flag.FlagSet) {
 	fs.Var(&cfg.SecretListWatchLabelSelector, "secret-label-selector", "Label selector to filter Secrets to watch")

 	fs.Float64Var(&memlimitRatio, "auto-gomemlimit-ratio", defaultMemlimitRatio, "The ratio of reserved GOMEMLIMIT memory to the detected maximum container or system memory. The value should be greater than 0.0 and less than 1.0. Default: 0.0 (disabled).")
-
+	fs.BoolVar(&disableUnmanagedPrometheusConfiguration, "disable-unmanaged-prometheus-configuration", false, "Disable support for unmanaged Prometheus configuration when all resource selectors are nil. As stated in the API documentation, unmanaged Prometheus configuration is a deprecated feature which can be avoided with `additionalScrapeConfigs` or the ScrapeConfig CRD. Default: false.")


Suggested change

fs.BoolVar(&disableUnmanagedPrometheusConfiguration, "disable-unmanaged-prometheus-configuration", false, "Disable support for unmanaged Prometheus configuration when all resource selectors are nil. As stated in the API documentation, unmanaged Prometheus configuration is a deprecated feature which can be avoided with `additionalScrapeConfigs` or the ScrapeConfig CRD. Default: false.")

fs.BoolVar(&disableUnmanagedPrometheusConfiguration, "disable-unmanaged-prometheus-configuration", false, "Disable support for unmanaged Prometheus configuration when all resource selectors are nil. As stated in the API documentation, unmanaged Prometheus configuration is a deprecated feature which can be avoided with '.spec.additionalScrapeConfigs' or the ScrapeConfig CRD. Default: false.")

simonpasquier · 2024-11-06T16:27:03Z

cmd/operator/main.go

@@ -274,6 +276,10 @@ func run(fs *flag.FlagSet) int {
 		promControllerOptions         = []prometheuscontroller.ControllerOption{}
 		thanosControllerOptions       = []thanoscontroller.ControllerOption{}
 	)
+	if disableUnmanagedPrometheusConfiguration {
+		logger.Info("unmanaged Prometheus configuration is disabled")


Suggested change

logger.Info("unmanaged Prometheus configuration is disabled")

logger.Info("Disabling support for unmanaged Prometheus configurations")

simonpasquier · 2024-11-06T16:27:58Z

pkg/prometheus/server/operator.go

+// WithoutUnmanagedConfiguration tells that the controller should deprecate
+// the custom configuration.


Suggested change

// WithoutUnmanagedConfiguration tells that the controller should deprecate

// the custom configuration.

// WithoutUnmanagedConfiguration tells that the controller should not support

// unmanaged configurations.

… deprecation behaviour

simonpasquier · 2024-11-07T15:52:25Z

Thanks!

* enhancement: allow toggling custom config deprecation behaviour --------- Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

pull-request-size bot added the size/S label Sep 22, 2024

rexagod force-pushed the deprecate-custom-config branch from e58644d to b667bf5 Compare September 22, 2024 18:30

simonpasquier reviewed Sep 25, 2024

View reviewed changes

rexagod force-pushed the deprecate-custom-config branch from f6de19a to b1e4375 Compare October 7, 2024 14:55

pull-request-size bot added size/M and removed size/S labels Oct 7, 2024

rexagod force-pushed the deprecate-custom-config branch from b1e4375 to b5797a8 Compare October 7, 2024 15:06

rexagod marked this pull request as ready for review October 8, 2024 06:07

rexagod requested a review from a team as a code owner October 8, 2024 06:07

rexagod force-pushed the deprecate-custom-config branch 2 times, most recently from 28fe238 to aee0477 Compare October 8, 2024 07:13

rexagod force-pushed the deprecate-custom-config branch from aee0477 to 7e9f70d Compare October 21, 2024 08:41

enhancement: allow toggling custom config deprecation behaviour

Loading
Loading status checks…

7a052f5

Allow toggling custom config deprecation behaviour; helpful in scenarios where a controller disables all service discovery methods. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

rexagod force-pushed the deprecate-custom-config branch from 7e9f70d to 7a052f5 Compare October 21, 2024 08:42

pull-request-size bot added size/S and removed size/M labels Oct 21, 2024

rexagod requested a review from simonpasquier October 21, 2024 09:32

simonpasquier reviewed Oct 21, 2024

View reviewed changes

rexagod mentioned this pull request Oct 22, 2024

enhancement: Reflect legacy behavior in .status #7034

Open

5 tasks

fixup! enhancement: allow toggling custom config deprecation behaviour

Loading
Loading status checks…

39ff9a0

Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

rexagod force-pushed the deprecate-custom-config branch from 83864a0 to 39ff9a0 Compare October 22, 2024 21:31

simonpasquier reviewed Oct 23, 2024

View reviewed changes

fixup! fixup! enhancement: allow toggling custom config deprecation b…

d7d7a51

…ehaviour

pull-request-size bot added size/M and removed size/S labels Nov 4, 2024

fixup! fixup! fixup! enhancement: allow toggling custom config deprec…

Loading
Loading status checks…

bc8c0c3

…ation behaviour

rexagod force-pushed the deprecate-custom-config branch from 88d46cc to bc8c0c3 Compare November 4, 2024 08:04

rexagod requested a review from simonpasquier November 5, 2024 08:32

simonpasquier reviewed Nov 6, 2024

View reviewed changes

fixup! fixup! fixup! fixup! enhancement: allow toggling custom config…

Loading
Loading status checks…

50d05b2

… deprecation behaviour

simonpasquier approved these changes Nov 7, 2024

View reviewed changes

simonpasquier enabled auto-merge (squash) November 7, 2024 15:52

simonpasquier merged commit f5218e1 into prometheus-operator:main Nov 7, 2024
23 checks passed

rexagod mentioned this pull request Dec 11, 2024

COO-5: disable custom config deprecation flag rhobs/obo-prometheus-operator#76

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement: allow toggling custom config deprecation behaviour #6955

enhancement: allow toggling custom config deprecation behaviour #6955

rexagod commented Sep 22, 2024

simonpasquier Sep 25, 2024

mviswanathsai commented Sep 30, 2024

simonpasquier commented Oct 3, 2024

rexagod commented Oct 7, 2024

simonpasquier left a comment

simonpasquier Oct 21, 2024

simonpasquier Oct 21, 2024

simonpasquier Oct 21, 2024

simonpasquier Oct 21, 2024

simonpasquier Oct 21, 2024

simonpasquier left a comment

simonpasquier Oct 23, 2024

simonpasquier Oct 23, 2024

simonpasquier Oct 23, 2024

simonpasquier left a comment

simonpasquier Nov 6, 2024

simonpasquier Nov 6, 2024

simonpasquier Nov 6, 2024

simonpasquier Nov 6, 2024

simonpasquier commented Nov 7, 2024

	fs.BoolVar(&disableUnmanagedPrometheusConfiguration, "disable-unmanaged-prometheus-configuration", false, "Toggle the legacy deprecation behaviour for unmanaged Prometheus configuration. Default: false.")
	fs.BoolVar(&disableUnmanagedPrometheusConfiguration, "disable-unmanaged-prometheus-configuration", false, "Disable support for unmanaged Prometheus configuration when all resource selectors are nil. Default: false.")

	func WithDisableUnmanagedPrometheusConfiguration() ControllerOption {
	func WithUnmanagedConfigurationDisabled() ControllerOption {

	disableUnmanagedPrometheusConfiguration bool
	disableUnmanagedConfiguration bool

	func logDeprecatedFields(logger slog.Logger, p monitoringv1.Prometheus, disableUnmanagedPrometheusConfiguration bool) {
	func (c Operator) logDeprecatedFields(logger slog.Logger, p *monitoringv1.Prometheus) {

	Disable support for unmanaged Prometheus configuration when all resource selectors are nil. Default: false.
	Disable support for unmanaged Prometheus configuration when all resource selectors are nil. As stated in the API documentation, unmanaged Prometheus configuration is a deprecated feature which can be avoided with "additionalScrapeConfigs` or the ScrapeConfig CRD. Default: false.

	func WithUnmanagedConfigurationDisabled() ControllerOption {
	func WithoutUnmanagedConfiguration() ControllerOption {

	logger.Info("unmanaged Prometheus configuration is disabled")
	logger.Info("Disabling support for unmanaged Prometheus configurations")

		// WithoutUnmanagedConfiguration tells that the controller should deprecate
		// the custom configuration.

enhancement: allow toggling custom config deprecation behaviour #6955

enhancement: allow toggling custom config deprecation behaviour #6955

Conversation

rexagod commented Sep 22, 2024

Description

Type of change

Changelog entry

Choose a reason for hiding this comment

mviswanathsai commented Sep 30, 2024

simonpasquier commented Oct 3, 2024

rexagod commented Oct 7, 2024

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Nov 7, 2024