Skip to content
New issue

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

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

Already on GitHub? # to your account

[grafana] fix: allow nameOverride for PVC #3374

Closed
wants to merge 6 commits into from

Conversation

anthr76
Copy link

@anthr76 anthr76 commented Oct 23, 2024

I have a usecase where using the volumeClaimTemplate .metadata.name as the {{ include "grafana.fullname" . }} would aid in flexibility.

Users currently using a statefulSet would need to set .Values.persistence.nameOverride to storage to avoid a breaking change.

@CLAassistant
Copy link

CLAassistant commented Oct 23, 2024

CLA assistant check
All committers have signed the CLA.

@@ -42,7 +42,7 @@ spec:
- apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: storage
name: {{ include "grafana.fullname" . }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name: {{ include "grafana.fullname" . }}
name: {{ .Values.persistence.storageName }}

and to it to storage by default to avoid a breaking change

Copy link
Author

@anthr76 anthr76 Oct 23, 2024

Choose a reason for hiding this comment

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

@jkroepke while you were reviewing I actually revised this in a similar approach in 710e8c4.

For backwards compatibility users can set .Values.persistence.nameOverride to storage while using statefulSets.

Let me know if this works for you!

Otherwise we can do something like .Values.persistence.statefulSetStorageName and just default it to storage

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you explain the general benefit, why it should be configurable? It's just the name of template which will be merged with the pod name above. And the pod name already contains {{ include "grafana.fullname" . }}

Copy link
Author

Choose a reason for hiding this comment

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

I have a use-case where I would prefer minimal intervention converting a deployment based helm chart to a statefulSet based helm chart installation on upgrade. Having the PVC name remain the same between a deployment mount and a statefulSet volumeClaimTemplate would be useful as Kubernetes will elect to use the existing one since the name remains the same.

Having the override configurable aids in backwards compatibility

Copy link
Collaborator

Choose a reason for hiding this comment

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

but keep in mind, there name here is not the name of the final PVC. Your use-case and the proposed change doesn't match

Copy link
Author

Choose a reason for hiding this comment

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

I see I would like the name of the PVC to be this https://github.com/grafana/helm-charts/blob/563977e617b8f6861fadfe567c9dd399a4c8c054/charts/grafana/templates/pvc.yaml#L5C8-L5C43 . Perhaps I'm mis understanding something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This mention name here describes a Volume Claim Template which is part of the statefulset. The statefulset controller will use the template an create own PVC for each pod. The final PVC name is the name of the PVC followed by the full name of the pod itself, and always ending with an ordered number.

Signed-off-by: Anthony Rabbito <arabbito@coreweave.com>
@anthr76 anthr76 changed the title fix!: migrate stateful set name to use templated name fix!: allow nameOverride for PVC name Oct 23, 2024
@anthr76 anthr76 changed the title fix!: allow nameOverride for PVC name fix!: allow nameOverride for PV Oct 23, 2024
@anthr76 anthr76 changed the title fix!: allow nameOverride for PV fix!: allow nameOverride for PVC Oct 23, 2024
@zanhsieh zanhsieh changed the title fix!: allow nameOverride for PVC [grafana] fix: allow nameOverride for PVC Oct 30, 2024
Copy link
Collaborator

@zanhsieh zanhsieh left a comment

Choose a reason for hiding this comment

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

@anthr76
Can you fix DCO please?

@jkroepke
Copy link
Collaborator

jkroepke commented Nov 5, 2024

@zanhsieh I feel the PR is pointless. Since Requester miss-understand the volumeTemplate naming.

@zanhsieh
Copy link
Collaborator

zanhsieh commented Nov 6, 2024

@jkroepke
Let's just close this.

@zanhsieh zanhsieh closed this Nov 6, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants