azurerm_cosmosdb_mongo_collection, azurerm_cosmosdb_mongo_database - support configuring throughput #4620

e96wic · 2019-10-15T11:18:06Z

No description provided.

e96wic · 2019-10-15T11:19:49Z

Changes functionality of 0caed50

katbyte · 2019-10-21T11:03:44Z

I don't think silently recreating the DB is the best user experience here. We are discussing this in #4616 and i think we should instead error with a message saying the resource needs to be recreated by hand linking to an issue on the azure-for-go SDK detailing the limitation of the API (it does sound like a bug on their end)

WDYT?

e96wic · 2019-10-21T13:34:11Z

Hi @katbyte, where do you think I'm recreating the DB - can you point me to the code? I totally agree with you, silently recreating resources is a bad idea!

katbyte · 2019-10-21T15:48:12Z

whoops 😅 i totally am misremembering this PR from last week. Your entirely right, i will give it a closer look later today/tomorrow.

It looks like you arebasicallyy doing what was suggested already by adding in some guards and removing the default throughput (which i can now see would cause some problems?)

katbyte · 2019-10-21T15:48:17Z

@e96wic

katbyte

@e96wic,

I've left some comments inline, overall this is looking great. Thanks for fixing this. I am concerned about a breaking change by making the schema default, however i think it should be fine as if its not configured it'll be ignored, otherwise a user will get a plan on upgrade. WDYT?

website/docs/r/cosmosdb_mongo_database.html.markdown

website/docs/r/cosmosdb_mongo_collection.html.markdown

katbyte · 2019-10-22T10:05:27Z

azurerm/resource_arm_cosmosdb_mongo_collection.go

+		} else if dbHasThroughputConfigured {
+			createUpdateOptions["throughput"] = utils.String(strconv.Itoa(throughput))
 		}


I think we'll need to change this to

Suggested change

} else if dbHasThroughputConfigured {

createUpdateOptions["throughput"] = utils.String(strconv.Itoa(throughput))

}

if hasThroughput {

createUpdateOptions["throughput"] = utils.String(strconv.Itoa(throughput))

}

looks correct, but don't we update the resource when executing "terraform import" with the current logic?

manual tests did not show what I was assuming so I guess it's fine

azurerm/resource_arm_cosmosdb_mongo_collection.go

katbyte · 2019-10-22T10:08:16Z

azurerm/resource_arm_cosmosdb_mongo_collection.go

-	}
+		throughputFuture, err := client.UpdateMongoDBCollectionThroughput(ctx, resourceGroup, account, database, name, throughputParameters)
+		if err != nil {
+			_ = d.Set("throughput", nil)


Why are you setting this to nil?

katbyte · 2019-10-22T10:11:00Z

azurerm/resource_arm_cosmosdb_mongo_collection.go

@@ -78,7 +80,7 @@ func resourceArmCosmosDbMongoCollection() *schema.Resource {
 			"throughput": {
 				Type:         schema.TypeInt,
 				Optional:     true,
-				Default:      400,
+				Default:      nil,


I think this will technically be a breaking change? While i agree we should make the change i'm not sure we can until 2.0 where we break/change schema

That is a good question. If I'm correct it should not break anyones code / resources.
Case 1 - user had no throughput configured for existing resource -> terraform won't re-apply the throughput meaning it won't override whatever is configured in azure.
Case 2 - user creates a new resource -> will be created with the azure default (which should be 1000)
Case 3 - user had throughput configured -> will use that value

In the past my team had quite some trouble with newly added parameters with default values. In some cases it destroyed resources for us. In this case setting the throughput to a default of 400 might scale down collections for users which had them previously configured in terraform, but scaled via azure directly. If users roll their terraform config out automatically this is dangerous.

What do you think, @katbyte ?

Documentation and testing showed 400 being the azure default - but I do agree that defaulting to nil here probably makes more sense

👍I think your correct, at worst the user will just have a plan with no affect.

azurerm/resource_arm_cosmosdb_mongo_database.go

katbyte · 2019-10-22T10:12:02Z

azurerm/resource_arm_cosmosdb_mongo_database.go

+	_ = d.Set("resource_group_name", id.ResourceGroup)
+	_ = d.Set("account_name", id.Account)


We don't need the assingment

Suggested change

_ = d.Set("resource_group_name", id.ResourceGroup)

_ = d.Set("account_name", id.Account)

d.Set("account_name", id.Account)

azurerm/resource_arm_cosmosdb_mongo_database.go

k-k · 2019-10-24T18:23:39Z

@e96wic Thanks for this follow up

@katbyte Once this PR is resolved, I'll use it for the basis of fixing #4616

e96wic · 2019-10-26T14:50:18Z

@katbyte Could you have another look at this PR? :)

katbyte

Hey @e96wic,

Thanks for making those revisions, but looks like we are getting some test failures:

------- Stdout: -------
=== RUN   TestAccAzureRMCosmosDbMongoCollection_basic
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_basic
=== CONT  TestAccAzureRMCosmosDbMongoCollection_basic
--- FAIL: TestAccAzureRMCosmosDbMongoCollection_basic (1125.66s)
    testing.go:569: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: azurerm_cosmosdb_mongo_collection.test
          account_name:        "acctest-191028043915315190" => "acctest-191028043915315190"
          database_name:       "acctest-191028043915315190" => "acctest-191028043915315190"
          default_ttl_seconds: "0" => "0"
          id:                  "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-191028043915315190/providers/Microsoft.DocumentDB/databaseAccounts/acctest-191028043915315190/apis/mongodb/databases/acctest-191028043915315190/collections/acctest-191028043915315190" => "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-191028043915315190/providers/Microsoft.DocumentDB/databaseAccounts/acctest-191028043915315190/apis/mongodb/databases/acctest-191028043915315190/collections/acctest-191028043915315190"
          indexes.#:           "0" => "0"
          name:                "acctest-191028043915315190" => "acctest-191028043915315190"
          resource_group_name: "acctestRG-191028043915315190" => "acctestRG-191028043915315190"
          throughput:          "400" => ""

you may have to simply not pull in the throughput if its not been set

e96wic · 2019-11-09T23:49:08Z

Hi @katbyte, the failing acceptance test actually indicated the desired behavior. The import test step failed because the config did not have throughput configured. Azure sets a default throughput though, which will be imported to the state by my changes.

katbyte

Hi @e96wic,

That failing test indicates a perpetual diff when no throughput is specified, so we will need to make sure that test step passes (the rest fail do to a change in the API causing a diff with indexes)

Also we'll need to write a complete test for the database resource & an update test to ensure the throughput can be updated if included on creation.

katbyte · 2019-11-15T00:40:03Z

azurerm/resource_arm_cosmosdb_mongo_collection_test.go

 				),
+				ExpectNonEmptyPlan: true,


We shouldn't need this, could we remove this and make the test pass?

katbyte · 2019-11-15T00:42:07Z

website/docs/r/cosmosdb_mongo_database.html.markdown

@@ -36,6 +37,8 @@ The following arguments are supported:

 * `account_name` - (Required) The name of the Cosmos DB Mongo Database to create the table within. Changing this forces a new resource to be created.

+* `throughput` - (Optional) The throughput of the MongoDB collection (RU/s). Must be set in increments of `100`. The minimum value is `400`. This must be set upon database creation otherwise it cannot be updated without a manual terraform destroy-apply.


Could we make this

Suggested change

* `throughput` - (Optional) The throughput of the MongoDB collection (RU/s). Must be set in increments of `100`. The minimum value is `400`. This must be set upon database creation otherwise it cannot be updated without a manual terraform destroy-apply.

* `throughput` - (Optional) The throughput of the MongoDB collection (RU/s). Must be set in increments of `100`, and defaults to `400`.

-> **NOTE:** The minimum value is `400`. This must be set upon database creation otherwise it cannot be updated without a manual terraform destroy-apply.

katbyte · 2019-11-15T00:42:16Z

website/docs/r/cosmosdb_mongo_collection.html.markdown

@@ -56,7 +56,7 @@ The following arguments are supported:
 * `database_name` - (Required) The name of the Cosmos DB Mongo Database in which the Cosmos DB Mongo Collection is created. Changing this forces a new resource to be created.
 * `default_ttl_seconds` - (Required) The default Time To Live in seconds. If the value is `-1` items are not automatically expired.
 * `shard_key` - (Required) The name of the key to partition on for sharding. There must not be any other unique index keys.
-* `throughput` - (Optional) The throughput of the MongoDB collection (RU/s). Must be set in increments of `100`. The default and minimum value is `400`.
+* `throughput` - (Optional) The throughput of the MongoDB collection (RU/s). Must be set in increments of `100`. The minimum value is `400`. This must be set upon database creation otherwise it cannot be updated without a manual terraform destroy-apply.


see my other comment

katbyte · 2019-11-15T00:44:08Z

azurerm/resource_arm_cosmosdb_mongo_database.go

 		},
 	}
 }

-func resourceArmCosmosDbMongoDatabaseCreate(d *schema.ResourceData, meta interface{}) error {
+func resourceArmCosmosDbMongoDatabaseCreateUpdate(d *schema.ResourceData, meta interface{}) error {


Could we use separate create and update functions? i think it'll simplify things a tad

katbyte · 2019-12-04T04:49:10Z

hi @e96wic,

I am looking into finishing this and it doesn't appear i have permission to push to your branch. Are you still working on this or should i go ahead and open a new PR?

katbyte · 2019-12-10T01:03:27Z

Hey @e96wic, i've opened #5116 that address the comments i've made so we can get this merged. As such i am going to close this in favour of it, thanks for all your work on this change!

continuation of #4620

ghost · 2019-12-16T12:50:41Z

This has been released in version 1.39.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.39.0"
}
# ... other configuration ...

ghost · 2020-01-09T14:50:19Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

ghost added size/L documentation labels Oct 15, 2019

e96wic marked this pull request as ready for review October 15, 2019 11:19

katbyte added service/cosmosdb waiting-response labels Oct 21, 2019

ghost removed the waiting-response label Oct 21, 2019

katbyte requested changes Oct 22, 2019

View reviewed changes

katbyte added the waiting-response label Oct 22, 2019

ghost removed the waiting-response label Oct 24, 2019

katbyte requested changes Oct 28, 2019

View reviewed changes

katbyte added this to the v1.37.0 milestone Oct 28, 2019

katbyte added the waiting-response label Oct 28, 2019

katbyte self-assigned this Oct 31, 2019

katbyte mentioned this pull request Nov 9, 2019

azurerm_cosmosdb_sql_container needs throughput and TTL settings #4802

Closed

e96wic added 4 commits November 9, 2019 23:05

Added throughput config for cosmos mongo databases

b0d3d10

Improved throughput config handling for cosmos mongo collections

a18fe62

Updated docs

08b72eb

Changed code according to review

20c0076

e96wic requested a review from katbyte November 9, 2019 23:41

Adapted acceptance test

12cd710

ghost removed the waiting-response label Nov 9, 2019

katbyte changed the title ~~Cosmos db throughput~~ azurerm_cosmosdb_mongo_collection, azurerm_cosmosdb_mongo_database - support configuring throughput Nov 10, 2019

katbyte requested changes Nov 15, 2019

View reviewed changes

katbyte added the waiting-response label Nov 15, 2019

tombuildsstuff modified the milestones: v1.37.0, v1.38.0 Nov 21, 2019

tombuildsstuff removed this from the v1.38.0 milestone Dec 2, 2019

katbyte mentioned this pull request Dec 10, 2019

azurerm_cosmos_mongo_*: fix throughput & deprecate indexes #5116

Merged

katbyte closed this Dec 10, 2019

katbyte added this to the v1.39.0 milestone Dec 10, 2019

katbyte added a commit that referenced this pull request Dec 11, 2019

azurerm_cosmos_mongo_*: fix throughput & deprecate indexes (#5116)

b711bd1

continuation of #4620

ghost removed the waiting-response label Dec 16, 2019

ghost locked and limited conversation to collaborators Jan 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_cosmosdb_mongo_collection, azurerm_cosmosdb_mongo_database - support configuring throughput #4620

azurerm_cosmosdb_mongo_collection, azurerm_cosmosdb_mongo_database - support configuring throughput #4620

e96wic commented Oct 15, 2019

e96wic commented Oct 15, 2019

katbyte commented Oct 21, 2019

e96wic commented Oct 21, 2019

katbyte commented Oct 21, 2019

katbyte commented Oct 21, 2019

katbyte left a comment

katbyte Oct 22, 2019

e96wic Oct 26, 2019

e96wic Oct 26, 2019

katbyte Oct 22, 2019

katbyte Oct 22, 2019

e96wic Oct 22, 2019

e96wic Oct 22, 2019

k-k Oct 24, 2019

katbyte Oct 24, 2019

katbyte Oct 22, 2019

k-k commented Oct 24, 2019

e96wic commented Oct 26, 2019 •

edited

Loading

katbyte left a comment

e96wic commented Nov 9, 2019

katbyte left a comment

katbyte Nov 15, 2019

katbyte Nov 15, 2019

katbyte Nov 15, 2019

katbyte Nov 15, 2019

katbyte commented Dec 4, 2019

katbyte commented Dec 10, 2019

ghost commented Dec 16, 2019

ghost commented Jan 9, 2020

		_ = d.Set("resource_group_name", id.ResourceGroup)
		_ = d.Set("account_name", id.Account)

	_ = d.Set("resource_group_name", id.ResourceGroup)
	_ = d.Set("account_name", id.Account)
	d.Set("account_name", id.Account)

		@@ -36,6 +37,8 @@ The following arguments are supported:

		* `account_name` - (Required) The name of the Cosmos DB Mongo Database to create the table within. Changing this forces a new resource to be created.

		* `throughput` - (Optional) The throughput of the MongoDB collection (RU/s). Must be set in increments of `100`. The minimum value is `400`. This must be set upon database creation otherwise it cannot be updated without a manual terraform destroy-apply.

-* `throughput` - (Optional) The throughput of the MongoDB collection (RU/s). Must be set in increments of `100`. The minimum value is `400`. This must be set upon database creation otherwise it cannot be updated without a manual terraform destroy-apply.
+* `throughput` - (Optional) The throughput of the MongoDB collection (RU/s). Must be set in increments of `100`, and defaults to `400`.
+-> **NOTE:** The minimum value is `400`. This must be set upon database creation otherwise it cannot be updated without a manual terraform destroy-apply.

azurerm_cosmosdb_mongo_collection, azurerm_cosmosdb_mongo_database - support configuring throughput #4620

azurerm_cosmosdb_mongo_collection, azurerm_cosmosdb_mongo_database - support configuring throughput #4620

Conversation

e96wic commented Oct 15, 2019

e96wic commented Oct 15, 2019

katbyte commented Oct 21, 2019

e96wic commented Oct 21, 2019

katbyte commented Oct 21, 2019

katbyte commented Oct 21, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-k commented Oct 24, 2019

e96wic commented Oct 26, 2019 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

e96wic commented Nov 9, 2019

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Dec 4, 2019

katbyte commented Dec 10, 2019

ghost commented Dec 16, 2019

ghost commented Jan 9, 2020

e96wic commented Oct 26, 2019 •

edited

Loading