Support makeDaemonSet() for Prometheus Agent's DaemonSet deployment #6652

haanhvu · 2024-06-06T10:11:45Z

Description

Support makeDaemonSet() and inner function makeDaemonSetSpec() for Prometheus Agent's DaemonSet deployment

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

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.)

Verification

Unit tests

Changelog entry

Support makeDaemonSet() for Prometheus Agent's DaemonSet deployment

haanhvu · 2024-06-06T10:41:55Z

@ArthurSens @simonpasquier @kakkoyun I just created makeDaemonSet() and makeDaemonSetSpec() with unit tests. The unit tests basically follow the current StatefulSet's unit tests. Could you check if these unit tests make sense or we need other unit tests?

There's a lot of code duplication between two modes right now. As we discussed, I'll refactor later.

I'm also waiting for #6636 to finish to add the WithNodeNameEnv() option to DaemonSet's config reloader.

ArthurSens

Sorry, couldn't find the time to review everything just yet. I've made some comments about test readability

I also have the impression that a lot of code is copy pasted from makeStatefulSet and makeStatefulSetSpec. Not a requirement for this PR, but I'm just wondering if we could reduce code duplication between daemonset and statefulsets

pkg/prometheus/agent/daemonset_test.go

haanhvu · 2024-06-13T09:13:20Z

I also have the impression that a lot of code is copy pasted from makeStatefulSet and makeStatefulSetSpec. Not a requirement for this PR, but I'm just wondering if we could reduce code duplication between daemonset and statefulsets

@ArthurSens Yes we can. But as we discussed in previous meetings, I think we should do the refactoring later. Because the implementation for daemonset could change, e.g., if later we find a bug in the implementation through e2e tests. You agree?

Most of your change suggestions are in the code duplication of both modes. I'll resolve in both modes then.

pkg/prometheus/agent/daemonset.go

simonpasquier · 2024-06-13T09:21:40Z

pkg/prometheus/agent/daemonset.go

+	// Mount web config and web TLS credentials as volumes.
+	// We always mount the web config file for versions greater than 2.24.0.
+	// With this we avoid redeploying prometheus when reconfiguring between
+	// HTTP and HTTPS and vice-versa.


I'd simplify and just consider that users must use a recent Prometheus version for DaemonSet deployment.

Yeah, it's a new mode so we don't need this check

Now I'm not entirely sure yet we should remove this check in DaemonSet. If we remove this check, it means we will officially require users to use >=2.24 right? If that's the case, we can remove this check and should clearly write the requirement in the doc.

Let me try to clarify: I would declare that Daemonset mode requires a "recent" version of Prometheus (for instance at least 2.45.0 which the current long-term support version) so we can simplify the support matrix. The check should be done as early as possible so makeDaemonSet() can safely assume a minimal version.

Yeah I wanted to clarify this, because we had a discussion in Slack (with @ArthurSens too) about whether we should limit the Prometheus version in DaemonSet: https://cloud-native.slack.com/archives/C071STDT4FP/p1718703164200639

If we agree to require a specific recent version, I'll remove this check then. I'll also note so that we'll remember to clarify this in the user doc of DaemonSet.

simonpasquier · 2024-06-13T09:23:48Z

pkg/prometheus/agent/daemonset.go

+	// minutes) to ensure that the readiness probe only comes into effect once
+	// Prometheus is effectively ready.
+	// We don't want to use the /-/healthy handler here because it returns OK as
+	// soon as the web server is started (irrespective of the WAL replay).


We can either use the same settings as with statefulset mode or consider that since daemonset doesn't support persistent storage, we can have tighter timeouts.

haanhvu · 2024-06-17T06:25:28Z

We discussed offline and agreed to do some refactoring of the common between StatefulSet and DaemonSet first, before continuing this PR.

haanhvu · 2024-06-24T08:21:02Z

I just rebased, haven't done anything new yet. Will ping when this's ready for review.

haanhvu · 2024-06-24T09:37:17Z

@ArthurSens @simonpasquier @kakkoyun This is ready for review now. Please help review so that we can move on to the next thing. Next thing will be operator's logic and e2e tests, which are more difficult and interesting 😄

ArthurSens

LGTM!

If we find something wrong along the way, we can just fix it in a follow-up PR :)

haanhvu · 2024-06-26T03:49:16Z

pkg/prometheus/agent/daemonset.go

+	// To avoid breaking users deploying an old version of the config-reloader image.
+	// TODO: remove the if condition after v0.72.0.
+	if cpf.Web != nil {
+		configReloaderWebConfigFile = confArg.Value
+		configReloaderVolumeMounts = append(configReloaderVolumeMounts, configMount...)
+	}


@ArthurSens @simonpasquier @kakkoyun We already agreed on Prometheus version in #6652 (comment) What about config reloader version here? Do we need this version check?

No, let's keep it simple.

pkg/prometheus/agent/daemonset.go

simonpasquier · 2024-06-27T08:54:54Z

pkg/prometheus/agent/daemonset.go

+	// To avoid breaking users deploying an old version of the config-reloader image.
+	// TODO: remove the if condition after v0.72.0.
+	if cpf.Web != nil {
+		configReloaderWebConfigFile = confArg.Value
+		configReloaderVolumeMounts = append(configReloaderVolumeMounts, configMount...)
+	}


No, let's keep it simple.

simonpasquier

One minor comment which you can address later.

simonpasquier · 2024-06-27T13:14:18Z

pkg/prometheus/agent/daemonset.go

+	// In cases where an existing selector label is modified, or a new one is added, new sts cannot match existing pods.
+	// We should try to avoid removing such immutable fields whenever possible since doing
+	// so forces us to enter the 'recreate cycle' and can potentially lead to downtime.
+	// The requirement to make a change here should be carefully evaluated.


(nit)

Suggested change

// In cases where an existing selector label is modified, or a new one is added, new sts cannot match existing pods.

// We should try to avoid removing such immutable fields whenever possible since doing

// so forces us to enter the 'recreate cycle' and can potentially lead to downtime.

// The requirement to make a change here should be carefully evaluated.

// In cases where an existing selector label is modified, or a new one is added, new daemonset cannot match existing pods.

// We should try to avoid removing such immutable fields whenever possible since doing

// so forces us to enter the 'recreate cycle' and can potentially lead to downtime.

// The requirement to make a change here should be carefully evaluated.

haanhvu requested a review from a team as a code owner June 6, 2024 10:11

pull-request-size bot added the size/XL label Jun 6, 2024

haanhvu added this to the [GSoC 2024] Daemonset mode for PrometheusAgent milestone Jun 6, 2024

haanhvu force-pushed the make-daemonset-object branch from 03bdb1a to ee45f5c Compare June 7, 2024 05:13

ArthurSens reviewed Jun 12, 2024

View reviewed changes

simonpasquier reviewed Jun 13, 2024

View reviewed changes

This was referenced Jun 17, 2024

Refactor the common implementation code (not including tests) between Prometheus's modes #6686

Merged

Refactor common test code between Prometheus Agent's StatefulSet and DaemonSet modes #6688

Merged

haanhvu force-pushed the make-daemonset-object branch from ee45f5c to 79e3c96 Compare June 19, 2024 09:03

pull-request-size bot added size/L and removed size/XL labels Jun 19, 2024

haanhvu mentioned this pull request Jun 21, 2024

Decide on ready probes' timeouts for Prometheus container in Prometheus Agent's DaemonSet mode #6704

Open

haanhvu force-pushed the make-daemonset-object branch from 79e3c96 to 6c052f5 Compare June 24, 2024 08:14

haanhvu marked this pull request as draft June 24, 2024 08:14

haanhvu force-pushed the make-daemonset-object branch from af8a297 to 0f5e96c Compare June 24, 2024 09:23

haanhvu marked this pull request as ready for review June 24, 2024 09:33

ArthurSens previously approved these changes Jun 25, 2024

View reviewed changes

haanhvu dismissed ArthurSens’s stale review via 6bd9e74 June 26, 2024 03:46

haanhvu force-pushed the make-daemonset-object branch from 6bd9e74 to 664c55c Compare June 26, 2024 03:47

haanhvu commented Jun 26, 2024

View reviewed changes

This was referenced Jun 26, 2024

Allow Prometheus Agent operator to create DaemonSet #6708

Merged

chore: refactor annotation propagation #6710

Merged

haanhvu force-pushed the make-daemonset-object branch from 664c55c to 411b13a Compare June 27, 2024 08:08

simonpasquier reviewed Jun 27, 2024

View reviewed changes

create daemonset.go

acf9e3a

haanhvu added 10 commits June 27, 2024 17:15

Create DaemonSet object for Prometheus Agent's DaemonSet deployment

0d08caf

nits

7d70a86

Fix lint

aabac2a

Add WithNodeNameEnv() option to config reloader

abd194d

Use refactored functions for TestWALCompressionForDaemonSet

903289a

Use the latest refactored functions in tests

cd85159

Use refactored functions

6d093bd

Remove Prometheus version check

054da59

Refactor annotation making

873de08

Simon's review

489d8ea

haanhvu force-pushed the make-daemonset-object branch from 411b13a to 489d8ea Compare June 27, 2024 10:36

simonpasquier approved these changes Jun 27, 2024

View reviewed changes

simonpasquier merged commit 25a7bdf into prometheus-operator:main Jun 27, 2024
18 checks passed

Support makeDaemonSet() for Prometheus Agent's DaemonSet deployment #6652

Support makeDaemonSet() for Prometheus Agent's DaemonSet deployment #6652

Uh oh!

Conversation

haanhvu commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Verification

Changelog entry

Uh oh!

haanhvu commented Jun 6, 2024

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

haanhvu commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haanhvu Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haanhvu commented Jun 17, 2024

Uh oh!

haanhvu commented Jun 24, 2024

Uh oh!

haanhvu commented Jun 24, 2024

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonpasquier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

haanhvu commented Jun 6, 2024 •

edited

Loading

haanhvu commented Jun 13, 2024 •

edited

Loading

haanhvu Jun 26, 2024 •

edited

Loading