Skip to content

Conversation

milesgranger
Copy link
Contributor

What does this PR do?

This adds an initial Helm chart at k8s/helm (maybe a different path is preferred?)

100% organic human generated code; I used helm create and tried to follow the recommended best practices. However I doubt it's bug free, in fact, I think something is wrong with the ingress portion. At least when I use Traefik it seems to keep getting routed to the headless service, resulting in no available server from Traefik. I think this has something to do with the selectors getting mixed up, but I'm not sure. I plan to poke at it a bit more today but unless I find quick success, I see adding a separate IngressRoute directly works w/o issue so may just use that in the shorter term.

Otherwise, I think it replicates pretty closely to what the existing static manifests accomplish and seems to work nicely on my end. I'm happy to continue to add to this as we use it more.

Motivation

We have a standard flow of using helm with ArgoCD for base services and would like to give ArcadeDB a go in the same way.

Related issues

#1465 (comment)

Checklist

  • I have run the build using mvn clean package command
  • My unit tests cover both failure and success scenarios

@gramian
Copy link
Collaborator

gramian commented Mar 4, 2025

@lvca I never used helm charts, so I don't think I can be of help here (at least in the short run). Sorry.

readinessProbe:
httpGet:
path: /
port: http
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an endpoint /api/v1/ready that return 204 if all is ok. It could be usefull?

@robfrank
Copy link
Collaborator

robfrank commented Mar 4, 2025

Great works, thanks.
Now, let me think about creating a separate "arcadedb-helm-chart" repository or keep this contribution in the main repository.

The dedicated repo approach works particularly well because:

  • Database code and deployment configuration have different lifecycles
  • It's easier for users to find deployment options in a dedicated repo
  • It allows for independent versioning of deployment configurations

Let me think a bit about the better way to manage this.

BTW, a great and valuable work, thank you!

@robfrank
Copy link
Collaborator

robfrank commented Mar 4, 2025

After some consultation, the PR will be merged here, instead of creating a new, dedicated repo

@lvca lvca requested review from robfrank and removed request for gramian March 4, 2025 17:16
@lvca lvca added the enhancement New feature or request label Mar 4, 2025
@lvca lvca added this to the 25.3.1 milestone Mar 4, 2025
@robfrank
Copy link
Collaborator

robfrank commented Mar 5, 2025

Just a small request: can you add a readme.md with some basic instructions on how to use it?

moreover, What should we do on relases? Just upgrade the version inside the files?

@milesgranger
Copy link
Contributor Author

milesgranger commented Mar 5, 2025

Yes, I'll add a README in a bit.
For versioning, I'd just worry about this file here: k8s/helm/Chart.yaml
The generated docs from helm do a decent job of explaining the two version settings I think.

@milesgranger
Copy link
Contributor Author

milesgranger commented Mar 5, 2025

Added a README in 631367e by using Bitnami's helm readme generator: https://github.com/bitnami/readme-generator-for-helm

If you like, and time opens up for me I can probably add that as a step to the helm test perhaps, install that doc generator and run it using the values.yaml file to ensure any changes are documented and README is updated.

Aside from that, could also do a minikube step to install the chart using default values and just make sure some pods/svc end up happy. But again, I'm a bit slim for time these days but can probably swing it to be work related to a certain extent. :)

@milesgranger milesgranger force-pushed the milesgranger/add-k8s-helm-chart branch from ac171f1 to 631367e Compare March 5, 2025 12:54
@robfrank
Copy link
Collaborator

robfrank commented Mar 5, 2025

a dedicated workflow to test helm with minikube and also the doc would be amazing.
If you don't have time right now, we can start to merge this pr and then do the additional work on a new one
I'm not a big fan of long-standing prs.

Let me know when I can merge, thanks

@milesgranger
Copy link
Contributor Author

I'm not a big fan of long-standing prs.

I think we're aligned here :) As mentioned we're intending to use this for a PoC project so will be sure to upstream anything we see during our use. Thanks, ready to merge from my end.

@lvca
Copy link
Contributor

lvca commented Mar 5, 2025

Thanks @milesgranger! @gramian the readme.md provided by @milesgranger should be also in our docs. WDYT?

@robfrank robfrank requested a review from lvca March 5, 2025 19:12
@gramian
Copy link
Collaborator

gramian commented Mar 5, 2025

I suggest to add a section, as for Docker and plain K8s, to the docs with some basic info about helm and a reference to this chart. I would rather not add the parameters as this is very specific to this very chart, and rather refer to the README instead.
As for the compose file, somebody seriously using a helm chart to deploy ArcadeDB will write their own adapted version (probably based on this) but understand the parameters. I would think the benefit of having this is to provide a good template to start of from which happens to work unmodified on its own, too, right?

Also from me, thanks @milesgranger !

BTW: I guess, the liveness probe should not be the ready endpoint, but likely user specific ie a SELECT 'alive'; query to a relevant database?

@robfrank robfrank merged commit 03a1621 into ArcadeData:main Mar 6, 2025
2 of 3 checks passed
@milesgranger milesgranger deleted the milesgranger/add-k8s-helm-chart branch March 8, 2025 14:42
@gramian gramian mentioned this pull request Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants