feat: [helm] Support for PVC Annotations for Non-Distributed Modes
What this PR does / why we need it:
This adds the ability to specify PVC annotations in the volume claim templates of all StatefulSets in the chart (read, write, backend, and singleBinary).
CSI drivers often allow for customizing PVCs, or modifying existing PVCs, via annotations. For example, in the open-source the AWS EBS CSI driver allows for changing IOPS, storage type, etc. via annotations: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/docs/modify-volume.md.
Supporting PVC annotations is quite common in Helm charts. For example, the minio chart that is a dependency of the loki chart also allows for such customization and this PR additionally sets that value so it is more clearly documented for loki chart users.
Which issue(s) this PR fixes: N/A
Special notes for your reviewer:
Checklist
- [x] Reviewed the
CONTRIBUTING.mdguide (required) - [x] Documentation added
- [ ] Tests updated
- [x]
CHANGELOG.mdupdated- [ ] If the change is worth mentioning in the release notes, add
add-to-release-noteslabel
- [ ] If the change is worth mentioning in the release notes, add
- [ ] Changes that require user attention or interaction to upgrade are documented in
docs/sources/setup/upgrade/_index.md - [x] For Helm chart changes bump the Helm chart version in
production/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PR - [ ] If the change is deprecating or removing a configuration option, update the
deprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR
Apologies messed up a rebase, one sec
There's some extra checks / reviewers still on the PR due to the rebase I messed up, but the PR is now ready for review again and up-to-date.
See https://github.com/grafana/loki/pull/12281 for another example where annotations on persistent storage would be valuable.
Thanks for the PR. I was facing the same issue and made the same fix.
For example in an NFS mount using the k8s NFS provider you define what path to mount like this:
annotations:
"nfs.io/storage-path": "/monitoring/loki"
Hopefully we can move this one forward!
Noticing @JStickler on https://github.com/grafana/loki/pull/12023/commits/91c5520049ff51812b7954df63f86b86921877de the checks that failed seemed all cosmetic - it says helm: isn't an appropriate PR title prefix (though swore I read in the contributor guide to use that) and then that I need to rerun make -C docs sources/setup/install/helm/reference.md. When doing that make (on MacOS) considered the build target "built" and that it was up-to-date.
I've added a fix to the Makefile to always builds the docs when run but that could be I'm missing something. Look forward to the feedback! And thanks @danielfrg for the other contribution and helping promote the need for this 🙇🏻
@MichelHollands @JStickler any thoughts on testing this one again or initially reviewing in case I missed anything obvious ?
We could really use the ability to have this (and another user has reported the need for it too) without having to resort to forking the chart for awhile.
I also see PRs like #12434 in-progress where new modes for running the microservices model are being added, so it'd be good to get this persistence annotations pattern in place so its not missed on new components going forward. Thanks.
@onelapahead I'm the technical writer on the team, I've run the build and fixed the title of your PR so that it passes the Conventional Commits test (which might be new since you opened this PR), but I'm not familiar enough with Helm charts to approve code changes. Hopefully @MichelHollands can take a look at this in the next day or two.
Awesome no worries @JStickler appreciate you running the checks just so I can get the feedback of anything I should fix before its further reviewed 🙇🏻
Could we get some eyes on this one please. Hopefully should be a quick one to review.
Thanks @trevorwhitney for the feedback and patience, believe everything has been addressed now! Noting that all PVCs / claim templates in the new distributed mode of deployment had annotations support, so this PR is largely just adding this support to single binary and simple scalable modes now.
@trevorwhitney @MichelHollands can this be given a final review soon before this PR falls out-of-date again ?