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

Kubernetes & Helm #64

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

sergefedorow
Copy link

I now you want to get Kubernetes deployment. There are two folders: kubernetes and helm.

Copy link
Contributor

@vanpelt vanpelt left a 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
Copy link
Contributor

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.
Copy link
Contributor

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?

Copy link

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.

Copy link
Contributor

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?

Copy link

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.

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link

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

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link

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

Copy link
Contributor

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 😭

@bitshop
Copy link

bitshop commented May 4, 2024

Thanks for your work on this, I added several comments above but will likely try this via Kubernetes instead of local / docker.

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

4 participants