-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add design for Extending VolumePolicies to support more actions #6956
Add design for Extending VolumePolicies to support more actions #6956
Conversation
f95c587
to
645456a
Compare
645456a
to
34c7938
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6956 +/- ##
==========================================
+ Coverage 61.76% 61.88% +0.12%
==========================================
Files 262 266 +4
Lines 28440 29456 +1016
==========================================
+ Hits 17567 18230 +663
- Misses 9642 9934 +292
- Partials 1231 1292 +61 ☔ View full report in Codecov by Sentry. |
34c7938
to
a11ff09
Compare
It sounds like we need to completely step back. @shubham-pampattiwar has been designing for one set of requirements, but the other reviews seem to have a completely different set of requirements in mind. So I think we have to step away from design for a bit and go back to requirements. I think we have a couple of options here:
To expand on the validation needs -- backup.Spec.DefaultVolumesToFsBackup is a bool pointer. If this is set to true or false, then we're using the legacy opt-in/opt-out approach, and any volume policies referencing fs backup or snapshots should result in a validation error -- if this is true or false, then existing workflows will be used. If this is nil (unspecified), then volume policies are used to determine what to snapshot and what to use fsbackup for. We'll need to modify install/server to make this value a bool pointer instead of a plain bool -- currently, not specifying this on install/server results in the backup getting a false rather than a nil value here. @shubham-pampattiwar what do you think? |
a11ff09
to
850ebf4
Compare
@reasonerjt @qiuming-best @sseago I have re-done the design, please take another look, Thanks ! |
Some notes from latest community call: My personal understanding from the call last night is that:
@shubham-pampattiwar I'm off base I can edit or delete this comment. |
850ebf4
to
77ae690
Compare
545f52c
to
4a43020
Compare
@reasonerjt Updated design implementation to use a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now. We might want to be explicit that none of this new design replaces the snaphotVolumes
param, that functions exactly as before. If it is true
, then any volume identified as a snapshot volume (either via new policies or legacy opt-in/opt-out) will be snapshotted via either CSI or native VolumeSnapshotter. If it is false
, then volumes identified as snapshot volumes will not be snapshotted. In other words, snapshotVolumes
is a param that simply says "Yes I want to include snapshots in this backup for the volumes where it's appropriate", or "No, I don't want to include any snapshots in this backup".
@shubham-pampattiwar |
4a43020
to
48bfa56
Compare
@Lyndon-Li Done, updated the design and resolved the comments. |
ShouldPerformSnapshot(obj runtime.Unstructured) (bool, error) | ||
} | ||
``` | ||
- The `VolumePolicyHelper` struct will implement the `VolumeHelper` interface and will consist of the functions that we will use through the backup workflow to accomodate volume policies for PVs and PVCs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham-pampattiwar
I was thinking the VolumeHelper
should be the source of truth for the backup, the return value should combine the value of volume-policy, fields in the backup, and the opt-in/opt-out annotations on the pod.
In the case of fs-backup, the volumeHelper should be a replacement of pdvolumeutil.GetVolumesByPod
and tell the backupper what volumes should be handled via fs-backup.
Therefore, IMO the VolumePolicyHelper
is a misuse b/c it only covers "volumePolicy".
Maybe we can clarify the volumeHelper's scope to "Help velero decide what to do with a volume during backup once the resource is selected by include/exclude filter and label selectors"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, Updated the PR.
- The implementation should be included in velero 1.14 | ||
|
||
- We will introduce a `VolumeHelper` interface. It will consist of two methods: | ||
- `ShouldPerformFSBackupForPodVolume(pod *corev1api.Pod)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have ProcessVolumePolicyFSBackup
(btw I think it should rename to GetVolumesForFSBackup
)
This func is probably not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
- Making sure that `snapshot` action is skipped for PVCs that do not fit the volume policy criteria, for this we will again use the `vph.ShouldPerformSnapshot` from the `VolumePolicyHelper(vph)` receiver. | ||
- We will pass the `VolumePolicyHelper(vph)` instance in `executeActions` method so that its available to use. | ||
```go | ||
func (v *VolumePolicyHelper) ShouldPerformSnapshot(obj runtime.Unstructured) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should also take other flags, like "snapshotVolumes" in the backup CR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, updated the PR
Thanks for working on this (and Velero). I greatly appreciate the backups Velero is managing for me. I came here via the issue referenced above. For my use cases the fs-backup feature is fine for many use cases (and I appreciate that it's explicit and the configuration is close to the workload configuration). I use snapshot backups for some workloads where I need a "as if the computer crashed" consistency of the file system and I use data mover to move the data off the infrastructure the cluster is running in. This PR doesn't seem to support that use case. The feature I'm missing currently (and I can't figure out from the resource filtering documentation is doing volume snapshots (and data mover) on some PVCs in a namespace instead of all. For example I have a namespace running clickhouse databases and it's a mix of 1TB data that gets replaced within a day and about 100GB data that changes much less (and is more valuable). I'd like to do snapshots and data mover on the 100GB data without also having to copy the 1TB other data. (I can't easily divide them between namespaces as clickhouse or the CH operator has features that helps me generate and update the small data set from the large data set within the cluster). |
@abh @shubham-pampattiwar Please correct me if my understanding is wrong. |
@asaf-erlich If your goal is to back up with a mixture of fs-backup and datamover, that is absolutely supported by this design. Policies will identify volumes for fs-backup and snapshot, and setting |
48bfa56
to
69a9a17
Compare
@reasonerjt PTAL, Thanks ! |
For me the important part is to be able to choose which volumes are being backed up. How I understand the current settings I can only choose by namespace when I want snapshot+data-mover. In my use case I want all the snapshots to be with data mover (and I don't think there are any scenarios where fs-backup is better if snapshot+data mover is possible?). For my use cases having the label on the pod (or on the PVC) is useful in many scenarios because it's so explicit. If the configuration is more complicated I'd worry I mistype a glob match or a label selector and accidentally don't have backups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks
Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> add changelog Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> fix codespell Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> update codeblocks for language syntax rendering Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> redo design Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> update volume policies design Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> add notes and modify design based on community feedback Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> add future scope add bia csi snapshot action details Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> add volumehelper package in implementation section Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> fix codespell Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> introduce volumehelper interface and volumepolicyhelper struct address feedback regarding volumehelper interface and its funcs Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> fix codespell Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
69a9a17
to
bec3425
Compare
@reasonerjt Thanks for the review, need a re-ack, fixed CI errors related to codespell. |
…re-tanzu#6956) add changelog fix codespell update codeblocks for language syntax rendering redo design update volume policies design add notes and modify design based on community feedback add future scope add bia csi snapshot action details add volumehelper package in implementation section fix codespell introduce volumehelper interface and volumepolicyhelper struct address feedback regarding volumehelper interface and its funcs fix codespell Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
This PR adds design for #6640
Please indicate you've done the following:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
.