Enable StatefulSetAutoDeletePVC feature for alertmanager, compactor, ingester & store-gateway StatefulSets #6106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
There was a problem hiding this comment.
Thank you for the contribution! You might have to run make build-helm-tests and commit and push the changes you get locally to get the CI passing.
There are three other statefulSets in the chart with persistent volumes - store-gateway, compactor, and alertmanager. It would be greatly appreciated if you have the time to carry out the changes in those too. If not, that's ok too, I can do it in a follow-up PR as well. Thanks again!
operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml
Outdated
Show resolved
Hide resolved
operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml
Outdated
Show resolved
Hide resolved
ivanfavi
force-pushed
the
enable-enableStatefulSetAutoDeletePVC-ingester-mimir-distributed
branch
from
9874e69
to
58f33d9
Compare
ivanfavi
force-pushed
the
enable-enableStatefulSetAutoDeletePVC-ingester-mimir-distributed
branch
from
58f33d9
to
a8e685e
Compare
operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml
Outdated
Show resolved
Hide resolved
dimitarvdimitrov
approved these changes
Nov 13, 2023
operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Ivan Farre <ivan@bluelabs.eu>
ivanfavi
changed the title
ivanfavi
changed the title
ivanfavi
force-pushed
the
enable-enableStatefulSetAutoDeletePVC-ingester-mimir-distributed
branch
from
bb5c5cd
to
ede4667
Compare
ivanfavi
force-pushed
the
enable-enableStatefulSetAutoDeletePVC-ingester-mimir-distributed
branch
from
ede4667
to
55b49d8
Compare
ivanfavi
force-pushed
the
enable-enableStatefulSetAutoDeletePVC-ingester-mimir-distributed
branch
from
55b49d8
to
d004fff
Compare
|
@dimitarvdimitrov could you add a review when you have time, please? 😀 |
There was a problem hiding this comment.
I left some suggestions wrt how this flag is called. I think enableStatefulSetAutoDeletePVC may be slightly confusing. Let me know what you think. Also can you add a line to the helm CHANGELOG.md to mention this new feature.
operations/helm/charts/mimir-distributed/templates/store-gateway/store-gateway-statefulset.yaml
Outdated
Show resolved
Hide resolved
operations/helm/charts/mimir-distributed/templates/alertmanager/alertmanager-statefulset.yaml
Outdated
Show resolved
Hide resolved
operations/helm/charts/mimir-distributed/templates/compactor/compactor-statefulset.yaml
Outdated
Show resolved
Hide resolved
operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml
Outdated
Show resolved
Hide resolved
operations/helm/charts/mimir-distributed/templates/store-gateway/store-gateway-statefulset.yaml
Outdated
Show resolved
Hide resolved
…ay/store-gateway-statefulset.yaml Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
…ompactor-statefulset.yaml Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
…gester-statefulset.yaml Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
…ay/store-gateway-statefulset.yaml Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
…r/alertmanager-statefulset.yaml Co-authored-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
…C-ingester-mimir-distributed
|
@dimitarvdimitrov I'm glad with the revisions that are being made. I've updated the Changelog too. Let me know if there is anything else to update. |
|
Let me know if I missed anything to pass the required checks 🤖 |
dimitarvdimitrov
approved these changes
Nov 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
I followed a similar implementation for
loki-distributedhttps://github.com/grafana/helm-charts/pull/2649/files to enable PersistentVolumeClaim retention policy.Which issue(s) this PR fixes or relates to
What is missing?
An ability to choose whether k8s deletes or retains PVCs in tandem with the lifecycle of a StatefulSet, due to the new beta field spec.persistentVolumeClaimRetentionPolicy in the latest version of Kubernetes. There is no way to configure the StatefulSet of Prometheus in kube-prometheus.
Why do we need it?
Because the new field defaults to Retain, PVCs remain bound to their PVs during cluster deletion while StatefulSets are being removed. This provides surface for a CSI Driver to see a stale, but intact PVC from a pod who no longer is requesting resources during cluster deletion. The CSI Driver then lacks the ability to manage(delete) unbound, available Volumes that otherwise would have been destroyed, resulting in orphaned resources.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]