handle asynchronous updates for DBInstance by requeueing (#65)

vijtrip2 · web-flow · commit 1dfe7bdb0355 · 2022-03-14T07:40:21.000-07:00
Issue [#1156](aws-controllers-k8s/community#1156) Description of changes: * During DBInstance update, when `ModifyDBInstance` call returns success then the ACK reconciler marks the resource as Synced and the reconciliation stops. However asynchronous update of DBInstance happens on server side and the ACK reconciler does not sync them. * Add two new hooks, one to requeue after successful update and another to handle delta for non-late-initialized fields. * I did not use the `requeueOnSuccessSeconds` construct because it will cause the reconciliation to go forever and for some amount of time, the resource condition will be synced, even when the DBInstance is in modifying state. * Do not set Synced condition true inside post_read_many hook, but depend on `ensureConditions` method **Test Updates** * Update test manifest to use smaller storage to improve the test time and use engine version 14.1 because engine version 13.2 was throwing unsupported errors * Added e2e test for 'MultiAZ' field update as mentioned in the issue [#1156](aws-controllers-k8s/community#1156) * Updated the test to wait for Synced condition and then assert DBInstance fields using AWS API calls By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml
@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2022-03-02T19:31:05Z"
+  build_date: "2022-03-05T00:11:28Z"
   build_hash: ade2429bb444ab635916395ea5773d141ba135e1
   go_version: go1.17.5
   version: v0.17.2
 api_directory_checksum: c5762d0b5707ca20866f2f0e85bc23863733ca11
 api_version: v1alpha1
 aws_sdk_go_version: v1.42.0
 generator_config_info:
-  file_checksum: a0cccebb7e683a3b9b6082b5cde6630fd5ad4237
+  file_checksum: 3afea95265e7836112e0b1e37166290f18511d09
   original_file_name: generator.yaml
 last_modification:
   reason: API generation
diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml
@@ -122,8 +122,12 @@ resources:
         template_path: hooks/db_instance/sdk_update_pre_build_request.go.tpl
       sdk_update_post_build_request:
         template_path: hooks/db_instance/sdk_update_post_build_request.go.tpl
+      sdk_update_post_set_output:
+        template_path: hooks/db_instance/sdk_update_post_set_output.go.tpl
       sdk_delete_pre_build_request:
         template_path: hooks/db_instance/sdk_delete_pre_build_request.go.tpl
+      delta_pre_compare:
+        template_path: hooks/db_instance/delta_pre_compare.go.tpl
     exceptions:
       terminal_codes:
         - InvalidParameter
diff --git a/generator.yaml b/generator.yaml
@@ -122,8 +122,12 @@ resources:
         template_path: hooks/db_instance/sdk_update_pre_build_request.go.tpl
       sdk_update_post_build_request:
         template_path: hooks/db_instance/sdk_update_post_build_request.go.tpl
+      sdk_update_post_set_output:
+        template_path: hooks/db_instance/sdk_update_post_set_output.go.tpl
       sdk_delete_pre_build_request:
         template_path: hooks/db_instance/sdk_delete_pre_build_request.go.tpl
+      delta_pre_compare:
+        template_path: hooks/db_instance/delta_pre_compare.go.tpl
     exceptions:
       terminal_codes:
         - InvalidParameter
diff --git a/pkg/resource/db_instance/delta.go b/pkg/resource/db_instance/delta.go
diff --git a/pkg/resource/db_instance/hooks.go b/pkg/resource/db_instance/hooks.go
@@ -90,6 +90,12 @@ var (
 		errors.New("DB instance in 'deleting' state, cannot be modified or deleted."),
 		ackrequeue.DefaultRequeueAfterDuration,
 	)
+
+	customWaitAtferUpdate = ackrequeue.NeededAfter(
+		errors.New("Requeueing resource after successful update; status fields "+
+			"retrieved asynchronously"),
+		ackrequeue.DefaultRequeueAfterDuration,
+	)
 )
 
 // requeueWaitUntilCanModify returns a `ackrequeue.RequeueNeededAfter` struct
diff --git a/pkg/resource/db_instance/sdk.go b/pkg/resource/db_instance/sdk.go
diff --git a/templates/hooks/db_instance/delta_pre_compare.go.tpl b/templates/hooks/db_instance/delta_pre_compare.go.tpl
@@ -0,0 +1,12 @@
+    // Do not consider AvailabilityZone field for delta if it is missing in
+    // desired(a) and is present in latest(b) because AvailabilityZone field is
+    // late-initialized
+    // This special handling is only needed for DBInstance because a requeue
+    // error needs to be returned even after successful ModifyDBInstance call.
+    // See sdk_update_post_set_output.go.tpl for more details.
+    // The requeue error from update prevents the late initialization in
+    // reconciler.go and without ignoring AvailabilityZone for delta , the
+    // reconciler will keep updating the dbinstance and constantly requeueing it.
+    if a != nil && a.ko.Spec.AvailabilityZone == nil && b != nil && b.ko.Spec.AvailabilityZone != nil {
+        a.ko.Spec.AvailabilityZone = b.ko.Spec.AvailabilityZone
+    }
diff --git a/templates/hooks/db_instance/sdk_read_many_post_set_output.go.tpl b/templates/hooks/db_instance/sdk_read_many_post_set_output.go.tpl
@@ -2,7 +2,5 @@
 		// Setting resource synced condition to false will trigger a requeue of
 		// the resource. No need to return a requeue error here.
 		ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, nil, nil)
-	} else {
-		ackcondition.SetSynced(&resource{ko}, corev1.ConditionTrue, nil, nil)
-    }
+	}
 
diff --git a/templates/hooks/db_instance/sdk_update_post_set_output.go.tpl b/templates/hooks/db_instance/sdk_update_post_set_output.go.tpl
@@ -0,0 +1,7 @@
+    // If there was no error during update, requeue the request.
+    // When ModifyDBInstance API is successful, it asynchronously
+    // updates the DBInstanceStatus. Requeue to find the current
+    // DBInstance status and set Synced condition accordingly
+    if err == nil {
+        return &resource{ko}, customWaitAtferUpdate
+    }
diff --git a/test/e2e/resources/db_instance_postgres14_t3_micro.yaml b/test/e2e/resources/db_instance_postgres14_t3_micro.yaml
@@ -3,15 +3,16 @@ kind: DBInstance
 metadata:
   name: $DB_INSTANCE_ID
 spec:
-  allocatedStorage: 20
+  allocatedStorage: 5
   copyTagsToSnapshot: $COPY_TAGS_TO_SNAPSHOT
   dbInstanceClass: db.t3.micro
   dbInstanceIdentifier: $DB_INSTANCE_ID
   dbSubnetGroupName: $DB_SUBNET_GROUP_NAME
   engine: postgres
-  engineVersion: "13.2"
+  engineVersion: "14.1"
   masterUsername: root
   masterUserPassword:
     namespace: $MASTER_USER_PASS_SECRET_NAMESPACE
     name: $MASTER_USER_PASS_SECRET_NAME
     key: $MASTER_USER_PASS_SECRET_KEY
+  multiAZ: False
diff --git a/test/e2e/tests/test_db_instance.py b/test/e2e/tests/test_db_instance.py
@@ -49,9 +49,9 @@
 #
 # This can take upwards of 7 minutes for the the DB instance to reach that
 # "final" available state
-CHECK_STATUS_WAIT_SECONDS = 60*8
+MAX_WAIT_FOR_SYNCED_MINUTES = 20
 
-MODIFY_WAIT_AFTER_SECONDS = 20
+MODIFY_WAIT_AFTER_SECONDS = 60
 
 
 @service_marker
@@ -64,11 +64,11 @@ class TestDBInstance:
     MUP_SEC_KEY = "master_user_password"
     MUP_SEC_VAL = "secretpass123456"
 
-    def test_crud_postgres13_t3_micro(
+    def test_crud_postgres14_t3_micro(
             self,
             k8s_secret,
     ):
-        db_instance_id = random_suffix_name("pg13-t3-micro", 20)
+        db_instance_id = random_suffix_name("pg14-t3-micro", 20)
         secret = k8s_secret(
             self.MUP_NS,
             self.MUP_SEC_NAME,
@@ -85,7 +85,7 @@ def test_crud_postgres13_t3_micro(
         replacements["DB_SUBNET_GROUP_NAME"] = get_bootstrap_resources().DBSubnetGroupName
 
         resource_data = load_rds_resource(
-            "db_instance_postgres13_t3_micro",
+            "db_instance_postgres14_t3_micro",
             additional_replacements=replacements,
         )
 
@@ -103,12 +103,14 @@ def test_crud_postgres13_t3_micro(
         assert cr['status']['dbInstanceStatus'] == 'creating'
         condition.assert_not_synced(ref)
 
-        db_instance.wait_until(
-            db_instance_id,
-            db_instance.status_matches('available'),
-        )
+        # Wait for the resource to get synced
+        assert k8s.wait_on_condition(ref, "ACK.ResourceSynced", "True", wait_periods=MAX_WAIT_FOR_SYNCED_MINUTES)
 
-        time.sleep(CHECK_STATUS_WAIT_SECONDS)
+        # After the resource is synced, assert that DBInstanceStatus is available
+        latest = db_instance.get(db_instance_id)
+        assert latest is not None
+        assert latest['DBInstanceStatus'] == 'available'
+        assert latest['MultiAZ'] is False
 
         # Before we update the DBInstance CR below, let's check to see that the
         # DbInstanceStatus field in the CR has been updated to something other
@@ -133,14 +135,34 @@ def test_crud_postgres13_t3_micro(
         assert latest['CopyTagsToSnapshot'] is False
         assert latest['DBSubnetGroup']['DBSubnetGroupName'] == get_bootstrap_resources().DBSubnetGroupName
         updates = {
-            "spec": {"copyTagsToSnapshot": True},
+            "spec": {"copyTagsToSnapshot": True, "multiAZ": True},
+        }
+        k8s.patch_custom_resource(ref, updates)
+        time.sleep(MODIFY_WAIT_AFTER_SECONDS)
+
+        # wait for the resource to get synced after the patch
+        assert k8s.wait_on_condition(ref, "ACK.ResourceSynced", "True", wait_periods=MAX_WAIT_FOR_SYNCED_MINUTES)
+
+        # After resource is synced again, assert that patches are reflected in the AWS resource
+        latest = db_instance.get(db_instance_id)
+        assert latest is not None
+        assert latest['CopyTagsToSnapshot'] is True
+        assert latest['MultiAZ'] is True
+
+        updates = {
+            "spec": {"copyTagsToSnapshot": False, "multiAZ": False},
         }
         k8s.patch_custom_resource(ref, updates)
         time.sleep(MODIFY_WAIT_AFTER_SECONDS)
 
+        # wait for the resource to get synced after the patch
+        assert k8s.wait_on_condition(ref, "ACK.ResourceSynced", "True", wait_periods=MAX_WAIT_FOR_SYNCED_MINUTES)
+
+        # After resource is synced again, assert that patches are reflected in the AWS resource
         latest = db_instance.get(db_instance_id)
         assert latest is not None
-        assert latest['CopyTagsToSnapshot'] == True
+        assert latest['CopyTagsToSnapshot'] is False
+        assert latest['MultiAZ'] is False
 
         k8s.delete_custom_resource(ref)
 

Original file line number	Diff line number	Diff line change
`@@ -90,6 +90,12 @@ var (`
`90`	`90`	`errors.New("DB instance in 'deleting' state, cannot be modified or deleted."),`
`91`	`91`	`ackrequeue.DefaultRequeueAfterDuration,`
`92`	`92`	`)`
	`93`	`+`
	`94`	`+ customWaitAtferUpdate = ackrequeue.NeededAfter(`
	`95`	`+ errors.New("Requeueing resource after successful update; status fields "+`
	`96`	`+ "retrieved asynchronously"),`
	`97`	`+ ackrequeue.DefaultRequeueAfterDuration,`
	`98`	`+ )`
`93`	`99`	`)`
`94`	`100`
`95`	`101`	// requeueWaitUntilCanModify returns a `ackrequeue.RequeueNeededAfter` struct