Design doc for RestoreItemAction wait for AdditionalItems to be ready

[sseago]

Design for #1350

[alaypatel07]

s/additioonal/additional

[sseago]

I'm going to update this shortly with a third implementation option which should get rid of the need for making interface changes. Instead of adding a new func to either an existing or new interface, we could replace the WaitForAdditionalItems bool in my proposed changes to RestoreItemActionExecuteOutput with AdditionalItemsReadyFunc which would be a pointer to a func. If a plugin ignores it (as would any pre-existing plugins), it remains nil and velero doesn't attempt to wait. If a plugin needs to wait for AdditionalItems to be ready, instead of setting an entry in the return struct to 'true', it will return a func pointer, which will be the func that velero's restoreItem func will call in its wait method rather than a named interface func. This will remove the need for existing plugins to have to make changes here without introducing optional interfaces.

[nrb]

Thanks @sseago! I know you're working on this, so I'm likely to review after the v1.5.0 release and you're a little further along. I'll also be on vacation from Sept 4 to Sept 14th, so it's probably going to be after that, just to set expectations.

One question I do have - are you/your team able to sign up for implementation of this once approved?

[sseago]

@nrb Yes, my plan was to put together a PR to implement this once we got sign-off on the design. I expect one more update to the design before it's ready to review -- to propose a third alternative which I think will eliminate both the backwards-compatibilty problem and the need to define a new optional interface, as I mention in the above comment. I may get to it today, or tomorrow at the latest. Either way I assume your current priority is getting 1.5.0 out the door.

[nrb]

So far I think this is reasonable.

I like the idea of making the AdditionalItemsReadyFunc a pointer so that not all RestoreItemAction plugins have to implement it.

I do still have a few questions on specific areas, and would like to hear from the rest of @vmware-tanzu/velero-maintainers

[sseago]

Updated with the WithItemsWait func definition added to the document.

[nrb]

@vmware-tanzu/velero-maintainers Please give this a review.

[ashish-amarnath]

@sseago Thanks for this proposal and apologies for not looking at this sooner.
I added a couple of comments. PTAL. Happy to discuss further either in the PR or in a separate design meeting. I can schedule a design meeting if that is preferred.

[ashish-amarnath]

resourceTerminatingTimeout is not meant to be used for this purpose. Suggest creating a different constant/config for this.

[sseago]

I agree that a different method of determining it would be better. I didn't include that in the design as I wasn't sure what made more sense. We can include it as a restore option as suggested above, although we'll need to provide a sensible default (perhaps via a new const). We could also allow plugin-level overriding, but it's not obvious whether or not we need that additional complexity and if we add it, what the priority would be for using. Perhaps first priority is to use the value set on the restore CR, if provided. Then plugin-level value (in the plugin RestoreItemExecuteOutput struct), and if both are empty, use something defined in a const.

[zubron]

I think I'm missing something but what would it look like for the timeout value to be configurable at the plugin level? Is it possible under the current plugin model for specific configuration to be passed in when the plugins are being added? Or did you mean that different plugins would define how long they would wait but it would be a fixed duration?

[sseago]

So one way to do it which avoids having to change any existing plugins that don't need the new functionality is to add an additional (optional) timeout field to the RestoreItemActionExecuteOutput struct. When the Execute func returns, it could add this from a const defined within the plugin implementation. We already have one field we're adding to this struct to define the "wait for ready" func, and based on recent review comments, we may be adding another bool field which velero will use to decide whether to wait at all for this plugin (instead of overloading the func return value for this, to allow for a default "does Get return it?" implementation. That's what I was considering for the next update.

[ashish-amarnath]

Adding a timeout field in the RestoreItemActionExecuteOutput with a new const for a default seems reasonable to me.

[ashish-amarnath]

This LGTM.
The only pending comment is about adding a timeout field in the RestoreItemActionExecuteOutput struct.

[ashish-amarnath]

Adding a timeout field in the RestoreItemActionExecuteOutput with a new const for a default seems reasonable to me.

[sseago]

@ashish-amarnath I've updated the timeout section to include a new server-wide value specified in serverConfig.additionalItemsReadyTimeout (with a default const), as well as a plugin-level override value as an optional field in RestoreItemActionExecuteOutput.

[zubron]

Thanks for your time and effort on this, @sseago! I think this proposed design looks good and I don't have any additional questions or comments. Thanks for addressing them all! 👍

[ashish-amarnath]

This LGTM! Thanks for addressing the comments.
Leaving the merge to @nrb

[carlisia]

This lgtm. Deferring to @nrb.

[nrb]

Thanks for your effort and patience on this @sseago!