Ensure Synced Condition on DB instance/cluster (#45)

Adds tests that the ACK.ResourceSynced Condition is present on resources
of kind DBCluster and DBInstance and that the status of those conditions
are either False or True depending on the expected state transition of
the DB instance or cluster.

This involved creating some helper functions in test/e2e/condition.py
that assert that a Condition of a specific type and status is present in
a resource's Status.Conditions collection.

In grep'ing through logs for the rds-controller, I noticed that the
AvailabilityZone field for the DBInstance should be late-initialized and
so added that to the generator.yaml file.

Finally, found that ACK.ResourceSynced/True was not being set on
DBCluster and DBInstance resources when the status of the instance or
cluster made its way to 'available'. I added an else block to the
generic hook code templates to ensure that ACK.ResourceSynced condition
with a status of True is present when the resource's status transitions
to 'available'.

Signed-off-by: Jay Pipes <[email protected]>

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

Author: jaypipes

generator.yaml

@@ -144,6 +144,8 @@ resources:
         - DBSubnetGroupNotFoundFault
         - DBParameterGroupNotFound
     fields:
+      AvailabilityZone:
+        late_initialize: {}
       DBInstanceIdentifier:
         is_primary_key: true
       MasterUserPassword:

pkg/resource/db_cluster/sdk.go

@@ -2,6 +2,7 @@
 		// Setting resource synced condition to false will trigger a requeue of
 		// the resource. No need to return a requeue error here.
 		setSyncedCondition(&resource{ko}, corev1.ConditionFalse, nil, nil)
-		return &resource{ko}, nil
+	} else {
+		setSyncedCondition(&resource{ko}, corev1.ConditionTrue, nil, nil)
 	}
 

pkg/resource/db_instance/manager.go

@@ -2,5 +2,7 @@
 		// Setting resource synced condition to false will trigger a requeue of
 		// the resource. No need to return a requeue error here.
 		setSyncedCondition(&resource{ko}, corev1.ConditionFalse, nil, nil)
-		return &resource{ko}, nil
-	}
+	} else {
+		setSyncedCondition(&resource{ko}, corev1.ConditionTrue, nil, nil)
+    }
+

pkg/resource/db_instance/sdk.go

@@ -14,3 +14,8 @@
 		setSyncedCondition(desired, corev1.ConditionTrue, nil, nil)
 		return desired, nil
 	}
+	if !instanceAvailable(latest) {
+		msg := "DB instance cannot be modifed while in '" + *latest.ko.Status.DBInstanceStatus + "' status"
+		setSyncedCondition(desired, corev1.ConditionFalse, &msg, nil)
+		return desired, requeueWaitUntilCanModify(latest)
+	}

templates/hooks/db_cluster/sdk_read_many_post_set_output.go.tpl

@@ -0,0 +1,145 @@
+# 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.
+
+"""Utility functions to help processing Kubernetes resource conditions"""
+
+# TODO(jaypipes): Move these functions to acktest library. The reason these are
+# here is because the existing k8s.assert_condition_state_message doesn't
+# actually assert anything. It returns true or false and logs messages.
+
+import pytest
+
+from acktest.k8s import resource as k8s
+
+CONDITION_TYPE_ADOPTED = "ACK.Adopted"
+CONDITION_TYPE_RESOURCE_SYNCED = "ACK.ResourceSynced"
+CONDITION_TYPE_TERMINAL = "ACK.Terminal"
+CONDITION_TYPE_RECOVERABLE = "ACK.Recoverable"
+CONDITION_TYPE_ADVISORY = "ACK.Advisory"
+CONDITION_TYPE_LATE_INITIALIZED = "ACK.LateInitialized"
+
+
+def assert_type_status(
+    ref: k8s.CustomResourceReference,
+    cond_type_match: str = CONDITION_TYPE_RESOURCE_SYNCED,
+    cond_status_match: bool = True,
+):
+    """Asserts that the supplied resource has a condition of type
+    ACK.ResourceSynced and that the Status of this condition is True.
+
+    Usage:
+        from acktest.k8s import resource as k8s
+
+        from e2e import condition
+
+        ref = k8s.CustomResourceReference(
+            CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
+            db_cluster_id, namespace="default",
+        )
+        k8s.create_custom_resource(ref, resource_data)
+        k8s.wait_resource_consumed_by_controller(ref)
+        condition.assert_type_status(
+            ref,
+            condition.CONDITION_TYPE_RESOURCE_SYNCED,
+            False)
+
+    Raises:
+        pytest.fail when condition of the specified type is not found or is not
+        in the supplied status.
+    """
+    cond = k8s.get_resource_condition(ref, cond_type_match)
+    if cond is None:
+        msg = (f"Failed to find {cond_type_match} condition in "
+               f"resource {ref}")
+        pytest.fail(msg)
+
+    cond_status = cond.get('status', None)
+    if str(cond_status) != str(cond_status_match):
+        msg = (f"Expected {cond_type_match} condition to "
+               f"have status {cond_status_match} but found {cond_status}")
+        pytest.fail(msg)
+
+
+def assert_synced_status(
+    ref: k8s.CustomResourceReference,
+    cond_status_match: bool,
+):
+    """Asserts that the supplied resource has a condition of type
+    ACK.ResourceSynced and that the Status of this condition is True.
+
+    Usage:
+        from acktest.k8s import resource as k8s
+
+        from e2e import condition
+
+        ref = k8s.CustomResourceReference(
+            CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
+            db_cluster_id, namespace="default",
+        )
+        k8s.create_custom_resource(ref, resource_data)
+        k8s.wait_resource_consumed_by_controller(ref)
+        condition.assert_synced_status(ref, False)
+
+    Raises:
+        pytest.fail when ACK.ResourceSynced condition is not found or is not in
+        a True status.
+    """
+    assert_type_status(ref, CONDITION_TYPE_RESOURCE_SYNCED, cond_status_match)
+
+
+def assert_synced(ref: k8s.CustomResourceReference):
+    """Asserts that the supplied resource has a condition of type
+    ACK.ResourceSynced and that the Status of this condition is True.
+
+    Usage:
+        from acktest.k8s import resource as k8s
+
+        from e2e import condition
+
+        ref = k8s.CustomResourceReference(
+            CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
+            db_cluster_id, namespace="default",
+        )
+        k8s.create_custom_resource(ref, resource_data)
+        k8s.wait_resource_consumed_by_controller(ref)
+        condition.assert_synced(ref)
+
+    Raises:
+        pytest.fail when ACK.ResourceSynced condition is not found or is not in
+        a True status.
+    """
+    return assert_synced_status(ref, True)
+
+
+def assert_not_synced(ref: k8s.CustomResourceReference):
+    """Asserts that the supplied resource has a condition of type
+    ACK.ResourceSynced and that the Status of this condition is False.
+
+    Usage:
+        from acktest.k8s import resource as k8s
+
+        from e2e import condition
+
+        ref = k8s.CustomResourceReference(
+            CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
+            db_cluster_id, namespace="default",
+        )
+        k8s.create_custom_resource(ref, resource_data)
+        k8s.wait_resource_consumed_by_controller(ref)
+        condition.assert_not_synced(ref)
+
+    Raises:
+        pytest.fail when ACK.ResourceSynced condition is not found or is not in
+        a False status.
+    """
+    return assert_synced_status(ref, False)

templates/hooks/db_instance/sdk_read_many_post_set_output.go.tpl

@@ -22,16 +22,32 @@
 from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_rds_resource
 from e2e.replacement_values import REPLACEMENT_VALUES
 from e2e.bootstrap_resources import get_bootstrap_resources
-from e2e.fixtures import k8s_secret
+from e2e import condition
 from e2e import db_cluster
+from e2e.fixtures import k8s_secret
 
 RESOURCE_PLURAL = 'dbclusters'
 
 DELETE_WAIT_AFTER_SECONDS = 120
 
 # Time we wait after resource becoming available in RDS and checking the CR's
-# Status has been updated
-CHECK_STATUS_WAIT_SECONDS = 20
+# Status has been updated.
+#
+# NOTE(jaypipes): I have witnessed DB clusters transition from creating ->
+# available -> creating again :(
+#
+# This is rare, but seems to be when RDS schedules a snapshot shortly after a
+# DB cluster is created. The cluster gets to 'available', then RDS pauses the
+# DB cluster, does some sort of snapshotting operation, then unpauses the DB
+# cluster. It seems that during this unpausing operation, the DB cluster's
+# status gets set back to 'creating' again.
+#
+# Unfortunately, because Aurora serverless doesn't emit Events for creation and
+# deletion like it does for DB instances, this is difficult, if not impossible,
+# to gather data on. Setting the check wait seconds here to 2 minutes since I
+# believe this whole "pause-then-snapshot-then-unpause" thing only takes about
+# a minute until the DB cluster status settles into an 'available' state.
+CHECK_STATUS_WAIT_SECONDS = 60*2
 
 MODIFY_WAIT_AFTER_SECONDS = 20
 
@@ -83,6 +99,7 @@ def test_crud_mysql_serverless(
         assert 'status' in cr
         assert 'status' in cr['status']
         assert cr['status']['status'] == 'creating'
+        condition.assert_not_synced(ref)
 
         db_cluster.wait_until(
             db_cluster_id,
@@ -104,6 +121,7 @@ def test_crud_mysql_serverless(
         assert 'status' in cr
         assert 'status' in cr['status']
         assert cr['status']['status'] != 'creating'
+        condition.assert_synced(ref)
 
         # We're now going to modify the CopyTagsToSnapshot field of the DB
         # instance, wait some time and verify that the RDS server-side resource

templates/hooks/db_instance/sdk_update_pre_build_request.go.tpl

@@ -22,16 +22,33 @@
 from e2e import service_marker, CRD_GROUP, CRD_VERSION, load_rds_resource
 from e2e.replacement_values import REPLACEMENT_VALUES
 from e2e.bootstrap_resources import get_bootstrap_resources
-from e2e.fixtures import k8s_secret
+from e2e import condition
 from e2e import db_instance
+from e2e.fixtures import k8s_secret
 
 RESOURCE_PLURAL = 'dbinstances'
 
-DELETE_WAIT_AFTER_SECONDS = 120
+DELETE_WAIT_AFTER_SECONDS = 60*2
 
 # Time we wait after resource becoming available in RDS and checking the CR's
-# Status has been updated
-CHECK_STATUS_WAIT_SECONDS = 20
+# Status has been updated.
+#
+# NOTE(jaypipes): RDS does an automated backup as soon as a DB instance is
+# created. This automated backup can take 2-3 minutes, during which time the DB
+# instance's status will be 'backing-up'. In addition, I have noticed that
+# sometimes RDS will reset master credentials after doing the initial snapshot
+# backup, and this involves restarting the DB instance. This itself can take an
+# additional 2-3 minutes.
+#
+# What this means is that the DB instance goes through the following status
+# transitions:
+#
+# creating -> available -> backing-up -> available ->
+# resetting-master-credentials -> restarting -> available
+#
+# This can take upwards of 7 minutes for the the DB instance to reach that
+# "final" available state
+CHECK_STATUS_WAIT_SECONDS = 60*8
 
 MODIFY_WAIT_AFTER_SECONDS = 20
 
@@ -82,6 +99,7 @@ def test_crud_postgres13_t3_micro(
         assert 'status' in cr
         assert 'dbInstanceStatus' in cr['status']
         assert cr['status']['dbInstanceStatus'] == 'creating'
+        condition.assert_not_synced(ref)
 
         db_instance.wait_until(
             db_instance_id,
@@ -103,6 +121,7 @@ def test_crud_postgres13_t3_micro(
         assert 'status' in cr
         assert 'dbInstanceStatus' in cr['status']
         assert cr['status']['dbInstanceStatus'] != 'creating'
+        condition.assert_synced(ref)
 
         # We're now going to modify the CopyTagsToSnapshot field of the DB
         # instance, wait some time and verify that the RDS server-side resource