provider/aws: Fix DynamoDB issues about GSIs indexes #13256

Ninir · 2017-03-31T18:43:21Z

Description

This fixes 2 DynamoDB issues about GSI indexes. The first one is represented by the first two linked issues and is about the update of multiple GSIs.

The second one is about a diff that was really big. In fact, the Set attribute on the global_secondary_indexes was using the read/write capacity. When this capacity was changed on the table, it has to be changed on indexes also, which generates the diff.

Couldn't do separate PRs for that, since the second one was impacting the resolution of the first one with this error:

testing.go:273: Step 1 error: After applying this step, the plan was not empty:
	UPDATE: aws_dynamodb_table.test
	  global_secondary_index.1396769947.hash_key:             "att1" => ""
	  global_secondary_index.1396769947.name:                 "att1-index" => ""
	  global_secondary_index.1396769947.non_key_attributes.#: "0" => "0"
	  global_secondary_index.1396769947.projection_type:      "ALL" => ""
	  global_secondary_index.1396769947.range_key:            "" => ""
	  global_secondary_index.1396769947.read_capacity:        "10" => "0"
	  global_secondary_index.1396769947.write_capacity:       "10" => "0"
	  global_secondary_index.2884824503.hash_key:             "" => "att2"
	  global_secondary_index.2884824503.name:                 "" => "att2-index"
	  global_secondary_index.2884824503.non_key_attributes.#: "0" => "0"
	  global_secondary_index.2884824503.projection_type:      "" => "ALL"
	  global_secondary_index.2884824503.range_key:            "" => ""
	  global_secondary_index.2884824503.read_capacity:        "" => "20"
	  global_secondary_index.2884824503.write_capacity:       "" => "20"
	  global_secondary_index.2989473846.hash_key:             "att3" => ""
	  global_secondary_index.2989473846.name:                 "att3-index" => ""
	  global_secondary_index.2989473846.non_key_attributes.#: "0" => "0"
	  global_secondary_index.2989473846.projection_type:      "ALL" => ""
	  global_secondary_index.2989473846.range_key:            "" => ""
	  global_secondary_index.2989473846.read_capacity:        "10" => "0"
	  global_secondary_index.2989473846.write_capacity:       "10" => "0"
	  global_secondary_index.3616786540.hash_key:             "" => "att1"
	  global_secondary_index.3616786540.name:                 "" => "att1-index"
	  global_secondary_index.3616786540.non_key_attributes.#: "0" => "0"
	  global_secondary_index.3616786540.projection_type:      "" => "ALL"
	  global_secondary_index.3616786540.range_key:            "" => ""
	  global_secondary_index.3616786540.read_capacity:        "" => "20"
	  global_secondary_index.3616786540.write_capacity:       "" => "20"
	  global_secondary_index.790634816.hash_key:              "att2" => ""
	  global_secondary_index.790634816.name:                  "att2-index" => ""
	  global_secondary_index.790634816.non_key_attributes.#:  "0" => "0"
	  global_secondary_index.790634816.projection_type:       "ALL" => ""
	  global_secondary_index.790634816.range_key:             "" => ""
	  global_secondary_index.790634816.read_capacity:         "10" => "0"
	  global_secondary_index.790634816.write_capacity:        "10" => "0"
	  global_secondary_index.922553537.hash_key:              "" => "att3"
	  global_secondary_index.922553537.name:                  "" => "att3-index"
	  global_secondary_index.922553537.non_key_attributes.#:  "0" => "0"
	  global_secondary_index.922553537.projection_type:       "" => "ALL"
	  global_secondary_index.922553537.range_key:             "" => ""
	  global_secondary_index.922553537.read_capacity:         "" => "20"
	  global_secondary_index.922553537.write_capacity:        "" => "20"

I also cleaned & normalized acc tests.

Related issues

Tests

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSDynamoDbTable_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/06 19:53:26 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSDynamoDbTable_ -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_importBasic (46.04s)
=== RUN   TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_importTags (45.03s)
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (79.78s)
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (51.41s)
=== RUN   TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_tags (49.15s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdate
--- PASS: TestAccAWSDynamoDbTable_gsiUpdate (130.87s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	402.306s

Ninir · 2017-03-31T18:45:15Z

builtin/providers/aws/resource_aws_dynamodb_table.go

@@ -162,8 +162,6 @@ func resourceAwsDynamoDbTable() *schema.Resource {
 					var buf bytes.Buffer
 					m := v.(map[string]interface{})
 					buf.WriteString(fmt.Sprintf("%s-", m["name"].(string)))
-					buf.WriteString(fmt.Sprintf("%d-", m["write_capacity"].(int)))
-					buf.WriteString(fmt.Sprintf("%d-", m["read_capacity"].(int)))


This solves the secondary issue and is needed for the first one.

Ninir · 2017-03-31T18:49:15Z

builtin/providers/aws/resource_aws_dynamodb_table.go

 			for _, updatedgsidata := range gsiSet.List() {
+				updates := []*dynamodb.GlobalSecondaryIndexUpdate{}


This is the main point solving the first issue.
When having multiple GSIs, updates was storing the new GSI along with the n-1 one.

On the first iteration, it was storing the GSI as an update, and launching an update for the first index.
On the second iteration, it was storing the second GSI along with the previous GSI, and launching a new update made of both the N & N-1 updates.
As the N-1 was already being updated, the N couldn't be updated too.

catsby · 2017-04-05T19:41:52Z

Hey @Ninir – can you confirm that we don't need a state migration here?

Ninir · 2017-04-06T17:34:56Z

builtin/providers/aws/resource_aws_dynamodb_table.go

+
+					if err := waitForGSIToBeActive(d.Id(), gsiName, meta); err != nil {
+						return errwrap.Wrapf("Error waiting for Dynamo DB GSI to be active: {{err}}", err)
+					}


This fixes an acceptance test issue where GSI were being updated, but a delete operation was happening before the update was finished.
As Indexes may take time to be updated, this ensures that the read /delete operation is made on updated data.

Ninir · 2017-04-06T19:50:05Z

Hi @catsby

A state migration was indeed required, and after some internal discussions with you & @apparentlymart, I ended up removing the Set function, which was bringing more issues than it could have solve. Also, the built-in Set function already handles the required stuff, so nothing was really needed there.

Played a bit with it and it's all OK on the migration part. Ready on my side!

Thanks for all the advices! 👍

catsby · 2017-04-07T15:12:52Z

Ran the tests:

TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSDynamoDbTable -timeout 120m
=== RUN   TestAccAWSDynamoDbTable_importBasic
--- PASS: TestAccAWSDynamoDbTable_importBasic (20.88s)
=== RUN   TestAccAWSDynamoDbTable_importTags
--- PASS: TestAccAWSDynamoDbTable_importTags (23.93s)
=== RUN   TestAccAWSDynamoDbTable_basic
--- PASS: TestAccAWSDynamoDbTable_basic (49.23s)
=== RUN   TestAccAWSDynamoDbTable_streamSpecification
--- PASS: TestAccAWSDynamoDbTable_streamSpecification (27.47s)
=== RUN   TestAccAWSDynamoDbTable_tags
--- PASS: TestAccAWSDynamoDbTable_tags (28.20s)
=== RUN   TestAccAWSDynamoDbTable_gsiUpdate
--- PASS: TestAccAWSDynamoDbTable_gsiUpdate (65.83s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    215.583s

Thanks!

ghost · 2020-04-14T02:07:37Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Ninir changed the title ~~B aws dynamodb gsi update~~ provider/aws: Fix DynamoDB issues about GSIs indexes Mar 31, 2017

Ninir commented Mar 31, 2017

View reviewed changes

radeksimko added bug provider/aws labels Apr 5, 2017

catsby added the waiting-response An issue/pull request is waiting for a response from the community label Apr 5, 2017

Ninir force-pushed the b-aws-dynamodb-gsi-update branch 2 times, most recently from 4c495f5 to 40fdc7d Compare April 5, 2017 23:17

Ninir added 3 commits April 6, 2017 19:15

provider/aws: Fixed DynamoDB GSI update when using multiple indexes

121a103

provider/aws: Fixed DynamoDB GSI set hash function

220b09b

Added DynamoDB table state migration

df8047b

Ninir force-pushed the b-aws-dynamodb-gsi-update branch from 40fdc7d to df8047b Compare April 6, 2017 17:30

Ninir commented Apr 6, 2017

View reviewed changes

catsby removed the waiting-response An issue/pull request is waiting for a response from the community label Apr 7, 2017

catsby merged commit c44afc4 into hashicorp:master Apr 7, 2017

Ninir deleted the b-aws-dynamodb-gsi-update branch April 7, 2017 15:16

mcwqy9 mentioned this pull request Jul 25, 2017

[WIP] resource/aws_dynamodb_table: Use just the name to determine hashcode of GSI hashicorp/terraform-provider-aws#1240

Closed

ghost locked and limited conversation to collaborators Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Fix DynamoDB issues about GSIs indexes #13256

provider/aws: Fix DynamoDB issues about GSIs indexes #13256

Ninir commented Mar 31, 2017 •

edited

Loading

Ninir Mar 31, 2017

Ninir Mar 31, 2017

catsby commented Apr 5, 2017

Ninir Apr 6, 2017

Ninir commented Apr 6, 2017 •

edited

Loading

catsby commented Apr 7, 2017

ghost commented Apr 14, 2020

		for _, updatedgsidata := range gsiSet.List() {
		updates := []*dynamodb.GlobalSecondaryIndexUpdate{}

provider/aws: Fix DynamoDB issues about GSIs indexes #13256

provider/aws: Fix DynamoDB issues about GSIs indexes #13256

Conversation

Ninir commented Mar 31, 2017 • edited Loading

Description

Related issues

Tests

Ninir Mar 31, 2017

Choose a reason for hiding this comment

Ninir Mar 31, 2017

Choose a reason for hiding this comment

catsby commented Apr 5, 2017

Ninir Apr 6, 2017

Choose a reason for hiding this comment

Ninir commented Apr 6, 2017 • edited Loading

catsby commented Apr 7, 2017

ghost commented Apr 14, 2020

Ninir commented Mar 31, 2017 •

edited

Loading

Ninir commented Apr 6, 2017 •

edited

Loading