Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Request: Add option to specify interim model definition in LROs for all languages #4706

Closed
m-nash opened this issue Aug 17, 2022 · 3 comments
Assignees
Labels
architecture board-review Request for an Architectural Board Review

Comments

@m-nash
Copy link
Member

m-nash commented Aug 17, 2022

There have been a couple issues (few examples: Azure/azure-sdk-for-net#29060, Azure/azure-sdk-for-js#22522, Azure/autorest.csharp#2571) that have popped up recently around how we define an LRO in track 2. The common theme amongst these is around a gap in the specification to define what the interim model is compared to the final state model.

In this issue Azure/autorest.csharp#2571 the problem comes from the response definition that is defined here specifies that the model returned is ManagedCluster, its resource type is Microsoft.ContainerService/managedClusters and its version is 2022-04-01. This information is correct however what isn't specified is that for each of the interim polling requests before we reach the final state the model is something different and more importantly the resource type is Microsoft.ContainerService/locations/operations which means it has a different set of possible versions in your given environment. In this particular case the only two valid versions are 2016-03-30 or 2017-08-31. With this decision #3817 we made a decision that in the case of query string api versions the service should NOT return them, but the client should specify what they want from the service. Since only the final state definition is defined in the spec there is no way to know that the polling versions are actually different.

In this issue Azure/azure-sdk-for-net#29060 dotnet made the decision to never deserialize the interim responses because we can't know if they are the same as the final state (not sure on other languages here). This means that in track 2 customers wouldn't be able to see the detailed status. We don't have the mismatch of resource type this time, but still provide a sub optimal generated client since the interim model is not known.

I propose that we add a new parameter to x-ms-long-running-operation-options called PollingSchema which needs to give us the ability to decipher 3 pieces of information.

  • Schema shape: This is fairly straightforward as long as we have a linter rule that validates a valid link like #/definitions/MyPollingModel or ../2017-08-31/spec.json#/definitions/MyPollingModel. At minimum modeler 4 should validate this points at something valid.
  • Resource type: This would be optional since for data plane this would not be needed. In management plane api version is tied to resource type so this is needed to look up if the version has been overridden in the current client.
  • api-version: This typically comes from the path of the swagger file which I believe should still work in all cases here.

CADL is already planning to allow you to independently specify the polling operation and the final state operation so this aligns with that future direction and allows us to more holistically tackle the entire set of issues versus coming up with one off changes for each special case.

If we took this type of approach I think we could make this opt in such that the absence of this property results in the same behavior that we have today, but when we find special LROs like the few issues linked above we can add this new property to the swagger and each language generator could take advantage of this however they would like. This nicely sets us up for CADL migration since this will always be defined so we will be able to take advantage of the additional information in all cases then.

I propose we take advantage of it in two ways

  • More accurate api-version replacement to support the design that the service should not be returning this.
  • Add a GetStatus method to the LRO object allowing customers to get the interim status easily and not changing the return type of GetValue since that method already represents the final state model.
@m-nash m-nash added architecture board-review Request for an Architectural Board Review labels Aug 17, 2022
@mikekistler
Copy link
Member

The new LRO guidelines say that the operation that initiates an LRO must document which operation is used to poll for completion. I think the response schema of that operation is the "interim" model.

So rather than "pollingSchema" being defined in x-ms-loing-running-operation-options, I think we want "pollingOperation".

@kyle-patterson
Copy link
Member

Scheduled for 9/6 2p-4p PDT

@tg-msft
Copy link
Member

tg-msft commented Sep 7, 2022

Recording (MS INTERNAL ONLY)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture board-review Request for an Architectural Board Review
Projects
None yet
Development

No branches or pull requests

5 participants