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

Issue #290 - helm chart deployment implementation #375

Merged
merged 8 commits into from
Feb 7, 2024
Merged

Issue #290 - helm chart deployment implementation #375

merged 8 commits into from
Feb 7, 2024

Conversation

jonasz-lasut
Copy link
Contributor

Description

PR includes the implementation of Tapir deployment using a helm chart with linting and docs using github workflow.

Implementation of Issue 290

Type of change

  • New feature (non-breaking change which adds functionality)

@tim-chaffin
Copy link
Contributor

I can pull these files, and try a deployment on a sandbox AKS I have, if that would help with validation?

@jonasz-lasut
Copy link
Contributor Author

jonasz-lasut commented Jan 18, 2024

That would be great, I've noticed that I didn't set image repository in values.yaml, I'll have to do that. For AWS you'll need to add similar adnotations as ones in terraform example.

EDIT: added image repository, I knew I forgot something when reusing my private golden template 😅

@PacoVK
Copy link
Owner

PacoVK commented Jan 18, 2024

Wow, nice! I will have a closer look latest this weekend, but at first glance this looks great :)

@jonasz-lasut
Copy link
Contributor Author

Thanks, let me know if you find anything over the weekend!
I can't run tests as clusters I have access to are either on GKE or on bare-metaland tapir currently doesn't support those so I'll have to rely on you @tlchaffi

@tim-chaffin
Copy link
Contributor

Will do, I'll give it a whirl tonight or tomorrow night at the latest. (MST Time Zone)

@jonasz-lasut
Copy link
Contributor Author

jonasz-lasut commented Jan 19, 2024

Pipeline failed because I didn't generate docs after changing the image. I'll push changed docs in a minute

EDIT: done

Copy link
Owner

@PacoVK PacoVK left a comment

Choose a reason for hiding this comment

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

Hey @jonasz-lasut really good work! thanks for providing this to Tapir :)
While i found a small typo in the ClusterRoleBinding i think we can use this as a first version.
One major improvement (but that could be in another PR) would be to allow PVC in case Tapir is deployed with local storage in order to not loose the data, stored in there.
After we got this integrated i will also consider to create a Tapir org in GitHub in order to move the Helm chart to a dedicated repo. That would reduce the amount of files to be fetched if you want to deploy the Helm Chart.
What is your opinion on that? Also asking @tlchaffi :)

charts/templates/clusterrolebinding.yaml Outdated Show resolved Hide resolved
@jonasz-lasut
Copy link
Contributor Author

Hi, glad you found that typo!
I can add PVC in the values and deployment tomorrow while I fix that typo :)
Moving the chart to separate repo sounds like the good next step, if on-prem integration is implemented (probably something like Minio for storage and some RDB for backend) we can also think about testing helm deployment on something like KinD cluster on github workflow

@jonasz-lasut
Copy link
Contributor Author

@PacoVK I'll push the change to namespace typo, I also have PVC code prepared but could you please let me know which path in the container should be persistent? I didn't really see that in the docs and I'm not proficient enough in Java to quickly check that

@PacoVK
Copy link
Owner

PacoVK commented Jan 21, 2024

Awesome, you can mount it under /tapir 👍

@jonasz-lasut
Copy link
Contributor Author

@PacoVK done, users will be able to create a new PVC, add keep policy to it, reuse exisiting PVC.
If storage type is local and persistence is not enabled tapir deployment will create a emptyDir by default for /tapir so that deployments with readOnlyFilesystem security context can be used with local storage type.

@PacoVK
Copy link
Owner

PacoVK commented Jan 23, 2024

Thanks, I'll give it a shot in the next few days as soon as I find the time for it 👍👍

Copy link
Owner

@PacoVK PacoVK left a comment

Choose a reason for hiding this comment

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

I think this is now, good to go after fixing the small issues i found in the template :)

charts/templates/_env.tpl Outdated Show resolved Hide resolved
charts/templates/_env.tpl Outdated Show resolved Hide resolved
charts/templates/_env.tpl Outdated Show resolved Hide resolved
@jonasz-lasut
Copy link
Contributor Author

Fixed, thanks for looking over the code!

@tim-chaffin
Copy link
Contributor

tim-chaffin commented Jan 31, 2024

I tried to deploy this into Azure on an AKS cluster and ran into a problem.

Error:

2024-01-31 23:13:45,122 WARN [io.qua.oid.com.run.OidcCommonUtils] (vert.x-eventloop-thread-1) OIDC Server is not available:: io.netty.channel.AbstractChannel$AnnotatedConnectException: Connection refused: localhost/127.0.0.1:8089

Steps to reproduce:

  • Login to Azure: az login
  • Set the subscription: az account set --subscription ###
  • Clone the repo. In this case I'm pulling @jonasz-lasut 's branch off his own fork
  • Get AKS credentials: az aks get-credentials --resource-group ### --name ###
  • Install Helm chart, from repo directory on local: helm install tapir charts/

Initial Output:

NAME: tapir
LAST DEPLOYED: Wed Jan 31 16:09:30 2024
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None

Troubleshooting steps:

  • Check for running pods: kubectl get pods
NAME                     READY   STATUS             RESTARTS      AGE
tapir-7bfc45565f-m245w   0/1     CrashLoopBackOff   7 (78s ago)   13m
  • Get the logs from the pod: kubectl logs tapir-7bfc45565f-m245w >> pod.log

Pod output attached as log file.
pod.log

@PacoVK
Copy link
Owner

PacoVK commented Feb 1, 2024

@tlchaffi did you configure Tapir Helm Chart values to use eg. Azure Blobstorage or Localstorage for artefact persistence (default is S3) and another Database than DynamoDb for the data persistence? --> if not that causes the AWS related Errors

Currently Tapir Helm Chart does not include an IDP like Keycloak, hence you need to configure the values to point to your Keycloak (maybe smth. that we can include as well, to make the Helm Chart selfcontained + ElasticSearch as data storage) Than the Helm Chart could be deployed in a greenfield without further config...

@jonasz-lasut
Copy link
Contributor Author

@PacoVK tbh I'd much rather add a documentation page about setting up Keycloak + Elasticsearch with links to the official documentation than create a umbrella chart. It creates more work as you need to update the default values regularly if the additional charts get updates.

Maybe we could some CRDs that are flag enabled if ES and Keycloak support official operators, I believe ES does but I'm not sure about Keycloak.

@PacoVK
Copy link
Owner

PacoVK commented Feb 1, 2024

Good points! I agree that docs should be enough to help getting started

@tim-chaffin
Copy link
Contributor

I did not do any config changes or modifications. I followed what was stated in the charts/README.md file. Perhaps we need to add a line or two where we suggest config changes prior to doing the Helm install?

That being said, I can make the necessary adjustments, perhaps using a local storage while in AKS. Where are the instructions for that?

@jonasz-lasut
Copy link
Contributor Author

Adjustments can be found under configuration for the Tapir specific things and in the helm docs for helm related values. @PacoVK I've commited the changes that you've requested, review them when you have time and we probably can merge the code and work on the deployment guides on multiple clouds :)

@PacoVK
Copy link
Owner

PacoVK commented Feb 7, 2024

@jonasz-lasut cool thanks again, i think we are good to go now. :) My near time goal is to setup a small homepage for Tapir and then move to a dedicated org, this way its easier for community to jump in and also allows Tapir to be recognized as open source non-profit community tool :) As soon as i have created the org, i will move the Helm Chart into a dedicated Repo. If you have any doubts about the plans, please let me know :)

@jonasz-lasut
Copy link
Contributor Author

@PacoVK sounds fine to me!
If you need any help with kubernetes/helm, writing docs on that or deploying on GCP (I see that you have that in plans) then feel free to write to me I'll be really glad to help!

@PacoVK PacoVK merged commit 637d09e into PacoVK:main Feb 7, 2024
2 checks passed
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

3 participants