search

kiwigrid / helm-charts

Helm charts for Kubernetes curated by Kiwigrid
https://kiwigrid.github.io
MIT License
184 stars 210 forks source link

[influxdb-backup] use copy instead of synch #386

Closed ph1l1ppk closed 3 years ago

ph1l1ppk commented 4 years ago

What this PR does / why we need it:

When uploading a backup to azure blob storage: I added a switch to optionally use copy command instead of sync command. The sync command may delete existing backups in the blob storage which do not exist (anymore) in the pvc which can be be indesirable. Besides I bumped the default azure-cli container image to a newer version as the old version had issues with the sync command with me

Special notes for your reviewer:

@monotek I hope you will find the changes useful

edit 28.07.2020: i changed the chart version to 0.3.0 and updated the checklist

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

monotek commented 4 years ago

This is wanted behaviour as retention setting relies on this. see: https://github.com/kiwigrid/helm-charts/blob/master/charts/influxdb-backup/values.yaml#L72

You could raise the value from 10 to 100 if you wnat to keep more files.

ph1l1ppk commented 4 years ago

Thanks for the answer. We would like to have different, decoupled retention policies on the persistent volume and the blob storage which the pr enables us to have. The persistent volume to us is more of a shortterm buffer in the backup process. The blob storage is our long-term backup space were we will keep more backups and longer as in the pv. Blob storage also comes cheaper to us than the pv so we do not want to keep more backups there than we need to.

monotek commented 4 years ago

So i guess soemthing like having local & online retention would be a better option. Per default online should have same value as offline retention but could be changed optionally.

ph1l1ppk commented 4 years ago

I am not sure whether I can relate local & online retention correctly to persistent volume and blob storage retention. Right now the pr implements this behavior: By default Values.backup.uploadProviders.azure.useCopyInsteadSync is set to false. In this case the behaviour is unchanged to the current chart and the same retention policy is active on PV and blob storage. If you set Values.backup.uploadProviders.azure.useCopyInsteadSync to true, no retention policy will be active on the blob storage as the az storage blob upload-batch command does not delete files on the azure blob storage. You will have to provide your on implementation of the desired retention policy for the blob storage. We see the blob storage as our main, long term backup space. Right now we do not really have a retention policy implemented for the blob storage. We are not quite sure yet how the long term retention policy for us should look like and we are not sure yet with what means we will implement a future retention policy. But we feel with this pr we have all the flexibility and possibilities we need to implement any future retention policies for the blob storage. Until then our backup persistent volume will not exceed it's capacity with old backups because the cron job will remove old backups on the PV after retentionDays. Please tell me if you find this approach of optionally decoupling the retention policies of PV and blob storage basically useful or you think that this is the wrong way to go.

monotek commented 4 years ago

If we add this change, it should be possible on all storage backends. So we only need 1 generic value, which needs to be changed. Could you implement it that way please?

ph1l1ppk commented 4 years ago

I will look into it. I do not have any experience with Google cloud but I will see what I can do.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ph1l1ppk commented 3 years ago

finally i added changes so the same function also works on google storage backends with only 1 generic value. I did not test it on google cloud but the docu https://cloud.google.com/storage/docs/gsutil/commands/rsync seems pretty straight forward. I am not sure what I can do about the failing tests.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.