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

feat(blobvault): Make blobvault pvc storage optional if s3 bucket is used #148

Merged

Conversation

mjasion
Copy link
Contributor

@mjasion mjasion commented Aug 2, 2023

Breaking Change

This 0.60.0 fixes the issue with Ingress objects not being cleaned up.
To upgrade to 0.60.0 and above, you need to manually delete the Ingress object before upgrading:

kubectl delete ingress -l app.kubernetes.io/managed-by=Helm --namespace studio
kubectl delete ingress blobvault --namespace studio

Purpose of the PR

This PR configures the blobvault service, which runs on the same node as the worker if there is no S3 bucket configured.
Also, it replaces nginx chart dependency for hosting Studio under the custom basePath. The nginx is deployed as a sidecar.

Closes #145

Tasks

@mjasion mjasion self-assigned this Aug 2, 2023
@mjasion mjasion marked this pull request as draft August 2, 2023 10:47
@mjasion mjasion force-pushed the 145-make-blobvault-pvc-storage-optional-if-s3-bucket-is-used branch from 1481e54 to 1014dad Compare August 2, 2023 12:48
@mjasion mjasion force-pushed the 145-make-blobvault-pvc-storage-optional-if-s3-bucket-is-used branch from 39d44ba to 3dcf452 Compare August 2, 2023 14:57
@mjasion mjasion force-pushed the 145-make-blobvault-pvc-storage-optional-if-s3-bucket-is-used branch 3 times, most recently from c02af57 to 218370a Compare August 9, 2023 20:06
@mjasion mjasion force-pushed the 145-make-blobvault-pvc-storage-optional-if-s3-bucket-is-used branch 5 times, most recently from c0eac4b to 38ac4f3 Compare August 23, 2023 17:08
@mjasion mjasion marked this pull request as ready for review August 23, 2023 17:08
@mjasion mjasion force-pushed the 145-make-blobvault-pvc-storage-optional-if-s3-bucket-is-used branch from 38ac4f3 to 6532e37 Compare August 23, 2023 17:09
Copy link
Contributor

@jesper7 jesper7 left a comment

Choose a reason for hiding this comment

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

Not nit-picking in this review, but I see that we need to improve resource naming a little bit (more simple/accurate names will make it easier when developing)

charts/studio/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/studio/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/studio/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/studio/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/studio/templates/ingress-blobvault-nginx.yaml Outdated Show resolved Hide resolved
charts/studio/templates/ingress-blobvault-nginx.yaml Outdated Show resolved Hide resolved
charts/studio/templates/ingress-blobvault-nginx.yaml Outdated Show resolved Hide resolved
charts/studio/templates/service-blobvault-nginx.yaml Outdated Show resolved Hide resolved
charts/studio/templates/service-blobvault-nginx.yaml Outdated Show resolved Hide resolved
@mjasion mjasion force-pushed the 145-make-blobvault-pvc-storage-optional-if-s3-bucket-is-used branch from b47fd1b to f837af6 Compare August 29, 2023 20:48
@jesper7
Copy link
Contributor

jesper7 commented Sep 1, 2023

I've done a lot of manual testing, and one thing I noticed is that if you upgrade from an existing installation, plots won't load:

image

This needs further investigation

This  0.60.0 fixes the issue with Ingress objects not being cleaned up.
To upgrade to 0.60.0 and above, you need to manually delete the Ingress object before upgrading:

```bash
kubectl delete ingress -l app.kubernetes.io/managed-by=Helm --namespace studio
kubectl delete ingress blobvault --namespace studio
```
@mjasion mjasion force-pushed the 145-make-blobvault-pvc-storage-optional-if-s3-bucket-is-used branch from 9460e2a to 0855e27 Compare September 4, 2023 19:38
@mjasion mjasion requested a review from jesper7 September 4, 2023 19:46
@mjasion
Copy link
Contributor Author

mjasion commented Sep 4, 2023

@jesper7 PTAL again :-)

In ☝️ commits I

  • fixed the Ingress issue with migrating to blobvault
  • Unified the nginx configm into single configmap.

Copy link
Contributor

@jesper7 jesper7 left a comment

Choose a reason for hiding this comment

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

Nice work on refactoring the ConfigMaps!
Only some very minor comments

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
charts/studio/nginx/nginx.conf Outdated Show resolved Hide resolved
Co-authored-by: Jesper Svendsen <99078145+jesper7@users.noreply.github.com>
@jesper7 jesper7 self-requested a review September 6, 2023 16:24
@jesper7 jesper7 added this pull request to the merge queue Sep 6, 2023
Merged via the queue into main with commit dc56bf6 Sep 6, 2023
4 checks passed
@jesper7 jesper7 deleted the 145-make-blobvault-pvc-storage-optional-if-s3-bucket-is-used branch September 6, 2023 16:32
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.

Make blobvault PVC storage optional, if S3 bucket is used
2 participants