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

feat: add persistent storage claim #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

morremeyer
Copy link
Contributor

To persist data, this adds a persistent storage claim.

BREAKING CHANGES: The default storage path is now "/storage" to make
the configuration to use a PVC easier.

This requires action only if you have not explicitly set the storage path
AND if you require the storage to be at "/tmp".

@rlex
Copy link
Collaborator

rlex commented Mar 17, 2023

two things probably worth adding:

  1. Deployment type - StatefulSet / Deployment https://github.com/parca-dev/helm-charts/blob/master/charts/parca/templates/server-deployment.yaml#L3
  2. existingClaim for pre-provisioned PVCs

@morremeyer
Copy link
Contributor Author

  1. makes sense, I'll do that.

I don't see what you mean with 1.? Are you thinking about adding a StatefulSet so that there could be multiple independent instances with each their own PVC? That could be helpful, but I'd push that to a later PR - it's not necessarily needed to support this.

@rlex
Copy link
Collaborator

rlex commented Mar 17, 2023

I don't see what you mean with 1.? Are you thinking about adding a StatefulSet so that there could be multiple independent instances with each their own PVC? That could be helpful, but I'd push that to a later PR - it's not necessarily needed to support this.

IIRC StatefulSet will not kick pod from assigned node automatically in case it's deployed by StatefulSet controller (assumes that PVC is local by default?)

In case of deployment, in case of node failure, and local PVC, pod will be reassigned to another node, losing local data.

On the other side, if PVC uses some HA storage (rook, longhorn, etc) then Deployment is preferred.

@morremeyer
Copy link
Contributor Author

IIRC StatefulSet will not kick pod from assigned node automatically in case it's deployed by StatefulSet controller (assumes that PVC is local by default?)

I don't think so, the difference between a Deployment and StatefulSet is that for a StatefulSet, every Pod gets a PVC and every Pod has a stable network identity.

I just checked the docs again and could not find any mention of that. If that is the case, I'd be very happy if you could point me to the docs or the code for that, you truly never stop learning.

In case of deployment, in case of node failure, and local PVC, pod will be reassigned to another node, losing local data.

Yes, but only if the PVC uses local storage. I would expect any cluster operator to know about the implications of local storage, so that's not something that I see the chart having to take care of.

Of course, this PR only works for replicas: 1 for now since it's a Deployment and just one PVC.

If we wanted to support multiple servers with persistence, we'll need to support a StatefulSet anyway so that every server has its own volume.

This could be done with a 5.1.0 version. For now, I'd be happy about adding support for a PVC in general, we can extend this at a later point anyways.

To persist data, this adds a persistent storage claim.

BREAKING CHANGES: The default storage path is now "/storage" to make
the configuration to use a PVC easier.

This requires action only if you have not explicitly set the storage path
AND if you require the storage to be at "/tmp".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants