Provide persistence option in values.yml

[PhilsLab]

There should be an option to create no persistent storage at all, e.g. when using the master instance for prometheus scraping only.

[cakrit]

memory mode = ram
https://docs.netdata.cloud/database/#memory-modes

Please reopen if you are facing problems with it.

[PhilsLab]

This is what I am referring to. The helm chart creates two persistent volume claims here:
templates/statefulset.yaml#L14

There should be an option to disable the creation of those two PVCs, so the containers can be fully stateless if desired.

[cakrit]

Ok, I get what you're saying. We'll make it optional.

[karuppiah7890]

@cakrit I'm interested to contribute to this! Let me know if you will be willing to guide and accept the PR. I can read about the memory modes and then we can discuss when the volumes needn't be created and not mounted too if they don't exist

[cakrit]

Sure! I was actually just going to make the section under https://github.com/netdata/helmchart/blob/master/templates/statefulset.yaml#L14 and the section under https://github.com/netdata/helmchart/blob/master/templates/daemonset.yaml#L90 controlled by a single config parameter in values.yaml. e.g. something like this in values.yaml:

master:
  persistentVolumes:
    enabled: true  
   database:
      storageclass: "standard"
      volumesize: 2Gi
    alarms:
      storageclass: "standard"
      volumesize: 100Mi

Then based on master.persistentVolumes.enabled we'd end up with a different config in statefulset.yaml. Note that the storageclass and volumesize are renamed here as well (e.g. instead of master.database.storageclass we'd have master.persistentVolumes.database.storageclass).

I didn't consider the possibility of detecting the existence of persistent volumes though. That would be a much better solution, I just don't know how to do it/haven't searched if it can be done yet.

Of course I'd be happy to test and merge a PR with either solution.

[karuppiah7890]

Umm, when I said

and not mounted too if they don't exist

I meant not using any mounts if we don't have any PVs 🙈 And I really have to read the chart completely to understand more. To fix this issue, the enabling and disabling option looks cool 👍 And we can probably add the feature of using an existing PV later I guess, if it's needed.

And about the above solution, we are controlling the creation of PVs for database and alarms together, is that okay? or would there be any case where a user would want to control them separately? in which case we will need to put the toggles separately

[cakrit]

And about the above solution, we are controlling the creation of PVs for database and alarms together, is that okay? or would there be any case where a user would want to control them separately? in which case we will need to put the toggles separately

Excellent idea, sure. Much better this way.

[karuppiah7890]

I'll make the changes then? Will ping here if I have doubts

[cakrit]

Definitely, have fun! :)

[cakrit]

Hi @karuppiah7890 , just a ping to check if you're still able to progress this.

[karuppiah7890]

sorry. been caught up with too many things. will be checking it out this weekend!

[karuppiah7890]

How does this values look?

  database:
    peristence: true
    storageclass: "standard"
    volumesize: 2Gi

  alarms:
    persistence: true
    storageclass: "standard"
    volumesize: 100Mi

or should we keep it as

  database:
    peristence: 
      enabled: true
    storageclass: "standard"
    volumesize: 2Gi

  alarms:
    persistence: 
      enabled: true
    storageclass: "standard"
    volumesize: 100Mi

The above two options won't break the existing chart users, which is good, or else we will have to bump up major version change. Or is it still not production ready, seeing that it's having the 0.0.x and breaking changes are allowed? If it's allowed, I also though of this one (just putting related things together):

  database:
    peristence: 
      enabled: true
      storageclass: "standard"
      volumesize: 2Gi

  alarms:
    persistence: 
      enabled: true
      storageclass: "standard"
      volumesize: 100Mi

But personally, it's annoying to have charts having breaking changes when upgrading, especially when you just see that the version change is only patch or minor version change, and then it ends up breaking up something. So, I would go with one of the first two. What are your thoughts on this?

[cakrit]

First option is fine, even though the last one is more correct.
To tell you the truth, I'm not sure yet how many people are using this helmchart, I'll need to spend some time with GA so see at least who accesses the README. But we have at netdata a tradition of not making breaking changes.

After we merge your change, I will change the netdata tag to 1.15.0 (planned to be released tomorrow) and get this baby to 1.0.0. It seems to be working in general, so I don't see why not.

One note, ensure it is persistence instead of peristence :)

[karuppiah7890]

So, which change should I make? The breaking change? Also, before releasing 1.0.0 let's also add apiVersion in the Chart.yaml as it's a required field in helm 3.0 and also fix the maintainer field to "maintainers" array. Noticed these two yesterday

…

On Thu, May 16, 2019, 10:29 PM Chris Akritidis ***@***.***> wrote: First option is fine, even though the last one is more correct. To tell you the truth, I'm not sure yet how many people are using this helmchart, I'll need to spend some time with GA so see at least who accesses the README. But we have at netdata a tradition of not making breaking changes. After we merge your change, I will change the netdata tag to 1.15.0 (planned to be released tomorrow) and get this baby to 1.0.0. It seems to be working in general, so I don't see why not. One note, ensure it is persistence instead of peristence :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17?email_source=notifications&email_token=ADBXB2CX2EYRAUF7QVOQUDLPVWHGPA5CNFSM4HFDV56KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVSN4KY#issuecomment-493149739>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADBXB2ECU2H3CTZLUIBV5MLPVWHGPANCNFSM4HFDV56A> .

[cakrit]

So, which change should I make? The breaking change?
No, let's go with your first suggestion.

let's also add apiVersion in the Chart.yaml
Sure, put that in your PR as well (not really sure what it is to tell you the truth).