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.

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

Add support for persistentVolumeClaimRetentionPolicy field in statefulset mode #3305

Open
dferrochio opened this issue Sep 24, 2024 · 5 comments · May be fixed by #3354
Open

Add support for persistentVolumeClaimRetentionPolicy field in statefulset mode #3305

dferrochio opened this issue Sep 24, 2024 · 5 comments · May be fixed by #3354
Assignees
Labels
area:collector Issues for deploying collector enhancement New feature or request

Comments

@dferrochio
Copy link

Component(s)

collector

Is your feature request related to a problem? Please describe.

I'm not able to change the persistentVolumeClaimRetentionPolicy field in statefulset mode. I need to change the default policy to delete the PVC's when the statefulset is scaled.

Describe the solution you'd like

Solution might be an optional configuration section for the statefulset specs like this:

apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
spec:
  mode: statefulset
  statefulset:
    spec:
      persistentVolumeClaimRetentionPolicy:
        whenDeleted: Retain
        whenScaled: Delete

Describe alternatives you've considered

No response

Additional context

No response

@dferrochio dferrochio added enhancement New feature or request needs triage labels Sep 24, 2024
@jaronoff97 jaronoff97 added area:collector Issues for deploying collector and removed needs triage labels Sep 25, 2024
@davidhaja
Copy link
Contributor

davidhaja commented Oct 3, 2024

I am happy to help you with this.
How about instead of adding statefulset.spec.persistentVolumeClaimRetentionPolicy to the opentelemetrycollector CRD, the persistentVolumeClaimRetentionPolicy would be a just separate field(object) like:

apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
spec:
  mode: statefulset
  persistentVolumeClaimRetentionPolicy:
    whenDeleted: Retain
    whenScaled: Delete

@dferrochio
Copy link
Author

I am happy to help you with this. How about instead of adding statefulset.spec.persistentVolumeClaimRetentionPolicy to the opentelemetrycollector CRD, the persistentVolumeClaimRetentionPolicy would be a just separate field(object) like:

apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
spec:
  mode: statefulset
  persistentVolumeClaimRetentionPolicy:
    whenDeleted: Retain
    whenScaled: Delete

Hi, and thanks for your propossal.

I thought on recommend that solution too. But I didn't choose it because could be less explicit and could generate some confussion to users. I prefer a solution that it's clear that only works for statefulset mode and not getting users trying to use persistentVolumeClaimRetentionPolicy field for other modes.

An intermediate solution could be:

kind: OpenTelemetryCollector
spec:
  mode: statefulset
  statefulset:
    persistentVolumeClaimRetentionPolicy:
      whenDeleted: Retain
      whenScaled: Delete

Also, your solution could work too if documentation is clear enough about the persistentVolumeClaimRetentionPolicy field only works for statefulset mode. And maybe adding a warning log if the field is used in a different mode.

@davidhaja
Copy link
Contributor

Yeah, I've recommended that because AFAIK only the statefulset has this persistentVolumeClaimRetentionPolicy field.
I guess we can let the core maintainers decide what do they prefer.
@jaronoff97 @pavolloffay @swiatekm

@swiatekm
Copy link
Contributor

We already have a field at top level which only applies if the mode is statefulset, so I think it's fine to do the same thing here. See

VolumeClaimTemplates []v1.PersistentVolumeClaim `json:"volumeClaimTemplates,omitempty"`
if you want to try implementing it yourself.

@davidhaja
Copy link
Contributor

Sure thing.
@swiatekm Thank you for your input.
Feel free to assign this ticket to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:collector Issues for deploying collector enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants