BackupItemAction v2 API implementation #5442

sseago · 2022-10-12T18:52:11Z

Signed-off-by: Scott Seago [email protected]

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #5488

Please indicate you've done the following:

[x ] Accepted the DCO. Commits without the DCO will delay acceptance.
[x ] Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
Updated the corresponding documentation in site/content/docs/main.

codecov-commenter · 2022-10-12T19:26:14Z

Codecov Report

Merging #5442 (ff748cc) into main (c0430b8) will decrease coverage by 0.19%.
The diff coverage is 31.64%.

@@            Coverage Diff             @@
##             main    #5442      +/-   ##
==========================================
- Coverage   40.62%   40.43%   -0.20%     
==========================================
  Files         236      240       +4     
  Lines       20449    20826     +377     
==========================================
+ Hits         8308     8421     +113     
- Misses      11532    11790     +258     
- Partials      609      615       +6

Impacted Files	Coverage Δ
pkg/backup/request.go	`100.00% <ø> (ø)`
pkg/plugin/framework/action_resolver.go	`5.75% <0.00%> (-1.15%)`	⬇️
...ramework/backupitemaction/v2/backup_item_action.go	`0.00% <0.00%> (ø)`
...k/backupitemaction/v2/backup_item_action_client.go	`0.00% <0.00%> (ø)`
pkg/plugin/framework/common/plugin_kinds.go	`0.00% <0.00%> (ø)`
pkg/plugin/framework/server.go	`0.00% <0.00%> (ø)`
...k/backupitemaction/v2/backup_item_action_server.go	`33.06% <33.06%> (ø)`
...kupitemaction/v2/restartable_backup_item_action.go	`65.21% <65.21%> (ø)`
pkg/plugin/clientmgmt/manager.go	`78.92% <88.88%> (+1.52%)`	⬆️
pkg/backup/backup.go	`78.88% <100.00%> (ø)`
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kaovilai · 2022-12-13T15:59:35Z

This PR closes #5488

weshayutin · 2022-12-13T16:06:41Z

my very newb scan of this leaves me w/ just a thought and I don't have a strong opinion here.
Thank you for updating the unit tests and labeling the overriden api tests w/ V2. Would it help the reader of the unit tests results to name the item w/ a V2 as well even though there is NOT a V1? e.g. NewBackupItemActionPlugin -> NewBackupItemActionPluginV2

sseago · 2022-12-13T19:56:55Z

@weshayutin I'm not seeing any funcs named NewBackupItemActionPlugin in the test files. Where I see a function of this name in the other code, it's in a package that identifies it as v1 vs. v2. Was there some test in particular that you were referencing?

Lyndon-Li · 2023-01-03T02:28:42Z

pkg/backup/item_backupper.go

-		updatedItem, additionalItemIdentifiers, err := action.Execute(obj, ib.backupRequest.Backup)
+		// Note: we're ignoring the operationID returned from Execute for now, it will be used
+		// with the async plugin action implementation
+		updatedItem, additionalItemIdentifiers, _, err := action.Execute(obj, ib.backupRequest.Backup)


After the Execute method is called, the method may return in a short time but the async operation is still ongoing.
As to the backup workflow, I guess the caller needs to call the Progress method to wait the completion of the async operation.
For different items in the same backup, multiple Execute methods may be called successively before their async operations complete, so before the backup workflow ends, multiple async operations should be waited for completion.

Will we handle this logic in the current PR or we do it in later PRs?

@Lyndon-Li This PR is just the API definition. It adds the appropriate functions to the various BackupItemAction-related packages. Implmeenting the controller workflow changes to actually call Progress, wait, maintain a list of items waited for, etc. will be implemented in a separate PR (there's a separate github issue to track that work).

Lyndon-Li · 2023-01-03T02:29:04Z

pkg/plugin/clientmgmt/backupitemaction/v2/restartable_backup_item_action.go

+	biav2 "github.com/vmware-tanzu/velero/pkg/plugin/velero/backupitemaction/v2"
+)
+
+// AdaptedBackupItemAction is a backup item action adapted to the v1 BackupItemAction API


I'll reword it to make it clearer. "is a v1 BackupItemAction adapted to implement the v2 API"

Lyndon-Li · 2023-01-03T02:37:54Z

pkg/plugin/clientmgmt/backupitemaction/v2/restartable_backup_item_action.go

+}
+
+// Execute restarts the plugin's process if needed, then delegates the call.
+func (r *RestartableBackupItemAction) Execute(item runtime.Unstructured, backup *api.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, string, error) {


Thinking of some plugin process restart cases:

If the plugin process quits unexpectedly (i.e. crash) during a method calling, the call will fail on the client side, so the backup/restore will fail because of the BIA/RIA method fails. Is that right?

Since the plugin runs some async operations, the plugin may quit outside a method calling, for example, the Execute method returns successfully but during running the async operation afterwards, the plugin crashes. In this case, I think the client side gets no notification from the plugin framework, but the specific BIA/RIA implementation needs to tell the caller during the later method calling (i.e., Progress method) if it wants to fail the current backup/restore? Or if we want the plugin implementation has the ability to recover from the quit, what is it like?

@Lyndon-Li

It should result in a partial failure, as the GRPC call itself will error out, but that's not any different from what happens with v1 plugins

If the plugin crashes without restarting, then the Progress method will return an error (similar to the Execute error in your first question), so this will push us to partial failure. If the plugin crashes but restarts, then it's up to the plugin to handle this. For example, for a datamover plugin which creates a separate datamover CR (where the actual async operation happens in a separate controller), a plugin restart after creating the datamover CR won't affect progress, as long as the plugin is running again by the time velero calls Progress next, since it just checks the datamover CR status. If the plugin is itself running the operation in a background thread of the plugin process, then a crash will probably result in an operation failure, so Progress will return an error.

Sounds good, either way is allowed, the behavior is up to the specific plugin.

Signed-off-by: Scott Seago <[email protected]>

shubham-pampattiwar · 2023-01-10T14:33:55Z

LGTM ! Thank you @sseago

github-actions bot added the has-unit-tests label Oct 12, 2022

github-actions bot assigned sseago Oct 12, 2022

sseago force-pushed the biav2-impl2 branch from 4dd2246 to 8e24f9a Compare October 12, 2022 18:53

github-actions bot added the has-changelog label Oct 12, 2022

sseago force-pushed the biav2-impl2 branch 2 times, most recently from 690a755 to e0e1e80 Compare October 12, 2022 19:16

sseago marked this pull request as draft October 13, 2022 15:59

sseago force-pushed the biav2-impl2 branch 3 times, most recently from 4a3b3be to ff748cc Compare October 17, 2022 21:25

blackpiglet mentioned this pull request Oct 18, 2022

Add whether volume is FS uploader handled method in BIA/RIA #5455

Closed

sseago mentioned this pull request Oct 18, 2022

Example BIA and RIA v2 plugins vmware-tanzu/velero-plugin-example#66

Merged

sseago force-pushed the biav2-impl2 branch from ff748cc to 68e96ac Compare October 25, 2022 19:11

weshayutin mentioned this pull request Oct 31, 2022

Implement new plugin v2 APIs for BackupItemAction #5488

Closed

sseago force-pushed the biav2-impl2 branch from 68e96ac to fd02297 Compare November 7, 2022 22:15

sseago force-pushed the biav2-impl2 branch from fd02297 to 2cae429 Compare December 13, 2022 19:58

github-actions bot added the Area/Design Design Documents label Dec 13, 2022

sseago marked this pull request as ready for review December 13, 2022 19:58

github-actions bot requested review from Lyndon-Li and ywk253100 December 13, 2022 19:59

sseago force-pushed the biav2-impl2 branch from 2cae429 to 89bb8bb Compare December 13, 2022 20:09

Lyndon-Li reviewed Jan 3, 2023

View reviewed changes

BackupItemAction v2 API implementation

9a54142

Signed-off-by: Scott Seago <[email protected]>

sseago force-pushed the biav2-impl2 branch from 89bb8bb to 9a54142 Compare January 9, 2023 23:37

Lyndon-Li approved these changes Jan 10, 2023

View reviewed changes

shubham-pampattiwar approved these changes Jan 10, 2023

View reviewed changes

shubham-pampattiwar merged commit ab642ff into vmware-tanzu:main Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BackupItemAction v2 API implementation #5442

BackupItemAction v2 API implementation #5442

sseago commented Oct 12, 2022 •

edited

Loading

codecov-commenter commented Oct 12, 2022 •

edited

Loading

kaovilai commented Dec 13, 2022

weshayutin commented Dec 13, 2022 •

edited

Loading

sseago commented Dec 13, 2022

Lyndon-Li Jan 3, 2023 •

edited

Loading

sseago Jan 3, 2023

shubham-pampattiwar Jan 6, 2023

Lyndon-Li Jan 3, 2023

sseago Jan 9, 2023

Lyndon-Li Jan 3, 2023 •

edited

Loading

sseago Jan 3, 2023

Lyndon-Li Jan 10, 2023

shubham-pampattiwar commented Jan 10, 2023

BackupItemAction v2 API implementation #5442

BackupItemAction v2 API implementation #5442

Conversation

sseago commented Oct 12, 2022 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

codecov-commenter commented Oct 12, 2022 • edited Loading

Codecov Report

kaovilai commented Dec 13, 2022

weshayutin commented Dec 13, 2022 • edited Loading

sseago commented Dec 13, 2022

Lyndon-Li Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

sseago Jan 3, 2023

Choose a reason for hiding this comment

shubham-pampattiwar Jan 6, 2023

Choose a reason for hiding this comment

Lyndon-Li Jan 3, 2023

Choose a reason for hiding this comment

sseago Jan 9, 2023

Choose a reason for hiding this comment

Lyndon-Li Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

sseago Jan 3, 2023

Choose a reason for hiding this comment

Lyndon-Li Jan 10, 2023

Choose a reason for hiding this comment

shubham-pampattiwar commented Jan 10, 2023

sseago commented Oct 12, 2022 •

edited

Loading

codecov-commenter commented Oct 12, 2022 •

edited

Loading

weshayutin commented Dec 13, 2022 •

edited

Loading

Lyndon-Li Jan 3, 2023 •

edited

Loading

Lyndon-Li Jan 3, 2023 •

edited

Loading