Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/prometheus]: add optional Prometheus StatefulSets #7116

Merged
merged 4 commits into from
Dec 3, 2018
Merged

[stable/prometheus]: add optional Prometheus StatefulSets #7116

merged 4 commits into from
Dec 3, 2018

Conversation

giacomoguiulfo
Copy link
Collaborator

@giacomoguiulfo giacomoguiulfo commented Aug 9, 2018 •
edited
Loading

What this PR does / why we need it:

Currently, the Prometheus chart uses Deployments to manage its server and alertmanager pods, and each one of them can be configured to use Persistent Volume Claims. However, it is bad practice (in this case, not always) to use Deployments to manage pods that deal with PVCs (#1863) because it might prevent them from having more than 1 replica (again, this is the case for Prometheus). Also, when deployments are deleted, (by default) the PVCs are automatically deleted with it. With statefulsets this doesn't happen (which is better if data wants to be persisted).

This PR updates the chart to use StatefulSets instead of Deployments to manage the Prometheus server and alertmanager pods.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):

fixes #2346.
fixes #8026.
fixes #5115.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 9, 2018
@giacomoguiulfo
Copy link
Collaborator Author

/assign @mgoodness

@gianrubio
Copy link
Collaborator

Thanks for your PR @giacomoguiulfo. I'm wondering what's gonna happen to users that upgrade this chart, if they will loose all the data because the pvc and the deployment gone.

Can you figure out this?

@giacomoguiulfo
Copy link
Collaborator Author

@gianrubio Yes, this is a breaking change. If users upgrade to this chart, they will lose their data because when the deployment gets deleted, the PVC gets deleted too (and probably also because the template is gone), but again this is one reason of why we may want to migrate to StatefulSets. This is also why I bumped the version to 8.0.0. However, I would like to know if there is a way to perform an upgrade that persists data in case you have a few ideas 🙂

@davidkarlsen
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 11, 2018
unguiculus
unguiculus suggested changes Aug 13, 2018
View reviewed changes
Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

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

Statefulsets require a governing headless service. The service names should be specified in the statefulsets under spec.serviceName.

@unguiculus
Copy link
Member

/assign

@giacomoguiulfo
Copy link
Collaborator Author

Thanks for the review @unguiculus. Just added the requested change.

@unguiculus
Copy link
Member

unguiculus commented Aug 15, 2018 •
edited
Loading

You simply configured the existing services as governing services for the statefulsets. These services are not headless. You need to create additional services.

@giacomoguiulfo
Copy link
Collaborator Author

@unguiculus Just added new headless services as per request, leaving the existing ones untouched.

@mattfarina mattfarina added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018
@giacomoguiulfo giacomoguiulfo force-pushed the feature/ZENKO-949-prometheus-server-statefulset branch from cdbfe18 to 2be8dbd Compare August 27, 2018 21:47
@ey-bot ey-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018
@giacomoguiulfo giacomoguiulfo force-pushed the feature/ZENKO-949-prometheus-server-statefulset branch from 2be8dbd to e4a7c58 Compare August 27, 2018 21:59
@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018
@giacomoguiulfo
Copy link
Collaborator Author

@unguiculus Just rebased the PR! :)

@giacomoguiulfo giacomoguiulfo force-pushed the feature/ZENKO-949-prometheus-server-statefulset branch 2 times, most recently from bd7f3e6 to 6e2cad5 Compare September 10, 2018 18:56
@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 14, 2018
@unguiculus
Copy link
Member

/retest

@unguiculus
Copy link
Member

ping @gianrubio

@ey-bot ey-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 18, 2018
@giacomoguiulfo giacomoguiulfo changed the title [stable/prometheus]: Update Prometheus Deployments to StatefulSets [stable/prometheus]: add optional Prometheus StatefulSets Nov 9, 2018
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 9, 2018
@giacomoguiulfo
Copy link
Collaborator Author

@gianrubio @unguiculus Any chance this can get in? Is there something else to work on? Thanks!

@gianrubio
Copy link
Collaborator

/Lgtm /ok-to-test

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2018
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2018
@giacomoguiulfo
Copy link
Collaborator Author

@unguiculus Could you take another look at this, please? It seems like it won't be merged because you requested changes. Thanks in advance 👍

@gianrubio
Copy link
Collaborator

/ok-to-test
/lgtm

@k8s-ci-robot k8s-ci-robot added ok-to-test lgtm Indicates that a PR is ready to be merged. labels Dec 2, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2018
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 2, 2018
@gianrubio
Copy link
Collaborator

/ok-to-test
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giacomoguiulfo, gianrubio

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2018
@k8s-ci-robot k8s-ci-robot merged commit 61c2862 into helm:master Dec 3, 2018
@giacomoguiulfo
Copy link
Collaborator Author

Thanks @gianrubio!

@giacomoguiulfo giacomoguiulfo deleted the feature/ZENKO-949-prometheus-server-statefulset branch December 3, 2018 07:46
@lukmdo lukmdo mentioned this pull request Dec 7, 2018
Merged
3 tasks
yuchaoran2011 pushed a commit to yuchaoran2011/charts that referenced this pull request Dec 11, 2018
@giacomoguiulfo @yuchaoran2011
[stable/prometheus]: add optional statefulset resource (helm#7116)
352addc 
Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>
Signed-off-by: Chaoran Yu <yuchaoran2011@gmail.com>
coreypobrien pushed a commit to FairwindsOps/helm-charts that referenced this pull request Dec 31, 2018
@giacomoguiulfo @coreypobrien
[stable/prometheus]: add optional statefulset resource (helm#7116)
a4174bc 
Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
@giacomoguiulfo @wgiddens
[stable/prometheus]: add optional statefulset resource (helm#7116)
402ce75 
Signed-off-by: Giacomo Guiulfo <giacomoguiulfo@gmail.com>
@sboschman sboschman mentioned this pull request Jan 24, 2019
Merged
@calohmn calohmn mentioned this pull request Aug 25, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidkarlsen davidkarlsen davidkarlsen left review comments

@mtparet mtparet mtparet left review comments

@unguiculus unguiculus unguiculus requested changes

@gianrubio gianrubio Awaiting requested review from gianrubio

@mgoodness mgoodness Awaiting requested review from mgoodness

Assignees

@unguiculus unguiculus

@mgoodness mgoodness

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone