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

Fix VolumeSnapshot backup capability #335

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cam-at-tactiq
Copy link

@cam-at-tactiq cam-at-tactiq commented Jul 24, 2024

Hi this is my first contribution so please let me know if I have missed anything.

This PR fixes two issues with VolumeSnapshots for backups:

  • Missing VolumeSnapshotConfiguration block on the Cluster resource
  • Prevents the chart from complaining about missing azure, google, or s3 options when we choose to use volumeSnapshot as a provider
  • Add support for recovery from a VolumeSnapshot

This is achieved by establishing 'volumeSnapshot' as a backups.provider.

There are no breaking changes.

Fixes #334

This fixes issues with using volumeSnapshots where the Cluster object didn't have an appropriate [VolumeSnapshotConfiguration](https://cloudnative-pg.io/documentation/1.23/cloudnative-pg.v1/#postgresql-cnpg-io-v1-VolumeSnapshotConfiguration) block. It also prevents the chart complaining that we haven't set azure/s3/google specific variables when we have chosen to use VolumeSnapshots.

Signed-off-by: Cam Smith <[email protected]>
@itay-grudev
Copy link
Collaborator

This looks pretty good!

Would you have the time to add the Recovery from VolumeSnapshots functionality as well?

It wouldn't want to add this as a backup method without adding a recovery path, as that could potentially make it more difficult to recover from.

@cam-at-tactiq
Copy link
Author

cam-at-tactiq commented Jul 26, 2024

Thanks @itay-grudev! I've added support for recovery from snapshots and tested this in a GKE environment.

The recovery config docs also mention being able to use a PVC as a recovery mechanism with the same block on the cluster, but that failed when I tested it, otherwise I would have added that too. It might be a nice to have when that capability stabilises in the operator.

Please let me know if there's anything else that could make this better.

@cam-at-tactiq
Copy link
Author

Hi @itay-grudev please let me know what blockers if any there are to merging, we are really keen to start using this when it is released :)

@itay-grudev itay-grudev added the chart( cluster ) Related to the cluster chart label Aug 9, 2024
@dragoangel
Copy link
Contributor

dragoangel commented Aug 26, 2024

I not think force user to decide between s3 and snapshot is good idea actually. Provider allow to utilize both S3 and snapshots, why we in Helm should cut off this capabilities, I see this option as: #359

@cam-at-tactiq
Copy link
Author

I not think force user to decide between s3 and snapshot is good idea actually. Provider allow to utilize both S3 and snapshots, why we in Helm should cut off this capabilities, I see this option as: #359

Hi @dragoangel, I like your PR a lot. Whatever the maintainers prefer I am happy with, I just really want some volume snapshot capability. I've posted a review on your PR with two suggested changes. @itay-grudev please feel free to close this PR if you are going to merge #359 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart( cluster ) Related to the cluster chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VolumeSnapshot doesn't work
3 participants