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

Helm: Server workspace volume persistence should be optional #12386

Closed
martimors opened this issue Apr 27, 2022 · 4 comments · Fixed by #12387
Closed

Helm: Server workspace volume persistence should be optional #12386

martimors opened this issue Apr 27, 2022 · 4 comments · Fixed by #12387
Labels
autoteam community team/tse Technical Support Engineers type/enhancement New feature or request

Comments

@martimors
Copy link
Contributor

martimors commented Apr 27, 2022

Tell us about the problem you're trying to solve

In general, Kubernetes (k8s) still handles persistent storage poorly, and many implementations disable PVs and PVCs entirely. Kubernetes really excels at running stateless container workloads. Statefulness in Kubernetes is convoluted at best and most environments prefer offloading this to a managed database provider if possible. In my opinion, the airbyte helm chart should facilitate this pattern (even encourage it).

Right now, the helm chart creates the RELEASE-NAME-airbyte-data PVC which is mounted to the server container, and it cannot be disabled.

I found the following indications that this volume is indeed no longer necessary:

Describe the solution you’d like

We should expose a configuration option server.persistence.enabled in the helm chart values.yaml to allow disabling the volume creation. It should be set to true by default for backwards compatibility.

Edit: Feedback from @alafanechere told me the PVCs are in fact already unused and can be safely removed. That solution works just as well (actually better). See PR.

Describe the alternative you’ve considered or used

I've created this option and it seems to run fine locally - however I'd love some advice on what potentials risks there are with running airbyte in this way.

Are you willing to submit a PR?

Yes, to follow shortly.

@martimors
Copy link
Contributor Author

Update on this: I've found that the connections do not work when missing the volume. Could anyone shed some light on why? It fails trying to fetch the schema of the data (tested with S3 to S3). Are there some logs I can find somewhere?

@alafanechere
Copy link
Contributor

Hi @dingobar,
Could you share the server log where you see the error about the missing volume?
@davinchia do you know if we can totally discard the use of PVC in our community helm chart?
I'm under the impression it's still defined for backward compatibility but I'm not sure how WORSPACE_ROOT works now.

@martimors
Copy link
Contributor Author

martimors commented Apr 28, 2022

Hi! I don't know if I was just too tired this morning, but when trying to re-create the error and make a screenshot, it started working.

So I've successfully deployed the helm chart with my changes from the PR, and used airbyte to copy a csv file from one S3 bucket to another without issues. My environment is minikube on linux.

This tells me that the PVC may indeed be unnecessary after all?

@alafanechere
Copy link
Contributor

This tells me that the PVC may indeed be unnecessary after all?

According to docs and comments in code, it indeed should be unnecessary for the latest versions but for backward compatibility, it's still required. Your optional approach with it being enabled by default looks good to me. But I'd like a confirmation from @davinchia that my assumptions are correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoteam community team/tse Technical Support Engineers type/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants