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

azurerm_cognitive_deployment - Upgrade cognitive api version, update resource schema to fix deprecated argument's issue #22223

Merged
merged 5 commits into from
Jul 6, 2023

Conversation

lonegunmanb
Copy link
Contributor

@lonegunmanb lonegunmanb commented Jun 20, 2023

This pr contains a breaking change to azurerm_cognitive_deployment and it should fix #22198.

In cognitive API version 2022-10-01 there's scale object in the resource's domain object, but it's no longer supported. If we create the cognitive deployment on the portal you would find that the portal use 2023-05-01 API and it won't send scale property to the service side. Actually, both of scale and new added sku object is optional, but when I tried adding an optional sku block and provision an azurerm_cognitive_deployment resource without sku, the service side would set default values to sku block and caused a drift (set name = Standard and capacity = 1).

So I set sku a required block since the service side would set default values if it's absent. To avoid a drift the easiest way is passing a sku block like we set scale block before:

sku {
    name = "Standard"
}

The reason I removed scale block which caused a breaking change is, if we preserve this block, the new API would complain like the following error message:

Error: creating Deployment (Subscription: "xxxxxxxx"
        Resource Group Name: "acctest-rg-230620144039447170"
        Account Name: "acctest-ca-230620144039447170"
        Deployment Name: "acctest-cd-230620144039447170"): performing CreateOrUpdate: unexpected status 400 with error: InvalidResourceProperties: The scale settings of account deployment is deprecated since API version '2023-05-01', please use 'sku' instead.
        
          with azurerm_cognitive_deployment.test,
          on terraform_plugin_test.tf line 32, in resource "azurerm_cognitive_deployment" "test":
          32: resource "azurerm_cognitive_deployment" "test" {
        
        creating Deployment (Subscription: "xxxxxxxx"
        Resource Group Name: "acctest-rg-230620144039447170"
        Account Name: "acctest-ca-230620144039447170"
        Deployment Name: "acctest-cd-230620144039447170"): performing CreateOrUpdate:
        unexpected status 400 with error: InvalidResourceProperties: The scale
        settings of account deployment is deprecated since API version '2023-05-01',
        please use 'sku' instead.
        --- FAIL: TestAccCognitiveDeploymentSequential/deployment/basic (205.40s)

Since we cannot set this block anymore, to avoid such errors in the future I removed this block from the resource.

@@ -94,21 +99,46 @@ func (r CognitiveDeploymentResource) Arguments() map[string]*pluginsdk.Schema {
},
},

"scale": {
"sku": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely removing/renaming will break existing users since this is ForceNew - since these look to be doing/controlling the same thing I think we should keep the existing names but map it to the new names e.g. scale -> sku and type -> name within the provider, this means users configs will remain the same. We can do a hard rename of this in the next major release.

Does that makes sense?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears we're also blocked by this and can't deploy changes to our OpenAI deployments via Terraform.

Is there a short-term workaround to make 'scale' not required, potentially?

Copy link
Contributor Author

@lonegunmanb lonegunmanb Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephybun I've tried the solution you suggested but we have a problem now. There's an argument named capacity in the new sku block:

"capacity": {
				Type:     pluginsdk.TypeInt,
				Optional: true,
				ForceNew: true,
				Default:  1,
			},

The capacity's default value is 1, and you cannot pass nil to API otherwise an error would be raised. That means the whole new sku block is required, otherwise when we try to import the resource and got 1 for capacity from the service side, the absent of sku block would cause a drift. Since all properties for this resource are ForceNew that would cause a re-creation.

But if users added a sku block to the existing code, that would cause a re-creation too. It seems like we cannot avoid a re-creation.

The good news is this deployment resource seems to be a stateless one, so a re-creation won't cause any data loss or long-term service downtime.

Another possible approach is just to hide all sku's properties, and just hard code the capacity to 1 in the provider's code, that looks better than the status quo to me.

I'll try this approach and you can decide whether you like this design or not.


I've changed my mind, I'll try to put all properties that belong to sku into scale block, then all we need to do in 4.0 is just rename the block's name to sku.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a reasonable plan to me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katbyte , Is there any process to track these kinds of improvements/breaking change to 4.0 ?

@lonegunmanb
Copy link
Contributor Author

@stephybun Would you please give this pr another review? Thanks!

@MarvelOrange
Copy link

Bump on this one, I am unable to deploy a model now and its impacting delivery :(

@unique-dominik
Copy link
Contributor

Same from our side, our delivery is blocked due to this ❤️ Hoping for a review.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lonegunmanb - LGTM 💾

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warnings for azurerm_cognitive_deployment documentation
7 participants