loki icon indicating copy to clipboard operation
loki copied to clipboard

feat: [helm] Support for PVC Annotations for Non-Distributed Modes

Open onelapahead opened this issue 2 years ago • 11 comments

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.md guide (required)
  • [x] Documentation added
  • [ ] Tests updated
  • [x] CHANGELOG.md updated
    • [ ] If the change is worth mentioning in the release notes, add add-to-release-notes label
  • [ ] 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.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • [ ] If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

onelapahead avatar Feb 21 '24 15:02 onelapahead

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 21 '24 15:02 CLAassistant

Apologies messed up a rebase, one sec

onelapahead avatar Feb 22 '24 16:02 onelapahead

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.

onelapahead avatar Feb 22 '24 16:02 onelapahead

See https://github.com/grafana/loki/pull/12281 for another example where annotations on persistent storage would be valuable.

onelapahead avatar Mar 21 '24 00:03 onelapahead

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!

danielfrg avatar Mar 21 '24 04:03 danielfrg

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 🙇🏻

onelapahead avatar Mar 22 '24 00:03 onelapahead

@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 avatar Apr 02 '24 21:04 onelapahead

@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.

JStickler avatar Apr 02 '24 22:04 JStickler

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 🙇🏻

onelapahead avatar Apr 03 '24 16:04 onelapahead

Could we get some eyes on this one please. Hopefully should be a quick one to review.

danielfrg avatar Apr 21 '24 00:04 danielfrg

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.

onelapahead avatar May 09 '24 23:05 onelapahead

@trevorwhitney @MichelHollands can this be given a final review soon before this PR falls out-of-date again ?

onelapahead avatar May 20 '24 19:05 onelapahead