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

alphabetically order apiFiles before PUT #373

Closed
wants to merge 1 commit into from

Conversation

lucashuet93
Copy link

@lucashuet93 lucashuet93 commented Aug 31, 2023

Corresponds to bug #371. All revisions should be deployed.

Currently, the APIs are deployed in random order, so revision 2 can be deployed before revision 1, which causes rev 1 to be missed.

Alphabetically ordering the API files prior to the PUT requests ensures that the list of files fed to .ForEachParallel is set in the correct order - v3 will come after v2 and v1/original, and rev3 will come after rev2 and rev1, so on and so forth.

I deployed the SampleArtifacts with an additional Swagger Petstore API to demonstrate the initial issue:
petstorebefore

testapibefore versionedapibefore

The same APIs have all revisions deployed after the change, with correct isCurrent and isOnline properties:
petstoreafter

testapiafter versionedapiafter

@waelkdouh and @guythetechie
The .ForEachParallel on line 387 of Api.cs in the publisher means that the APIs can still be deployed in incorrect order. While the list itself that is fed to .ForEachParallel is in now in the correct order, the parallel nature of the executions may result in order issues. In the deployment logs below, you can see that the PUT request for myversionedapi-v2 occurred before the PUT for myversionedapi.

Updating the .ForEachParallel to a .ForEach ensures the PUT requests are made in the correct order. I have deployed using .ForEachParallel many times and have not been able to actually cause an error or any deployment issues, though the version order can be incorrect, so I haven't made any updates here.

The logs before the change were as follows:

info: Publisher[0]
      Putting apiVersionSet 6286439d9a310d01468c083c...
Publisher: Information: Putting apiVersionSet 6286439d9a310d01468c083c...
info: Publisher[0]
      Putting API myversionedapi...
Publisher: Information: Putting API myversionedapi...
Publisher: Information: Putting API testapi;rev=2...
info: Publisher[0]
      Putting API testapi;rev=2...
info: Publisher[0]
      Putting API myversionedapi-v2;rev=2...
Publisher: Information: Putting API myversionedapi-v2;rev=2...
info: Publisher[0]
      Putting API myversionedapi-v2...
Publisher: Information: Putting API myversionedapi-v2...
info: Publisher[0]
      Putting API unversioned-petstore;rev=2...
Publisher: Information: Putting API unversioned-petstore;rev=2...
Publisher: Information: Putting API testapi...
info: Publisher[0]
      Putting API testapi...
info: Publisher[0]
      Putting API unversioned-petstore...
Publisher: Information: Putting API unversioned-petstore...
info: Publisher[0]
      Execution complete.
Publisher: Information: Execution complete.
info: Microsoft.Hosting.Lifetime[0]
      Application is shutting down...
Microsoft.Hosting.Lifetime: Information: Application is shutting down...

The logs after the change were as follows:

info: Publisher[0]
      Putting apiVersionSet 6286439d9a310d01468c083c...
Publisher: Information: Putting apiVersionSet 6286439d9a310d01468c083c...
Publisher: Information: Putting API myversionedapi-v2...
info: Publisher[0]
      Putting API myversionedapi-v2...
info: Publisher[0]
      Putting API myversionedapi...
Publisher: Information: Putting API myversionedapi...
info: Publisher[0]
      Putting API myversionedapi-v2;rev=2...
Publisher: Information: Putting API myversionedapi-v2;rev=2...
info: Publisher[0]
      Putting API testapi...
Publisher: Information: Putting API testapi...
info: Publisher[0]
      Putting API testapi;rev=2...
Publisher: Information: Putting API testapi;rev=2...
info: Publisher[0]
      Putting API unversioned-petstore...
Publisher: Information: Putting API unversioned-petstore...
Publisher: Information: Putting API unversioned-petstore;rev=2...
info: Publisher[0]
      Putting API unversioned-petstore;rev=2...
info: Publisher[0]
      Execution complete.
Publisher: Information: Execution complete.
info: Microsoft.Hosting.Lifetime[0]
      Application is shutting down...

If I change .ForEachParallel to .ForEach:

info: Publisher[0]
      Putting apiVersionSet 6286439d9a310d01468c083c...
Publisher: Information: Putting apiVersionSet 6286439d9a310d01468c083c...
info: Publisher[0]
      Putting API myversionedapi...
Publisher: Information: Putting API myversionedapi...
info: Publisher[0]
      Putting API myversionedapi-v2...
Publisher: Information: Putting API myversionedapi-v2...
info: Publisher[0]
      Putting API myversionedapi-v2;rev=2...
Publisher: Information: Putting API myversionedapi-v2;rev=2...
info: Publisher[0]
      Putting API testapi...
Publisher: Information: Putting API testapi...
Publisher: Information: Putting API testapi;rev=2...
info: Publisher[0]
      Putting API testapi;rev=2...
info: Publisher[0]
      Putting API unversioned-petstore...
Publisher: Information: Putting API unversioned-petstore...
info: Publisher[0]
      Putting API unversioned-petstore;rev=2...
Publisher: Information: Putting API unversioned-petstore;rev=2...
info: Publisher[0]
      Execution complete.
Publisher: Information: Execution complete.
info: Microsoft.Hosting.Lifetime[0]
      Application is shutting down...
Microsoft.Hosting.Lifetime: Information: Application is shutting down...

@waelkdouh waelkdouh self-assigned this Aug 31, 2023
@waelkdouh waelkdouh added the bug Something isn't working label Aug 31, 2023
@lucashuet93 lucashuet93 marked this pull request as draft August 31, 2023 17:40
@lucashuet93
Copy link
Author

lucashuet93 commented Aug 31, 2023

@waelkdouh @guythetechie moving back to draft as I am still able to trigger errors by making certain revisions current. Testing edge cases. Will re-open once I have the behavior squared away.

@lucashuet93
Copy link
Author

lucashuet93 commented Sep 1, 2023

The changes do allow all revisions to be deployed and appear in API Management successfully, but it introduces a bug - if any revision other than rev1 is current, the PUT request fails with the message:
[{"code":"ValidationError","target":"IsCurrent","message":"Can't set new revision as current."}]

I then updated the list order so current revisions are deployed before non-current revisions. In the case where revN is current and rev 1 is not, if revN is deployed before rev1, the error disappears but we're back to the original issue, where only revN deploys successfully and rev1 is missing.

I then reverted my changes back to what is currently in main and paused the execution of the publisher after each API is deployed, examining how the exported template from Azure changes. When rev2 is deployed before rev1, the following occurs regardless of which API revision is current (using testapi and testapi;rev=2 as an example):

  • PUT for testapi;rev=2 is made. A "Microsoft.ApiManagement/service/apis" resource is created with "name": "..../testapi" and "apiRevision": "2"
  • PUT for testapi is made, but the URL that corresponds to a now existing API ^. After the PUT, there is still only one resource.

When I deploy in alphabetical order so rev2 is deployed after rev1 and pause execution the same way, the following occurs (using testapi and testapi;rev=2 as an example again):

  • PUT for testapi is made. A "Microsoft.ApiManagement/service/apis" resource is created with "name": "..../testapi" and "apiRevision": "1"
  • PUT for testapi;rev=2 is made. A "Microsoft.ApiManagement/service/apis" resource is created with "name": "..../testapi;rev-2" and "apiRevision": "2". Two "Microsoft.ApiManagement/service/apis" resources now exist.

Essentially, when revision N is PUT first, it creates an API resource with the API version's base name and sets the revision number to N. When revision 1 is then PUT, it updates an existing API.

On the other hand, when revision 1 is PUT first, it creates an API resource with the API version's base name and sets the revision number to 1. When another revision is then PUT, the url the PUT request is sent to does not correspond to an existing API, and thus creates a new API. If that revision is current, however, an error is thrown.

I am closing this PR as alphabetically ordering the APIs before their PUT requests introduces an error if revN is current.

@lucashuet93 lucashuet93 closed this Sep 1, 2023
@waelkdouh
Copy link
Contributor

@guythetechie thoughts on this?

@lucashuet93
Copy link
Author

@waelkdouh @guythetechie API Release support would potentially fix/alleviate the issue the ordering introduces as it would enable making other revisions current.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants