cetic / helm-zabbix

Helm Chart For Zabbix
https://artifacthub.io/packages/helm/cetic/zabbix
Apache License 2.0
56 stars 57 forks source link

feat(zabbix-proxy): add support for persistent volume #73

Closed tomsozolins closed 2 years ago

tomsozolins commented 2 years ago

Add persistent volume support for zabbix-proxy

tomsozolins commented 2 years ago

Will update later currently getting error - cannot open database file "/var/lib/zabbix/db_data/zabbix-proxy.sqlite": [2] No such file or directory

tomsozolins commented 2 years ago

Zabbix process didn't have access to the volume as it had root ownership by default. Updating ownership to zabbix user id 1997 fixed the problem.

aeciopires commented 2 years ago

Hi, @tomsozolins!

Thank you very much for your contribution. I really appreciate this initiative.

Sorry for the delay in reviewing your Pull Request. I was on vacation. Tomorrow I will take the time to review and test the changes. Have a good week.

CC: @sa-ChristianAnton for your information.

aeciopires commented 2 years ago

Hi @tomsozolins!

I understood your contribution, but I think that parameters have same result and are more simple. Do you agree? Could you test this parameters?

zabbixproxy:
  # -- additional volumeMounts to the zabbix proxy container
  extraVolumeMounts: []
  # -- additional volumes to make available to the zabbix proxy pod
  extraVolumes: [] 

If works, I think this Pull Request can be closed without merge.

The same idea works to zabbixserver, zabbixagent, zabbixweb.

CC: @sa-ChristianAnton

tomsozolins commented 2 years ago

Hi! @aeciopires what about the volumeClaimTemplates? Can it be specified in extra fields? Also noticed that security context is global value, but we only need to change it for zabbix proxy when mounting persistent volume as zabbix user.

aeciopires commented 2 years ago

Hi @tomsozolins !

I agree with the idea of ​​securityContext having the option to be localized and with the idea of ​​changing the implementation to support extraVolumeClaimTemplate... but looking again at values.yaml... we have the extraPodSpecs parameter which makes the code generic enough to add any settings. Does this work for you?

zabbixproxy:
  # -- additional specifications to the zabbix proxy pod
  extraPodSpecs: {}

The same idea works to zabbixserver, zabbixservice, zabbixweb.

If it doesn't work, could you change the PR to add securityContext and extraVolumeClaimTemplate support for zabbixProxy, zabbixServer, zabbixWeb, zabbixWebservice?

aeciopires commented 2 years ago

Hi @tomsozolins!

I'm sorry!

The extraPodSpecs won't work for you because it is at the pod level and the extraVolumeClaim would go into the Spec at the StateFulSet level (one level higher).

Also volumeClaimTemplates only works for statefulset... it doesn't work for deployment [1] [2]. In this case, you can only change it in zabbixProxy.

Could you change the PR implementation to consider the extraVolumeClaimTemplate correctly (making it very generic similar to the other extra parameters) and leave the securityContext localized for each component, overriding the global? Example: if the localized securityContext exists, use it, otherwise use the global.

tomsozolins commented 2 years ago

@aeciopires Ok, i will look into this later.

tomsozolins commented 2 years ago

@aeciopires Hi! I removed all the extra stuff i added previously and added only extraVolumeClaimTemplate. This is enough to add persistence for zabbixproxy:

extraVolumeMounts:
    - mountPath: /var/lib/zabbix/db_data
      name: zabbixproxy-data
extraPodSpecs:
    securityContext:
        fsGroup: 1997
extraVolumeClaimTemplate:
    - metadata:
        name: zabbixproxy-data
      spec:
        accessModes:
          - ReadWriteOnce
        resources:
          requests:
            storage: 1Gi
        storageClassName: gp3
tomsozolins commented 2 years ago

@aeciopires Found an issue with the addition to VolumeClaimTemplate now there is error: executing "zabbix/templates/statefulset-zabbix-proxy.yaml" at <.Values.zabbixproxy.extraVolumeClaimTemplate>: can't evaluate field Values in type []interface {}

tomsozolins commented 2 years ago

@aeciopires Found an issue with the addition to VolumeClaimTemplate now there is error: executing "zabbix/templates/statefulset-zabbix-proxy.yaml" at <.Values.zabbixproxy.extraVolumeClaimTemplate>: can't evaluate field Values in type []interface {}

Should it be like this? This works without errors.

  volumeClaimTemplates:
    {{- toYaml . | nindent 4 }}
aeciopires commented 2 years ago

I understood the problem @tomsozolins. Works with the new config?

aeciopires commented 2 years ago

Great. Thanks for hotfix. I will test in the next days and will make the merge and publish the version.

Feel free for new improvements.