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: support atlas migration, issueref #56 #57

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

shreyasHpandya
Copy link

No description provided.

@shreyasHpandya shreyasHpandya force-pushed the main branch 3 times, most recently from b6c1c91 to ec04c2a Compare November 21, 2024 04:43
@shreyasHpandya shreyasHpandya changed the title [DRAFT]feat: support atlas migration, issueref #56 feat: support atlas migration, issueref #56 Nov 26, 2024
@sunnyyip
Copy link
Contributor

sunnyyip commented Dec 3, 2024

Thanks for the PR, @shreyasHpandya, @semmet95 @ANIRUDH-333!

May I ask you to help me documenting the new features?

  1. We use readme-generator to generate docs for the inputs. Can we add the docs for "atlas" in values.yaml please?
    This is how I run readme-generator at helm-charts/charts/guac: readme-generator-for-helm/bin/readme-generator -v ./values.yaml -r ./README.md -s schema.json
  2. Can you please share your learning on running atlas migration? Perhaps a little "Atlas Migration" section under "Install Chart" to call out the importance?
    E.g.
  • do you keep atlas.enabled on or only during version upgrade?
  • Is it ok to run multiple replicas of graphql-server while atlas.enabled is on?
  • Order of operations for migration...etc.

* adds atlas init container to graphql-server deployment
* updates readme docs
* updates guac version
* updates chart version

Signed-off-by: Amit Singh <[email protected]>
Signed-off-by: Ayush Shyam Kumar <[email protected]>
Signed-off-by: Anirudh Edpuganti <[email protected]>
Signed-off-by: Shreyas Pandya <[email protected]>
@ANIRUDH-333
Copy link

Hi @sunnyyip . Thank you for sharing the instructions on how to update the PR.
We have followed it and updated the PR to include all the params in Readme file. Please review these changes and let us know if there is anything else that needs to be added/changed to the documentation.
We are still working on the second point you mentioned.

@semmet95
Copy link
Contributor

semmet95 commented Dec 12, 2024

Fixes: #56

@semmet95
Copy link
Contributor

Hi @sunnyyip
We have added an Atlas Migration section as well.
For reference, Atlas was introduced in the guac repo here

  • Since the atlas migrations are supposed to be idempotent, if atlas.enabled is set to true the init container runs each time the graphql-server is updated/restarted.
  • We observed that even when running multiple replicas of graphql-server atlas migration operation doesn't cause any issues. We also tested this while upgrading Guac version from v0.11.2 to v0.12.3
  • When using atlas we set guac.backend.ent.db-migrate to false, so atlas has to take care of schema migrations before the graphql-server container starts. This is why we are running the atlas image as an init container.

@sunnyyip
Copy link
Contributor

Thanks @semmet95! Let's rebase to pick up the blobstore addr update to get pass the tests?
Also, please see my comment above about atlas migration's image.

@semmet95
Copy link
Contributor

Hey @sunnyyip
Regarding the blobstore addr update, that change is already in our branch here
I was working on both these PRs in parallel, so I had the fix added to them both.

Regarding atlas migration's image, I'm not sure which specific point you are referring to. Please let us know if we missed adding some doc or explanation.

@sunnyyip
Copy link
Contributor

sunnyyip commented Jan 6, 2025

@semmet95 please have a look at my comment at https://github.com/kusaridev/helm-charts/pull/57/files/62579f2f11f8d213307e6b9d0ade4c3adf5c475a

We should only be using the atlas image because the guac image (default/fallback right now) won't work for running atlas. This applies to both digest/image tag.

@semmet95
Copy link
Contributor

semmet95 commented Jan 7, 2025

Hey @sunnyyip
I can't see the comment you've shared. The link takes me to the PR diff page.
Do you need to submit that comment or is it some visibility issue?

@sunnyyip
Copy link
Contributor

sunnyyip commented Jan 8, 2025

Hey @sunnyyip I can't see the comment you've shared. The link takes me to the PR diff page. Do you need to submit that comment or is it some visibility issue?

sorry about the link...I mentioned you in the comment. Let me know if you see it. Thanks

@semmet95
Copy link
Contributor

semmet95 commented Jan 9, 2025

Hi @sunnyyip
I still can't see the comment :(
Can you please tag @ANIRUDH-333 there as well?

@pxp928
Copy link
Member

pxp928 commented Jan 9, 2025

It could be @sunnyyip and @semmet95 that the comment is not published yet. @sunnyyip you can see it but he cannot because you have not submitted to review.

value: '{{ index .Values.guac.backend.ent "db-address" }}'
{{- end }}
{{- if .Values.atlas.image.digest }}
image: "{{ .Values.image.repository | default .Values.guac.guacImage.repository }}@{{ .Values.atlas.image.digest }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the image always be .Values.atlas.image.repository and not have a default?
The guac image won't have the bins for running migration.

Copy link
Contributor

Choose a reason for hiding this comment

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

@semmet95 this is what I was referring to about image repo for atlas

@sunnyyip
Copy link
Contributor

It could be @sunnyyip and @semmet95 that the comment is not published yet. @sunnyyip you can see it but he cannot because you have not submitted to review.

Oops...just submitted the comments for review. It should be visible now. Thanks.

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.

5 participants