loki icon indicating copy to clipboard operation
loki copied to clipboard

Helm-Chart: Make PVCs in Statefulsets optional

Open atze234 opened this issue 2 years ago • 4 comments

Signed-off-by: atze234 [email protected]

What this PR does / why we need it: Make PVCs of Statefulsets optional as of the old loki Chart

Which issue(s) this PR fixes: Fixes #7068

Special notes for your reviewer:

Checklist

  • [x] Reviewed the CONTRIBUTING.md guide
  • [x] Documentation added
  • [ ] Tests updated
  • [x] CHANGELOG.md updated
  • [ ] Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

atze234 avatar Sep 21 '22 13:09 atze234

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 21 '22 13:09 CLAassistant

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
- querier/queryrange	-0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot avatar Sep 21 '22 13:09 grafanabot

Hi @trevorwhitney i put the persistence.enabled default to true because of the wal and the possibility of data loss. When persistence is disabled the wal will be deleted on every restart, but thats not really a Problem for a test cluster where i just want to have an easy, low-cost loki running. So i ported this non-pvc-mode over from old chart.

Looking at that this way i‘d say an Installation without PVC would be easier/lightweight as well, because in an easy mode installation i dont want to have pvc‘s. Also i try to avoid them as much as possible.

I agree that for the „read“ a deployment would fit better then, but it makes the installation a bit more complicated or confusing and the chart as well.

atze234 avatar Sep 21 '22 16:09 atze234

Can you describe your "test cluster" a little better (ie. what do you use it for)?

The main goal of this chart is an easy, opinionated, and production ready installation. There is already a lot of feedback that Loki has too many configuration options, and can be difficult to get started with. Thus, it is the goal of this helm chart to reduce configuration as much as possible.

We publish jsonnet for people needing highly configurable deployments, and I wonder if that would be a better candidate for your "test cluster"? Or perhaps you could use helm post rendering with something like jsonnet or kustomize to achieve this?

I'm hesitant to expand the config surface area of the chart for a test use case that most people will not use, and that we would never recommend for production.

trevorwhitney avatar Sep 21 '22 17:09 trevorwhitney

Ok i understand that. I just wanted to make the pvc configureable for any environment where wal dataloss would be ok. for example a cluster for staging area for applications, where i just want to test applications. in such environments i generally try to avoid pvc to save some money and/or resources. As the old chart supported to disable the PVC i thought it would be ok here too, but i see that the persistent WAL is part of loki and its mandatory to have it persistent. From this point of view it was a mistake that the PVC could be disabled in the old chart and so its ok to not make it possible to opt-out.

atze234 avatar Sep 26 '22 09:09 atze234

@atze234 thanks for your understanding. One thing we're trying to do with moving the chart into the Loki repo is to take a more involved (from the Loki team) and more opinionated approach, to help people get up and running with production ready clusters faster, so we may need to make a few departures from the old chart in support of that goal.

trevorwhitney avatar Sep 26 '22 15:09 trevorwhitney