Declarative Tests for ScalingUpAndDown and Async rollback fixes #67

nmvk · 2021-10-12T18:05:08Z

Declarative tests for ScaleUpAndDown and fixes for Async rollback.

Upon Async rollback we would set the terminal condition to notify the user about the rollback. This would allow user to take the necessary action.

 Conditions:
    Last Transition Time:  2021-11-23T11:45:48Z
    Message:               replication group currently being modified. cannot delete.
    Status:                False
    Type:                  ACK.ResourceSynced
    Message:               InvalidParameterCombination: Cannot update CacheNodeType, Refer to Events for more details
    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.

kumargauravsharma · 2021-10-14T18:05:00Z

test/e2e/scenarios/AddRemoveReplica/cmd_decrease_replica.yaml

+  apiVersion: $CRD_GROUP/$CRD_VERSION
+  kind: ReplicationGroup
+  metadata:
+    name: test$RANDOM_SUFFIX


If the name matches the scenario, it helps in debugging using cli later in case of failure.
Please consider names such as: cmd-dec-rep-$RANDOM_SUFFIX for this scenario

kumargauravsharma · 2021-10-14T18:07:31Z

test/e2e/scenarios/AddRemoveReplica/cmd_decrease_replica_multi_az.yaml

+            primaryAvailabilityZone: "us-west-2b"
+            replicaAvailabilityZones:
+              - "us-west-2c"


Consider supplying these as replacement parameters. It will help in running the scenarios for other Region, AZ with minimal changes.

kumargauravsharma · 2021-10-15T19:00:35Z

test/e2e/declarative_test_fwk/loader.py

+    scenario_files = sorted(os.listdir(scenarios_directory))
+    for scenario_file in scenario_files:
        scenario_file_full_path = join(scenarios_directory, scenario_file)
+        if isdir(scenario_file_full_path):
+            for nested_scenario_file in sorted(os.listdir(scenario_file_full_path)):
+                scenario_files.append(join(scenario_file, nested_scenario_file))
+            continue
+


Consider using glob.glob(scenarios_directory + '**/*.yaml', recursive=True).

vijtrip2 · 2021-10-20T20:55:10Z

/test elasticache-kind-e2e

Declarative tests for ScaleUpAndDown.

nmvk · 2021-11-29T07:55:28Z

/retest

pkg/resource/replication_group/custom_set_output.go

a-hilaly · 2021-11-29T10:22:03Z

pkg/resource/replication_group/custom_update_api.go

+	// This means that we have already attempted to apply the CacheNodeType once and
+	// were not successful hence we will set a terminal condition.
+	if !cacheNodeTypeRequiresUpdate(desired) && delta.DifferentAt("Spec.CacheNodeType") {
+		return nil, awserr.New("InvalidParameterCombination", "Cannot update CacheNodeType, "+


Is there a reason why you are using awserr.New and not errors.New ?

This handles setting terminal codes correctly. Terminal codes are set using awsErr.Code()

a-hilaly · 2021-11-29T10:41:10Z

pkg/resource/replication_group/custom_update_api.go

 	return input
 }
+
+// cacheNodeTypeRequiresUpdate retrieves the last requested cacheNodeType saved in annotations and compares them


if i understand well there will always be only on cacheNodeType saved in the AnnotationLastRequestedCNT and not multiple ones?

This is correct

pkg/resource/replication_group/custom_update_api.go

pkg/resource/replication_group/custom_update_api_test.go

nmvk · 2021-12-01T02:41:44Z

/retest

kumargauravsharma · 2021-12-01T18:42:01Z

pkg/resource/replication_group/custom_set_output.go

+	// Keep the value of desired for CacheNodeType.
+	ko.Spec.CacheNodeType = r.ko.Spec.CacheNodeType


Should this be moved to customSetOutput instead?

kumargauravsharma · 2021-12-01T18:46:15Z

pkg/resource/replication_group/custom_update_api.go

+	desiredNodeGroupConfig := desired.ko.Spec.NodeGroupConfiguration
+	if val, ok := annotations[AnnotationLastRequestedNGC]; ok && val != "null" {
+		_ = json.Unmarshal([]byte(val), &desiredNodeGroupConfig)
+		return !reflect.DeepEqual(desiredNodeGroupConfig, val)


Could delta APIs be used here instead of reflect?

We use the reflect in delta.go

ack-bot · 2021-12-01T18:47:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kumargauravsharma, nmvk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kumargauravsharma,nmvk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Handle scaling in/out, Scaling up/down rollback fixes.

Delta was not getting generated for the NodeGroupConfiguration this resulted in Test failure.

nmvk · 2021-12-03T21:36:36Z

/retest

nmvk · 2021-12-03T23:00:11Z

/retest

nmvk · 2021-12-04T02:45:43Z

/retest

nmvk · 2021-12-05T23:44:29Z

/retest

With many tests, timeout needs to be increased to avoid random failures.

kumargauravsharma · 2021-12-06T19:34:42Z

e2e tests have passed. merging the changes.

ack-bot requested review from RedbackThomson and a-hilaly October 12, 2021 18:05

ack-bot added the approved label Oct 12, 2021

kumargauravsharma self-requested a review October 12, 2021 18:20

nmvk force-pushed the e2e-cme branch 3 times, most recently from 6f92804 to e0f8511 Compare October 13, 2021 17:17

kumargauravsharma reviewed Oct 14, 2021

View reviewed changes

nmvk force-pushed the e2e-cme branch from e0f8511 to bd17f0d Compare October 14, 2021 23:13

kumargauravsharma reviewed Oct 15, 2021

View reviewed changes

nmvk force-pushed the e2e-cme branch 3 times, most recently from 1cb5156 to 95c671f Compare October 21, 2021 03:23

Declarative Tests for Scaling

26a1c9a

Declarative tests for ScaleUpAndDown.

nmvk force-pushed the e2e-cme branch from 95c671f to 26a1c9a Compare November 11, 2021 17:23

nmvk force-pushed the e2e-cme branch 4 times, most recently from 324ccda to 196496e Compare November 24, 2021 19:37

nmvk changed the title ~~Declarative Tests for Scaling~~ Declarative Tests for ScalingUpAndDown and Async rollback fixes Nov 24, 2021

nmvk force-pushed the e2e-cme branch 5 times, most recently from f2418f3 to 0f83142 Compare November 25, 2021 05:04

a-hilaly reviewed Nov 29, 2021

View reviewed changes

nmvk force-pushed the e2e-cme branch 2 times, most recently from b4f4cd4 to 69c1925 Compare November 30, 2021 23:17

kumargauravsharma approved these changes Dec 1, 2021

View reviewed changes

nmvk force-pushed the e2e-cme branch from 69c1925 to 77bb1f2 Compare December 1, 2021 20:04

Asynchronous rollback fixes for Scaling

8d854fd

Handle scaling in/out, Scaling up/down rollback fixes.

nmvk force-pushed the e2e-cme branch 4 times, most recently from 7b1a50b to d065d72 Compare December 2, 2021 21:46

Update NodeGroupConfiguration in ReadOne

b5bb14d

Delta was not getting generated for the NodeGroupConfiguration this resulted in Test failure.

nmvk force-pushed the e2e-cme branch from d065d72 to b5bb14d Compare December 3, 2021 20:19

Increase test timeout

7eba151

With many tests, timeout needs to be increased to avoid random failures.

kumargauravsharma merged commit 5b1314a into aws-controllers-k8s:main Dec 6, 2021

		// Keep the value of desired for CacheNodeType.
		ko.Spec.CacheNodeType = r.ko.Spec.CacheNodeType

Declarative Tests for ScalingUpAndDown and Async rollback fixes #67

Declarative Tests for ScalingUpAndDown and Async rollback fixes #67

Uh oh!

Conversation

nmvk commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vijtrip2 commented Oct 20, 2021

Uh oh!

nmvk commented Nov 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nmvk commented Dec 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ack-bot commented Dec 1, 2021

Uh oh!

nmvk commented Dec 3, 2021

Uh oh!

nmvk commented Dec 3, 2021

Uh oh!

nmvk commented Dec 4, 2021

Uh oh!

nmvk commented Dec 5, 2021

Uh oh!

kumargauravsharma commented Dec 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nmvk commented Oct 12, 2021 •

edited

Loading