feat: add support for deploying as a StatefulSet #116
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
jdetroyes
requested changes
Nov 28, 2024
There was a problem hiding this comment.
Hello @eric-zadara
Thanks you for your PR,
Can you create a statefulset.yaml template in order to not mix the Deployment with StatefulSet. It's a better approach, each workload type is cleanly separated, making it easier to read and understand the templates.
You can add a root value:
# -- Specify the workload management: "Deployment" or "StatefulSet"
workload:
type: "Deployment"| {{- if not .Values.autoscaling.enabled }} | ||
| replicas: {{ .Values.replicaCount }} | ||
| {{- end }} | ||
| {{- if or .Values.updateStrategy.type .Values.updateStrategy.rollingUpdate }} | ||
| {{- if and .Values.persistence.enabled .Values.persistence.podManagementPolicy }} | ||
| podManagementPolicy: {{ .Values.persistence.podManagementPolicy }} |
There was a problem hiding this comment.
Can you add it at root values with a comment for StatefulSet only.
Comment on lines
+280
to
+301
| persistence: | ||
| # -- Enable persistence using a StatefulSet | ||
| # This disables the Deployment in favor of a StatefulSet | ||
| enabled: false | ||
|
|
||
| # -- Ollama server data Persistent Volume Claim Template size used | ||
| size: 30Gi | ||
|
|
||
| # -- Ollama server data Persistent Volume Storage Class | ||
| # If defined, storageClassName: <storageClass> | ||
| # If set to "-", storageClassName: "", which disables dynamic provisioning | ||
| # If undefined (the default) or set to null, no storageClassName spec is | ||
| # set, choosing the default provisioner. (gp2 on AWS, standard on | ||
| # GKE, AWS & OpenStack) | ||
| storageClass: "" | ||
|
|
||
| # -- Subdirectory of Ollama server data Persistent Volume to mount | ||
| # Useful if the volume's root directory is not empty | ||
| subPath: "" | ||
|
|
||
| # -- Additional volumeClaimTemplates | ||
| volumeClaimTemplates: [] |
There was a problem hiding this comment.
We can reuse the persistentVolume values to avoid duplicate code and add comment to specify to indicate whether it's for Deployment or Statefulsets.
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.
Summary of changes:
Attempting minimal changes to allow deploying as a StatefulSet instead of a Deployment. This is to enable predictable pod naming for use by litellm bypassing the k8s service and address the pods directly.
Chart defaults as existing behavior, but can now be deployed as:
Enabling PVC configuration will override the use of VolumeClaimTemplates with StatelfulSet, incase the user intends to use a shared volume instead of per-pod volumes.
Missing case is using a StatefulSet with an emptyDir but that feels counterintuitive to the intention of a StatefulSet.
Checklist:
artifacthub.io/changesannotation in Chart.yml according to the documentation