Skip to content

Commit b52e89f

Browse files
Julian-ChuJulian-Chu
authored
Fix: creating gsi under on-demand mode (#85)
Issue #, if available: Description of changes: - fix empty NonKeyAttributes validation issue - fix GSI creation issue when using `pay-per-request` mode ```shell 2023-04-19T17:31:29.932+0200 ERROR Reconciler error {"controller": "table", "controllerGroup": "dynamodb.services.k8s.aws", "controllerKind": "Table", "Table": {"name":"ack-demo-table-provisioned-global-secondary-indexes","namespace":"ack-system"}, "namespace": "ack-system", "name": "ack-demo-table-provisioned-global-secondary-indexes", "reconcileID": "c7354a77-56c4-4c56-b715-f16b59d1926e", "error": "ValidationException: One or more parameter values were invalid: Neither ReadCapacityUnits nor WriteCapacityUnits can be specified when BillingMode is PAY_PER_REQUEST\n\tstatus code: 400, request id: R901I15N84RUVS24D4HVRP6L3JVV4KQNSO5AEMVJF66Q9ASUAAJG"} 2023-04-19T18:54:54.887+0200 ERROR Reconciler error {"controller": "table", "controllerGroup": "dynamodb.services.k8s.aws", "controllerKind": "Table", "Table": {"name":"ack-demo-table-provisioned-global-secondary-indexes","namespace":"ack-system"}, "namespace": "ack-system", "name": "ack-demo-table-provisioned-global-secondary-indexes", "reconcileID": "223f9f39-f2cc-460c-ad78-56fce55b4bf3", "error": "ValidationException: One or more parameter values were invalid: ProvisionedThroughput should not be specified for index: id-index when BillingMode is PAY_PER_REQUEST\n\tstatus code: 400, request id: JO5OGQR8IN74JJ2C62O43ESU43VV4KQNSO5AEMVJF66Q9ASUAAJG"} sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler ``` ```yaml conditions: - message: | InvalidParameter: 3 validation error(s) found. - minimum field size of 1, UpdateTableInput.GlobalSecondaryIndexUpdates[0].Create.Projection.NonKeyAttributes. - minimum field value of 1, UpdateTableInput.GlobalSecondaryIndexUpdates[0].Create.ProvisionedThroughput.ReadCapacityUnits. - minimum field value of 1, UpdateTableInput.GlobalSecondaryIndexUpdates[0].Create.ProvisionedThroughput.WriteCapacityUnits. status: "True" type: ACK.Terminal ``` By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent ccb4c56 commit b52e89f

File tree

7 files changed

+205
-22
lines changed

7 files changed

+205
-22
lines changed

‎pkg/resource/table/hooks_global_secondary_indexes.go‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -233,21 +233,21 @@ func (rm *resourceManager) newUpdateTableGlobalSecondaryIndexUpdatesPayload(
233233

234234
// newSDKProvisionedThroughput builds a new *svcsdk.ProvisionedThroughput
235235
func newSDKProvisionedThroughput(pt *v1alpha1.ProvisionedThroughput) *svcsdk.ProvisionedThroughput {
236-
provisionedThroughput := &svcsdk.ProvisionedThroughput{}
237-
if pt != nil {
238-
if pt.ReadCapacityUnits != nil {
239-
provisionedThroughput.ReadCapacityUnits = aws.Int64(*pt.ReadCapacityUnits)
240-
} else {
241-
provisionedThroughput.ReadCapacityUnits = aws.Int64(0)
242-
}
243-
if pt.WriteCapacityUnits != nil {
244-
provisionedThroughput.WriteCapacityUnits = aws.Int64(*pt.WriteCapacityUnits)
245-
} else {
246-
provisionedThroughput.WriteCapacityUnits = aws.Int64(0)
247-
}
248-
} else {
249-
provisionedThroughput.ReadCapacityUnits = aws.Int64(0)
250-
provisionedThroughput.WriteCapacityUnits = aws.Int64(0)
236+
if pt == nil {
237+
return nil
238+
}
239+
provisionedThroughput := &svcsdk.ProvisionedThroughput{
240+
// ref: https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_ProvisionedThroughput.html
241+
// Minimum capacity units is 1 when using provisioned capacity mode
242+
ReadCapacityUnits: aws.Int64(1),
243+
WriteCapacityUnits: aws.Int64(1),
244+
}
245+
if pt.ReadCapacityUnits != nil {
246+
provisionedThroughput.ReadCapacityUnits = aws.Int64(*pt.ReadCapacityUnits)
247+
}
248+
249+
if pt.WriteCapacityUnits != nil {
250+
provisionedThroughput.WriteCapacityUnits = aws.Int64(*pt.WriteCapacityUnits)
251251
}
252252
return provisionedThroughput
253253
}
@@ -263,12 +263,10 @@ func newSDKProjection(p *v1alpha1.Projection) *svcsdk.Projection {
263263
}
264264
if p.NonKeyAttributes != nil {
265265
projection.NonKeyAttributes = p.NonKeyAttributes
266-
} else {
267-
projection.NonKeyAttributes = []*string{}
268266
}
269267
} else {
270268
projection.ProjectionType = aws.String("")
271-
projection.NonKeyAttributes = []*string{}
269+
projection.NonKeyAttributes = nil
272270
}
273271
return projection
274272
}

‎pkg/resource/table/hooks_global_secondary_indexes_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package table
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
7+
"github.com/aws/aws-sdk-go/aws"
8+
svcsdk "github.com/aws/aws-sdk-go/service/dynamodb"
9+
10+
"github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1"
11+
)
12+
13+
func Test_newSDKProvisionedThroughput(t *testing.T) {
14+
type args struct {
15+
pt *v1alpha1.ProvisionedThroughput
16+
}
17+
tests := []struct {
18+
name string
19+
args args
20+
want *svcsdk.ProvisionedThroughput
21+
}{
22+
{
23+
name: "provisioned throughput is nil",
24+
args: args{
25+
pt: nil,
26+
},
27+
want: nil,
28+
},
29+
{
30+
name: "provisioned throughput is not nil, read capacity units is nil",
31+
args: args{
32+
pt: &v1alpha1.ProvisionedThroughput{
33+
ReadCapacityUnits: nil,
34+
WriteCapacityUnits: aws.Int64(10),
35+
},
36+
},
37+
want: &svcsdk.ProvisionedThroughput{
38+
ReadCapacityUnits: aws.Int64(1),
39+
WriteCapacityUnits: aws.Int64(10),
40+
},
41+
},
42+
{
43+
name: "provisioned throughput is not nil, write capacity units is nil",
44+
args: args{
45+
pt: &v1alpha1.ProvisionedThroughput{
46+
ReadCapacityUnits: aws.Int64(10),
47+
WriteCapacityUnits: nil,
48+
},
49+
},
50+
want: &svcsdk.ProvisionedThroughput{
51+
ReadCapacityUnits: aws.Int64(10),
52+
WriteCapacityUnits: aws.Int64(1),
53+
},
54+
},
55+
{
56+
name: "provisioned throughput is not nil, write and read capacity units are not nil",
57+
args: args{
58+
pt: &v1alpha1.ProvisionedThroughput{
59+
ReadCapacityUnits: aws.Int64(5),
60+
WriteCapacityUnits: aws.Int64(5),
61+
},
62+
},
63+
want: &svcsdk.ProvisionedThroughput{
64+
ReadCapacityUnits: aws.Int64(5),
65+
WriteCapacityUnits: aws.Int64(5),
66+
},
67+
},
68+
}
69+
for _, tt := range tests {
70+
t.Run(tt.name, func(t *testing.T) {
71+
if got := newSDKProvisionedThroughput(tt.args.pt); !reflect.DeepEqual(got, tt.want) {
72+
t.Errorf("newSDKProvisionedThroughput() = %v, want %v", got, tt.want)
73+
}
74+
})
75+
}
76+
}

‎pkg/resource/table/sdk.go‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎templates/hooks/table/sdk_read_one_post_set_output.go.tpl‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@
5959
if isTableUpdating(&resource{ko}) {
6060
return &resource{ko}, requeueWaitWhileUpdating
6161
}
62+
if !canUpdateTableGSIs(&resource{ko}) {
63+
return &resource{ko}, requeueWaitGSIReady
64+
}
6265
if err := rm.setResourceAdditionalFields(ctx, ko); err != nil {
6366
return nil, err
6467
}

‎test/e2e/resources/table_basic_pay_per_request.yaml‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# Table used to test GSI creation under on-demand billing mode
2+
apiVersion: dynamodb.services.k8s.aws/v1alpha1
3+
kind: Table
4+
metadata:
5+
name: $TABLE_NAME
6+
spec:
7+
tableName: $TABLE_NAME
8+
billingMode: PAY_PER_REQUEST
9+
tableClass: STANDARD
10+
attributeDefinitions:
11+
- attributeName: Bill
12+
attributeType: S
13+
- attributeName: Total
14+
attributeType: S
15+
keySchema:
16+
- attributeName: Bill
17+
keyType: HASH
18+
- attributeName: Total
19+
keyType: RANGE

‎test/e2e/table.py‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,16 @@ def __init__(self, gsis):
166166
self.match_on = gsis
167167

168168
def __call__(self, record: dict) -> bool:
169+
gsi_key = "GlobalSecondaryIndexes"
169170
if len(self.match_on) == 0:
170-
return (not 'GlobalSecondaryIndexes' in record) or len(record["GlobalSecondaryIndexes"] == 0)
171+
return (gsi_key not in record) or len(record[gsi_key]) == 0
171172

172-
awsGSIs = record["GlobalSecondaryIndexes"]
173-
if len(self.match_on) != len(record["GlobalSecondaryIndexes"]):
173+
# if GSI is still in creating status , it will not be present in the record
174+
if gsi_key not in record:
175+
return False
176+
177+
awsGSIs = record[gsi_key]
178+
if len(self.match_on) != len(record[gsi_key]):
174179
return False
175180

176181
for awsGSI in awsGSIs:

‎test/e2e/tests/test_table.py‎

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,18 @@ def table_basic():
140140
except:
141141
pass
142142

143+
@pytest.fixture(scope="module")
144+
def table_basic_pay_per_request():
145+
resource_name = random_suffix_name("table-basic-pay-per-request", 32)
146+
(ref, cr) = create_table(resource_name, "table_basic_pay_per_request")
147+
148+
yield ref, cr
149+
try:
150+
_, deleted = k8s.delete_custom_resource(ref, wait_periods=3, period_length=10)
151+
assert deleted
152+
except:
153+
pass
154+
143155
@service_marker
144156
@pytest.mark.canary
145157
class TestTable:
@@ -726,7 +738,7 @@ def test_multi_updates(self, table_gsi):
726738
"readCapacityUnits": 10,
727739
"writeCapacityUnits": 10
728740
}
729-
cr["spec"][ "sseSpecification"] = {
741+
cr["spec"]["sseSpecification"] = {
730742
"enabled": True,
731743
"sseType": "KMS"
732744
}
@@ -750,7 +762,11 @@ def test_multi_updates(self, table_gsi):
750762
# GSI is the last element to get update in the code path... so we just wait for it
751763
# to know that all the fields got updated.
752764

765+
# encounter an issue when running E2E test locally, sometimes the gsi is updated,
766+
# but SSEDescription is still being updated, add 2mins to wait (Julian Chu)
767+
time.sleep(120)
753768
latestTable = table.get(table_name)
769+
logging.info("latestTable: %s", latestTable)
754770
assert latestTable["StreamSpecification"] is not None
755771
assert latestTable["StreamSpecification"]["StreamEnabled"]
756772

@@ -760,3 +776,66 @@ def test_multi_updates(self, table_gsi):
760776
assert latestTable["ProvisionedThroughput"] is not None
761777
assert latestTable["ProvisionedThroughput"]["ReadCapacityUnits"] == 10
762778
assert latestTable["ProvisionedThroughput"]["WriteCapacityUnits"] == 10
779+
780+
def test_create_gsi_pay_per_request(self, table_basic_pay_per_request):
781+
(ref, res) = table_basic_pay_per_request
782+
783+
table_name = res["spec"]["tableName"]
784+
785+
# Check DynamoDB Table exists
786+
assert self.table_exists(table_name)
787+
788+
# Get CR latest revision
789+
cr = k8s.wait_resource_consumed_by_controller(ref)
790+
791+
# Creating two more GSIs
792+
cr["spec"]["attributeDefinitions"] = [
793+
{
794+
"attributeName": "Bill",
795+
"attributeType": "S"
796+
},
797+
{
798+
"attributeName": "Total",
799+
"attributeType": "S"
800+
},
801+
{
802+
"attributeName": "User",
803+
"attributeType": "S"
804+
},
805+
]
806+
807+
gsi = {
808+
"indexName": "bill-per-user",
809+
"keySchema": [
810+
{
811+
"attributeName": "User",
812+
"keyType": "HASH",
813+
},
814+
{
815+
"attributeName": "Bill",
816+
"keyType": "RANGE",
817+
}
818+
],
819+
"projection": {
820+
"projectionType": "ALL",
821+
}
822+
}
823+
824+
cr["spec"]['globalSecondaryIndexes'] = [
825+
gsi,
826+
]
827+
828+
# Patch k8s resource
829+
k8s.patch_custom_resource(ref, cr)
830+
k8s.wait_resource_consumed_by_controller(ref)
831+
table.wait_until(
832+
table_name,
833+
table.gsi_matches(
834+
[
835+
gsi
836+
],
837+
),
838+
timeout_seconds=MODIFY_WAIT_AFTER_SECONDS*40,
839+
interval_seconds=15,
840+
)
841+

0 commit comments

Comments
 (0)