ItemSnapshotter Plugin APIs #4077

dsu-igeek · 2021-08-30T20:00:45Z

ItemSnapshotter plugin APIs.
Added ItemSnapshotter.proto
Added item_snapshotter Go interface
Added framework components for item_snapshotter
Updated plugins doc with ItemSnapshotter info
Addresses #3754

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file or added /kind changelog-not-required.
Updated the corresponding documentation in site/content/docs/main.

site/content/docs/main/custom-plugins.md

reasonerjt · 2021-11-05T15:52:07Z

pkg/plugin/clientmgmt/restartable_item_snapshotter.go

+	return r
+}
+
+// getBackupItemAction returns the backup item action for this restartableBackupItemAction. It does *not* restart the


wrong comment

reasonerjt · 2021-11-05T15:53:19Z

pkg/plugin/clientmgmt/restartable_item_snapshotter.go

+	return itemSnapshotter, nil
+}
+
+// getDelegate restarts the plugin process (if needed) and returns the backup item action for this restartableBackupItemAction.


wrong comment

pkg/plugin/framework/item_snapshotter_client.go

reasonerjt · 2021-11-05T16:24:56Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+	ItemID velero.ResourceIdentifier
+	// SnapshotID is the snapshot ID returned by ItemSnapshotter
+	SnapshotID string
+	// Backup is the representation of the restore resource processed by Velero.


restore resource -> backup resource

reasonerjt · 2021-11-05T16:33:11Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+type SnapshotItemOutput struct {
+	UpdatedItem      runtime.Unstructured
+	SnapshotID       string
+	SnapshotMetadata map[string]string


could you give a concrete example of SnapshotMetadata? This is not mentioned in the design and have you considered adding a GetSnapshot method to return the detail information of the snapshot?

reasonerjt · 2021-11-05T16:33:51Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+	// which will be handled by this plugin when snapshotting the item.  For example, a database may expose
+	// a resource that can be snapshotted. If the database uses a PVC that will be snapshotted/backed up as
+	// part of the database snapshot, that PVC should be returned when AlsoHandles is invoked
+	AlsoHandles(input *AlsoHandlesInput) ([]velero.ResourceIdentifier, error)


This returns the same result as Additionalitems in SnapshotItemOutput?

I updated the comment to explain:

// AlsoHandles is called for each item this ItemSnapshotter should handle and returns any items // which will be handled by this plugin when snapshotting the item. These items will be excluded from the // items being backed up. AlsoHandles will be called before SnapshotItem is called. For example, a database may expose // a database resource that can be snapshotted. If the database uses a PVC that will be snapshotted/backed up as // part of the database snapshot, that PVC should be returned when AlsoHandles is invoked. This is different from // AdditionalItems (returned in SnapshotItemOutput and CreateItemOutput) which are specifying additional resources // that Velero should store in the backup or create.

I think we should be explicit in the comment here to explain if AlsoHandles returns the direct children PEs only, or does it recursively traverse the subtree to return all leaf items.

It will return everything that should be excluded from the caller's list of things to do. If Astrolabe is backing the ItemSnapshotter, the expectation is that it returns all of the PEs that will be snapshotted so they can be removed from the list. ItemSnapshotter is not limited to Astrolabe, however, and is not a Protected Entity interface here.

So all children are handled in one call of SnapshotItem, right? The resources returned by AlsoHandles are only used to tell the caller to just ignore them

Yes, that's the plan

reasonerjt · 2021-11-05T16:37:42Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+type CreateItemInput struct {
+	SnapshottedItem  runtime.Unstructured
+	SnapshotID       string
+	ItemFromBackup   runtime.Unstructured


Could you add some comment to explain ItemFromBackup and SnapshotMetadata

reasonerjt · 2021-11-05T16:44:50Z

pkg/plugin/framework/item_snapshotter_client.go

+	if err != nil {
+		return nil, errors.WithStack(err)
+	}
+	itemFromBackupJSOB, err := json.Marshal(input.ItemFromBackup.UnstructuredContent())


itemFromBackupJSOB -> itemFromBackupJSON

reasonerjt · 2021-11-08T03:47:14Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+// ProgressInput contains the input parameters for the ItemSnapshotter's Progress function.
+type ProgressInput struct {
+	// ItemID is the id of item that was stored in the backup
+	ItemID velero.ResourceIdentifier


I notice here's a little inconsistency, in SnapshotItemInput the attribute is Item but in ProgressInput the attribute is ItemID

When we snapshot we pass in the whole resource in case anything in it needs to be used. On progress we only need the ID and we may only have the ID at that point. If the Velero server is restarted before the progress is finished, we will lose the in-memory copy. The resource may be modified or deleted after the backup finishes so in order to pass in the resource here we would need to extract it from the backup. There's no need for anything other than the ID for checking progress so this saves a lot of trouble trying to recover the whole resource.

reasonerjt · 2021-11-08T04:01:06Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+
+type DeleteSnapshotInput struct {
+	SnapshotID       string
+	ItemFromBackup   runtime.Unstructured


I don't quite understand why ItemFromBackup is needed in the DeleteSnapshotInput, is it for cascade deletion?

We take it in the existing DeleteItemAction, it is probably useful and we have it available.

// DeleteItemActionExecuteInput contains the input parameters for the ItemAction's Execute function. type DeleteItemActionExecuteInput struct { // Item is the item taken from the pristine backed up version of resource. Item runtime.Unstructured // Backup is the representation of the restore resource processed by Velero. Backup *velerov1api.Backup }

reasonerjt · 2021-11-08T04:02:09Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+	UpdatedItem      runtime.Unstructured
+	SnapshotID       string
+	SnapshotMetadata map[string]string			// SnapshotMetadata is information in addition to the SnapshotID that the
+												// plugin wants to store in the backup.  SnapshotMetadata will be passed


The line of comments are not aligned, could you re-format?

reasonerjt · 2021-11-08T04:02:29Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+
+type CreateItemInput struct {
+	SnapshottedItem  runtime.Unstructured	// The snapshotted item at this stage of the restore (RestoreItemActions may
+											// have modified the item prior to CreateItemFromSnapshot being called)


The lines of comments are not aligned, could you re-format?

reasonerjt · 2021-11-08T04:08:59Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+	SnapshottedItem  runtime.Unstructured	// The snapshotted item at this stage of the restore (RestoreItemActions may
+											// have modified the item prior to CreateItemFromSnapshot being called)
+	SnapshotID       string
+	ItemFromBackup   runtime.Unstructured	// The snapshotted item that was stored in the backup


Is this the "original version" of the SnapshottedItem?

IMO, if an item is modified by RestoreItemActions, the purpose is to let the subsequent plugins handle the modified version, otherwise, it should not be modified.

Is it for some corner cases that the two versions of an item is needed for ItemSnapshotterplugin to create the item from snapshot?

It's useful to be able to see what it looked like before it got backed up. This is the same construct currently used in RestoreItemAction

// RestoreItemActionExecuteInput contains the input parameters for the ItemAction's Execute function. type RestoreItemActionExecuteInput struct { // Item is the item being restored. It is likely different from the pristine backed up version // (metadata reset, changed by various restore item action plugins, etc.). Item runtime.Unstructured // ItemFromBackup is the item taken from the pristine backed up version of resource. ItemFromBackup runtime.Unstructured // Restore is the representation of the restore resource processed by Velero. Restore *api.Restore }

reasonerjt · 2021-11-08T04:27:45Z

Per discussion with Dave, some of the implementation of the input/output parameters to the API such as

type DeleteSnapshotInput struct {
	SnapshotID       string
	ItemFromBackup   runtime.Unstructured
	SnapshotMetadata map[string]string		// SnapshotMetadata returned from SnapshotItem
	Params           map[string]string
}

maybe modified in the near future before it's established to other APP developers.
My questions in the attributes of the structs can be ignored.

jglick · 2021-11-08T15:30:15Z

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

+	// that Velero should store in the backup or create.
+	AlsoHandles(input *AlsoHandlesInput) ([]velero.ResourceIdentifier, error)
+
+	// SnapshotItem cause the ItemSnapshotter to snapshot the specified item.  Ite may also


Suggested change

// SnapshotItem cause the ItemSnapshotter to snapshot the specified item. Ite may also

// SnapshotItem cause the ItemSnapshotter to snapshot the specified item. It may also

zubron

This looks good - I just have a few comments, mostly around the values set in the proto request/response objects. I don't know if they are intentionally not set or whether this is something that will be addressed in a follow up change.

pkg/plugin/clientmgmt/restartable_item_snapshotter.go

pkg/plugin/clientmgmt/restartable_item_snapshotter_test.go

pkg/plugin/framework/util.go

pkg/plugin/clientmgmt/restartable_item_snapshotter.go

pkg/plugin/velero/item_snapshotter/v1/item_snapshotter.go

pkg/plugin/framework/item_snapshotter_server.go

pkg/plugin/framework/item_snapshotter_client.go

pkg/plugin/framework/item_snapshotter_server.go

zubron · 2021-11-09T15:58:18Z

This looks good - I just have a few comments, mostly around the values set in the proto request/response objects. I don't know if they are intentionally not set or whether this is something that will be addressed in a follow up change.

Ah, I just read @reasonerjt's comment above and I'm assuming these are the parameters he's referring to. @dsu-igeek If that is the case, feel free to ignore/resolve my comments below about setting the fields in the requests and responses and I'll approve once the relevant copyright headers are added 👍

Added ItemSnapshotter.proto Added item_snapshotter Go interface Added framework components for item_snapshotter Updated plugins doc with ItemSnapshotter info Added SnapshotPhase to item_snapshotter.go ProgressOutputOutput now includes a phase as well as an error string for problems that occured Signed-off-by: Dave Smith-Uchida <dsmithuchida@vmware.com>

reasonerjt

I still have some different thoughts on the details of the parameters to the functions of the interface.
Given we have the plan to tweak the parameters in near future before the API is published, I'm approving this PR to unblock the progress.

zubron

LGTM, thanks!

dsu-igeek · 2021-11-17T01:10:51Z

Fixes #3754

…u#4077) Added ItemSnapshotter.proto Added item_snapshotter Go interface Added framework components for item_snapshotter Updated plugins doc with ItemSnapshotter info Added SnapshotPhase to item_snapshotter.go ProgressOutputOutput now includes a phase as well as an error string for problems that occured Signed-off-by: Dave Smith-Uchida <dsmithuchida@vmware.com>

dsu-igeek force-pushed the dsu-item-snapshotter-08-21-2021 branch from 8e3b6d6 to 0db329e Compare August 30, 2021 20:12

github-actions bot added the has-changelog label Aug 30, 2021

dsu-igeek force-pushed the dsu-item-snapshotter-08-21-2021 branch from 0db329e to 20c9c41 Compare August 30, 2021 20:15

github-actions bot added the Documentation label Aug 30, 2021

dsu-igeek changed the title ~~parent 74476db9d791fa91bba0147eac8ec189820adb3d~~ ItemSnapshotter Plugin APIs Aug 30, 2021

dsu-igeek force-pushed the dsu-item-snapshotter-08-21-2021 branch from 20c9c41 to 078aadd Compare September 4, 2021 01:03

github-actions bot added the has-unit-tests label Sep 4, 2021

dsu-igeek force-pushed the dsu-item-snapshotter-08-21-2021 branch from 078aadd to baf5053 Compare September 4, 2021 01:12

dsu-igeek added this to the v1.8.0 milestone Sep 8, 2021

dsu-igeek force-pushed the dsu-item-snapshotter-08-21-2021 branch 2 times, most recently from f7b2d6d to 95b4fdf Compare September 13, 2021 18:48

dsu-igeek marked this pull request as ready for review September 28, 2021 05:58

github-actions bot assigned dsu-igeek Sep 28, 2021

github-actions bot requested review from reasonerjt and zubron September 28, 2021 05:58

dsu-igeek requested review from jenting, sseago and dims and removed request for dims September 28, 2021 05:59

dsu-igeek added this to To Do in v1.8.0 Oct 20, 2021

dsu-igeek moved this from To Do to In Progress in v1.8.0 Oct 20, 2021

jglick reviewed Nov 2, 2021

View reviewed changes

site/content/docs/main/custom-plugins.md Show resolved Hide resolved

reasonerjt reviewed Nov 5, 2021

View reviewed changes

pkg/plugin/framework/item_snapshotter_client.go Outdated Show resolved Hide resolved

reasonerjt reviewed Nov 5, 2021

View reviewed changes

reasonerjt reviewed Nov 8, 2021

View reviewed changes

dsu-igeek force-pushed the dsu-item-snapshotter-08-21-2021 branch from 01b8c63 to 7cbab01 Compare November 8, 2021 07:31

jglick reviewed Nov 8, 2021

View reviewed changes

zubron suggested changes Nov 9, 2021

View reviewed changes

jglick mentioned this pull request Nov 10, 2021

[Epic] Backup Replication #103

Open

1 task

dsu-igeek force-pushed the dsu-item-snapshotter-08-21-2021 branch 3 times, most recently from ece6665 to fe5a799 Compare November 11, 2021 04:26

dsu-igeek force-pushed the dsu-item-snapshotter-08-21-2021 branch from fe5a799 to 9a6e0f2 Compare November 11, 2021 21:39

reasonerjt approved these changes Nov 15, 2021

View reviewed changes

zubron mentioned this pull request Nov 16, 2021

Include all fields in marshalling of SnapshotItemRequest and DeleteItemSnapshotRequest in item_snapshotter_client.go #4356

Closed

zubron approved these changes Nov 16, 2021

View reviewed changes

zubron merged commit 5150ce4 into vmware-tanzu:main Nov 16, 2021

dsu-igeek mentioned this pull request Nov 17, 2021

[Upload Progress Monitoring Epic #3533] Add ItemSnapshotter plug-in to handle Upload Progress monitoring #3754

Closed

eleanor-millman modified the milestones: Prioritized issues, 1.9.0 Feb 22, 2022

eleanor-millman moved this from In Progress to Done in v1.8.0 Feb 22, 2022

reasonerjt mentioned this pull request Jul 13, 2022

Item action progress monitoring design #5108

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ItemSnapshotter Plugin APIs #4077

ItemSnapshotter Plugin APIs #4077

dsu-igeek commented Aug 30, 2021 •

edited

reasonerjt Nov 5, 2021

dsu-igeek Nov 7, 2021

reasonerjt Nov 5, 2021

dsu-igeek Nov 7, 2021

reasonerjt Nov 5, 2021

dsu-igeek Nov 7, 2021

reasonerjt Nov 5, 2021

reasonerjt Nov 5, 2021

dsu-igeek Nov 7, 2021

reasonerjt Nov 8, 2021

dsu-igeek Nov 8, 2021

ywk253100 Nov 8, 2021

dsu-igeek Nov 10, 2021

reasonerjt Nov 5, 2021

dsu-igeek Nov 7, 2021

reasonerjt Nov 5, 2021

dsu-igeek Nov 7, 2021

reasonerjt Nov 8, 2021

dsu-igeek Nov 8, 2021

reasonerjt Nov 8, 2021

dsu-igeek Nov 8, 2021

reasonerjt Nov 8, 2021

dsu-igeek Nov 8, 2021

reasonerjt Nov 8, 2021

reasonerjt Nov 8, 2021

dsu-igeek Nov 8, 2021

reasonerjt commented Nov 8, 2021

jglick Nov 8, 2021

dsu-igeek Nov 10, 2021

zubron left a comment

zubron commented Nov 9, 2021 •

edited

reasonerjt left a comment

zubron left a comment

dsu-igeek commented Nov 17, 2021 •

edited

	// SnapshotItem cause the ItemSnapshotter to snapshot the specified item. Ite may also
	// SnapshotItem cause the ItemSnapshotter to snapshot the specified item. It may also

ItemSnapshotter Plugin APIs #4077

ItemSnapshotter Plugin APIs #4077

Conversation

dsu-igeek commented Aug 30, 2021 • edited

Please indicate you've done the following:

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reasonerjt commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zubron left a comment

Choose a reason for hiding this comment

zubron commented Nov 9, 2021 • edited

reasonerjt left a comment

Choose a reason for hiding this comment

zubron left a comment

Choose a reason for hiding this comment

dsu-igeek commented Nov 17, 2021 • edited

dsu-igeek commented Aug 30, 2021 •

edited

zubron commented Nov 9, 2021 •

edited

dsu-igeek commented Nov 17, 2021 •

edited