design: progress on backup/restore operations by volume snapshotters #2543

vishnuitta · 2020-05-18T12:53:04Z

This PR contains design proposal using which volume snapshotters can let know its users about the progress on backup/restore operations.

Signed-off-by: Vitta vitta@mayadata.io

Signed-off-by: Vitta <vitta@mayadata.io>

vishnuitta · 2020-05-22T21:18:35Z

just want to make sure that nothing is pending on me to make this PR go to next stage.. please let me know if thats not the case
sorry for asking it..

nrb

Thanks for this @vishnuitta! Definitely appreciate the though that's gone into it, and it's a feature we want in Velero.

On the whole, I like the proposal. I have some requests I've made on comments inline that I think would make the proposal clearer. @vmware-tanzu/velero-maintainers Please read through when you have a chance.

design/plugin-backup-and-restore-progress-design.md

nrb · 2020-06-01T20:11:19Z

design/plugin-backup-and-restore-progress-design.md

+Though no restrictions are required on the name of CR, as a general practice, volume snapshotter can name this CR with the value same as return value of CreateSnapshot.
+
+After return from `CreateSnapshot` in `takePVSnapshot`, if VolumePluginBackup CR exists for particular backup of the volume, velero adds this CR to `backupRequest`.
+During persistBackup call, this CR also will be backed up to backup location.


So these would need to be written in a json.gz file, similar to how the snapshots are now?

It may be helpful to create a section in the design for "core" Velero server/client changes. I think the only things that need to be changed are as follows, based on what I've read in this document.

Creation of the VolumePluginBackup/VolumePluginRestore CRDs at installation time

Persistence of VolumePluginBackup CRs towards the end of the back up operation

Sync of VolumePluginBackup/VolumePluginRestore CRDs at the same time as backup synchronization.

Otherwise, volumesnapshotter plugins themselves are responsible for interacting with the VolumePlugin(Backup|Restore) CRs, and Velero's core won't change them.

Then, to see this reported, the velero describe commands would need to be aware of the VolumePlugin(Backup|Restore) CRs associated with a Backup CR in order to display the progress fields.

sure. I will add a section @nrb. And, the only changes that are required in "core" are those that you mentioned.
Regarding 'velero describe' support for these CRs, if I'm not wrong, 'velero describe' is not aware of PodVolume(Backup|Restore) CRs as well. To keep this PR simple, I thought - we can take it for these new CRs also when PodVolume(Backup|Restore) CRs support is added to 'velero describe'.

Currently, no, it would not be aware of these new CRDs, but we could make it support them.

Within the backup describer, we have a function for describing PodVolumeBackups, as well as a function for describing VolumeSnapshotContents.

I would think describing these new CRs would work roughly the same way, where they would get queried from the Kube API and then passed to the top level Describe function.

I don't know that all this detail needs to go into the design document, but this is kind of what I envisioned the scope of work to be.

As mentioned on Slack, I think the progress tracking is information that's really only necessary when scoped to an in-progress back up operation. When a Backup is in a Completed or Failed state, we know it's at 100% completion.

Given that, does it make sense to synchronize these CRs across clusters?

Synchronizing VolumePluginBackup CR to another cluster will help user to know the backup size, number of volumes in it even before user triggers restore of the backup.

nrb · 2020-06-01T20:12:56Z

design/plugin-backup-and-restore-progress-design.md

+	// Plugin name
+	Plugin string `json:"plugin"`
+
+	// PluginSpecific are a map of key-value pairs that plugin want to provide


Can you give an example of what values might go in here, and how it might differ from ConfigMaps that we use?

I hadn't thought any specific, but, added a way so that plugins can provide more info specific to this restore operation to users.
Configmaps in plugin is related to config at plugin level. This field is specific to details regarding this restore operation.

One thing which I had in my mind when added this was specific to OpenEBS plugin, but, also thought that every plugin may have their specific things to convey to user about that particular backup/restore operation.
OpenEBS plugin supports incremental snapshots also. So, an entry can be added to this map which provides name of its parent backup.

nrb · 2020-06-01T20:19:29Z

design/plugin-backup-and-restore-progress-design.md

+
+## Security Considerations
+
+N/A


Are there any sort of RBAC considerations keeping plugins from modifying VolumePlugin(Backup|Restore) CRs that were created by another plugin?

Very good question @nrb. I hadn't thought of this. Let me also think of this.
Do you have any suggestions? and any such considerations already handled in Velero?

Currently everything runs under the same velero service account so all plugins have broad access, which would include being able to modify CRs created by another plugin.

design/plugin-backup-and-restore-progress-design.md

nrb · 2020-06-02T17:01:48Z

@dsu-igeek I would appreciate your eyes on this, even a quick glance. Would this proposal work for the vSphere plugin?

ashish-amarnath

I added a few comments to explore potential re-use of the existing pod_volume_backup type.

I would also like to understand how we might use something like this in the CSI snapshot APIs.

nrb · 2020-06-02T19:42:06Z

I would also like to understand how we might use something like this in the CSI snapshot APIs.

I'm unclear how we might be able to get information from the CSI snapshot APIs. I'd have to look at how they work again, as right now I think the CSI drivers are more or less a black box to Velero, and we don't have a way of asking for progress. I don't believe there's more than one field for bytes written, just the final total for restoreSize.

- backward compatibility - label value fix - typo fix Signed-off-by: Vitta <vitta@mayadata.io>

skriss · 2020-06-03T16:45:18Z

Couple helpful links from today's call:

internal Snapshot type -- https://github.com/vmware-tanzu/velero/blob/master/pkg/volume/snapshot.go
creation of Snapshots -- https://github.com/vmware-tanzu/velero/blob/master/pkg/backup/item_backupper.go#L477-L489
serialization of Snapshots to JSON for uploading to object storage as a file -- https://github.com/vmware-tanzu/velero/blob/master/pkg/controller/backup_controller.go#L667-L671
fetching of Snapshots from object storage on restore -- https://github.com/vmware-tanzu/velero/blob/master/pkg/controller/restore_controller.go#L450-L453
using the Snapshots for restore: https://github.com/vmware-tanzu/velero/blob/master/pkg/restore/restore.go#L195 and https://github.com/vmware-tanzu/velero/blob/master/pkg/restore/pv_restorer.go#L76-L104

Signed-off-by: Vitta <vitta@mayadata.io>

nrb

A few more questions from me around the move to the snapshot struct.

design/plugin-backup-and-restore-progress-design.md

….gz file Signed-off-by: Vitta <vitta@mayadata.io>

design/plugin-backup-and-restore-progress-design.md

skriss

Added a few more comments; I think the general flow and the changes to velero core make sense, we just need to finalize the exact API design.

design/plugin-backup-and-restore-progress-design.md

skriss · 2020-06-09T14:50:02Z

design/plugin-backup-and-restore-progress-design.md

+	// PluginSpecific are a map of key-value pairs that plugin want to provide
+	// to user to identify plugin properties related to this backup
+	// +optional
+	PluginSpecific map[string]string `json:"pluginSpecific,omitempty"`


Let's try to come up with a different name here...I'll think on it.

as this is supposed to be properties specific to Backup/restore operation, thinking of changing to
BackupProperties and RestoreProperties..
does that make sense

Would the properties be the same on backup and restore? Are they copied from the backup CR to the restore CR?

Need not be @nrb .. Backup related and Restore related ones are different.
As mentioned in one of the comments, one Backup property can be its parent snapshot name, in case of OpenEBS as it supports incremental backups.

BackupProperties or BackupInfo would work.

skriss · 2020-06-09T15:11:35Z

design/plugin-backup-and-restore-progress-design.md

+
+### Progress of backup operation handled by volume snapshotter
+
+Progress will be updated by volume snapshotter in VolumePluginBackup CR which is specific to that backup operation.


I'm just going back and forth on the API design a bit. I think either option is workable for now for this specific use case, just trying to think through other uses/future extensions and whether that should push us in one direction or the other.

Approach 1 combines the volume snapshot & volume backup info into a single CRD, and then has a parallel CRD on the restore side

Approach 2 models the backup separately from the snapshot (the snapshot remains as an internal struct, not a CRD), but there's a lot of duplication of info between the snapshot & backup structs. On the restore side, there's one struct that models both the restore from durable storage, and the snapshot restore.

we could consider fully breaking things out, so 4 CRDs: on the backup side, one for the snapshot, one for the backup to durable storage; on the restore side, one for the restore from durable storage, and one for the snapshot restore

I think I'm still leaning towards approach 1, as it solves the need and is simplest overall, but I'd like other maintainers' inputs on this (including any other options you see).

I think I'm leaning towards approach 1, too, with the caveat that it needs to be optional. We can't require it if not all plugins are updating these CRs.

thats true @nrb .. With approach 1, a CR will be created for every backup/restore operation even though plugin doesn't update it.

design/plugin-backup-and-restore-progress-design.md

skriss · 2020-06-09T15:17:27Z

design/plugin-backup-and-restore-progress-design.md

+
+## Security Considerations
+
+N/A


Currently everything runs under the same velero service account so all plugins have broad access, which would include being able to modify CRs created by another plugin.

Signed-off-by: Vitta <vitta@mayadata.io>

nrb · 2020-06-23T15:49:52Z

design/plugin-backup-and-restore-progress-design.md

+
+At a high level, in this approach, this struct will be converted to a CR by adding new fields (related to Progress tracking) to it, and gets rid of `volume.Snapshot` struct.
+
+Instead of backing up of Go struct, CRs will be backed up to backup location, and it gets synced into other cluster by backupSyncController running in that cluster.


Line 47 contradicts line 41. Is the plan to sync the CR on a regular basis, or at restore time?

First para i.e., lines 41, 42 and 43 are related to current behavior. I will update it.
At line 47 is the proposed solution. So, plan is to sync the CR on regular basis.

updated the text

nrb

I think this is fairly close, but I need to read the backwards compatibility section more closely to be sure I understand.

@ashish-amarnath and @carlisia, please read through this proposal as well

Signed-off-by: Vitta <vitta@mayadata.io>

carlisia · 2020-07-01T00:17:47Z

Will review tomorrow.

vishnuitta · 2020-07-06T03:14:21Z

@carlisia or @nrb or @ashish-amarnath .. would you guys prefer a hangout to close the things on this thread?

stephbman · 2020-07-06T20:42:12Z

@vishnuitta we could do a hangout or zoom meeting to review this - @carlisia since you are going to take a look at reviewing this would it be better for you to provide comments here or to have a short call w/ Vishnu on this?

nrb

My apologies for my delay. I don't believe I need a call to sync on this, but can if others believe its helpful.

I'm in favor of approach 1, and if others are as well, then I'd simply ask approach 2 be marked as an alternative considered.

Also, this isn't necessarily for the design phase, but upon implementation, please use Kubebuilder to implement the new CRD.

carlisia

I think this is in really good shape and it's good to go! 🎉

I agree that approach 1 is better, and I also like the continuous sync'ing to the storage location.

carlisia · 2020-07-07T14:20:17Z

design/plugin-backup-and-restore-progress-design.md

+	// PluginSpecific are a map of key-value pairs that plugin want to provide
+	// to user to identify plugin properties related to this backup
+	// +optional
+	PluginSpecific map[string]string `json:"pluginSpecific,omitempty"`


BackupProperties or BackupInfo would work.

carlisia · 2020-07-07T14:20:38Z

design/plugin-backup-and-restore-progress-design.md

+	ProviderSnapshotID string `json:"providerSnapshotID,omitempty"`
+
+	// Phase is the current state of the VolumeSnapshot.
+	Phase SnapshotPhase `json:"phase,omitempty"`


What will the phases be for backup/restore?

carlisia · 2020-07-07T14:28:34Z

@vishnuitta if you use the scaffold feature of kubebuilder to generate the new CRDs with the Velero project it won't work, because we structure things differently than kubebuilder expects (no main.go for example). The way we do it is to have a dummy project and copy/paste things. Ping me on #velero if I can help you with this.

carlisia · 2020-07-07T14:29:41Z

I'm in favor of approach 1, and if others are as well, then I'd simply ask approach 2 be marked as an alternative considered

@vishnuitta please move approach 2 to the "alternatives" section.

ashish-amarnath

Thank you for your patience on this proposal @vishnuitta
Apart from moving the creation of the CRs into the plugins themselves, the other comments are minor one.

ashish-amarnath · 2020-07-07T15:45:44Z

design/plugin-backup-and-restore-progress-design.md

+
+### Approach 1
+
+Existing `Snapshot` Go struct from `volume` package have most of the details related to backup operation performed by volumesnapshotters.


suggest making these as links to the code.

will update with code links.

this doesn't have to block the merge of this PR.

ashish-amarnath · 2020-07-07T15:47:33Z

design/plugin-backup-and-restore-progress-design.md

+This struct also gets backed up to backup location. But, this struct doesn't get synced on other clusters at regular intervals.
+It is currently synced only during restore operation, and velero CLI shows few of its contents.
+
+At a high level, in this approach, this struct will be converted to a CR by adding new fields (related to Progress tracking) to it, and gets rid of `volume.Snapshot` struct.


Did you mean CRD?
Can you please update what this CRD will look like?

PR have CR struct below (not exact CRD) for better readability.

Look for heading VolumePluginBackup CR

design/plugin-backup-and-restore-progress-design.md

ashish-amarnath

This should be good to move forward.
Thank you for your patience on this PR @vishnuitta. Really appreciated!

Stale

vishnuitta · 2020-07-08T17:39:16Z

thanks @carlisia for the merge and thanks to every one for the reviews.
Apologizes for delay in updating the last set of review comments. I will get them incorporated in the implementation related PRs.

vishnuitta added 3 commits May 18, 2020 01:50

design: progress on backup/restore operations by volume snapshotters

0f7cf18

Signed-off-by: Vitta <vitta@mayadata.io>

removed restriction on name, handled upgrades

bdc5014

Signed-off-by: Vitta <vitta@mayadata.io>

renamed file to include design in it

7803dfc

Signed-off-by: Vitta <vitta@mayadata.io>

skriss added the Area/Design Design Documents label May 19, 2020

carlisia requested review from skriss, nrb, carlisia and ashish-amarnath May 28, 2020 18:28

nrb suggested changes Jun 1, 2020

View reviewed changes

ashish-amarnath reviewed Jun 2, 2020

View reviewed changes

design/plugin-backup-and-restore-progress-design.md Show resolved Hide resolved

ashish-amarnath suggested changes Jun 2, 2020

View reviewed changes

- deletion of new created CRs

42a7948

- backward compatibility - label value fix - typo fix Signed-off-by: Vitta <vitta@mayadata.io>

vishnuitta added 4 commits June 4, 2020 18:10

new approach to convert existing Snapshot Go struct to CR

a77c4a4

Signed-off-by: Vitta <vitta@mayadata.io>

fixing formatting issues

421bb21

Signed-off-by: Vitta <vitta@mayadata.io>

few more formatting fixes

0fa8426

Signed-off-by: Vitta <vitta@mayadata.io>

added provider name to identify volumepluginrestore CR

b424824

Signed-off-by: Vitta <vitta@mayadata.io>

nrb reviewed Jun 4, 2020

View reviewed changes

design/plugin-backup-and-restore-progress-design.md Outdated Show resolved Hide resolved

design/plugin-backup-and-restore-progress-design.md Show resolved Hide resolved

design/plugin-backup-and-restore-progress-design.md Show resolved Hide resolved

corrected statement regarding creation of backup-volumesnapshots.json…

3ae0e44

….gz file Signed-off-by: Vitta <vitta@mayadata.io>

skriss reviewed Jun 8, 2020

View reviewed changes

design/plugin-backup-and-restore-progress-design.md Outdated Show resolved Hide resolved

skriss assigned vishnuitta Jun 9, 2020

skriss previously requested changes Jun 9, 2020

View reviewed changes

vishnuitta added 2 commits June 16, 2020 20:47

removing volume.Snapshot completely

41b8e49

Signed-off-by: Vitta <vitta@mayadata.io>

intentation fix

fd41791

Signed-off-by: Vitta <vitta@mayadata.io>

nrb added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Jun 22, 2020

nrb reviewed Jun 23, 2020

View reviewed changes

correcting few sentences

4cd518e

Signed-off-by: Vitta <vitta@mayadata.io>

nrb approved these changes Jul 6, 2020

View reviewed changes

carlisia approved these changes Jul 7, 2020

View reviewed changes

ashish-amarnath suggested changes Jul 7, 2020

View reviewed changes

ashish-amarnath approved these changes Jul 8, 2020

View reviewed changes

carlisia merged commit c3cac0a into vmware-tanzu:master Jul 8, 2020


		### Progress of backup operation handled by volume snapshotter

		Progress will be updated by volume snapshotter in VolumePluginBackup CR which is specific to that backup operation.


		At a high level, in this approach, this struct will be converted to a CR by adding new fields (related to Progress tracking) to it, and gets rid of `volume.Snapshot` struct.

		Instead of backing up of Go struct, CRs will be backed up to backup location, and it gets synced into other cluster by backupSyncController running in that cluster.


		### Approach 1

		Existing `Snapshot` Go struct from `volume` package have most of the details related to backup operation performed by volumesnapshotters.

design: progress on backup/restore operations by volume snapshotters #2543

design: progress on backup/restore operations by volume snapshotters #2543

Conversation

vishnuitta commented May 18, 2020

vishnuitta commented May 22, 2020

nrb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrb commented Jun 2, 2020

ashish-amarnath left a comment

Choose a reason for hiding this comment

nrb commented Jun 2, 2020

skriss commented Jun 3, 2020 • edited

nrb left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss Jun 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrb left a comment

Choose a reason for hiding this comment

carlisia commented Jul 1, 2020

vishnuitta commented Jul 6, 2020

stephbman commented Jul 6, 2020

nrb left a comment

Choose a reason for hiding this comment

carlisia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia commented Jul 7, 2020

carlisia commented Jul 7, 2020

ashish-amarnath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish-amarnath left a comment

Choose a reason for hiding this comment

vishnuitta commented Jul 8, 2020

skriss commented Jun 3, 2020 •

edited

skriss Jun 9, 2020 •

edited