Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

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

Merged
merged 19 commits into from
Sep 9, 2022
Merged

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

merged 19 commits into from
Sep 9, 2022

Conversation

tomsozolins
Copy link
Contributor

Add persistent volume support for zabbix-proxy

@tomsozolins tomsozolins requested a review from aeciopires as a code owner August 4, 2022 15:21
@tomsozolins
Copy link
Contributor Author

tomsozolins commented Aug 4, 2022

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
Copy link
Contributor Author

tomsozolins commented Aug 5, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

aeciopires commented Aug 22, 2022

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
Copy link
Contributor Author

tomsozolins commented Aug 23, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

aeciopires commented Aug 23, 2022

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
Copy link
Contributor Author

@aeciopires Ok, i will look into this later.

Cleanup volume mount templates - extra sections is enough for use case
Cleanup redundant values - extra values is enough for use case
@tomsozolins
Copy link
Contributor Author

tomsozolins commented Sep 2, 2022

@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

Chart.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@aeciopires aeciopires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tomsozolins!

Great job! Thanks for your contribution.

I wroten some suggestions.

Please, do you can make other changes?

  1. In values.yaml file change ubuntu-6.0.7 to ubuntu-6.0.8

  2. In docs/example/kind/values.yaml file change alpine-6.2.0 to alpine-6.2.2

  3. Run the helm-docs command following the instructions of CONTRIBUTING.md file to update the README.md.

Have a nice day.

CC: @sa-ChristianAnton do you can review this pull request too?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@aeciopires aeciopires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @tomsozolins!

Thank you again for your contribution.

@tomsozolins
Copy link
Contributor Author

@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
Copy link
Contributor Author

tomsozolins commented Sep 2, 2022

@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
Copy link
Collaborator

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

@aeciopires
Copy link
Collaborator

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants