“azurerm_spring_cloud_java_deployment” – the cpu and memory_in_gb properties have been deprecated in favour of the quota block

[mybayern1974]

Now the spring cloud Java deployment resource supports users to set half cpu and memory values. The current cpu and memory properties whose types are int are unable to support that. This PR introduces new cpu and memory properties that take precedence of corresponding legacy properties. On the service side, new properties supporting setting half values also take precedence of their corresponding legacy properties.

Service api reference:
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/appplatform/resource-manager/Microsoft.AppPlatform/preview/2021-06-01-preview/appplatform.json#L4111

[mybayern1974]

@katbyte , with favoretti's explanation on what 500m CPU means (it does not mean mhz but follows K8S CPU definition), could you please suggest a coding solution for that? For now I can think of below alternative solutions:

1. As what you suggested towards mem: introduce mem_in_mb besides mem_in_gb, we introduce cpu_units (this is the K8S definition: "Limits and requests for CPU resources are measured in cpu units") besides cpu.

2. As suggested by favoretti, introduce a new block resource_requests which exactly maps swagger, making TF schema ending up in

cpu (int)
mem (int)
resource_requests
{
   cpu (string supporting "500m")
   mem (string supporting "xGi")
}

with documenting the inner cpu/mem takes precedence

3. If there is concern of solution # 2 that may bring users confusion, then we can still keep the current cpu and mem but change their type from int to string. This change on the surface is a breaking change but in real it's not, since cpu=1 in original can still survive in new code because 1 can be correctly recognized as both int and string in tf.
   While this solution introduces some burden in maintaining the code: Take memory for example, we need to main the mapping between original 1 and new 1Gi, and in future we may have to add more mapping if service introduces more values.

I prefer more for solution # 1 and has made a commit for that, while would honor any suggestions here.

[mybayern1974]

The service sprint cloud started running into massive acc_test failures. From the symptom it sounds like some service response got changed. I'm looking into it. While meanwhile assume that problem has no relation with this PR.

[jackofallops]

Hi @mybayern1974
Thanks for this, I've left a few comments below that should be addressed. I'm going to catch up with @katbyte on the new property names asap, as they may need a little additional tweak to better guide users on their use.

[mybayern1974]

Thank you @jackofallops for your review and I've made changes towards all of them. Now I'm looking forward to your suggestion on naming conventions of the newly introduced properties : )

[favoretti]

@mybayern1974 would you mind rebasing this one? :) I'm also waiting on this new API and functionality, even shot in a new PR, but then saw yours. Let me know if you need any help finalizing this? Thanks!

[mybayern1974]

@favoretti , I just made a commit to resolve conflict. Now I'm waiting for jackofallops to suggest naming for newly introduced properties cpu_units and memory that are expected to take precedence to their existing counterparts.

@jackofallops , with assuming the only left discussion is on naming of new properties, a kindly ping to those suggested names. Or please let me know if there is any further logic needs to be changed to fit these newly brought "0.5 cpu/memory" stories by service.

@favoretti , if you found this PR still no progress in the several upcoming days, feel free to bring your Spring Cloud api upgrade move forward. I'm fine to rebase this PR based on your efforts.

[favoretti]

I won’t be able to, cause old values are deprecated and we need these changes combined. But I’ll try to talk to Steve to settle on the naming. Thank you!

…

On Mon, 27 Sep 2021 at 13:22, mybayern1974 ***@***.***> wrote: @favoretti <https://github.com/favoretti> , I just made a commit to resolve conflict. Now I'm waiting for jackofallops to suggest naming for newly introduced properties cpu_units and memory that are expected to take precedence to their existing counterparts. @jackofallops <https://github.com/jackofallops> , with assuming the only left discussion is on naming of new properties, a kindly ping to those suggested names. Or please let me know if there is any further logic needs to be changed to fit these newly brought "0.5 cpu/memory" stories by service. @favoretti <https://github.com/favoretti> , if you found this PR still no progress in the several upcoming days, feel free to bring your Spring Cloud api upgrade move forward. I'm fine to rebase this PR based on your efforts. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12924 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGERNULGG6XAQ2N35YTS73UEBHX3ANCNFSM5B4GPRJQ> .

[katbyte]

@favoretti - @mybayern1974 and i talked about a naming convention to move forward with, so this is just waiting for those changes to be made once they are back from vacation

[mybayern1974]

Sorry for responding late.

I've tuned codes per suggested schema as below

quota {
    cpu
    memory
}

@katbyte , could you please take another took?

[katbyte]

@mybayern1974 - looks like there are two failing tests:

------- Stdout: -------
=== RUN   TestAccSpringCloudApp_update
=== PAUSE TestAccSpringCloudApp_update
=== CONT  TestAccSpringCloudApp_update
    testcase.go:108: Step 3/6 error: Error running apply: exit status 1
        
        Error: waiting for update of Spring Cloud App: (App Name "acctest-sca-211020234753568049" / Spring Name "acctest-sc-211020234753568049" / Resource Group "acctestRG-spring-211020234753568049"): Code="InternalServerError" Message="112032: Failed to create or update app acctest-sca-211020234753568049|bfa9cb60fcbf40888f14d3560cb972a9 of service instance with partition key ******* and row key acctestrg-spring-211020234753568049|acctest-sc-211020234753568049|00aa07c5310a443b9e103aac3a58e7ee, isUpdate:True."
        
          with azurerm_spring_cloud_app.test,
          on terraform_plugin_test.tf line 19, in resource "azurerm_spring_cloud_app" "test":
          19: resource "azurerm_spring_cloud_app" "test" {
        
--- FAIL: TestAccSpringCloudApp_update (1323.82s)
FAIL

[mybayern1974]

@katbyte , after checking with Azure service team, the reason of those failed TC is because of a service side issue, which as indicated in the error log that was "internal server error". Service team is actively fixing it now. This issue has been there for a while and not related with this PR. With that, I would honor your suggestions whether merge this PR first or waiting for service team's fix.

[katbyte]

Have a commitment to fix the service issue for the remaining test failures in nov, as much am going to merge this! 👍

[github-actions]

This functionality has been released in v2.83.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.