-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Kubernetes & Helm #64
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together, very cool. Let's discuss the comments and go from there.
nohup.out | ||
backend/openui-backend.tar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this was added?
@@ -0,0 +1,99 @@ | |||
Deploying to a Kubernetes cluster involves several steps. Below is a step-by-step guide that assumes you have a Kubernetes cluster already set up and configured, and you have `kubectl` installed and configured to communicate with your cluster. This guide also assumes you have already built and pushed your Docker images to a container registry that your Kubernetes cluster can access. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why we would want both RAW kubernetes YAML and a HELM chart. Can you share more about you're thinking here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those of us that use Kustomize or other systems we don't want helm charts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just feels redundant. Couldn't we just use helm to render the manifests without using it to deploy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do that. But if you are a user that does not in fact use helm, should they have to install helm cli locally to be able to generate manifests so they can avoid using helm?
In scenarios where gitops is the primary deployment mechanisms the barrier of entry will be a lot lower if the manifests is provided as well depending on the tooling used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ornell I agree we shouldn't require helm but ideally we would just have a github action or something that generates the manifests whenever we change the helm chart so we don't duplicate all this logic. I'll look into something here.
tag: latest | ||
port: 11434 | ||
volumePath: /mnt/data/ollama | ||
volumeSize: 2Gi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to make Ollama optional but not required. Some users may only deploy this with OPENAI configured. Additionally to really deploy ollama in production you would likely want to target nodes with Nvidia GPU's likely through selectors. Not sure how best to wire that up generically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ollama should be it's own deployment and just a param. That's how most tools handle this. The assumption should be ollama deployment handled that and exposes in backspace usually just with the DNS name ollama
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'm just saying we should be able to choose not to deploy it in this chart with an enabled flag, or just make the official Ollama helm chart a dependency.
name: openai-api-key-secret | ||
type: Opaque | ||
data: | ||
OPENAI_API_KEY: YOUR_BASE64_ENCODED_API_KEY # Replace with your base64-encoded API key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like a pretty big anti-pattern to have a secret like this in a YAML file on disk. Basically just asking for someone to accidentally check it in to get. If we do end up keeping the raw k8s manifests I would advocate for just adding instructions for setting the secret from the cli like the HELM README does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not oddly. Only authorized users can get to the secrets. You wouldn't put the actual secret in git hopefully but the opaque is the most basic and standard secret.
Most will face their own secret manager wired up and replace this for a real deployment. This however is how the basic YAML should be I would agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Anti-pattern is having a file that's checked into git telling the user to put a secret in it. Someone will just run git commit -am 'configured yaml'
and end up checking in a secret 😭
Thanks for your work on this, I added several comments above but will likely try this via Kubernetes instead of local / docker. |
I now you want to get Kubernetes deployment. There are two folders: kubernetes and helm.