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

helm: Enable persistence in custom mode #11368

Closed
wants to merge 17 commits into from
Closed

helm: Enable persistence in custom mode #11368

wants to merge 17 commits into from

Conversation

pierreyves-lebrun
Copy link

Users might want to persist data while providing a custom config file, this PR enables persistence in that scenario

@github-actions github-actions bot added the helm label Mar 23, 2022
@webvictim
Copy link
Contributor

@pierreyves-lebrun Thanks for the PR! Could you please take a look at https://github.com/gravitational/teleport/blob/master/examples/chart/CONTRIBUTING.md and make sure to do everything on the list?

@webvictim webvictim self-assigned this Mar 23, 2022
@webvictim webvictim mentioned this pull request Mar 23, 2022
Closed
@webvictim
Copy link
Contributor

/gcbrun

@webvictim
Copy link
Contributor

@pierreyves-lebrun Could you please add some tests to deployment_test.yaml which validate the functionality that you added?

@pierreyves-lebrun
Copy link
Author

@pierreyves-lebrun Could you please add some tests to deployment_test.yaml which validate the functionality that you added?

@webvictim Added a few tests, please review again

@webvictim
Copy link
Contributor

/gcbrun

@pierreyves-lebrun
Copy link
Author

/gcbrun

All jobs passed but I rebased onto master so you might want to rerun the pipeline

webvictim
webvictim reviewed Apr 8, 2022
View reviewed changes
Copy link
Contributor

@webvictim webvictim left a comment

Choose a reason for hiding this comment

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

Thanks again @pierreyves-lebrun. Just realised another backwards compatibility situation and a couple of other comments, then we're good to go. This is a tricky PR and I really don't want to break anyone's current setup which is why I'm being such a stickler - apologies!

examples/chart/teleport-cluster/values.yaml Outdated Show resolved Hide resolved
examples/chart/teleport-cluster/templates/deployment.yaml Show resolved Hide resolved
examples/chart/teleport-cluster/tests/deployment_test.yaml Outdated Show resolved Hide resolved
@pierreyves-lebrun
Copy link
Author

Thanks again @pierreyves-lebrun. Just realised another backwards compatibility situation and a couple of other comments, then we're good to go. This is a tricky PR and I really don't want to break anyone's current setup which is why I'm being such a stickler - apologies!

No worries, it's definitely worth being cautious when it comes to data persistence

@webvictim
Copy link
Contributor

/gcbrun

webvictim
webvictim approved these changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@webvictim webvictim left a comment

Choose a reason for hiding this comment

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

LGTM - just one final nitpick in the values file. We do need to add these new values to the Helm reference for the teleport-cluster chart, but given how much work you've done on the PR already, I'm OK with making those updates myself in a separate PR. Thanks @pierreyves-lebrun!

examples/chart/teleport-cluster/values.yaml Outdated Show resolved Hide resolved
pierreyves-lebrun and others added 6 commits April 12, 2022 09:10
@webvictim
Copy link
Contributor

/gcbrun

@webvictim
Copy link
Contributor

/gcbrun

@webvictim
Copy link
Contributor

/gcbrun

@webvictim
Copy link
Contributor

/gcbrun

@webvictim
Copy link
Contributor

/gcbrun

@webvictim
Copy link
Contributor

/gcbrun

@webvictim webvictim mentioned this pull request Apr 15, 2022
Merged
@webvictim
Copy link
Contributor

Hey @pierreyves-lebrun, thanks for all the work. I've opened #11693 and will merge this there - our Github bot sometimes always handle PRs from forks very well (and this is one of those cases)

@webvictim webvictim closed this Apr 15, 2022
auto-merge was automatically disabled April 15, 2022 02:53

Pull request was closed

webvictim added a commit that referenced this pull request Apr 16, 2022
@webvictim
helm: Buddy merge for #11368 (Enable persistence in custom mode) (#11993
a2ece60 
)

* feat: Standardize persistence

* helm: Use deprecated standalone.existingClaimName when specified over newer persistence.existingClaimName

* chore: Change pvc comment

* feat: Update chart reference

* Update docs/pages/kubernetes-access/helm/reference/teleport-cluster.mdx

Co-authored-by: Pierre Lebrun <pierreyves.lebrun@rakuten.com>
@webvictim webvictim mentioned this pull request Apr 25, 2022
Closed
webvictim added a commit that referenced this pull request Apr 25, 2022
@webvictim
helm: Buddy merge for #11368 (Enable persistence in custom mode) (#11993
9886111 
)

* feat: Standardize persistence

* helm: Use deprecated standalone.existingClaimName when specified over newer persistence.existingClaimName

* chore: Change pvc comment

* feat: Update chart reference

* Update docs/pages/kubernetes-access/helm/reference/teleport-cluster.mdx

Co-authored-by: Pierre Lebrun <pierreyves.lebrun@rakuten.com>
webvictim added a commit that referenced this pull request May 6, 2022
@webvictim
helm: Buddy merge for #11368 (Enable persistence in custom mode) (#11993
535b785 
) (#12218)

* feat: Standardize persistence

* helm: Use deprecated standalone.existingClaimName when specified over newer persistence.existingClaimName

* chore: Change pvc comment

* feat: Update chart reference

* Update docs/pages/kubernetes-access/helm/reference/teleport-cluster.mdx

Co-authored-by: Pierre Lebrun <pierreyves.lebrun@rakuten.com>
@webvictim webvictim mentioned this pull request Jun 8, 2022
Closed
@stefansedich
Copy link
Contributor

stefansedich commented Aug 15, 2022 •
edited
Loading

Thanks again @pierreyves-lebrun. Just realised another backwards compatibility situation and a couple of other comments, then we're good to go. This is a tricky PR and I really don't want to break anyone's current setup which is why I'm being such a stickler - apologies!

@webvictim FWIW this one broke our setup 😆 😭 we don't use persistence and found after a 9.x upgrade it was turned on by default due to this PR, as we run multiple replicas things went 💥 as the PVC can not be mounted to multiple pods so not sure myself we want this enabled by default?

@webvictim
Copy link
Contributor

Sorry @stefansedich. I think that enabling persistence by default is what we do want for the chart, just to avoid people failing to set it up and then inadvertently losing cluster data when upgrading.

I apologise that it broke your setup - hopefully setting persistance.enabled: false in your values ended up fixing things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webvictim webvictim webvictim approved these changes

@russjones russjones russjones approved these changes

@zmb3 zmb3 zmb3 approved these changes

@r0mant r0mant Awaiting requested review from r0mant

Assignees

@webvictim webvictim

Labels
helm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pierreyves-lebrun @webvictim @stefansedich @russjones @zmb3