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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
Sep 6, 2023
Merged

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

merged 14 commits into from
Sep 6, 2023

Conversation

mjasion
Copy link
Contributor

@mjasion mjasion commented Aug 2, 2023 •
edited
Loading

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
Beta Give feedback

@mjasion mjasion requested review from 0x2b3bfa0 and jesper7 August 2, 2023 10:47
@mjasion mjasion self-assigned this Aug 2, 2023
@mjasion mjasion marked this pull request as draft August 2, 2023 10:47
@mjasion mjasion mentioned this pull request Aug 2, 2023
Merged
@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 mentioned this pull request Aug 2, 2023
Closed
1 task
@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
jesper7
jesper7 suggested changes Aug 28, 2023
View reviewed changes
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/configmap-studio-backend-nginx-sidecar.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/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

@mjasion
feat(nginx): combine into single configmap nginx configs
d9d89f4
@mjasion
review: remove nginx from names, leave blobvault
81cbc20 
Leaved only configmap
@mjasion
fix(ingress): Fix Ingress resources management
0855e27 
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.

jesper7
jesper7 suggested changes Sep 5, 2023
View reviewed changes
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
@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
Reviewers

@jesper7 jesper7 jesper7 approved these changes

@0x2b3bfa0 0x2b3bfa0 Awaiting requested review from 0x2b3bfa0

Assignees

@mjasion mjasion

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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