Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some changes and modifications #708

Merged
merged 22 commits into from
Jun 27, 2022
Merged

Some changes and modifications #708

merged 22 commits into from
Jun 27, 2022

Conversation

mojtabash78
Copy link
Contributor

@mojtabash78 mojtabash78 commented Jun 18, 2022

Description

  • Barman connection to postgres fixed via scopeName
  • barman receive-wal is now done via barman cron
  • post_backup_retry_script and pre_recovery_retry_script added to global configuration
  • configmap-entrypoint.yaml created and scopeName added to it
  • additionalENVs added for deployment
  • values.yaml file updated
  • README.md file updated
  • .helmignore file added

Issues

Checklist

  • This PR contains a description of the changes I'm making
  • I updated the version in Chart.yaml
  • I updated applicable README.md files using pre-commit run
  • I documented any high-level concepts I'm introducing in docs/
  • CI is currently green and this is ready for review
  • I am ready to test changes after they are applied and released

@mojtabash78 mojtabash78 requested review from a team and michaelimfeld as code owners June 18, 2022 10:21
@mojtabash78 mojtabash78 requested review from hairmare and tongpu and removed request for a team June 18, 2022 10:21
@github-actions github-actions bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 18, 2022
Copy link
Member

@tongpu tongpu left a comment

Choose a reason for hiding this comment

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

Hi @mojtabash78

Thank you for your contribution. Could you please have a look at the changes that I've proposed and quickly elaborate on the benefits of using an entrypoint script instead of cron.

charts/barman/Chart.yaml Outdated Show resolved Hide resolved
charts/barman/Chart.yaml Outdated Show resolved Hide resolved
charts/barman/templates/configmap-barman.yaml Outdated Show resolved Hide resolved
charts/barman/templates/configmap-barman.yaml Outdated Show resolved Hide resolved
mojtabash78 and others added 3 commits June 20, 2022 11:29
Co-authored-by: Lukas Grossar <lukas.grossar@gmail.com>
Co-authored-by: Lukas Grossar <lukas.grossar@gmail.com>
Co-authored-by: Lukas Grossar <lukas.grossar@gmail.com>
@mojtabash78
Copy link
Contributor Author

Hi @mojtabash78

Thank you for your contribution. Could you please have a look at the changes that I've proposed and quickly elaborate on the benefits of using an entrypoint script instead of cron.

Hi @tongpu
I am using ubc image for Barman: https://github.com/ubc/barman-docker
I suppose you use this image as well.
This image has a entrypoint.sh script. In this script, cron schedules and Barman configurations are generated, but we already generated them in configmaps and this script tries to overwrite them in configmaps which results in error because files mounted by configmaps are read-only. I also set scopeName in configmap-entrypoint, so Barman commands for replication-status and creating replication slot will be done successfully.

@hairmare
Copy link
Contributor

Thank you for your contribution. Could you please have a look at the changes that I've proposed and quickly elaborate on the benefits of using an entrypoint script instead of cron.

Hi @tongpu I am using ubc image for Barman: https://github.com/ubc/barman-docker I suppose you use this image as well. This image has a entrypoint.sh script. In this script, cron schedules and Barman configurations are generated, but we already generated them in configmaps and this script tries to overwrite them in configmaps which results in error because files mounted by configmaps are read-only. I also set scopeName in configmap-entrypoint, so Barman commands for replication-status and creating replication slot will be done successfully.

Personally i'm not a big fan of overriding the entrypoint with one generated into a configmap, but i see that it is most likely the straight forward way to make the chart be usable for a large range of requirements.

if i recall correctly the original argument against using a kubernetes-native CronJob was something about the CronJob and barman's internal ssh service being tightly coupled. We investigated using a K8s CronJob a while back and came to the conclusion that too much re-architecting would be needed to make it work.

@mojtabash78
Copy link
Contributor Author

Thank you for your contribution. Could you please have a look at the changes that I've proposed and quickly elaborate on the benefits of using an entrypoint script instead of cron.

Hi @tongpu I am using ubc image for Barman: https://github.com/ubc/barman-docker I suppose you use this image as well. This image has a entrypoint.sh script. In this script, cron schedules and Barman configurations are generated, but we already generated them in configmaps and this script tries to overwrite them in configmaps which results in error because files mounted by configmaps are read-only. I also set scopeName in configmap-entrypoint, so Barman commands for replication-status and creating replication slot will be done successfully.

Personally i'm not a big fan of overriding the entrypoint with one generated into a configmap, but i see that it is most likely the straight forward way to make the chart be usable for a large range of requirements.

if i recall correctly the original argument against using a kubernetes-native CronJob was something about the CronJob and barman's internal ssh service being tightly coupled. We investigated using a K8s CronJob a while back and came to the conclusion that too much re-architecting would be needed to make it work.

Hi @hairmare
Thanks for your descriptions.
I see that you agree that entrypoint.sh is getting overridden with one generated in configmap and this indeed prevents Barman to perform the backup procedure. The script is implemented in the ubc image (which is used in values.yaml) and gets overridden anyway. So how do you suggest to use the chart other than making entrypoint.sh a configmap?

Best

@hairmare
Copy link
Contributor

So how do you suggest to use the chart other than making entrypoint.sh a configmap?

The only idea i have so far is landing the needed changes directly in the upstream ubc container. That would de-facto mean rewriting to script to support a wide range of args so we can configure it to our liking at runtime. Looking at the current entrypoint, I don't think that would be a reasonable proposal for the ubc image.

Because of that, i think that generating the entrypoint into a ConfigMap and then mounting it is a reasonable approach. This is not something i'd usually recommend, but for this use-case it looks ok.

@hairmare
Copy link
Contributor

@tongpu i think this lgtm, do we need any additional feedback from our team to get this merged? I'll raise it in tomorrows stand-up to be sure but i assume we can probably land it soon.

@mojtabash78
Copy link
Contributor Author

mojtabash78 commented Jun 27, 2022

@tongpu i think this lgtm, do we need any additional feedback from our team to get this merged? I'll raise it in tomorrows stand-up to be sure but i assume we can probably land it soon.

Hi @hairmare @tongpu
any updates on this PR ?

@tongpu
Copy link
Member

tongpu commented Jun 27, 2022

@tongpu i think this lgtm, do we need any additional feedback from our team to get this merged? I'll raise it in tomorrows stand-up to be sure but i assume we can probably land it soon.

Hi @hairmare any updates on this PR ?

I haven't found time last week to look into it again, but I'll provide some feedback until tomorrow.

Copy link
Member

@tongpu tongpu left a comment

Choose a reason for hiding this comment

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

@mojtabash78 Thanks for your contribution. You probably need to rebase your branch before we can merge it.

@hairmare hairmare merged commit b7cfbf9 into adfinis:main Jun 27, 2022
@hairmare
Copy link
Contributor

Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants