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

Add Table resource custom hooks, terminalCodes and e2e tests #3

Merged
merged 1 commit into from
Jun 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@ resources:
errors:
404:
code: ResourceNotFoundException
terminal_codes:
- InternalServerError
- LimitExceededException
- ResourceInUseException
hooks:
sdk_read_one_post_set_output:
template_path: hooks/table/sdk_read_one_post_set_output.go.tpl
sdk_update_pre_build_request:
template_path: hooks/table/sdk_update_pre_build_request.go.tpl
sdk_delete_pre_build_request:
template_path: hooks/table/sdk_delete_pre_build_request.go.tpl
GlobalTable:
exceptions:
errors:
Expand Down
102 changes: 102 additions & 0 deletions pkg/resource/table/conditions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package table

import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
)

// getSyncedCondition returns the Condition in the resource's Conditions
// collection that is of type ConditionTypeResourceSynced. If no such condition
// is found, returns nil.
//
// TODO(jaypipes): Move to ACK code-gen templates.
Copy link
Contributor

Choose a reason for hiding this comment

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

?? Are these files not auto generated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, I guess we still need to experiment with other controllers to figure out a general design

/cc @jaypipes

Copy link
Collaborator

Choose a reason for hiding this comment

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

@RedbackThomson yeah, eventually I'd like to generate functions like these and/or move into ACK runtime. I added these for the mq-controller:

https://github.com/aws-controllers-k8s/mq-controller/blob/main/pkg/resource/broker/conditions.go
https://github.com/aws-controllers-k8s/mq-controller/blob/main/pkg/resource/broker/hooks.go

and then adapted them for the RDS controller:

https://github.com/aws-controllers-k8s/rds-controller/blob/main/pkg/resource/db_instance/conditions.go
https://github.com/aws-controllers-k8s/rds-controller/blob/main/pkg/resource/db_instance/hooks.go

I use the service controllers as a bit of a laboratory for experimentation in this way while I figure out what works as a standard.

Copy link
Member

Choose a reason for hiding this comment

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

3 controllers using same code, good time to move ?
aws-controllers-k8s/community#806

Copy link
Collaborator

Choose a reason for hiding this comment

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

3 controllers using same code, good time to move ?
aws-controllers-k8s/community#806

after I get those logging changes pushed up, yeah...

func getSyncedCondition(r *resource) *ackv1alpha1.Condition {
return getConditionOfType(r, ackv1alpha1.ConditionTypeResourceSynced)
}

// getTerminalCondition returns the Condition in the resource's Conditions
// collection that is of type ConditionTypeTerminal. If no such condition is
// found, returns nil.
//
// TODO(jaypipes): Move to ACK code-gen templates.
func getTerminalCondition(r *resource) *ackv1alpha1.Condition {
return getConditionOfType(r, ackv1alpha1.ConditionTypeTerminal)
}

// getConditionOfType returns the Condition in the resource's Conditions
// collection of the supplied type. If no such condition is found, returns nil.
//
// TODO(jaypipes): Move to ACK code-gen templates.
func getConditionOfType(
r *resource,
condType ackv1alpha1.ConditionType,
) *ackv1alpha1.Condition {
for _, condition := range r.ko.Status.Conditions {
if condition.Type == condType {
return condition
}
}
return nil
}

// setSyncedCondition sets the resource's Condition of type
// ConditionTypeResourceSynced to the supplied status, optional message and
// reason.
//
// TODO(jaypipes): Move to ACK code-gen templates.
func setSyncedCondition(
r *resource,
status corev1.ConditionStatus,
message *string,
reason *string,
) {
c := getSyncedCondition(r)
if c == nil {
c = &ackv1alpha1.Condition{
Type: ackv1alpha1.ConditionTypeResourceSynced,
}
r.ko.Status.Conditions = append(r.ko.Status.Conditions, c)
}
now := metav1.Now()
c.LastTransitionTime = &now
c.Status = status
}

// setTerminalCondition sets the resource's Condition of type
// ConditionTypeTerminal to the supplied status, optional message and reason.
//
// TODO(jaypipes): Move to ACK code-gen templates.
func setTerminalCondition(
r *resource,
status corev1.ConditionStatus,
message *string,
reason *string,
) {
c := getTerminalCondition(r)
if c == nil {
c = &ackv1alpha1.Condition{
Type: ackv1alpha1.ConditionTypeTerminal,
}
r.ko.Status.Conditions = append(r.ko.Status.Conditions, c)
}
now := metav1.Now()
c.LastTransitionTime = &now
c.Status = status
c.Message = message
c.Reason = reason
}
97 changes: 97 additions & 0 deletions pkg/resource/table/hooks.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package table

import (
"errors"
"time"

"github.com/aws-controllers-k8s/dynamodb-controller/apis/v1alpha1"
ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
)

var (
ErrTableDeleting = errors.New("Table in 'DELETING' state, cannot be modified or deleted")
ErrTableCreating = errors.New("Table in 'CREATING' state, cannot be modified or deleted")
ErrTableUpdating = errors.New("Table in 'UPDATING' state, cannot be modified or deleted")
)

var (
// TerminalStatuses are the status strings that are terminal states for a
// DynamoDB table
TerminalStatuses = []v1alpha1.TableStatus_SDK{
v1alpha1.TableStatus_SDK_ARCHIVING,
v1alpha1.TableStatus_SDK_DELETING,
}
)

var (
requeueWaitWhileDeleting = ackrequeue.NeededAfter(
ErrTableDeleting,
5*time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an arbitrary amount? I'd like to see requeue times standardised over all controllers, perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not arbitrary, just remarked that tables are created super fast. The default value is 30seconds https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/requeue/requeue.go#L20-L22

)
requeueWaitWhileCreating = ackrequeue.NeededAfter(
ErrTableDeleting,
5*time.Second,
)
requeueWaitWhileUpdating = ackrequeue.NeededAfter(
ErrTableUpdating,
5*time.Second,
)
)

// tableHasTerminalStatus returns whether the supplied Dynamodb table is in a
// terminal state
func tableHasTerminalStatus(r *resource) bool {
if r.ko.Status.TableStatus == nil {
return false
}
ts := *r.ko.Status.TableStatus
for _, s := range TerminalStatuses {
if ts == string(s) {
return true
}
}
return false
}

// isTableCreating returns true if the supplied DynamodbDB table is in the process
// of being created
func isTableCreating(r *resource) bool {
if r.ko.Status.TableStatus == nil {
return false
}
dbis := *r.ko.Status.TableStatus
return dbis == string(v1alpha1.TableStatus_SDK_CREATING)
}

// isTableDeleting returns true if the supplied DynamodbDB table is in the process
// of being deleted
func isTableDeleting(r *resource) bool {
if r.ko.Status.TableStatus == nil {
return false
}
dbis := *r.ko.Status.TableStatus
return dbis == string(v1alpha1.TableStatus_SDK_DELETING)
}

// isTableUpdating returns true if the supplied DynamodbDB table is in the process
// of being deleted
func isTableUpdating(r *resource) bool {
if r.ko.Status.TableStatus == nil {
return false
}
dbis := *r.ko.Status.TableStatus
return dbis == string(v1alpha1.TableStatus_SDK_UPDATING)
}
50 changes: 48 additions & 2 deletions pkg/resource/table/sdk.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions templates/hooks/table/sdk_delete_pre_build_request.go.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
if isTableDeleting(r) {
return requeueWaitWhileDeleting
}
if isTableUpdating(r) {
return requeueWaitWhileUpdating
}
6 changes: 6 additions & 0 deletions templates/hooks/table/sdk_read_one_post_set_output.go.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
if isTableCreating(&resource{ko}) {
return &resource{ko}, requeueWaitWhileCreating
}
if isTableUpdating(&resource{ko}) {
return &resource{ko}, requeueWaitWhileUpdating
}
Comment on lines +1 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need the above code hooks in the sdkReadOne post-set-output code paths?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes tables needs few seconds to get created. re-queuing here is needed to sync the .status.TableStatus field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here what i get when i create a table and wait for few seconds

k get tables
NAME   TABLENAME   TABLESTATUS
tb1    tb1         CREATING
aws dynamodb describe-table --table-name tb1 | jq
{
    ...
    "TableStatus": "ACTIVE",
    ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fixing a very similar issue to what @phanimullapudi was seeing in mq controller aws-controllers-k8s/community#831

Copy link
Collaborator

Choose a reason for hiding this comment

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

for reviewers: @a-hilaly and I walked through the code and Amine patiently waited for me to understand he was totally correct. :)

21 changes: 21 additions & 0 deletions templates/hooks/table/sdk_update_pre_build_request.go.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
if isTableDeleting(latest) {
msg := "table is currently being deleted"
a-hilaly marked this conversation as resolved.
Show resolved Hide resolved
setSyncedCondition(desired, corev1.ConditionFalse, &msg, nil)
return desired, requeueWaitWhileDeleting
}
if isTableCreating(latest) {
msg := "table is currently being created"
setSyncedCondition(desired, corev1.ConditionFalse, &msg, nil)
return desired, requeueWaitWhileCreating
}
if isTableUpdating(latest) {
msg := "table is currently being updated"
setSyncedCondition(desired, corev1.ConditionFalse, &msg, nil)
return desired, requeueWaitWhileUpdating
}
if tableHasTerminalStatus(latest) {
msg := "table is in '"+*latest.ko.Status.TableStatus+"' status"
setTerminalCondition(desired, corev1.ConditionTrue, &msg, nil)
setSyncedCondition(desired, corev1.ConditionTrue, nil, nil)
return desired, nil
}
3 changes: 3 additions & 0 deletions test/e2e/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
__pycache__/
*.py[cod]
**/bootstrap.yaml
Loading