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

Add the Chronicle Server chart #441

Merged
merged 24 commits into from
Nov 29, 2023
Merged

Add the Chronicle Server chart #441

merged 24 commits into from
Nov 29, 2023

Conversation

AlexMapley
Copy link
Contributor

@AlexMapley AlexMapley commented Nov 15, 2023

Background

Part of https://github.com/rstudio/chronicle/issues/683.

We are migrating this from a private posit repo, where we've been hosting the chart previously.

Summary

  • Migrates the chronicle chart to this repo
  • Updates with rstudio helm best practices, standard features
  • Update the global file charts/_templates.gotmpl
  • Adds ci components and documentation

Notes

This PR is still an early draft, though I'd like to get some early feedback if possible and open the PR now for discussion

I've added a number of new features to the chronicle chart like pod tolerations, affinities, nodeSelectors, etc. - though i didn't add every feature under the sun. I'm on the lookout for more basic features we think the chronicle chart should include

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2023

CLA assistant check
All committers have signed the CLA.

@AlexMapley AlexMapley force-pushed the add_chronicle_chart branch 2 times, most recently from 9b3bcfa to dfe0063 Compare November 15, 2023 19:14
charts/_templates.gotmpl Outdated Show resolved Hide resolved
@AlexMapley AlexMapley changed the title Add the Chronicle Chart Add The Chronicle Chart Nov 15, 2023
@AlexMapley AlexMapley changed the title Add The Chronicle Chart Add the Chronicle chart Nov 15, 2023
@AlexMapley AlexMapley force-pushed the add_chronicle_chart branch 3 times, most recently from b36a930 to 63eab6f Compare November 17, 2023 19:19
@AlexMapley AlexMapley changed the title Add the Chronicle chart Add the Chronicle Server chart Nov 17, 2023
@AlexMapley
Copy link
Contributor Author

AlexMapley commented Nov 22, 2023

@colearendt @shahmonanshi what do you 2 think of this chart in its current form?
Are there others you might think we'd benefit from tagging as reviewers? I wasn't sure who else to add.

I've added you as a reviewer too @tnederlof as I see you've been making some cross cutting changes to this repo as well

@AlexMapley AlexMapley requested a review from tnederlof November 22, 2023 17:06
charts/posit-chronicle/NEWS.md Outdated Show resolved Hide resolved
charts/_templates.gotmpl Outdated Show resolved Hide resolved
charts/posit-chronicle/README.md.gotmpl Outdated Show resolved Hide resolved
charts/posit-chronicle/README.md.gotmpl Outdated Show resolved Hide resolved
charts/posit-chronicle/README.md.gotmpl Outdated Show resolved Hide resolved
charts/posit-chronicle/README.md.gotmpl Outdated Show resolved Hide resolved
charts/posit-chronicle/README.md.gotmpl Outdated Show resolved Hide resolved
charts/posit-chronicle/ci/simple-values.yaml Outdated Show resolved Hide resolved
charts/posit-chronicle/ci/no-local-storage-values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@colearendt colearendt left a comment

Choose a reason for hiding this comment

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

Just a few nits. Totally fine to update at a later date 😄 I try to avoid Statefulsets if possible, but if you have a good reason to use them (slower to roll over, etc. etc.), then carry on!

@@ -0,0 +1,80 @@
---
apiVersion: apps/v1
kind: StatefulSet
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting!! Why is a Statefulset important? Is that for a caching disk or some such?

Copy link
Contributor Author

@AlexMapley AlexMapley Nov 27, 2023

Choose a reason for hiding this comment

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

It's mostly as a convenience for the pvc provisioning, if you're running with local storage enabled Persistent storage a pvc is a must-have for most use cases. It's nice to not have to worry about duplicate replica sets being created on helm updates as well, the pod replacement policy of stateful-sets is generally simple and non-problematic for chronicle.

Theoretically we could run chronicle on a deployment/replica-set model, there's not too many technical blockers to think of. The stateful-set model has worked well so far though

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense.

It's worth noting that statefulsets do have some downsides: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#limitations

Usually services that uses statefulsets have a particular reason to do so. Otherwise replicasets/deployments are the more standard choice.

If you use a PVC, does each chronicle node store its own data? Or do they share data storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's tough, we do have a choice to make for sure. As a developer I feel like it's nice to have those pvcs deleted by default:

Deleting and/or scaling a StatefulSet down will not delete the volumes associated with the StatefulSet. This is done to ensure data safety, which is generally more valuable than an automatic purge of all related StatefulSet resources.

Though on the other hand, we are trying to build a data archive too. If we scaled a chronicle server from 1->5 replicas for a day, and stored a bunch of data, it would be nice if 2 weeks later when we tried scaling 1->5 again we found our old data in those local storage systems that we could add to.

If you use a PVC, does each chronicle node store its own data? Or do they share data storage?

Yeah each chronicle server does need a dedicated pvc, that's a known technical limitation right now in the alpha though. We need to rewrite some in-memory locks into file-locks for our compactor engine, and then we can support nfs or shared storage. We'll definitely need to make some helm changes around that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This conversation has made me think of an epic though that the chronicle team should probably undertake: https://github.com/rstudio/chronicle/issues/715.
I made a note there about looking into deployments vs. statefulsets and seeing what's more practical in our testing

charts/posit-chronicle/values.yaml Outdated Show resolved Hide resolved
charts/posit-chronicle/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@tnederlof tnederlof left a comment

Choose a reason for hiding this comment

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

Looks good, just some comments about using the latest tag.

charts/posit-chronicle/values.yaml Outdated Show resolved Hide resolved
charts/posit-chronicle/README.md.gotmpl Outdated Show resolved Hide resolved
charts/posit-chronicle/README.md.gotmpl Outdated Show resolved Hide resolved
Copy link
Contributor

@SamEdwardes SamEdwardes 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! I left a few nit-picky comments. Where ever we can drive consistency across the other products I think it is very helpful for our end users!

P.s. I have yet to try to deploy the helm chart, but I plan to. I will update this PR once I am done to report if I run into any issues.

charts/_templates.gotmpl Show resolved Hide resolved
charts/posit-chronicle/Chart.yaml Outdated Show resolved Hide resolved
charts/posit-chronicle/Chart.yaml Outdated Show resolved Hide resolved
charts/posit-chronicle/README.md Show resolved Hide resolved
charts/posit-chronicle/README.md Outdated Show resolved Hide resolved
charts/posit-chronicle/ci/complex-values.yaml Outdated Show resolved Hide resolved
charts/posit-chronicle/ci/complex-values.yaml Outdated Show resolved Hide resolved
charts/posit-chronicle/ci/complex-values.yaml Show resolved Hide resolved
charts/posit-chronicle/ci/complex-values.yaml Show resolved Hide resolved
charts/posit-chronicle/values.yaml Outdated Show resolved Hide resolved
@AlexMapley AlexMapley force-pushed the add_chronicle_chart branch 2 times, most recently from 3b39c52 to 6039a50 Compare November 27, 2023 22:48
@AlexMapley
Copy link
Contributor Author

Thanks everyone for all for your feedback! I don't think I disagree with anything, I tried to make 80-85% of the adjustments suggested and made a note of the rest with some github issue tracking.

  • Most notably from @SamEdwardes, we're now using pascal case for our config values and following other shared config conventions.
  • @tnederlof your comments pushed us to make a new release, and we now tag against the 2023.11.2 docker image
  • We have a new epic around your nfs comments @colearendt, we've definitely been thinking about supporting nfs too post this launch

@AlexMapley AlexMapley merged commit d9e7407 into main Nov 29, 2023
5 checks passed
@AlexMapley AlexMapley deleted the add_chronicle_chart branch November 29, 2023 21:52
@AlexMapley AlexMapley mentioned this pull request Dec 5, 2023
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.

9 participants