search

filecoin-project / helm-charts

7 stars 7 forks source link

feat: add config values for queue #144

Closed frrist closed 2 years ago

frrist commented 2 years ago
frrist commented 2 years ago

FWIW I took a look at how Lotus helm charts handle setting up the daemon configs (thanks @travisperson for the pointer) and think there would be some value in adopting the patterns used there. It would decouple configuration changes in Lily from its helm-chart. Here's an example: https://github.com/filecoin-project/helm-charts/blob/master/charts/lotus-fullnode/templates/statefulset-daemon.yaml#L1-L9 https://github.com/filecoin-project/helm-charts/blob/master/charts/lotus-fullnode/values.yaml#L16-L20 (for secrets, we could use a path reference from the values file)

I expect more configuration changes to land in Lily as it continues to be developed, and modifying the helm-charts every time this happens is unnecessary toil. What do you think? I can open an issue with this request if you're amenable to the idea.

placer14 commented 2 years ago

This is convenient for us but a poor experience for helm-chart users.

frrist commented 2 years ago

This is convenient for us but a poor experience for helm-chart users.

Our poll of users indicates that few use the helm charts.

We still handle secrets in the config.toml which forces the helm-chart user to handle them specially.

I mentioned this in my comment, we just configure them in a volume https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod and reference the path in the volumes files. (a plus here is we should be able to hot reload secrets!)

We shouldn't expect users to understand our config.toml in order to run Lily. Particularly for the most important values which will change regularly.

Im all for sane defaults, but given the rapid development lily is undergoing this isn't practical right now and creates more overhead for making changes and moving quickly -especially for a team of 1. Also, we could support both, just have some flag that either takes the config wholesale or does what we do now. From my perspective, I want less opinionated helm charts so I can move quickly building lily, and right now this just feels like extra work.

The cost of updating this config has been small (two changes in the past year?) and the overhead is not so taxing.

I don't expect this to be the case moving forward as Lily gets built out more. For example, I need to add more values via. I don't want to update this chart again. https://github.com/filecoin-project/lily/pull/955

frrist commented 2 years ago

I filed and issue for these here: https://github.com/filecoin-project/helm-charts/issues/148