BackupItemAction v2 design #5382
Conversation
|
/kind changelog-not-required |
github-actions
bot
requested review from
blackpiglet and
shubham-pampattiwar
github-actions
bot
added
the
kind/changelog-not-required
blackpiglet
added
kind/changelog-not-required
|
I'm going to update this to use the google.protobuf.Empty predefined type rather than defining my own empty message type. |
sseago
force-pushed
the
biav2-design
branch
from
e94234a to
31d795c
Compare
|
Can we also consider this change in BIA V2 #5274? |
|
@Lyndon-Li I commented on the issue you linked -- it's not clear to me what you'd like added to the interface, but if we can identify the changes required, I can include them in this design (and in the RestoreItemAction v2 design). |
|
Working through a draft implementation of this, I realized that I'm missing a couple fields in the |
sseago
force-pushed
the
biav2-design
branch
2 times, most recently
from
a59bb69 to
8211109
Compare
This includes necessary changes to support async item action monitoring. Signed-off-by: Scott Seago <[email protected]>
|
Now that the overall design changes have been approved, this PR is ready for review. |
|
@dsu-igeek @ywk253100 @blackpiglet @qiuming-best @Lyndon-Li @reasonerjt Please review as time permits, thank you! |
| rpc AppliesTo(BackupItemActionAppliesToRequest) returns (BackupItemActionAppliesToResponse); | ||
| rpc Execute(ExecuteRequest) returns (ExecuteResponse); | ||
| rpc Progress(BackupItemActionProgressRequest) returns (BackupItemActionProgressResponse); | ||
| rpc Cancel(BackupItemActionCancelRequest) returns (google.protobuf.Empty); |
There was a problem hiding this comment.
I'm new to this, but is it a common pattern to use google.protobuf.Empty for golang's error type?
Is it documented somewhere? I tried to quickly search but not found a good reference.
Will this cause information loss if the func in plugin implementation returns an error?
There was a problem hiding this comment.
@reasonerjt "empty" isn't for error. The golang api has no return value other than the error that all api funcs have, but the protobuf infrastructure requires something. Empty is just a standard way of doing this vs. creating a custom struct with no elements in it. Note that none of the protobuf definitions include the error return in the protobuf signature, but the corresponding golang apis (in pkg/plugin/velero) have the error return.
There was a problem hiding this comment.
Yes, I noticed that.
I think we need to understand if this causes problems like the info in the error returned by plugin code is propagated correctly to the client, and that's beyond the scope of this PR.
There was a problem hiding this comment.
The error return isn't covered by this for any of the methods. If you compare this to AppliesTo or Execute, you'll see that those repsonse messages include everything but the error return. Compare to the golang files in pkg/plugin/generated for the existing v1 API code and you'll see the error resposes there.
Basically the actual grpc Invoke call puts the input and output structs as args to Invoke, and error is returned by Invoke. So the protobuf output structs don't include their error returns since that's a separate return from invoke, and the error is added to the generated golang funcs for all of the api methods.
This includes necessary changes to support async item action monitoring.
This includes the design for the BackupItemAction v2 API which is to be implemented in the Velero 1.11 dev cycle.
Does your change fix a particular issue?
Fixes #(issue)
Please indicate you've done the following:
/kind changelog-not-requiredas a comment on this pull request.site/content/docs/main.