Skip to content

Commit 277966a

Browse files
authored
Fix delta comparison for auto failover, multi AZ, primary cluster ID (#69)
Nice. Description of changes: Properly retrieve the latest state for these 3 fields and compare them to the desired state. For each of these 3 spec fields, obtaining the corresponding latest state requires processing of a different `Status` field due to the structure of the `DescribeReplicationGroups` API response By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent cb701e2 commit 277966a

File tree

6 files changed

+112
-30
lines changed

6 files changed

+112
-30
lines changed

apis/v1alpha1/ack-generate-metadata.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ api_directory_checksum: 8e4808b17b3a814f0a624eee8d68a0b45ba5e2c4
77
api_version: v1alpha1
88
aws_sdk_go_version: v1.38.52
99
generator_config_info:
10-
file_checksum: 28e3cd3119df8028a65eb0729a462b4354d27fa9
10+
file_checksum: 23f444fb86eaa5c1acb4d4f51430e606e7ee554a
1111
original_file_name: generator.yaml
1212
last_modification:
1313
reason: API generation

apis/v1alpha1/generator.yaml

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ resources:
4848
from:
4949
operation: ListAllowedNodeTypeModifications
5050
path: ScaleDownModifications
51+
AutomaticFailoverEnabled:
52+
compare:
53+
is_ignored: true
5154
Events:
5255
is_read_only: true
5356
from:
@@ -62,6 +65,12 @@ resources:
6265
path: ReplicationGroup.LogDeliveryConfigurations
6366
compare: # removes the spec field from automatic delta comparison
6467
is_ignored: true
68+
MultiAZEnabled:
69+
compare:
70+
is_ignored: true
71+
PrimaryClusterId: # note: "PrimaryClusterID" will not function properly
72+
compare:
73+
is_ignored: true
6574
hooks:
6675
sdk_read_many_post_set_output:
6776
template_path: hooks/replication_group/sdk_read_many_post_set_output.go.tpl
@@ -72,7 +81,7 @@ resources:
7281
sdk_update_post_build_request:
7382
template_path: hooks/replication_group/sdk_update_post_build_request.go.tpl
7483
delta_post_compare:
75-
code: "filterDelta(delta, a, b)"
84+
code: "modifyDelta(delta, a, b)"
7685
sdk_file_end:
7786
template_path: hooks/replication_group/sdk_file_end.go.tpl
7887
sdk_file_end_set_output_post_populate:

generator.yaml

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ resources:
4848
from:
4949
operation: ListAllowedNodeTypeModifications
5050
path: ScaleDownModifications
51+
AutomaticFailoverEnabled:
52+
compare:
53+
is_ignored: true
5154
Events:
5255
is_read_only: true
5356
from:
@@ -62,6 +65,12 @@ resources:
6265
path: ReplicationGroup.LogDeliveryConfigurations
6366
compare: # removes the spec field from automatic delta comparison
6467
is_ignored: true
68+
MultiAZEnabled:
69+
compare:
70+
is_ignored: true
71+
PrimaryClusterId: # note: "PrimaryClusterID" will not function properly
72+
compare:
73+
is_ignored: true
6574
hooks:
6675
sdk_read_many_post_set_output:
6776
template_path: hooks/replication_group/sdk_read_many_post_set_output.go.tpl
@@ -72,7 +81,7 @@ resources:
7281
sdk_update_post_build_request:
7382
template_path: hooks/replication_group/sdk_update_post_build_request.go.tpl
7483
delta_post_compare:
75-
code: "filterDelta(delta, a, b)"
84+
code: "modifyDelta(delta, a, b)"
7685
sdk_file_end:
7786
template_path: hooks/replication_group/sdk_file_end.go.tpl
7887
sdk_file_end_set_output_post_populate:

pkg/resource/replication_group/delta.go

Lines changed: 1 addition & 22 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/resource/replication_group/delta_util.go

Lines changed: 88 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ import (
2525
"github.com/aws-controllers-k8s/elasticache-controller/pkg/common"
2626
)
2727

28-
// filterDelta removes non-meaningful differences from the delta and adds additional differences if necessary
29-
func filterDelta(
28+
// modifyDelta removes non-meaningful differences from the delta and adds additional differences if necessary
29+
func modifyDelta(
3030
delta *ackcompare.Delta,
3131
desired *resource,
3232
latest *resource,
@@ -54,6 +54,19 @@ func filterDelta(
5454
delta.Add("Spec.LogDeliveryConfigurations", desired.ko.Spec.LogDeliveryConfigurations,
5555
unmarshalLastRequestedLDCs(desired))
5656
}
57+
58+
if multiAZRequiresUpdate(desired, latest) {
59+
delta.Add("Spec.MultiAZEnabled", desired.ko.Spec.MultiAZEnabled, latest.ko.Status.MultiAZ)
60+
}
61+
62+
if autoFailoverRequiresUpdate(desired, latest) {
63+
delta.Add("Spec.AutomaticFailoverEnabled", desired.ko.Spec.AutomaticFailoverEnabled,
64+
latest.ko.Status.AutomaticFailover)
65+
}
66+
67+
if updateRequired, current := primaryClusterIDRequiresUpdate(desired, latest); updateRequired {
68+
delta.Add("Spec.PrimaryClusterID", desired.ko.Spec.PrimaryClusterID, *current)
69+
}
5770
}
5871

5972
// returns true if desired and latest engine versions match and false otherwise
@@ -100,3 +113,76 @@ func unmarshalLastRequestedLDCs(desired *resource) []*svcapitypes.LogDeliveryCon
100113

101114
return lastRequestedConfigs
102115
}
116+
117+
// multiAZRequiresUpdate returns true if the latest multi AZ status does not yet match the
118+
// desired state, and false otherwise
119+
func multiAZRequiresUpdate(desired *resource, latest *resource) bool {
120+
// no preference for multi AZ specified; no update required
121+
if desired.ko.Spec.MultiAZEnabled == nil {
122+
return false
123+
}
124+
125+
// API should return a non-nil value, but if it doesn't then attempt to update
126+
if latest.ko.Status.MultiAZ == nil {
127+
return true
128+
}
129+
130+
// true maps to "enabled"; false maps to "disabled"
131+
// this accounts for values such as "enabling" and "disabling"
132+
if *desired.ko.Spec.MultiAZEnabled {
133+
return *latest.ko.Status.MultiAZ != string(svcapitypes.MultiAZStatus_enabled)
134+
} else {
135+
return *latest.ko.Status.MultiAZ != string(svcapitypes.MultiAZStatus_disabled)
136+
}
137+
}
138+
139+
// autoFailoverRequiresUpdate returns true if the latest auto failover status does not yet match the
140+
// desired state, and false otherwise
141+
func autoFailoverRequiresUpdate(desired *resource, latest *resource) bool {
142+
// the logic is exactly analogous to multiAZRequiresUpdate above
143+
if desired.ko.Spec.AutomaticFailoverEnabled == nil {
144+
return false
145+
}
146+
147+
if latest.ko.Status.AutomaticFailover == nil {
148+
return true
149+
}
150+
151+
if *desired.ko.Spec.AutomaticFailoverEnabled {
152+
return *latest.ko.Status.AutomaticFailover != string(svcapitypes.AutomaticFailoverStatus_enabled)
153+
} else {
154+
return *latest.ko.Status.AutomaticFailover != string(svcapitypes.AutomaticFailoverStatus_disabled)
155+
}
156+
}
157+
158+
// primaryClusterIDRequiresUpdate retrieves the current primary cluster ID and determines whether
159+
// an update is required. If no desired state is specified or there is an issue retrieving the
160+
// latest state, return false, nil. Otherwise, return false or true depending on equality of
161+
// the latest and desired states, and a non-nil pointer to the latest value
162+
func primaryClusterIDRequiresUpdate(desired *resource, latest *resource) (bool, *string) {
163+
if desired.ko.Spec.PrimaryClusterID == nil {
164+
return false, nil
165+
}
166+
167+
// primary cluster ID applies to cluster mode disabled only; if API returns multiple
168+
// or no node groups, or the provided node group is nil, there is nothing that can be done
169+
if len(latest.ko.Status.NodeGroups) != 1 || latest.ko.Status.NodeGroups[0] == nil {
170+
return false, nil
171+
}
172+
173+
// attempt to find primary cluster in node group. If for some reason it is not present, we
174+
// don't have a reliable latest state, so do nothing
175+
ng := *latest.ko.Status.NodeGroups[0]
176+
for _, member := range ng.NodeGroupMembers {
177+
if member == nil {
178+
continue
179+
}
180+
181+
if member.CurrentRole != nil && *member.CurrentRole == "primary" && member.CacheClusterID != nil {
182+
val := *member.CacheClusterID
183+
return val != *desired.ko.Spec.PrimaryClusterID, &val
184+
}
185+
}
186+
187+
return false, nil
188+
}

test/e2e/tests/test_replicationgroup.py

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -555,7 +555,6 @@ def test_rg_update_misc(self, rg_update_misc_input, rg_update_misc):
555555
assert_misc_fields(reference, rg_update_misc_input['RG_ID'], pmw, description, srl, sw)
556556

557557
# test modifying properties related to tolerance: replica promotion, multi AZ, automatic failover
558-
@pytest.mark.blocked # TODO: remove when passing
559558
def test_rg_fault_tolerance(self, rg_fault_tolerance):
560559
(reference, _) = rg_fault_tolerance
561560
assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "True", wait_periods=30)
@@ -607,8 +606,8 @@ def test_rg_fault_tolerance(self, rg_fault_tolerance):
607606
raise AssertionError(f"Unknown node {node['cacheClusterID']}")
608607

609608
# assert AF and multi AZ
610-
assert resource['status']['automaticFailover'] == "disabled"
611-
assert resource['status']['multiAZ'] == "disabled"
609+
assert resource['status']['automaticFailover'] == "enabled"
610+
assert resource['status']['multiAZ'] == "enabled"
612611

613612
# test association and disassociation of other resources (VPC security groups, SNS topic, user groups)
614613
@pytest.mark.blocked # TODO: remove when passing

0 commit comments

Comments
 (0)