Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: enable config reload via signal #5690

Merged
merged 1 commit into from
Nov 27, 2023
Merged

feat: enable config reload via signal #5690

merged 1 commit into from
Nov 27, 2023

Conversation

simonpasquier
Copy link
Contributor

@simonpasquier simonpasquier commented Jun 15, 2023 •
edited

Description

Requires thanos-io/thanos#6453

Closes #3381

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

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Add `.spec.reloadStrategy` to the Prometheus and PrometheusAgent CRDs. The value can be `HTTP` (default if not specified) or `ProcessSignal`.

@simonpasquier simonpasquier force-pushed the reload-via-signal branch 3 times, most recently from 52b4244 to 084ffeb Compare June 16, 2023 15:06
@simonpasquier simonpasquier mentioned this pull request Jun 16, 2023
Merged
2 tasks
@simonpasquier simonpasquier force-pushed the reload-via-signal branch 3 times, most recently from d831438 to 4c95722 Compare June 19, 2023 15:42
@simonpasquier simonpasquier force-pushed the reload-via-signal branch 2 times, most recently from 64f0dfa to e855890 Compare November 15, 2023 16:16
@simonpasquier simonpasquier changed the title WIP: enable config reload via signal feat: enable config reload via signal Nov 15, 2023
@simonpasquier simonpasquier marked this pull request as ready for review November 15, 2023 16:16
@simonpasquier simonpasquier requested a review from a team as a code owner November 15, 2023 16:16
@simonpasquier simonpasquier mentioned this pull request Nov 15, 2023
Closed
ArthurSens
ArthurSens reviewed Nov 16, 2023
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍 , let's see how the PR upstream evolves

pkg/prometheus/agent/statefulset.go
Comment on lines -191 to -195
webRoutePrefix := "/"
if cpf.RoutePrefix != "" {
webRoutePrefix = cpf.RoutePrefix
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean up by moving this to CommonPrometheusFields!

cmd/prometheus-config-reloader/main.go Outdated
Comment on lines 267 to 270
r, err := http.NewRequestWithContext(ctx, "GET", prt.baseURL.JoinPath("/api/v1/status/runtimeinfo").String(), nil)
if err != nil {
return false, time.Time{}, err
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea to get reload status even when using signal 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes otherwise it would be like shooting in the dark. This was the blocking point for me to move this forward.

cmd/prometheus-config-reloader/main.go Outdated
Comment on lines 282 to 288
var runtimeInfo = struct {
Status string `json:"status"`
Data struct {
ReloadConfigSuccess bool `json:"reloadConfigSuccess"`
LastConfigTime time.Time `json:"lastConfigTime"`
} `json:"data"`
}{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know if prometheus has a history of changing the API response here? I wonder if we'll need to have a test to make sure we don't break when upgrading prometheus versions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://prometheus.io/docs/prometheus/latest/stability/

The v1 API should be stable. The API returns these fields since at least late 2019 so I wouldn't check the exact Prometheus version (if it ever happens, the only advice would be to upgrade).

nicolastakashi
nicolastakashi reviewed Nov 17, 2023
View reviewed changes
cmd/prometheus-config-reloader/main.go Outdated
}
defer resp.Body.Close()

if resp.StatusCode/100 != 2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL
So cleaver.
Better than check if >= 200 <= 299

nicolastakashi
nicolastakashi reviewed Nov 17, 2023
View reviewed changes
cmd/prometheus-config-reloader/main.go Outdated
select {
case <-ctx.Done():
return fmt.Errorf("%w: last configuration status: success=%v, success_timetamp=%v", ctx.Err(), success, last)
case <-t.C:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont you need to reset the timer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it will automatically resend to the channel at the next tick.

@nicolastakashi
Copy link
Contributor

LGTM

mrueg
mrueg reviewed Nov 23, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@simonpasquier simonpasquier force-pushed the reload-via-signal branch 2 times, most recently from 5f0f0d6 to 81d4734 Compare November 24, 2023 13:02
@simonpasquier
Copy link
Contributor Author

The Thanos PR has merged and this is now ready for a final review.

ArthurSens
ArthurSens approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

@simonpasquier
feat: enable config reload via signal
a143c8d 
This commit adds the option to reload Prometheus configuration using
signal instead of the /-/reload endpoint.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier simonpasquier merged commit ffd2f20 into prometheus-operator:main Nov 27, 2023
17 checks passed
@simonpasquier simonpasquier deleted the reload-via-signal branch November 27, 2023 12:33
@ArthurSens ArthurSens mentioned this pull request Nov 30, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrueg mrueg mrueg left review comments

@nicolastakashi nicolastakashi nicolastakashi approved these changes

@ArthurSens ArthurSens ArthurSens approved these changes

Assignees
No one assigned
Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Secure Prometheus Config Reload
4 participants
@simonpasquier @nicolastakashi @mrueg @ArthurSens